Closed
Bug 1298950
Opened 8 years ago
Closed 8 years ago
Support about:home overrides in chrome_url_overrides
Categories
(WebExtensions :: General, defect, P3)
WebExtensions
General
Tracking
(firefox51 wontfix, firefox52 wontfix, firefox53 wontfix, firefox54 fixed)
RESOLVED
FIXED
mozilla54
webextensions | + |
People
(Reporter: andy+bugzilla, Assigned: mattw)
References
(Blocks 1 open bug)
Details
(Whiteboard: [design-decision-approved][triaged])
Attachments
(1 file)
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.
Reporter | ||
Updated•8 years ago
|
Priority: -- → P3
Whiteboard: [design-decision-approved][triaged]
Reporter | ||
Updated•8 years ago
|
webextensions: --- → +
Comment 1•8 years ago
|
||
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.
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → mwein
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•8 years ago
|
||
mozreview-review |
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?
Assignee | ||
Comment 6•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 8•8 years ago
|
||
mozreview-review-reply |
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•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8836670 [details]
Bug 1298950 - Add support for overriding about:home to chrome_url_overrides
https://reviewboard.mozilla.org/r/112042/#review113450
Attachment #8836670 -
Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 13•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e88f2e7f0ee5
Add support for overriding about:home to chrome_url_overrides r=mixedpuppy
Keywords: checkin-needed
Comment 14•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•8 years ago
|
Attachment #8836670 -
Flags: review?(aswan)
Comment 15•8 years ago
|
||
No plan to have dot release for 51. Mark 51 won't fix.
status-firefox52:
--- → affected
Updated•8 years ago
|
status-firefox53:
--- → affected
Updated•8 years ago
|
Keywords: dev-doc-needed
Reporter | ||
Comment 17•8 years ago
|
||
(In reply to Julien Cristau [:jcristau] from comment #16)
> Is this worth uplifting to 53?
No.
Flags: needinfo?(mwein)
Updated•8 years ago
|
Comment 18•8 years ago
|
||
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.
Flags: needinfo?(mwein)
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(amckay)
Comment 19•8 years ago
|
||
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.
Flags: needinfo?(mixedpuppy)
Assignee | ||
Comment 20•8 years ago
|
||
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.
Flags: needinfo?(amckay)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(mwein)
Assignee | ||
Comment 21•8 years ago
|
||
s/extend/extent
Comment 22•8 years ago
|
||
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.
Flags: needinfo?(mwein)
Flags: needinfo?(mixedpuppy)
Assignee | ||
Comment 23•8 years ago
|
||
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.
Flags: needinfo?(mwein)
Assignee | ||
Comment 24•8 years ago
|
||
Shane, would you mind doing the honor of backing this one out?
Comment 25•8 years ago
|
||
It was backed out in bug 1344773
Depends on: 1344773
Flags: needinfo?(mixedpuppy)
Assignee | ||
Comment 26•8 years ago
|
||
Thanks
Updated•8 years ago
|
Keywords: dev-doc-needed
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•