1298950 - Support about:home overrides in chrome_url_overrides

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[Andy McKay]

This is a split off of bug 1234150 comment 13. That bug is about allowing developers to override about:newtab. In that comment is was suggested we also allow the override about:home.

Comment 1

[Stephan Sokolow]

Does Firefox have a provision for editable dropdown widgets in preferences?

I've always felt that the "Restore Default" button in General > Preferences was a bit of an inelegant solution and, with a controlled means for handling overrides, it seems like the most elegant solution would be to remove the "Restore Default" button and replace the text field with an editable "pick an available home page or type/paste your own" dropdown field.

Comment 2

[Matthew Wein [:mattw]]

Attachment: Bug 1298950 - Add support for overriding about:home to chrome_url_overrides

Review commit: https://reviewboard.mozilla.org/r/112042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112042/

Comment 3

[Matthew Wein [:mattw]]

Comment on attachment 8836670 [details]
Bug 1298950 - Add support for overriding about:home to chrome_url_overrides

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112042/diff/1-2/

Comment 4

[Matthew Wein [:mattw]]

Comment on attachment 8836670 [details]
Bug 1298950 - Add support for overriding about:home to chrome_url_overrides

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112042/diff/2-3/

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8836670 [details]
Bug 1298950 - Add support for overriding about:home to chrome_url_overrides

https://reviewboard.mozilla.org/r/112042/#review113374

::: browser/components/extensions/ext-url-overrides.js:19
(Diff revision 3)
>  // Bug 1320736 tracks creating a generic precedence manager for handling
>  // 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 = {

I think overrides should be a weakmap, or at least its members should be weakmaps.

::: browser/components/extensions/test/browser/browser_ext_url_overrides_home.js:12
(Diff revision 3)
> +                                  "resource://gre/modules/Preferences.jsm");
> +
> +const HOME_URI_1 = "webext-home-1.html";
> +const HOME_URI_2 = "webext-home-2.html";
> +const HOME_URI_3 = "webext-home-3.html";
> +

can you add some tests with both home and newtab?

Comment 6

[Matthew Wein [:mattw]]

Comment on attachment 8836670 [details]
Bug 1298950 - Add support for overriding about:home to chrome_url_overrides

https://reviewboard.mozilla.org/r/112042/#review113374

> I think overrides should be a weakmap, or at least its members should be weakmaps.

Could you explain to me how I would do this with WeakMaps? The reason I'm using queues is to maintain the order in which overrides are added so I can update them properly when extensions are uninstalled.

> can you add some tests with both home and newtab?

Yeah, we should definitely have a test for that :)

Comment 7

[Matthew Wein [:mattw]]

Comment on attachment 8836670 [details]
Bug 1298950 - Add support for overriding about:home to chrome_url_overrides

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112042/diff/3-4/

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8836670 [details]
Bug 1298950 - Add support for overriding about:home to chrome_url_overrides

https://reviewboard.mozilla.org/r/112042/#review113374

> Could you explain to me how I would do this with WeakMaps? The reason I'm using queues is to maintain the order in which overrides are added so I can update them properly when extensions are uninstalled.

sigh.  weakmap not map.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8836670 [details]
Bug 1298950 - Add support for overriding about:home to chrome_url_overrides

https://reviewboard.mozilla.org/r/112040/#review113448

::: browser/components/extensions/ext-url-overrides.js:87
(Diff revision 4)
>  
> -    overrides.newtab.push({extension, url});
> +  for (let page of Object.keys(overrides)) {
> +    if (manifest.chrome_url_overrides[page]) {
> +      let relativeURL = manifest.chrome_url_overrides[page];
> +      let url = extension.baseURI.resolve(relativeURL);
> +      overrides[page].push({extension, url});

Can we change this to {extension.id, url}, that way we at least do not hold a hard reference to an extension.

Comment 10

[Matthew Wein [:mattw]]

Comment on attachment 8836670 [details]
Bug 1298950 - Add support for overriding about:home to chrome_url_overrides

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112042/diff/4-5/

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8836670 [details]
Bug 1298950 - Add support for overriding about:home to chrome_url_overrides

https://reviewboard.mozilla.org/r/112042/#review113450

Comment 12

[Matthew Wein [:mattw]]

Comment on attachment 8836670 [details]
Bug 1298950 - Add support for overriding about:home to chrome_url_overrides

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112042/diff/5-6/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e88f2e7f0ee5
Add support for overriding about:home to chrome_url_overrides r=mixedpuppy

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e88f2e7f0ee5

Comment 15

[Gerry Chang [:gchang]]

No plan to have dot release for 51. Mark 51 won't fix.

Comment 16

[Julien Cristau [:jcristau]]

Is this worth uplifting to 53?

Comment 17

[Andy McKay]

(In reply to Julien Cristau [:jcristau] from comment #16)
> Is this worth uplifting to 53?

No.

Comment 18

[Tim Nguyen :ntim]

I wonder if it would be better to support this Chrome API instead ? 
https://developer.chrome.com/extensions/settings_override

Notably the "startup_pages" and the "homepage" parts. They seem more complete than the one added here.

Comment 19

[Shane Caraveo (:mixedpuppy)]

We've already got that started, Bug 1301315.  chrome_url_overrides allows for overriding pages that are not part of settings_override, so different purpose.

Comment 20

[Matthew Wein [:mattw]]

Essentially just reiterating what Shane said - they are both different APIs that we plan to support to the extend that they make sense in Firefox. Let us know if you have any other questions about this. I'm going to clear Andy's needinfo since I think Shane and/or I can hopefully take it from here.

Comment 21

[Matthew Wein [:mattw]]

s/extend/extent

Comment 22

[Tim Nguyen :ntim]

Shane, Matt, I don't understand how the "homepage" field is any different than this bug. The patch in this bug just sets browser.startup.homepage, which is basically the browser homepage setting.

So chrome_settings_override.homepage is essentially equivalent to this.

Comment 23

[Matthew Wein [:mattw]]

Okay, now I understand... When this landed, Shane and I weren't aware that Chrome had chrome_settings_overrides for the homepage, so we added support for it here. Bug 1341458 was filed right after this landed and moved support for overriding the homepage to chrome_settings_overrides. Now that that has landed, we need to back this bug out.

Comment 24

[Matthew Wein [:mattw]]

Shane, would you mind doing the honor of backing this one out?

Comment 25

[Shane Caraveo (:mixedpuppy)]

It was backed out in bug 1344773

Comment 26

[Matthew Wein [:mattw]]

Thanks