This patch creates a new browser component related to the remote newtab page. The original landing of this component includes some code duplication from browser/module/DirectoryLinksProvider.jsm and toolkit/module/NewTabUtils.jsm Some more related browser modules have been moved to this component. The idea is that we'll remove the aforementioned modules once we move code from the privileged context to the remote page tracked in bug 1210936. In fact, a content script in bug 1210936 invokes code from this module. It is to provide a subset of functionality as to the non-remote newtab page for now.

This functionality is meant to be under pref. It won't replace the current newtab page for now. The remoted newtab page is currently in development, but we want to land these patches so that we can de-risk our engineering, iterate on our code and give the platform team a real-world use case for service workers and more.

try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a719446ee7bc

Just making sure, this is effectively the summary of changes (without actually removing the existing NewTabUtils and DirectoryLinksProvider): > renamed: browser/modules/NewTabURL.jsm -> browser/components/newtab/NewTabURL.jsm > renamed: browser/modules/test/xpcshell/test_NewTabURL.js -> browser/components/newtab/tests/xpcshell/test_NewTabURL.js > renamed: browser/modules/DirectoryLinksProvider.jsm -> browser/components/newtab/RemoteDirectoryLinksProvider.jsm > renamed: browser/modules/test/xpcshell/test_DirectoryLinksProvider.js -> browser/components/newtab/tests/xpcshell/test_RemoteDirectoryLinksProvider.js > renamed: toolkit/modules/NewTabUtils.jsm -> browser/components/newtab/RemoteNewTabUtils.jsm > renamed: toolkit/modules/tests/xpcshell/test_NewTabUtils.js -> browser/components/newtab/tests/xpcshell/test_RemoteNewTabUtils.js > modified: browser/components/moz.build > modified: browser/components/nsBrowserGlue.js > modified: browser/modules/moz.build > modified: browser/modules/test/xpcshell/xpcshell.ini > new file: browser/components/newtab/RemoteAboutNewTab.jsm > new file: browser/components/newtab/RemoteNewTabLocation.jsm > new file: browser/components/newtab/moz.build > new file: browser/components/newtab/tests/browser/browser.ini > new file: browser/components/newtab/tests/browser/browser_remotenewtab_pageloads.js > new file: browser/components/newtab/tests/browser/dummy_page.html > new file: browser/components/newtab/tests/xpcshell/test_RemoteNewTabLocation.js > new file: browser/components/newtab/tests/xpcshell/xpcshell.ini I've attached a diff of the "renamed" files: > git diff -w head^:browser/modules/NewTabURL.jsm head:browser/components/newtab/NewTabURL.jsm > git diff -w head^:browser/modules/test/xpcshell/test_NewTabURL.js head:browser/components/newtab/tests/xpcshell/test_NewTabURL.js > git diff -w head^:browser/modules/DirectoryLinksProvider.jsm head:browser/components/newtab/RemoteDirectoryLinksProvider.jsm > git diff -w head^:browser/modules/test/xpcshell/test_DirectoryLinksProvider.js head:browser/components/newtab/tests/xpcshell/test_RemoteDirectoryLinksProvider.js > git diff -w head^:toolkit/modules/NewTabUtils.jsm head:browser/components/newtab/RemoteNewTabUtils.jsm > git diff -w head^:toolkit/modules/tests/xpcshell/test_NewTabUtils.js head:browser/components/newtab/tests/xpcshell/test_RemoteNewTabUtils.js

Yes, and we also modified RemoteDirectoryLinksProvider.jsm and RemoteNewTabUtils.jsm by removing functions and tests that we are not using anymore.

so the diffs will show some changed lines in both of the files above (changing their names to the "remote" variant), as well as a lot of removed lines.

NewTabURL has also been modified to allow for setting about:remote-newtab as the default at the switch of a boolean pref.

> git diff -w mozilla/master:./browser/modules/NewTabURL.jsm ./browser/components/newtab/NewTabURL.jsm | diffstat > NewTabURL.jsm | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > git diff -w mozilla/master:./browser/modules/test/xpcshell/test_NewTabURL.js ./browser/components/newtab/tests/xpcshell/test_NewTabURL.js | diffstat > test_NewTabURL.js | 5 +++++ > 1 file changed, 5 insertions(+) > git diff -w mozilla/master:./browser/modules/DirectoryLinksProvider.jsm ./browser/components/newtab/RemoteDirectoryLinksProvider.jsm | diffstat > RemoteDirectoryLinksProvider.jsm | 211 ++++++++++----------------------------- > 1 file changed, 56 insertions(+), 155 deletions(-) > git diff -w mozilla/master:./browser/modules/test/xpcshell/test_DirectoryLinksProvider.js ./browser/components/newtab/tests/xpcshell/test_RemoteDirectoryLinksProvider.js | diffstat > test_RemoteDirectoryLinksProvider.js | 1236 +++++++---------------------------- > 1 file changed, 278 insertions(+), 958 deletions(-) > git diff -w mozilla/master:./toolkit/modules/NewTabUtils.jsm ./browser/components/newtab/RemoteNewTabUtils.jsm | diffstat > RemoteNewTabUtils.jsm | 755 +------------------------------------------------- > 1 file changed, 25 insertions(+), 730 deletions(-) > git diff -w mozilla/master:./toolkit/modules/tests/xpcshell/test_NewTabUtils.js ./browser/components/newtab/tests/xpcshell/test_RemoteNewTabUtils.js | diffstat > test_RemoteNewTabUtils.js | 167 ++++++++++++++++++++++------------------------ > 1 file changed, 82 insertions(+), 85 deletions(-) Where most of the changes look as follows: > < do_check_links(NewTabUtils.links.getLinks(), links); > --- > > do_check_links(RemoteNewTabUtils.links.getLinks(), links); > 266c263 > < do_check_links(NewTabUtils.links.getLinks(), links); > --- > > do_check_links(RemoteNewTabUtils.links.getLinks(), links); > 268c265

Both look equivalent > $ diffstat mardaksdiff.patch > NewTabURL.jsm | 7 > RemoteDirectoryLinksProvider.jsm | 211 --- > RemoteNewTabUtils.jsm | 755 ------------ > tests/xpcshell/test_NewTabURL.js | 5 > tests/xpcshell/test_RemoteDirectoryLinksProvider.js | 1236 ++++---------------- > tests/xpcshell/test_RemoteNewTabUtils.js | 167 +- > 6 files changed, 452 insertions(+), 1929 deletions(-) > $ diffstat mardaksdiff.patch > NewTabURL.jsm | 7 > RemoteDirectoryLinksProvider.jsm | 211 --- > RemoteNewTabUtils.jsm | 755 ------------ > tests/xpcshell/test_NewTabURL.js | 5 > tests/xpcshell/test_RemoteDirectoryLinksProvider.js | 1236 ++++---------------- > tests/xpcshell/test_RemoteNewTabUtils.js | 167 +- > 6 files changed, 452 insertions(+), 1929 deletions(-)

Comment on attachment 8671493 [details] [diff] [review] renamed diffs Review of attachment 8671493 [details] [diff] [review]: ----------------------------------------------------------------- ::: head:browser/components/newtab/NewTabURL.jsm @@ +18,5 @@ > _overridden: false, > > get: function() { > + let output = this._url; > + if (Services.prefs.getBoolPref("browser.newtabpage.remote")) { I see this pref being added in bug 1210936, but if this lands first, getBoolPref will throw and prevent a new tab from opening. Probably add the pref to firefox.js to be safe. If someone toggles it to true and gets a broken about:remote-newtab.. that should be fine for now. ::: head:browser/components/newtab/RemoteDirectoryLinksProvider.jsm @@ +3,5 @@ > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > +this.EXPORTED_SYMBOLS = ["RemoteDirectoryLinksProvider"]; What's the plan for DirectoryLinksProvider? Will a local newtab page always be available with the pref remote = false? If so, maintaining two similar files will probably be challenging. Perhaps another high level question is why not just use DirectoryLinksProvider instead of the slightly smaller RemoteDirectoryLinksProvider? @@ +213,5 @@ > > /** > * Set appropriate default ping behavior controlled by enhanced pref > */ > + _setDefaultEnhanced: function RemoteDirectoryLinksProvider_setDefaultEnhanced() { nit: you didn't really need to give the new name to these named functions unless you just did a mass replace all. These jsms can use the fancy new method syntax, e.g., _setDefaultEnhanced() { @@ -498,5 @@ > - * @param action String of the behavior to report > - * @param triggeringSiteIndex optional Int index of the site triggering action > - * @return download promise > - */ > - reportSitesAction: function DirectoryLinksProvider_reportSitesAction(sites, action, triggeringSiteIndex) { I'm assuming this reporting will be handled from the content code? ::: head:browser/components/newtab/tests/xpcshell/test_RemoteDirectoryLinksProvider.js @@ +8,3 @@ > */ > > +const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu, Constructor: CC } = Components; I believe using const for these can cause problems if there's a head.js that also assigns those. @@ -509,5 @@ > - NewTabUtils.isTopPlacesSite = origIsTopPlacesSite; > - NewTabUtils.getProviderLinks = origGetProviderLinks; > -}); > - > -add_task(function test_suggestedAttributes() { These tile content and decisioning related tests aren't needed as they're being tested within the content space? ::: head:browser/components/newtab/RemoteNewTabUtils.jsm @@ -46,5 @@ > -// The preference that tells the number of rows of the newtab grid. > -const PREF_NEWTAB_ROWS = "browser.newtabpage.rows"; > - > -// The preference that tells the number of columns of the newtab grid. > -const PREF_NEWTAB_COLUMNS = "browser.newtabpage.columns"; Are we migrating these prefs to some content-equivalent pref? E.g., someone previously turned off suggested tiles would start seeing suggested tiles in the remote newtab? @@ -117,5 @@ > - > - get _prefs() { > - return Object.freeze({ > - pinnedLinks: "browser.newtabpage.pinned", > - blockedLinks: "browser.newtabpage.blocked", Similar to the pref migration question, any support for storage migration? E.g., someone previously blocked undesired.site.com and remote newtab could end up showing it? @@ -196,5 @@ > - > -/** > - * Singleton that serves as a registry for all open 'New Tab Page's. > - */ > -var AllPages = { Will multiple open remote newtab pages update when the user changes something in one? If so, how is that handled? @@ +286,5 @@ > > /** > + * The reason to update the pages. > + */ > + _reason: null, What's this for? I see it get assigned links-changed, but this is only a single value, but potentially there's multiple things triggering a _reason change. @@ +375,2 @@ > if (link) { > + link.baseDomain = RemoteNewTabUtils.extractSite(link.url); Looks like this for loop and logic can be merged into the links.filter where we already have the extracted site. Will this original logic be reimplemented in the content space to merge pinned with the links gotten here (and remove blocked)? @@ -1313,5 @@ > - value: AllPages.enhanced }, > - { histogram: "NEWTAB_PAGE_PINNED_SITES_COUNT", > - value: PinnedLinks.links.length }, > - { histogram: "NEWTAB_PAGE_BLOCKED_SITES_COUNT", > - value: Object.keys(BlockedLinks.links).length } Are these telemetry probes actually being removed? If so, the Histograms.json should probably be updated.

Comment on attachment 8669118 [details] [diff] [review] bug_1210940.patch Review of attachment 8669118 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/newtab/RemoteAboutNewTab.jsm @@ +45,5 @@ > + this.pageListener.addMessageListener("NewTab:UpdateGrid", this.updateGrid.bind(this)); > + this.pageListener.addMessageListener("NewTab:CaptureBackgroundPageThumbs", > + this.captureBackgroundPageThumb.bind(this)); > + this.pageListener.addMessageListener("NewTab:PageThumbs", this.createPageThumb.bind(this)); > + this.pageListener.addMessageListener("NewTabFrame:GetInit", this.initContentFrame.bind(this)); Not sure if it would be useful to keep the event <-> method naming consistent for ease of finding what message triggers what method. @@ +62,5 @@ > + initializeGrid: function(message) { > + RemoteNewTabUtils.links.populateCache(() => { > + message.target.sendAsyncMessage("NewTab:InitializeLinks", { > + links: RemoteNewTabUtils.links.getLinks(), > + enhancedLinks: this.getEnhancedLinks(), Are enhanced links still being supported? Or at least originally an enhanced link were history links that would have a custom image and text shown, but that was confusing to users. So Suggested and Directory links were their own link object that had an enhanced image property for the hover/static views. @@ +201,5 @@ > + if (aTopic !== "page-thumbnail:create") { > + // Change the topic for enhanced and enabled observers. > + aTopic = aData; > + } > + this.pageListener.sendAsyncMessage("NewTab:Observe", {topic: aTopic, data: extraData}); Any reason to pass down a generic Observe instead of a more specific NewTab:RegularThumbnailURI? ::: browser/components/newtab/moz.build @@ +18,5 @@ > + 'RemoteNewTabUtils.jsm', > +] > + > +if CONFIG['MOZILLA_OFFICIAL']: > + DEFINES['MOZILLA_OFFICIAL'] = 1 Is this MOZILLA_OFFICIAL needed here? ::: browser/components/nsBrowserGlue.js @@ +601,3 @@ > _init: function BG__init() { > let os = Services.obs; > os.addObserver(this, "prefservice:after-app-defaults", false); Not sure if you intended the whitespace change. But there's an upstream conflict here. os.addObserver(this, "notifications-open-settings", false);

(In reply to Ed Lee :Mardak from comment #11) > I see this pref being added in bug 1210936, but if this lands first, > getBoolPref will throw and prevent a new tab from opening. Probably add the > pref to firefox.js to be safe. If someone toggles it to true and gets a > broken about:remote-newtab.. that should be fine for now. Good point. I'll add it to firefox.js as well to be safe. > What's the plan for DirectoryLinksProvider? Will a local newtab page always > be available with the pref remote = false? If so, maintaining two similar > files will probably be challenging. > > Perhaps another high level question is why not just use > DirectoryLinksProvider instead of the slightly smaller > RemoteDirectoryLinksProvider? The plan is actually to get rid of RemoteDirectoryLinksProvider and RemoteNewTabUtils. Over the next couple of weeks, both of these modules are going to disappear. The initial landing contains the modules until we develop those features in-content. The reason I duplicated the code was so that we'd just get rid of them without having to modify DirectoryLinksProvider to add observers on init. > > - reportSitesAction: function DirectoryLinksProvider_reportSitesAction(sites, action, triggeringSiteIndex) { > > I'm assuming this reporting will be handled from the content code? That is correct. > > +const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu, Constructor: CC } = Components; > > I believe using const for these can cause problems if there's a head.js that > also assigns those. Indeed. We don't have a head in the test directory, which is why this was added. > > -add_task(function test_suggestedAttributes() { > > These tile content and decisioning related tests aren't needed as they're > being tested within the content space? Yes, that's correct. > ::: head:browser/components/newtab/RemoteNewTabUtils.jsm > @@ -46,5 @@ > > -// The preference that tells the number of rows of the newtab grid. > > -const PREF_NEWTAB_ROWS = "browser.newtabpage.rows"; > > - > > -// The preference that tells the number of columns of the newtab grid. > > -const PREF_NEWTAB_COLUMNS = "browser.newtabpage.columns"; > > Are we migrating these prefs to some content-equivalent pref? E.g., someone > previously turned off suggested tiles would start seeing suggested tiles in > the remote newtab? We are moving everything to the remote page, except for 3 prefs. We'll enable bi-directional for those 3 prefs in an upcoming patch. > > @@ -117,5 @@ > > - > > - get _prefs() { > > - return Object.freeze({ > > - pinnedLinks: "browser.newtabpage.pinned", > > - blockedLinks: "browser.newtabpage.blocked", > > Similar to the pref migration question, any support for storage migration? > E.g., someone previously blocked undesired.site.com and remote newtab could > end up showing it? We are handling that in-content. > > @@ -196,5 @@ > > - > > -/** > > - * Singleton that serves as a registry for all open 'New Tab Page's. > > - */ > > -var AllPages = { > > Will multiple open remote newtab pages update when the user changes > something in one? If so, how is that handled? It is handled by using same-origin window messaging, indexeddb and serviceworkers depending on what needs to be updated. > > @@ +286,5 @@ > > > > /** > > + * The reason to update the pages. > > + */ > > + _reason: null, > > What's this for? I see it get assigned links-changed, but this is only a > single value, but potentially there's multiple things triggering a _reason > change. It is currently unused and should be removed, as well as updatePages. > > @@ +375,2 @@ > > if (link) { > > + link.baseDomain = RemoteNewTabUtils.extractSite(link.url); > > Looks like this for loop and logic can be merged into the links.filter where > we already have the extracted site. > > Will this original logic be reimplemented in the content space to merge > pinned with the links gotten here (and remove blocked)? It will be re-implemented in the content page. I'll merge it in the filter function for now. > > @@ -1313,5 @@ > > - value: AllPages.enhanced }, > > - { histogram: "NEWTAB_PAGE_PINNED_SITES_COUNT", > > - value: PinnedLinks.links.length }, > > - { histogram: "NEWTAB_PAGE_BLOCKED_SITES_COUNT", > > - value: Object.keys(BlockedLinks.links).length } > > Are these telemetry probes actually being removed? If so, the > Histograms.json should probably be updated. Good point. We are keeping them for the local newtab page, and we'll remove them when we replace the local newtab page

(In reply to Ed Lee :Mardak from comment #12) > Not sure if it would be useful to keep the event <-> method naming > consistent for ease of finding what message triggers what method. Good point. > > @@ +62,5 @@ > > + initializeGrid: function(message) { > > + RemoteNewTabUtils.links.populateCache(() => { > > + message.target.sendAsyncMessage("NewTab:InitializeLinks", { > > + links: RemoteNewTabUtils.links.getLinks(), > > + enhancedLinks: this.getEnhancedLinks(), > > Are enhanced links still being supported? Or at least originally an enhanced > link were history links that would have a custom image and text shown, but > that was confusing to users. So Suggested and Directory links were their own > link object that had an enhanced image property for the hover/static views. You are correct, "Enhanced Tiles" is now thought of as a non-directory tile. That said, we still allow for history links to have an enhanced imageURI in the local newtab. We are trying to get as close to 1-1 functionality. I'm not sure if we'll keep the "enhanced" feature going forward. I hope not. We need to keep this message as we don't have access to the getEnhancedLink function in-content. > > @@ +201,5 @@ > > + if (aTopic !== "page-thumbnail:create") { > > + // Change the topic for enhanced and enabled observers. > > + aTopic = aData; > > + } > > + this.pageListener.sendAsyncMessage("NewTab:Observe", {topic: aTopic, data: extraData}); > > Any reason to pass down a generic Observe instead of a more specific > NewTab:RegularThumbnailURI? We used to observe more events in this codepath, including pref changes. You're correct, this is the only thing this is observing here, however, pref observers are coming back in another form and we want to keep NewTab:Observe as the message name for things we "subscribe to" in-content. > > ::: browser/components/newtab/moz.build > @@ +18,5 @@ > > + 'RemoteNewTabUtils.jsm', > > +] > > + > > +if CONFIG['MOZILLA_OFFICIAL']: > > + DEFINES['MOZILLA_OFFICIAL'] = 1 > > Is this MOZILLA_OFFICIAL needed here? Not needed. I'll remove it. > ::: browser/components/nsBrowserGlue.js > @@ +601,3 @@ > > _init: function BG__init() { > > let os = Services.obs; > > os.addObserver(this, "prefservice:after-app-defaults", false); > > Not sure if you intended the whitespace change. But there's an upstream > conflict here. Ah my linter applied the whitespace change. I'll update and fix the conflict.

Comment on attachment 8669118 [details] [diff] [review] bug_1210940.patch (In reply to Olivier Yiptong [:oyiptong] from comment #13) > > > - { histogram: "NEWTAB_PAGE_PINNED_SITES_COUNT", > > > - { histogram: "NEWTAB_PAGE_BLOCKED_SITES_COUNT", > > Are these telemetry probes actually being removed? If so, the > > Histograms.json should probably be updated. > Good point. We are keeping them for the local newtab page, and we'll remove > them when we replace the local newtab page Oh duh. Yeah, don't remove them now as they're still used elsewhere. Rest of the comments make sense. r=Mardak

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f6375754b863ed00f65e77cd2d4a5d05d0809ac Bug 1210940 - New Browser Component: Newtab r=Mardak

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/57b33e5f3ef0 for bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=15465542&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=15465449&repo=mozilla-inbound

investigating

carrying over Mardak's r+. This fixes an intermittent failure on linux debug builds in browser chrome tests.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ea636dce7e60c06589b45d8737c0461d91b55b3 Bug 1210940 - New Browser Component: Newtab r=Mardak

Looks like this bug added "RemoteNewTabUtils.jsm", which is 95% identical to a preexisting file, NewTabUtils.jsm. For future reference, this addition should *really* have been done using "hg copy path/to/NewTabUtils.jsm path/to/RemoteNewTabUtils.jsm" Then "hg blame" would be preserved on the (vast majority of) lines in RemoteNewTabUtils.jsm which are really copies of older NewTabUtils.jsm code. As it stands, "hg blame" for every line in RemoteNewTabUtils.jsm points to this bug here, which isn't really helpful to trace the history of the code.

(I filed bug 1223451 [with patches] to address comment 24, BTW.)

Thank you!

(In reply to Daniel Holbert from comment #24) > Looks like this bug added "RemoteNewTabUtils.jsm", which is 95% identical to > a preexisting file, NewTabUtils.jsm. > > For future reference, this addition should *really* have been done using > "hg copy path/to/NewTabUtils.jsm path/to/RemoteNewTabUtils.jsm" Not only that, but it looks as if NewTabURL.jsm wasn't moved using hg move, so finding blame on older versions is trickier.