We can't support the bookmarks or history values since we don't have an incontent bookmark or history page yet, but we can support the newtab one.

The newtab override is frequently used in chrome extensions.

I think you need to add the new files to the patch.

Forgot to hg add the test. About the test, in my opinion, just testing if aboutNewTabService.newTabURL has the correct value should be enough, since aboutNewTabService already has a test on it's own that checks it's behaviour.

...and forgot the ext-url-overrides.js file too. I really need some sleep :)

Comment on attachment 8702771 [details] [diff] [review] Patch v1.2 Review of attachment 8702771 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks. I just want to see the new version. ::: browser/components/extensions/ext-url-overrides.js @@ +11,5 @@ > + > +extensions.on("manifest_chrome_url_overrides", (type, directive, extension, manifest) => { > + let newTabOverride = manifest.chrome_url_overrides.newtab; > + if (newTabOverride) { > + let url = extension.baseURI.resolve(newTabOverride); Does Chrome allow remote new tab pages (i.e., not part of the extension)? If not, we should check extension.isExtensionURL. @@ +12,5 @@ > +extensions.on("manifest_chrome_url_overrides", (type, directive, extension, manifest) => { > + let newTabOverride = manifest.chrome_url_overrides.newtab; > + if (newTabOverride) { > + let url = extension.baseURI.resolve(newTabOverride); > + aboutNewTabService.newTabURL = url; I think we should save the previous new tab URL so that we can restore it on shutdown. ::: browser/components/extensions/test/browser/browser_ext_url_overrides.js @@ +2,5 @@ > +/* vim: set sts=2 sw=2 et tw=80: */ > +"use strict"; > + > +/* globals content */ > +/* eslint-disable mozilla/no-cpows-in-tests */ Is this stuff needed? I don't see |content| being used.

Not my code, but I have answers. > Does Chrome allow remote new tab pages (i.e., not part of the extension)? If not, we should check extension.isExtensionURL. No, it doesn't. And FYI the file names are relative to the path in the extension, so putting "foo.html" will be a file at the root of the extension package. > I think we should save the previous new tab URL so that we can restore it on shutdown. Do you mean shutdown of the extension or shutdown of the browser? For extension shutdown, it should restore the old URL. For Firefox shutdown, it doesn't matter since new tab changes aren't preserved across browser invocations.

(In reply to Bill McCloskey (:billm) from comment #6) > I think we should save the previous new tab URL so that we can restore it on > shutdown. I agree with this, but I also think we need UX input before we decide on the behavior here... > ::: browser/components/extensions/test/browser/browser_ext_url_overrides.js > @@ +2,5 @@ > > +/* vim: set sts=2 sw=2 et tw=80: */ > > +"use strict"; > > + > > +/* globals content */ > > +/* eslint-disable mozilla/no-cpows-in-tests */ > > Is this stuff needed? I don't see |content| being used. We shouldn't be disabling the no-cpows-in-tests rule in new tests, anyway. I'm assuming you copied this from the context menu test, and the only reason I added it there was to avoid rewriting the test. If you need access to content pages in new tests, please use ContentTask: https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/ContentTask.jsm

(In reply to Bill McCloskey (:billm) from comment #6) > Comment on attachment 8702771 [details] [diff] [review] > Patch v1.2 > > Review of attachment 8702771 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, thanks. I just want to see the new version. > > ::: browser/components/extensions/ext-url-overrides.js > @@ +11,5 @@ > > + > > +extensions.on("manifest_chrome_url_overrides", (type, directive, extension, manifest) => { > > + let newTabOverride = manifest.chrome_url_overrides.newtab; > > + if (newTabOverride) { > > + let url = extension.baseURI.resolve(newTabOverride); > > Does Chrome allow remote new tab pages (i.e., not part of the extension)? If > not, we should check extension.isExtensionURL. I'm not sure, but I think :mkaply is right. > ::: browser/components/extensions/test/browser/browser_ext_url_overrides.js > @@ +2,5 @@ > > +/* vim: set sts=2 sw=2 et tw=80: */ > > +"use strict"; > > + > > +/* globals content */ > > +/* eslint-disable mozilla/no-cpows-in-tests */ > > Is this stuff needed? I don't see |content| being used. Yeah, just a copy paste leftover. (In reply to Mike Kaply [:mkaply] from comment #7) > > I think we should save the previous new tab URL so that we can restore it on shutdown. > > Do you mean shutdown of the extension or shutdown of the browser? For > extension shutdown, it should restore the old URL. For Firefox shutdown, it > doesn't matter since new tab changes aren't preserved across browser > invocations. From my testing, it seems the aboutNewTabService does persist the URL across restarts. (In reply to Kris Maglione [:kmag] from comment #8) > (In reply to Bill McCloskey (:billm) from comment #6) > > I think we should save the previous new tab URL so that we can restore it on > > shutdown. True, this is quite tricky in some cases, if you follow these steps: - Run a build of Firefox with no add-ons - Install an add-on that overrides the NTP - Install another one that overrides the NTP - Disable one of them What should we do ? and if we disable both ? I guess we'd have to maintain a history of enabled NTP ? Not to mention non-webextensions addons that override the NTP. > I agree with this, but I also think we need UX input before we decide on the > behavior here... There are a bunch of cases to handle here, especially with users using multiple add-ons with that override the NTP.

> I'm not sure, but I think :mkaply is right. I tested this which is where my answer came from. > From my testing, it seems the aboutNewTabService does persist the URL across restarts. That's odd. The implementation only sets it as an internal variable. http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#2534 > What should we do ? and if we disable both ? > I guess we'd have to maintain a history of enabled NTP ? > Not to mention non-webextensions addons that override the NTP. Here's what I've observed about the overriding of new tab pages for the chrome,. In Chrome, it seems to be the most recently installed or enabled add-on that takes the new tab page gets it. It seems to maintain some sort of queue of when add-ons are installed/enabled when so if you you have four add-ons that take the new tab page, as you disable them, they pass control to the next add-on in the list. As you point out, putting other add-ons into this equation makes things even more complicated. Unfortunately the new about new tab service does nothing to try to solve this problem, but that's probably where this belongs. Maybe the about new tab service should be smarter and require extensions to pass an extension ID when they set the new tab page and the new tab service can listen for uninstall/disabling events and maintain the queue. > There are a bunch of cases to handle here, especially with users using multiple add-ons with that override the NTP. This is not a new problem. As it stands, we rely on add-ons to do the right thing and yield the new tab when another add-on requests it.

(In reply to Mike Kaply [:mkaply] from comment #10) > Unfortunately the new about new tab service does nothing to try to solve > this problem, but that's probably where this belongs. Maybe the about new > tab service should be smarter and require extensions to pass an extension ID > when they set the new tab page and the new tab service can listen for > uninstall/disabling events and maintain the queue. The interposition service is responsible for mapping API calls to add-on IDs. Having add-ons pass their own IDs tends not to work out well.

Chromes behavior described in comment 10 by :mkaply on how to handle multiple add-ons overriding new tabs page sounds like the right thing to do.

(In reply to Tim Nguyen :ntim from comment #0) > We can't support the ... bookmark or history page yet, but we can support the newtab one. about:home should also be replaceable. Chrome's home page is either chrome://newtab or a user-specified URL, so 'home' isn't in their list.

can we increase the priority of this bug? it's blocking the launch of one of our partners.

(In reply to Nancy Grossman from comment #13) > about:home should also be replaceable. Chrome's home page is either > chrome://newtab or a user-specified URL, so 'home' isn't in their list. I've split that off into a bug 1298950.

Based on the AMO policy around this kind of change, there would need to be some kind of explicit opt-in for the end user to allow it to be switched. This makes us incompatible with Chrome from the get go. Probably the best way to do this is by doing permissions in bug 1197420. An alternative is to add in a callback that the developer has to respond to, this will allow add-ons to implement the opt-in UI for users. Blog post on opt-in UI: https://blog.mozilla.org/addons/2016/07/15/writing-an-opt-in-ui-for-an-extension/

Distribution partnerships we have override homepage and new tab. These are firefox distributions downloaded directly from partner sites, and the addons are already included built-in. In this sense, an opt-in is not necessary. I have built a work-around addon for overriding newtab, it can be found here: https://github.com/mixedpuppy/newtaboverride

:andym Not quite understand the AMO policy. Does it require Firefox to implement an opt-in UI or it's just add-on developers' responsibility?

(In reply to Yen Chi Hsuan from comment #18) > :andym Not quite understand the AMO policy. Does it require Firefox to > implement an opt-in UI or it's just add-on developers' responsibility? The policy requires developers of *listed* add-ons to have an opt-in. For unlisted it's not so black or white. It would be very nice if the API came with an opt-in option, so developers can be compliant with less code.

(In reply to Jorge Villalobos [:jorgev] from comment #19) > The policy requires developers of *listed* add-ons to have an opt-in. For > unlisted it's not so black or white. It would be very nice if the API came > with an opt-in option, so developers can be compliant with less code. Well, the issue is that, as implemented in Chrome, it doesn't actually give developers a way to let users opt in or out. If the key is in the manifest, the pages are overridden. I don't think it makes sense to implement a WebExtension feature that's completely incompatible with our listed add-on policies, and at odds with our ecosystem guidelines.

Chrome enforces this via a single purpose policy (in theory), so if your add-on takes over the new tab, that's all it does and it really shouldn't need opt-in. It's only function is to take over the new tab. And the optout is to uninstall the addon. And they tell you which addon is taking over the new tab page and make it easy to uninstall that addon. If someone created an addon called "Super Cool new tab page", would it really need new tab optin on AMO?

(In reply to Mike Kaply [:mkaply] from comment #21) > If someone created an addon called "Super Cool new tab page", would it > really need new tab optin on AMO? Our policy doesn't require it in that case. I would be in favor of the API providing the option to show an opt-in, though I wouldn't mind requiring it for all cases. It wouldn't be too different from permissions, where the purpose of the add-on wouldn't factor into whether the permission prompt is shown or not.

(In reply to Jorge Villalobos [:jorgev] from comment #22) > (In reply to Mike Kaply [:mkaply] from comment #21) > > If someone created an addon called "Super Cool new tab page", would it > > really need new tab optin on AMO? > > Our policy doesn't require it in that case. I would be in favor of the API > providing the option to show an opt-in, though I wouldn't mind requiring it > for all cases. It wouldn't be too different from permissions, where the > purpose of the add-on wouldn't factor into whether the permission prompt is > shown or not. I do appreciate the permissions dialog on google play, and I think many users are used to something like that. If we have a good design/ux for this I'd be comfortable with always prompting. Lacking that, if a listed addon, and I'll use a shopping assistant as an example, states in the listing that it implements a new newTab page that incorporates information from your favorite shopping sites, and has a button that can be used to get price comparisons, it seems to me that a user installing it is expecting those features. IOW, explicit and transparent features in an addon listing, when those are the major features, shouldn't necessarily need an additional level of opt-in to enable the feature.

Matt, the new patch addresses Bill's comments in comment 6. The case where multiple overrides are installed is now also handled as comment 10 describes.

Thanks for the fix ntim, I'd like to get a nod from Kev on our policy for this change before we land it to address concerns about this kind of change. The card, for reference in Trello is https://trello.com/c/rNPRypjI/76-privileged-apis-and-policy

Note that this patch only adds support for new tab overrides.

I think there are some larger questions that need to be addressed before we can land this - This bug is marked as dependent on permissions (it was actually dependent on the wrong bug but I've updated it). At the very least, I think we should have user permissions prompts in place before we add this. - Chrome also displays an extra notification the first you visit an overridden page to make it clear that an extension modified the page and giving you the opportunity to immediately uninstall the extension (without having to manually navigate to the add-ons manager etc) if you don't like the changes. I think we should consider something like that as well. - We should make a conscious decision about the manifest key. Using "chrome" would of course promote compatibility and we happen to use the word chrome in a way that's consistent with its use here so perhaps this is okay. I don't think the browserext w3c group has considered this topic yet but if they standardize this with a name other than "chrome_url_overrides" we should follow the proposed standard, perhaps with the chrome version available for compatibility.

Comment on attachment 8809346 [details] Bug 1234150 - Support chrome_url_overrides field in WebExtension manifests. https://reviewboard.mozilla.org/r/91966/#review91998 r- mostly for the other issues raised directly in the bug. When we're ready to land this though, the new properties should also be added to the manifest schema so they can be validated. ::: browser/components/extensions/ext-url-overrides.js:28 (Diff revision 4) > + > + if (!extension.isExtensionURL(newtabOverride)) { > + return; > + } > + > + previousOverrides.newtab.push(aboutNewTabService.newTabURL); Using a stack here isn't going to work, it assumes a last-in-first-out ordering of extension loads/unloads, but that's not a valid assumption.

I am a terrible person who hasn't responded to this, despite opening the bug a thousand times (ish). Given that we don't prompt on NewTab overrides currently, and that it is a feature a number of people are looking at, I think we can accept the feature before the UX piece is done. That is contingent, however, on a couple commitments: 1. We'll need a general mechanism for ensuring users are aware of, and in control of, changes that impact user experience. This needs to be scoped with UX, and tracked as a separate project with the goal of having it in place sooner rather than later. It impacts changes like new tab, search, settings overrides, and other preferences, and is not limited to add-ons changes. It should be spun out, with the newtab override covering all cases if possible. 2. Ensure the newtab team is aware of these types of changes (I've spoken with Product already, but it'd be good to get a proper proposal up. 3. Develop a proper best practices/policy write up on what behaviours add-ons that implement wrt new tab overrides (e.g. respecting changes, no surprises, etc.). (In reply to Andy McKay [:andym] from comment #29) > Thanks for the fix ntim, I'd like to get a nod from Kev on our policy for > this change before we land it to address concerns about this kind of change. > The card, for reference in Trello is > https://trello.com/c/rNPRypjI/76-privileged-apis-and-policy

Based on comment 33, let's continue with this.

It seems to me that the best solution would be to combine the "newest addition wins" behaviour with a drop-down "New Tab Page:" selector below the "Home Page:" block in Preferences > General, similar in concept to the action drop-downs in Preferences > Applications.

Does the proposed implementation cover the case that a user can set a arbitrary URL in the settings UI of a WebExtension to replace the new tab page with, let's say, google.com? It's definitively not enough to have a not changable site in the WebExtension manifest. Almost all of the users of New Tab Override (December's featured add-on, reached 99.000 users on one day in December) uses this add-on to set a arbitrary website or a local file as new tab page, not a page provided by the add-on itself.

The attached patch certainly doesn't. I have a similar use-case (to set the new tab page to an arbitrary URL) for New Tab Homepage (~115,000 average daily users). Currently achieving this via direct access to nsIAboutNewTabService. Functionality is also currently provided by the wrapper module NewTabURL.jsm. Nothing in this bug so far sounds like it's going to cover equivalent functionality. Should we file a new bug or will that be addressed here?

I'm sorry to hear that custom URL in new tabs functionality will not be included in this patch. I also maintain an add-on that offers this functionality ("Custom New Tab" 13k daily average users. I originally wrote it back in 2011!). What is the reasoning behind not providing this feature? Frankly, I cannot understand why this feature has not been implemented years ago.

You can allow the user to override the new tab page in an extension by simply setting the new tab page to a chrome URL in your extension and then having that chrome URL navigate to the correct page. (That's how you do it in the Chrome browser). As a side note, Chrome finally added a way to set a custom home page independent of an add-on. We really should be doing that in combination with this feature.

I believe the consensus is to land support for chrome_url_overrides for the newtab page in parallel with the UX piece described in comment 33. In that case, I think we should try to get this part landed in 53 if possible. Tim (ntim) is unavailable until the 16th, so for that reason I'll try to go ahead and finish this bug up for him this week.

I wonder if we should hit two birds with one patch...and add bug 1298950 at the same time. I'm inclined to do that.

(In reply to Shane Caraveo (:mixedpuppy) from comment #42) > I wonder if we should hit two birds with one patch...and add bug 1298950 at > the same time. I'm inclined to do that. On second thought...that may not be easily overrideable. I'm actually inclined to consider it if it's simple, otherwise not.

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review104418 ::: browser/components/extensions/ext-url-overrides.js:12 (Diff revision 1) > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > +XPCOMUtils.defineLazyServiceGetter(this, "aboutNewTabService", > + "@mozilla.org/browser/aboutnewtab-service;1", > + "nsIAboutNewTabService"); > + > +// TODO(mattw): Use the pref manager to handle precedence when is a bug filed for the followup? replace todo with Bug # ::: browser/components/extensions/ext-url-overrides.js:13 (Diff revision 1) > +XPCOMUtils.defineLazyServiceGetter(this, "aboutNewTabService", > + "@mozilla.org/browser/aboutnewtab-service;1", > + "nsIAboutNewTabService"); > + > +// TODO(mattw): Use the pref manager to handle precedence when > +// multiple overrides exist once bugzil.la/1320736 lands. expand on the comment a touch that we are purposly ignoring edge cases around multiple addons overriding newtab until we have precedence handling ::: browser/components/extensions/ext-url-overrides.js:17 (Diff revision 1) > +// TODO(mattw): Use the pref manager to handle precedence when > +// multiple overrides exist once bugzil.la/1320736 lands. > + > +/* eslint-disable mozilla/balanced-listeners */ > +extensions.on("manifest_chrome_url_overrides", (type, directive, extension, manifest) => { > + if (manifest.chrome_url_overrides.newtab) { What happens if "history" or "bookmarks" is in chrome_url_overrides (rather than newtab)? I'm curious from an error logging perspective, if it's going to be obvious to devs that we don't support those two values. ::: browser/components/extensions/test/browser/browser_ext_url_overrides.js:22 (Diff revision 1) > + "chrome_url_overrides": { > + newtab: NEWTAB_URI, > + }, > + }, > + files: { > + [NEWTAB_URI]: "This is my fancy custom new tab page", I'm like 99.something% sure that the page will have browser.* access, but it might be nice to test for it.

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review104494 Thanks for taking over ::: browser/components/extensions/ext-url-overrides.js:12 (Diff revision 1) > +// TODO(mattw): Use the pref manager to handle precedence when > +// multiple overrides exist once bugzil.la/1320736 lands. If you plan to take a look later, it should land in the same release, because we don't want to end up with a confusing/broken behaviour. Also, my mozreview patch was quite close from finished, the array that listed the new tab page just needed to include extension IDs, and instead of popping off the last item, we needed to remove the proper item. I was hoping someone could finish off that part for me. I don't think it should be too much work, but if it is, feel free to do this as a followup. ::: browser/components/extensions/ext-url-overrides.js:16 (Diff revision 1) > +extensions.on("manifest_chrome_url_overrides", (type, directive, extension, manifest) => { > + if (manifest.chrome_url_overrides.newtab) { > + let newtab = manifest.chrome_url_overrides.newtab; > + let url = extension.baseURI.resolve(newtab); > + aboutNewTabService.newTabURL = url; > + } > +}); > + > +extensions.on("shutdown", (type, extension) => { > + aboutNewTabService.newTabURL = "about:newtab"; > +}); This patch is basically the same patch as my first patch, in which case you should address billm's comments: https://bugzilla.mozilla.org/show_bug.cgi?id=1234150#c6

(In reply to Tim Nguyen :ntim (not accepting requests 7-16 Jan) from comment #45) > Comment on attachment 8825605 [details] > Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. > > https://reviewboard.mozilla.org/r/103720/#review104494 > > Thanks for taking over > > ::: browser/components/extensions/ext-url-overrides.js:12 > (Diff revision 1) > > +// TODO(mattw): Use the pref manager to handle precedence when > > +// multiple overrides exist once bugzil.la/1320736 lands. > > If you plan to take a look later, it should land in the same release, > because we don't want to end up with a confusing/broken behaviour. Handling shutdown better (just passing off to other addon if one is installed]) is enough until precedence handling is in, even if it's that lands a version or two later. More than one newtab handler will be fairly rare, lets not over think it at this stage. So the array handling in the prior patch is enough (given shutdown removes the right addon from the list). > This patch is basically the same patch as my first patch, in which case you > should address billm's comments: > https://bugzilla.mozilla.org/show_bug.cgi?id=1234150#c6 The part about making sure the url is not remote is a good item to add.

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review104716 This should get a look from billm or kmag. In particular, I'm unclear on what the limitations are on what URLs may be used here. If they must not be remote then we need to have a plan for out of process webextensions that want to override with an extension page. I'm also uneasy about landing this without any sort of permissions or indications to the user but I guess that's the directive from product management... ::: browser/components/extensions/ext-url-overrides.js:24 (Diff revision 1) > +extensions.on("shutdown", (type, extension) => { > + aboutNewTabService.newTabURL = "about:newtab"; > +}); This doesn't look right, this will reset the new tabl url any time any webextension is disabled

Andrew, "Out of process" web-extension addon-ons can only point the URL at an internal URI (a file that came packaged with the add-on). However, as someone mentioned in a previous comment (I cannot say claim this clever idea as my own) the add-on author can simply define a file with a javascript or html <meta> redirect. e.g. <meta http-equiv="refresh" content="0; url=http://www.mydomain.com/new-page.html"> So I'm not sure if there is any security in "restricting" what aboutNewTabService.newTabURL can be pointed at.

I may have mis-read billm's original comment, I thought there was concern about having a remote page with the e10s meaning of "remote".

I meant "remote" as in not over HTTP (as opposed to moz-extension).

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review105066 ::: browser/components/extensions/ext-url-overrides.js:17 (Diff revision 1) > +// TODO(mattw): Use the pref manager to handle precedence when > +// multiple overrides exist once bugzil.la/1320736 lands. > + > +/* eslint-disable mozilla/balanced-listeners */ > +extensions.on("manifest_chrome_url_overrides", (type, directive, extension, manifest) => { > + if (manifest.chrome_url_overrides.newtab) { I'll add them to the manifest and mark them as unsupported so the proper error messages will be logged. ::: browser/components/extensions/ext-url-overrides.js:24 (Diff revision 1) > +extensions.on("shutdown", (type, extension) => { > + aboutNewTabService.newTabURL = "about:newtab"; > +}); I don't think it really matters unless we plan to land this before we handle this using the precedence manager. Either way though I changed it so it handles precedence similar to how the omnibox api currently handles precedence.

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review105076 ::: browser/components/extensions/test/browser/browser_ext_url_overrides.js:22 (Diff revision 1) > + "chrome_url_overrides": { > + newtab: NEWTAB_URI, > + }, > + }, > + files: { > + [NEWTAB_URI]: "This is my fancy custom new tab page", What would the implications of this be? If it does have access, should it not?

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review105116 ::: browser/components/extensions/ext-url-overrides.js:44 (Diff revision 2) > + > +extensions.on("shutdown", (type, extension) => { > + if (overrides.newtab.length === 1) { > + // The last extension in line has been uninstalled, so > + // reset the newtab page back to its default. > + aboutNewTabService.newTabURL = "about:newtab"; Drive by comment, but why not aboutNewTabService.resetNewTabURL(); ?

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review105116 > Drive by comment, but why not aboutNewTabService.resetNewTabURL(); ? Thanks for the input - I actually didn't know about this method so I'll switch to using it here. Although, it looks like setting newTabURL to "about:newtab" will indirectly call resetNewTabURL() anyways. - http://searchfox.org/mozilla-central/source/browser/components/newtab/aboutNewTabService.js#242.

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review104418 > I'm like 99.something% sure that the page will have browser.* access, but it might be nice to test for it. What would the implications of this be? If it does have access, should it not?

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review106098 ::: browser/components/extensions/ext-url-overrides.js:41 (Diff revision 3) > + overrides.newtab.push({extension, url}); > + } > +}); > + > +extensions.on("shutdown", (type, extension) => { > + if (overrides.newtab.length === 1) { You should add an early return in the case where the extension is not in the list of overrides. Otherwise, it'll break for the following case: - Install an extension (call it Ext1) that doesn't override the new tab page - Install an extension (call it Ext2) that overrides the new tab page - Uninstall Ext1 Because overrides.newtab.length === 1 is true, the new tab page set by Ext2 gets reset, which is unexpected behaviour.

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review106098 > You should add an early return in the case where the extension is not in the list of overrides. > > Otherwise, it'll break for the following case: > > - Install an extension (call it Ext1) that doesn't override the new tab page > - Install an extension (call it Ext2) that overrides the new tab page > - Uninstall Ext1 > > Because overrides.newtab.length === 1 is true, the new tab page set by Ext2 gets reset, which is unexpected behaviour. Good catch, thanks!

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review106098 > Good catch, thanks! I fixed this and added tests for it in the latest revision.

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review104418 > What would the implications of this be? If it does have access, should it not? It should have access. A simple test like in the browserAction test would be sufficient. https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_browserAction_simple.js#14

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103718/#review107142 ::: browser/components/extensions/ext-url-overrides.js:22 (Diff revision 4) > +let overrides = { > + info: new WeakMap(), // Keeps track of the overrides an extension has. > + newtab: [], > +}; > + > +function updateNewtabPage(extension) { I think this could be simplified to just use the filter to remove the extension, then update aboutNewTabService based on what is left in overrides.newtab. ::: browser/components/extensions/ext-url-overrides.js:22 (Diff revision 4) > +let overrides = { > + info: new WeakMap(), // Keeps track of the overrides an extension has. > + newtab: [], > +}; > + > +function updateNewtabPage(extension) { The name initially made me think it was doing something else...maybe this could be called something like removeExtensionOverrides

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review107198 Sorry for the slow review... I realize this is a placeholder until bug 1330494 lands but I'd like to see some of the things mentioned below cleaned up first. ::: browser/components/extensions/ext-url-overrides.js:18 (Diff revision 4) > +// multiple addons modifying the same properties, and bug 1330494 has been filed > +// to track utilizing this manager for chrome_url_overrides. Until those land, > +// the edge cases surrounding multiple addons using chrome_url_overrides will > +// be ignored and precedence will be first come, first serve. > +let overrides = { > + info: new WeakMap(), // Keeps track of the overrides an extension has. Please make this comment more explicit. E.g., Maps instances of Extension to an object indicating the overrides it has registered. Actually, why not use a simple Set or Array as the value instead? And upon further reading, what's the point of this map? It seems completely redundant with the contents of the `newtab` array. ::: browser/components/extensions/ext-url-overrides.js:22 (Diff revision 4) > +let overrides = { > + info: new WeakMap(), // Keeps track of the overrides an extension has. > + newtab: [], > +}; > + > +function updateNewtabPage(extension) { I'm finding that putting this in a separate function is confusing -- some of the "cleanup when an extension is shut down" logic is below and some of it is here. Can we put it all in one place? (either inline this code into the shutdown handler below or move the code that cleans up `overrdes.info` into this function?) ::: browser/components/extensions/ext-url-overrides.js:26 (Diff revision 4) > + > +function updateNewtabPage(extension) { > + if (overrides.newtab.length === 1) { > + // The only extension overriding the newtab page has been uninstalled, so > + // reset the newtab page back to its default. > + aboutNewTabService.resetNewTabURL(); The entry in `overrides.newtab` should also be removed here. But having 3 different ways to update `overrides.newtab` seems like a recipe for bugs. How about doing the filter on line 36 unconditionally and then follow that with something like: ``` if (overrides.newtab.length == 0) { aboutNewTabService.resetNewTabURL(); } else { aboutNewTabService.newTabURL = overrides.newtab[0].url; } ``` ::: browser/components/extensions/ext-url-overrides.js:48 (Diff revision 4) > + if (!extension.isExtensionURL(url)) { > + return; > + } I thought the consensus from the bug was to allow anything here? If you do want this restriction, I think it would be better to use strictRelativeUrl in the schema so that we get a helpful error instead of silently ignoring. ::: browser/components/extensions/schemas/url_overrides.json:16 (Diff revision 4) > + "properties": { > + "newtab": { > + "type": "string", > + "format": "relativeUrl", > + "optional": true, > + "preprocess": "localize" Is this intentional? Different newtab pages for different locales? Why can't the new page just vary its behavior based on the locale? ::: browser/components/extensions/test/browser/browser_ext_url_overrides.js:32 (Diff revision 4) > + "chrome_url_overrides": { > + newtab: NEWTAB_URI_2, > + }, > + }, > + files: { > + [NEWTAB_URI_2]: "This is my fancy custom new tab page", The brackets are unneeded here. Actually the whole files entry is unneeded... Same with the rest of the extensions below.

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review107198 > Please make this comment more explicit. > E.g., Maps instances of Extension to an object indicating the overrides it has registered. > Actually, why not use a simple Set or Array as the value instead? > And upon further reading, what's the point of this map? It seems completely redundant with the contents of the `newtab` array. I was originally using it as a way to check which pages the extension has overriden to make the searching more efficient, but I figured out a way to do this without needed the Map anymore. > I'm finding that putting this in a separate function is confusing -- some of the "cleanup when an extension is shut down" logic is below and some of it is here. Can we put it all in one place? (either inline this code into the shutdown handler below or move the code that cleans up `overrdes.info` into this function?) Sounds good. With the simplicifications I made above I'm more comfortable now putting this all in one place. > The entry in `overrides.newtab` should also be removed here. > But having 3 different ways to update `overrides.newtab` seems like a recipe for bugs. How about doing the filter on line 36 unconditionally and then follow that with something like: > ``` > if (overrides.newtab.length == 0) { > aboutNewTabService.resetNewTabURL(); > } else { > aboutNewTabService.newTabURL = overrides.newtab[0].url; > } > ``` Sounds good. Shane also recommended this in his review and doing this makes it much simpler. > I thought the consensus from the bug was to allow anything here? If you do want this restriction, I think it would be better to use strictRelativeUrl in the schema so that we get a helpful error instead of silently ignoring. I think consensus is not to allow this, based on the comments in the bug (see comment 6 and comment 7). I think it makes sense to use ExtensionURL here then in the schema. > The brackets are unneeded here. > Actually the whole files entry is unneeded... > Same with the rest of the extensions below. Done.

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review107198 > Is this intentional? Different newtab pages for different locales? Why can't the new page just vary its behavior based on the locale? We preprocess all the urls for locale, lets stay consistent and preprocess this.

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review107260

Comment on attachment 8825605 [details] Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides. https://reviewboard.mozilla.org/r/103720/#review107598 ::: browser/components/extensions/ext-url-overrides.js:18 (Diff revision 7) > + // A queue of extensions in line to override the newtab page, with index 0 > + // currently being the front of the line. nit: I think this could be a little clearer if it just said that the array is ordered from oldest to newest.

Autoland can't push this until all pending issues are marked as resolved in MozReview.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/485014423bd9 Support overriding "about:newtab" using chrome_url_overrides. r=aswan,mixedpuppy

Hi guys. I have a small note. In newtab I am showing different kind of pages (depending on user beeing logged in to our app or not). Anyway, I also have extension-side settings to disable the overriding. chrome_url_overrides->newtab can't be programatically unset. So in my extension it is configured for tiny "newtab.html" that basically makes decision which page to load or to load the default newtab. In some cases I'd like to point user back to default newtab - in Firefox it doesn't work: > Unchecked lastError value: Error: Illegal URL: about:newtab I guess it's a bug - whats the point of preventing "about:newtab" to be loaded? I tried to load it via: - window.location.href = 'about:newtab', or - chrome.tabs.update(tab.id, {url: 'about:newtab'}); Also - redirecting such way to "about:home" works ;-) So it's just matter of "about:newtab" url.

In theory you could route to: chrome://browser/content/newtab/newTab.xhtml instead. But we would probably need to add code to hide that URL from the user. A change was made a while ago to make about:newtab route to the current new tab page NOT the internal new tab page. So what you're creating is a loop. Maybe with this change, we should go back to making about:newtab mean "the firefox new tab page" That's a big complaint with Chrome that there is no way to route the user to the chrome internal new tab page if you need to.

Thaks for response - but FYI: > Unchecked lastError value: Error: Illegal URL: chrome://browser/content/newtab/newTab.xhtml I would be totally fine if this would work ;)

Wojchiech, could you file a follow-up bug to fix this?

(In reply to Mike Kaply [:mkaply] from comment #77) > In theory you could route to: > > chrome://browser/content/newtab/newTab.xhtml > > instead. But we would probably need to add code to hide that URL from the > user. > > A change was made a while ago to make about:newtab route to the current new > tab page NOT the internal new tab page. So what you're creating is a loop. > > Maybe with this change, we should go back to making about:newtab mean "the > firefox new tab page" > > That's a big complaint with Chrome that there is no way to route the user to > the chrome internal new tab page if you need to. Mike, In Momentum, we've been allowing users to get to the default Chrome New Tab using a link and keyboard shortcut by redirecting to "chrome-search://local-ntp/local-ntp.html". It's not officially documented anywhere in their documentation, but has been working for a few years. It would be nice if Firefox could explicitly support navigating to the default (Firefox) new tab using something like "about:firefoxnewtab" or "about:defaultnewtab", while keeping "about:newtab" navigating to the currently active new tab. P.S. We're excited to get Momentum launched on Firefox (we've been holding off for this newtab support in the manifest, so we can keep our required permissions as minimal as possible), so thanks to all for the hard work on the WebExtensions support!

Please file new bugs for new functionality and not on this bug.

Hi Andy, I've created https://bugzilla.mozilla.org/show_bug.cgi?id=1335102 shortly after :aswan asked me to ;)

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/chrome_url_overrides Matt, please let me know if this covers it.

It looks great. I think it might be good idea to add a note for those who want to override the homepage, saying that they should use chrome_settings_overrides instead.

> I think it might be good idea to add a note for those who want to override the homepage, saying that they should use chrome_settings_overrides instead. Done, thanks!

Great that this is now implemented but had hoped it would hide the ugly "moz-extension" address in the address bar in the same way chrome does?

(In reply to Andrew Moff from comment #87) > Great that this is now implemented but had hoped it would hide the ugly > "moz-extension" address in the address bar in the same way chrome does? This is bug 1372996