Tim Nguyen :ntim Reporter
	Description • 9 years ago

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.

	Tim Nguyen :ntim Reporter
	Comment 1 • 9 years ago

The newtab override is frequently used in chrome extensions.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 3 • 9 years ago

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

	Tim Nguyen :ntim Reporter
	Comment 4 • 9 years ago

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.

	Tim Nguyen :ntim Reporter
	Comment 5 • 9 years ago

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

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 6 • 9 years ago

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.

	Mike Kaply [:mkaply]
	Comment 7 • 9 years ago

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.

	Kris Maglione [:kmag]
	Comment 8 • 9 years ago

(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

	Tim Nguyen :ntim Reporter
	Comment 9 • 9 years ago

(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.

	Mike Kaply [:mkaply]
	Comment 10 • 9 years ago

> 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.

	Kris Maglione [:kmag]
	Comment 11 • 9 years ago

(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.

	Markus Jaritz [:designakt] (UX)
	Comment 12 • 9 years ago

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.

	Nancy Grossman
	Comment 13 • 8 years ago

(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.

	Carter Trout
	Comment 14 • 8 years ago

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

	Andy McKay
	Comment 15 • 8 years ago

(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.

	Andy McKay
	Comment 16 • 8 years ago

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/

	Shane Caraveo (:mixedpuppy)
	Comment 17 • 8 years ago

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

	Chih-Hsuan Yen [:yan12125] (UTC+8)
	Comment 18 • 8 years ago

: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?

	Jorge Villalobos [:jorgev] (he/him)
	Comment 19 • 8 years ago

(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.

	Kris Maglione [:kmag]
	Comment 20 • 8 years ago

(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.

	Mike Kaply [:mkaply]
	Comment 21 • 8 years ago

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?

	Jorge Villalobos [:jorgev] (he/him)
	Comment 22 • 8 years ago

(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.

	Shane Caraveo (:mixedpuppy)
	Comment 23 • 8 years ago

(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.

	Tim Nguyen :ntim Reporter
	Comment 26 • 8 years ago

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.

	Andy McKay
	Comment 29 • 8 years ago

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

	Tim Nguyen :ntim Reporter
	Comment 30 • 8 years ago

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

	Andrew Swan [:aswan]
	Comment 31 • 8 years ago

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.

	Andrew Swan [:aswan]
	Comment 32 • 8 years ago
mozreview-review

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.

	Kev Needham [:kev]
	Comment 33 • 8 years ago

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

	Andy McKay
	Comment 34 • 8 years ago

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

	Stephan Sokolow
	Comment 35 • 8 years ago

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.

	Sören Hentzschel
	Comment 36 • 8 years ago

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.

	Ben Basson
	Comment 37 • 8 years ago

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?

	Ed
	Comment 38 • 8 years ago

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.

	Mike Kaply [:mkaply]
	Comment 39 • 8 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.

	Matthew Wein [:mattw] Assignee
	Comment 40 • 8 years ago

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.

	Shane Caraveo (:mixedpuppy)
	Comment 42 • 8 years ago

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.

	Shane Caraveo (:mixedpuppy)
	Comment 43 • 8 years ago

(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.

	Shane Caraveo (:mixedpuppy)
	Comment 44 • 8 years ago
mozreview-review

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.

	Tim Nguyen :ntim Reporter
	Comment 45 • 8 years ago
mozreview-review

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

	Shane Caraveo (:mixedpuppy)
	Comment 46 • 8 years ago

(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.

	Andrew Swan [:aswan]
	Comment 47 • 8 years ago
mozreview-review

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

	Ed
	Comment 48 • 8 years ago

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.

	Andrew Swan [:aswan]
	Comment 49 • 8 years ago

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".

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 50 • 8 years ago

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

	Matthew Wein [:mattw] Assignee
	Comment 52 • 8 years ago
mozreview-review

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.

	Matthew Wein [:mattw] Assignee
	Comment 53 • 8 years ago
mozreview-review

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?

	Philipp Kewisch [:Fallen] [:📆][:🧩]
	Comment 54 • 8 years ago
mozreview-review

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(); ?

	Matthew Wein [:mattw] Assignee
	Comment 55 • 8 years ago
mozreview-review-reply

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.

	Matthew Wein [:mattw] Assignee
	Comment 56 • 8 years ago
mozreview-review-reply

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?

	Tim Nguyen :ntim Reporter
	Comment 58 • 8 years ago
mozreview-review

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.

	Matthew Wein [:mattw] Assignee
	Comment 59 • 8 years ago
mozreview-review-reply

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!

	Matthew Wein [:mattw] Assignee
	Comment 60 • 8 years ago
mozreview-review-reply

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.

	Shane Caraveo (:mixedpuppy)
	Comment 62 • 8 years ago
mozreview-review-reply

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

	Shane Caraveo (:mixedpuppy)
	Comment 63 • 8 years ago
mozreview-review

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

	Andrew Swan [:aswan]
	Comment 64 • 8 years ago
mozreview-review

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.

	Matthew Wein [:mattw] Assignee
	Comment 65 • 8 years ago
mozreview-review-reply

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.

	Shane Caraveo (:mixedpuppy)
	Comment 69 • 8 years ago
mozreview-review-reply

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.

	Shane Caraveo (:mixedpuppy)
	Comment 70 • 8 years ago
mozreview-review

Comment on attachment 8825605 [details]
Bug 1234150 - Support overriding "about:newtab" using chrome_url_overrides.

https://reviewboard.mozilla.org/r/103720/#review107260

	Andrew Swan [:aswan]
	Comment 71 • 8 years ago
mozreview-review

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 73 • 8 years ago

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

	Pulsebot
	Comment 74 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 75 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/485014423bd9

	Wojciech Walek [:wojwal]
	Comment 76 • 8 years ago

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.

	Mike Kaply [:mkaply]
	Comment 77 • 8 years ago

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.

	Wojciech Walek [:wojwal]
	Comment 78 • 8 years ago

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 ;)

	Andrew Swan [:aswan]
	Comment 79 • 8 years ago

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

	Jason Waterman
	Comment 80 • 8 years ago

(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!

	Andy McKay
	Comment 81 • 8 years ago

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

	Wojciech Walek [:wojwal]
	Comment 82 • 8 years ago

Hi Andy, 

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

	Will Bamberg [:wbamberg]
	Comment 83 • 7 years ago

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/chrome_url_overrides

Matt, please let me know if this covers it.

	Matthew Wein [:mattw] Assignee
	Comment 84 • 7 years ago

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.

	Will Bamberg [:wbamberg]
	Comment 85 • 7 years ago

> 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!

	Andrew Moff
	Comment 87 • 7 years ago

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?

	Mike Kaply [:mkaply]
	Comment 88 • 7 years ago

(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