Closed Bug 1440069 Opened 6 years ago Closed 6 years ago

"headerURL" set to "" should be treated as no headerURL

Categories

(WebExtensions :: Themes, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: ntim, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1404688 didn't fix existing add-ons, because a lot of them use "headerURL": "" to workaround the fact that headerURL was mandatory. Unfortunately, "headerURL": "" computes to "headerURL": "moz-extension://.../" rather than "no headerURL".
Comment on attachment 8952853 [details]
Bug 1440069 - Don't try to resolve empty headerURLs.

https://reviewboard.mozilla.org/r/222082/#review228008


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/test/browser/browser_ext_themes_lwtsupport.js:76
(Diff revision 1)
> +add_task(async function test_LWT_image_attribute_with_empty_headerURL() {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "theme": {
> +        "images": {
> +          "headerURL": ""

Error: Missing trailing comma. [eslint: comma-dangle]

::: toolkit/components/extensions/test/browser/browser_ext_themes_lwtsupport.js:101
(Diff revision 1)
> +add_task(async function test_LWT_image_attribute_with_empty_theme_frame() {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "theme": {
> +        "images": {
> +          "theme_frame": ""

Error: Missing trailing comma. [eslint: comma-dangle]
Comment on attachment 8952853 [details]
Bug 1440069 - Don't try to resolve empty headerURLs.

https://reviewboard.mozilla.org/r/222082/#review228066

I don't understand this, all those properties are optional, why would we want to change the meaning of empty string in this way?
(In reply to Andrew Swan [:aswan] from comment #3)
> Comment on attachment 8952853 [details]
> Bug 1440069 - Don't try to resolve empty headerURLs.
> 
> https://reviewboard.mozilla.org/r/222082/#review228066
> 
> I don't understand this, all those properties are optional, why would we
> want to change the meaning of empty string in this way?

headerURL was mandatory before bug 1404688 (it was enforced in ext-theme.js), so many themes worked around that by using an empty headerURL.
So what's the plan here then?  That we support this quirk indefinitely?
(In reply to Andrew Swan [:aswan] from comment #5)
> So what's the plan here then?  That we support this quirk indefinitely?

So we can either:
- only have bug 1404688 fixed for new post-Firefox 60 themes that don't have headerURL
- or have bug 1404688 fixed for some a big subset of existing WE themes (that set headerURL to "")

So this patch is mostly for backwards compatibility...

If we go with the first path, existing themes would need to remove headerURL: "" to get bug 1404688 fixed, but then their themes wouldn't work with older versions of Firefox which require headerURL. I guess that isn't too much of a problem though, since Firefox 60 is an ESR release, this wouldn't be something super important.

I don't mind either way, but this patch does look simple enough IMO.
(In reply to Tim Nguyen :ntim from comment #6)
> I don't mind either way, but this patch does look simple enough IMO.

Well it adds some code, tests, etc. that if we land, we'll have to support indefinitely.  I agree that this one is simple but if we make this a habit, we end up with death by a thousand cuts.

Given how new themes are, lets go with the first option.  And to be explicit about it: I think we've been relatively loose in what we land for themes given that they were new and somewhat experimental, but we've reached a point where we need to be more careful.  In particular, new features need careful thought about their design since we are obligated to support them for a long time.  If we're really not sure exactly how a new feature should work, it needs to be clearly marked experimental so we can make breaking changes later rather than supporting two different ways to do the same thing because we changed our minds along the way.
Sounds good then.
Assignee: ntim.bugs → nobody
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Attachment #8952853 - Flags: review?(aswan)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: