Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Tab Page WebUI: Use Profile Preferences to store page options instead of localStorage #2830

Closed
wants to merge 3 commits into from

Conversation

petemill
Copy link
Member

@petemill petemill commented Jun 28, 2019

This is created as a PR on top of #2762 since it has not been merged yet. That PR contains all the extra settings. If we want to merge this refactor before that PR then we can remove the extra preferences from this PR and keep only the preference for background image.

This allows:

  • Surfacing these preferences outside of the NTP
  • Syncing / migrating the preferences
  • Making the change immediately for all open NTP tabs

Use FireWebUIListener to push data to javascript event subscribers.
Still uses render_view_host->SetWebUIProperty (like the existing NTP data), which we may port in the future to a DataSource Dictionary like the other WebUI in chromium.

Provides any data retrieved or updated to redux store via redux action, but does not use redux actions to ask for a data change.

Also includes

Refactor NTP WebUI backend

Focus on the MessageHandler in order to use FireWebUIListener and follow chromium best practice for setting data and events

TS types for NTP actions

Small change for intellisense™ and action parameter type checking

Submitter Checklist:

Test Plan:

  • Open multiple windows with NTP visible in each.
  • Toggle each of the settings
  • Expected: settings change and widgets show/hide in current window and other windows

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

petemill added 2 commits June 28, 2019 00:24
… MessageHandler

Following best practices from chromium webui documentation, move towards using DataSource keys for the initial page data, and FireWebUIListener for updating data.
We're still manually setting data on the render_host in the WebUIController, but this refactor allows us to use FireWebUIListener now, and move towards using DataSource for that data in the future. MessageHandler allows us to use an established JS lifecycle to ensure that data is only sent to the correct contexts at the correct time.

Use FireWebUIListener for 'stats-updated' instead of expecting a global 'statsUpdated' function to exist (which was causing some JS errors).
@petemill petemill force-pushed the ntp-pref-store branch 2 times, most recently from 800fe6a to fae770d Compare June 28, 2019 07:30
…tead of localStorage

Fixes brave/brave-browser#5064

This allows:
- Surfacing these preferences outside of the NTP
- Syncing / migrating the preferences
- Making the change immediately for all open NTP

Use FireWebUIListener to push data to javascript event subscribers.
Still uses render_view_host->SetWebUIProperty (like the existing NTP data), which we may port in the future to a DataSource Dictionary like the other WebUI in chromium.

Provides any data retreived or updated to redux store via redux action, but does not use redux actions to ask for a data change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant