1489930 - No way to specify a chrome URL as legacy extension options page

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, defect)

Description

[Geoff Lankow (:darktrojan)]

From bug 1476259 comment 9:

> The extension itself works, but I can't find a way to open the options page.
> Is there a way to specify the options page in manifest.json if the options are
> still XUL-based? The manifest.json docs refer only to HTML options pages...

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1489930-legacy-options-1.diff

This allows a manifest to use the keys legacyOptionsType and legacyOptionsURL, which do the same as in install.rdf. Only options type 3, or not specifying it, is allowed.

Comment 2

[Geoff Lankow (:darktrojan)]

I could be persuaded to instead turn legacy into an object with keys optionsType and optionsURL.

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

Interesting, didn't know it would be that easy. I'd of course like to encourage add-on authors to use WebExtension options instead. Maybe it would be better to ask them to do this migration now, and then work on a port between the legacy part and the webextension, e.g. via browser.runtime.onMessage.addListener() similar to how embedded WebExtensions work.

If we are going with more legacy, I think turning legacy into an object would make sense. If optionsType 3 is the only accepted options type, then I'd go with just options_url (snake case for consistency). If we can make the real options_ui accept a chrome url and then take appropriate action, that would be even more smooth.

Comment 4

[Geoff Lankow (:darktrojan)]

Type 3 is an option, but not specifying a value is a different option (actually it used to be type 1 but nobody specified it).

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

Ok, then use { "open_in_tab": true } for optionsType == 3 and { "open_in_tab": false } for optionsType == 1 or unspecified. If there is no options_url, then don't show options. This again sounds just like the parameters available at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/options_ui so if you can make it re-use those instead it would be great!

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 1489930-legacy-options-2.diff

I made it tidier as well as changing the manifest schema.

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: 1489930-legacy-options-3.diff

Forgot to make the legacy key optional. :-/

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9010886 [details] [diff] [review]
1489930-legacy-options-3.diff

Review of attachment 9010886 [details] [diff] [review]:
-----------------------------------------------------------------

r- for now since this wasn't quite what I was looking for (see the last point), but happy to discuss!

::: mail/base/content/mailWindowOverlay.js
@@ +3747,5 @@
>    }
>  
>    // Enumerate all enabled addons with URL to XUL document with prefs.
>    let addonsFound = await AddonManager.getAddonsByTypes(["extension"]);
> +  addonsFound = addonsFound.map(addon => {

If you map then filter, you should probably reduce instead. If you map with such an elaborate body, you should probably loop and push instead :)

@@ +3771,5 @@
> +        optionsOpenInTab: legacy.options.open_in_tab,
> +      };
> +    }
> +    return null;
> +  }).filter(addon => addon);

If you do have a good reason to map+filter, then .filter(Boolean) is supposedly faster.

::: mail/components/extensions/schemas/legacy.json
@@ +13,5 @@
> +                "optional": true
> +              },
> +              {
> +                "type": "object",
> +                "properties": {

Sorry I was unclear about this. I'm wondering if we can re-use the real options_ui manifest key, instead of creating a copy of it in the legacy namespace?

Comment 9

[Geoff Lankow (:darktrojan)]

We can't use a chrome URL there, it's forbidden:

> Loading extension '…': Reading manifest: Error processing options_ui.page: SyntaxError: String "…" must be a relative URL

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 1489930-legacy-options-4.diff

I clearly didn't test that previous patch properly, found two bugs. Because the preferred way doesn't work, this may be the next best option.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9013196 [details] [diff] [review]
1489930-legacy-options-4.diff

Review of attachment 9013196 [details] [diff] [review]:
-----------------------------------------------------------------

Code looks ok to me, sorry for the delay.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e0208c4fc4e2
Allow legacy extensions to specify a chrome URL as options page. r=philipp

Comment 13

[Martin Pecka]

Thanks, Jorg ;)

Just a question - if I convert my addon to work since TB 63, but this patch is targeted at 65, does it mean that in 63 and 64 the addon settings will not be available at all?

Comment 14

[Geoff Lankow (:darktrojan)]

That's correct.

Comment 15

[Jorg K (CEST = GMT+2)]

Unless we backport this to TB 64 beta.

Comment 16

[Jorg K (CEST = GMT+2)]

That caused quite a bit of bustage :-( (as discussed on IRC): 24 failing tests from toolkit/components/extensions/test/xpcshell/ and 66 failing tests from toolkit/mozapps/extensions/test/xpcshell/ and more. I'll spare you the details.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dc586f96bb1c
Fix debug bustage caused by broken schema; rs=bustage-fix

Comment 18

[Joachim Herb]

Could you please provide an example addon code of how to use this? What is syntax of adding this to manifest.json?

In my install.rdf I have:
    <em:optionsURL>chrome://CompactHeader/content/preferences.xul</em:optionsURL>

I tried in manifest.json:
  "optionsType ": 3, (and also 1)
  "optionsURL": "chrome://CompactHeader/content/preferences.xul",


but there is no options button in the addon manager and the menu at Tools->Addon-Options stays emtpy.

Comment 19

[Geoff Lankow (:darktrojan)]

Sure. Instead of "legacy": true, put:

"legacy": {
  "options": {
    "page": "chrome://CompactHeader/content/preferences.xul",
    "open_in_tab": true
  }
}

Comment 20

[Joachim Herb]

Thank you for your prompt answer.

But sorry, another question: If I add this to manifest.json it looks like the addon cannot longer be installed with Thunderbird  64.0. Is this an expected behaviour?


(In reply to Geoff Lankow (:darktrojan) from comment #19)
> Sure. Instead of "legacy": true, put:
> 
> "legacy": {
>   "options": {
>     "page": "chrome://CompactHeader/content/preferences.xul",
>     "open_in_tab": true
>   }
> }

Comment 21

[Geoff Lankow (:darktrojan)]

Expected? Yes. Wanted? Sadly, no.

Comment 22

[Geoff Lankow (:darktrojan)]

Comment on attachment 9013196 [details] [diff] [review]
1489930-legacy-options-4.diff

We could just take the schema changes here to prevent add-ons breaking until 65 comes along. Or everything.

Comment 23

[Geoff Lankow (:darktrojan)]

If this does go to beta, don't forget there's a follow-up in comment 17.

Comment 24

[Jorg K (CEST = GMT+2)]

Yes, the one that fixed an entire hill of orange trees ;-)

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 9013196 [details] [diff] [review]
1489930-legacy-options-4.diff

Hopefully we can do TB 64 beta 4 towards the end of November and find some more content.

Comment 26

[Jorg K (CEST = GMT+2)]

Beta (TB 64 beta 4):
https://hg.mozilla.org/releases/comm-beta/rev/f39e51011ff40923019532de40e1422d4af3b04a
https://hg.mozilla.org/releases/comm-beta/rev/50ff20d2e44b8f64a9521671f2f114409a8bc19d