Closed Bug 1196762 Opened 5 years ago Closed 4 years ago

Implement a PrefProvider for Remote New Tab Page

Categories

(Firefox :: New Tab Page, defect)

43 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Iteration:
43.3 - Sep 21
Tracking Status
firefox43 --- affected
firefox45 --- fixed

People

(Reporter: oyiptong, Assigned: ursula)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files)

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.
Blocks: 1193862
Assignee: nobody → oyiptong
Assignee: oyiptong → ursulasarracini
Bug 1196762 - Implement a pref provider for remote newtab 1/2 r?Mardak
Attachment #8685583 - Flags: review?(edilee)
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/
Attachment #8685583 - Attachment description: MozReview Request: Bug 1196762 - Implement a pref provider for remote newtab 1/2 r?Mardak → MozReview Request: Bug 1196762 - Part 1: Initial Prefs Provider module for the remote newtab page r?Mardak
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/
Attachment #8685584 - Attachment description: MozReview Request: Bug 1196762 - Implement a pref provider for remote newtab 2/2 r?Mardak → MozReview Request: Bug 1196762 - Part 2: PrefsProvider messaging integration in RemoteAboutNewTab r?Mardak
Summary: Implement a PrefHandler for AboutNewTab → Implement a PrefProvider for Remote New Tab Page
Attachment #8685584 - Flags: review?(edilee)
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).
Attachment #8685583 - Flags: review?(edilee)
Attachment #8685583 - Flags: review+
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
Attachment #8685584 - Flags: review+
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/
Attachment #8685583 - Attachment description: MozReview Request: Bug 1196762 - Part 1: Initial Prefs Provider module for the remote newtab page r?Mardak → MozReview Request: Bug 1196762 - Part 1: Initial Prefs Provider module for the remote newtab page r=Mardak
Attachment #8685584 - Attachment description: MozReview Request: Bug 1196762 - Part 2: PrefsProvider messaging integration in RemoteAboutNewTab r?Mardak → MozReview Request: Bug 1196762 - Part 2: PrefsProvider and PlacesProvider messaging integration in RemoteAboutNewTab r=Mardak
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
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Depends on: 1242218
You need to log in before you can comment on or make changes to this bug.