Closed Bug 1489930 Opened 1 year ago Closed 1 year ago

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

Categories

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

defect
Not set

Tracking

(thunderbird64 fixed, thunderbird65 fixed)

RESOLVED FIXED
Thunderbird 65.0
Tracking Status
thunderbird64 --- fixed
thunderbird65 --- fixed

People

(Reporter: darktrojan, Assigned: darktrojan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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...
Attached patch 1489930-legacy-options-1.diff (obsolete) β€” β€” Splinter Review
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.
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #9007663 - Flags: review?(philipp)
I could be persuaded to instead turn legacy into an object with keys optionsType and optionsURL.
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.
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).
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!
Attached patch 1489930-legacy-options-2.diff (obsolete) β€” β€” Splinter Review
I made it tidier as well as changing the manifest schema.
Attachment #9007663 - Attachment is obsolete: true
Attachment #9007663 - Flags: review?(philipp)
Attachment #9010883 - Flags: review?(philipp)
Attached patch 1489930-legacy-options-3.diff (obsolete) β€” β€” Splinter Review
Forgot to make the legacy key optional. :-/
Attachment #9010883 - Attachment is obsolete: true
Attachment #9010883 - Flags: review?(philipp)
Attachment #9010886 - Flags: review?(philipp)
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?
Attachment #9010886 - Flags: review?(philipp) → review-
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
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.
Attachment #9010886 - Attachment is obsolete: true
Attachment #9013196 - Flags: review?(philipp)
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.
Attachment #9013196 - Flags: review?(philipp) → review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e0208c4fc4e2
Allow legacy extensions to specify a chrome URL as options page. r=philipp
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
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?
That's correct.
Unless we backport this to TB 64 beta.
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.
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/dc586f96bb1c
Fix debug bustage caused by broken schema; rs=bustage-fix
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.
Sure. Instead of "legacy": true, put:

"legacy": {
  "options": {
    "page": "chrome://CompactHeader/content/preferences.xul",
    "open_in_tab": true
  }
}
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
>   }
> }
Expected? Yes. Wanted? Sadly, no.
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.
Attachment #9013196 - Flags: approval-comm-beta?
If this does go to beta, don't forget there's a follow-up in comment 17.
Yes, the one that fixed an entire hill of orange trees ;-)
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.
Attachment #9013196 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.