This pref observer will listen to preference change events and trigger functionality in the AboutNewTab object. It will also be responsible for writing to prefs, as we want bi-directional data binding between the content processes and prefs to allow sync to work.

Bug 1196762 - Implement a pref provider for remote newtab 1/2 r?Mardak

Bug 1196762 - Implement a pref provider for remote newtab 2/2 r?Mardak

Comment on attachment 8685583 [details] MozReview Request: Bug 1196762 - Part 1: Initial Prefs Provider module for the remote newtab page r=Mardak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24835/diff/1-2/

Comment on attachment 8685584 [details] MozReview Request: Bug 1196762 - Part 2: PrefsProvider and PlacesProvider messaging integration in RemoteAboutNewTab r=Mardak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24837/diff/1-2/

Comment on attachment 8685584 [details] MozReview Request: Bug 1196762 - Part 2: PrefsProvider and PlacesProvider messaging integration in RemoteAboutNewTab r=Mardak https://reviewboard.mozilla.org/r/24837/#review22515 ::: browser/components/newtab/NewTabPrefsProvider.jsm:10 (Diff revision 2) > +const Cc = Components.classes; nit: const {classes: Cc, interfaces: Ci, utils: Cu} = Components; ::: browser/components/newtab/NewTabPrefsProvider.jsm:47 (Diff revision 2) > + break; Do we really want to just emit the name for other prefs, e.g., locale.matchOS and locale? It might make more sense to always annotate the prefSet -> prefMap with the type of value you want to emit. ::: browser/components/newtab/RemoteAboutNewTab.jsm:92 (Diff revision 2) > + this.pageListener.sendAsyncMessage("NewTab:PlacesDeleteURI", data.url); Can we get a test that makes sure we're sending the expected values for each of the above? Otherwise someone could easily accidentally change placesDeleteURI to give the object instead of url. Although keeping the object would make it more like placesLinkChanged. ::: browser/components/newtab/tests/xpcshell/test_NewTabPrefsProvider.js:22 (Diff revision 2) > + }); Some reason I seem to recall that it's better to check/assert outside of the callback to make it easier to debug where things failed. But perhaps with new fancy promises, this isn't an issue. In any case, would be good to check the name of the pref as well in addition to the explicit value of the pref (true) instead of calling the same function the NewTabPrefsProvider happens to use. And should check for the other types of pref values, e.g., the string prefs.

Comment on attachment 8685583 [details] MozReview Request: Bug 1196762 - Part 1: Initial Prefs Provider module for the remote newtab page r=Mardak https://reviewboard.mozilla.org/r/24835/#review22511 ::: browser/components/newtab/NewTabPrefsProvider.jsm:1 (Diff revision 2) > +/* global Services, EventEmitter, XPCOMUtils */ MPL2 notice ::: browser/components/newtab/NewTabPrefsProvider.jsm:23 (Diff revision 2) > +]); nit: I believe the style is 2 spaces ::: browser/components/newtab/NewTabPrefsProvider.jsm:34 (Diff revision 2) > + this.emit(data); It probably would have been easier to review if these two patches were just combined. I was about to comment about how this emit probably wants to include the pref and value. ::: browser/components/newtab/NewTabPrefsProvider.jsm:45 (Diff revision 2) > + startTracking() { Any reason for the start/stopTracking instead of init/uninit as the other services? The non-init name might make it seem like one could call start/stop multiple times (potentially from different places).

Comment on attachment 8685583 [details] MozReview Request: Bug 1196762 - Part 1: Initial Prefs Provider module for the remote newtab page r=Mardak https://reviewboard.mozilla.org/r/24835/#review22517

Comment on attachment 8685584 [details] MozReview Request: Bug 1196762 - Part 2: PrefsProvider and PlacesProvider messaging integration in RemoteAboutNewTab r=Mardak https://reviewboard.mozilla.org/r/24837/#review22519

https://reviewboard.mozilla.org/r/24837/#review22515 > Can we get a test that makes sure we're sending the expected values for each of the above? Otherwise someone could easily accidentally change placesDeleteURI to give the object instead of url. Although keeping the object would make it more like placesLinkChanged. We currently have tests for PlacesProvider.jsm. We will add tests for RemoteAboutNewTab.jsm when we pare down the messages needed, in bug 1218992

Comment on attachment 8685583 [details] MozReview Request: Bug 1196762 - Part 1: Initial Prefs Provider module for the remote newtab page r=Mardak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24835/diff/2-3/

Comment on attachment 8685584 [details] MozReview Request: Bug 1196762 - Part 2: PrefsProvider and PlacesProvider messaging integration in RemoteAboutNewTab r=Mardak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24837/diff/2-3/

https://reviewboard.mozilla.org/r/24835/#review22641 ::: browser/components/newtab/NewTabPrefsProvider.jsm:10 (Diff revisions 2 - 3) > +Cu.import("resource://gre/modules/Preferences.jsm"); neat :) ::: browser/components/newtab/NewTabPrefsProvider.jsm:19 (Diff revisions 2 - 3) > - "browser.newtabpage.enabled", > +const PrefsMap = new Map([ I believe naming convention has LargeFirstLetter for constructors/services. Probably better as gPrefsMap. ::: browser/components/newtab/NewTabPrefsProvider.jsm:24 (Diff revisions 2 - 3) > + ["general.useragent.locale", "localizedStr"], Probably go with the full "string" and simply "localized" (as localized bool or localized number don't make sense ;))

https://reviewboard.mozilla.org/r/24835/#review22511 > It probably would have been easier to review if these two patches were just combined. I was about to comment about how this emit probably wants to include the pref and value. Sorry, split it up in two so ursula gets credit for her patch

Comment on attachment 8685583 [details] MozReview Request: Bug 1196762 - Part 1: Initial Prefs Provider module for the remote newtab page r=Mardak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24835/diff/3-4/

Comment on attachment 8685584 [details] MozReview Request: Bug 1196762 - Part 2: PrefsProvider and PlacesProvider messaging integration in RemoteAboutNewTab r=Mardak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24837/diff/3-4/

Comment on attachment 8685583 [details] MozReview Request: Bug 1196762 - Part 1: Initial Prefs Provider module for the remote newtab page r=Mardak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24835/diff/4-5/

Comment on attachment 8685584 [details] MozReview Request: Bug 1196762 - Part 2: PrefsProvider and PlacesProvider messaging integration in RemoteAboutNewTab r=Mardak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24837/diff/4-5/

Comment on attachment 8685583 [details] MozReview Request: Bug 1196762 - Part 1: Initial Prefs Provider module for the remote newtab page r=Mardak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24835/diff/5-6/

Comment on attachment 8685584 [details] MozReview Request: Bug 1196762 - Part 2: PrefsProvider and PlacesProvider messaging integration in RemoteAboutNewTab r=Mardak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24837/diff/5-6/

https://hg.mozilla.org/integration/mozilla-inbound/rev/c47a9867a76573fc8fc83fde56ec749ff83e2181 Bug 1196762 - Part 1: Initial Prefs Provider module for the remote newtab page r=Mardak https://hg.mozilla.org/integration/mozilla-inbound/rev/7f064c7d4d1208e468a54c82c6813fd4249a6924 Bug 1196762 - Part 2: PrefsProvider and PlacesProvider messaging integration in RemoteAboutNewTab r=Mardak

https://hg.mozilla.org/mozilla-central/rev/c47a9867a765 https://hg.mozilla.org/mozilla-central/rev/7f064c7d4d12