Closed Bug 1517162 Opened 2 years ago Closed 2 years ago

Provide a messenger.* global as an alias

Categories

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

enhancement
Not set
normal

Tracking

(thunderbird_esr68 fixed, thunderbird70 fixed)

RESOLVED FIXED
Thunderbird 70.0
Tracking Status
thunderbird_esr68 --- fixed
thunderbird70 --- fixed

People

(Reporter: Fallen, Assigned: Fallen)

Details

Attachments

(1 file, 2 obsolete files)

There is chrome.*, browser.*, I think it would make sense for us to have a mail.* global for our APIs, or something similar.

I recall that there was a way to make APIs only work on some of the globals, i.e. there was a Firefox API that worked on browser.* but not on chrome.*. This may have been the menus API.

We could consider allowing all our mail specific APIs on mail.* and anything that is shared with Firefox we can leave on browser.*/chrome.*.
Attached patch Fix - v1 (obsolete) β€” β€” Splinter Review

Here is the simple version where we just provide an alias. Open questions:

  • Should we name this mail ? If we have calendar or chat APIs this would be confusing and I am not sure we should be adding more aliases. I guess we could go with mail.calendar.* and mail.chat.* for those.
  • Should we be restricting access to mail-specific APIs so they can only go through mail.*? I think I know how to do this.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #9060029 - Flags: review?(geoff)
Attached patch Fix - v2 (obsolete) β€” β€” Splinter Review

And now with a better understanding of what the unprivileged path does, including a test for it.

Attachment #9060029 - Attachment is obsolete: true
Attachment #9060029 - Flags: review?(geoff)
Attachment #9060061 - Flags: review?(geoff)
Comment on attachment 9060061 [details] [diff] [review]
Fix - v2

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

I think this is okay from a code point of view, but I'd like to have someone more familiar with the toolkit code look over it.

My other concern is that if somehow the monkey-patching is broken and we're unable to fix it (e.g. if the importing semantics change or the objects you're modifying get frozen), what happens to all the extensions that that used the mail object? I know it's unlikely but I've seen a lot of unlikely things happen in the last year.

::: mail/components/extensions/parent/ext-mail.js
@@ +53,5 @@
> +
> +    return initExtensionContext.apply(ExtensionContent, arguments);
> +  };
> +
> +  // This patches priviledged pages such as the background script

privileged

While we're here, shouldn't these comment sentences have a full stop?
Attachment #9060061 - Flags: review?(geoff) → review+

(In reply to Philipp Kewisch [:Fallen] [:πŸ“†] from comment #1)

  • Should we name this mail ? If we have calendar or chat APIs this would be confusing and I am not sure we should be adding more aliases. I guess we could go with mail.calendar.* and mail.chat.* for those.

I thought messenger would be the right analogue to browser. Or we could do what Chrome did and call it thunderbird.

  • Should we be restricting access to mail-specific APIs so they can only go through mail.*? I think I know how to do this.

I'm not convinced. Had you asked the same question several betas ago I think I would've said yes.

You should take these questions to maildev, there's not a lot of audience here.

(In reply to Geoff Lankow (:darktrojan) from comment #3)

Comment on attachment 9060061
My other concern is that if somehow the monkey-patching is broken and we're
unable to fix it (e.g. if the importing semantics change or the objects
you're modifying get frozen), what happens to all the extensions that that
used the mail object? I know it's unlikely but I've seen a lot of unlikely
things happen in the last year.

Then we patch m-c to provide us an observer notification of sorts that we can hook into when creating these globals. The monkey-patching may break, but I don't think there will be many cases where we can no longer monkeypatch, unless maybe they seal all the objects or move code to C++/rust.

While we're here, shouldn't these comment sentences have a full stop?
I'd love to say no because I'm not used to this style in comments, but it doesn't seem I have a good case to argue it :)

I'll take the remaining question to maildev, I was thinking of that as well.

What's the status here? r+, didn't land?

Needs to change from mail to messenger, and maybe a new try run to make sure it is still working.

Attached patch Fix - v3 β€” β€” Splinter Review

I've tested test_ext_alias locally and it seems to work. I'd appreciate if someone could trigger a try run for this, it looks like trychooser has gone away and based on the docs I'm not clear what approach now works.

Attachment #9060061 - Attachment is obsolete: true
Attachment #9084624 - Flags: review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/69ee60f0bf41
Provide a |messenger| alias for WebExtension APIs. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

Try was green, so I landed it.

Target Milestone: --- → Thunderbird 70.0
Comment on attachment 9084624 [details] [diff] [review]
Fix - v3

Getting this into Thunderbird 68 would be great for add-on authors.
Attachment #9084624 - Flags: approval-comm-esr68?
Attachment #9084624 - Flags: approval-comm-beta?
Comment on attachment 9084624 [details] [diff] [review]
Fix - v3

No need for beta since it is already in TB 70 (beta). I'd have shipped it in TB 68.1 had the request come earlier ;-)
Attachment #9084624 - Flags: approval-comm-esr68?
Attachment #9084624 - Flags: approval-comm-esr68+
Attachment #9084624 - Flags: approval-comm-beta?
Summary: Provide a mail.* global as an alias → Provide a messenger.* global as an alias
You need to log in before you can comment on or make changes to this bug.