Port commands WebExtensions API

RESOLVED FIXED in Thunderbird 66.0

Status

enhancement
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 66.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

The commands API sounds simple to port, so I took a stab at it. There are only a few differences to the browser/ commands API, namely lack of pageAction and sidebarAction related code. This is expected though since we don't have those.

I have not ported the tests as they require mochitests, and those are not quite finished yet. We should add them with bug 1509246 or a followup. While I did mostly get mochitests running locally, there were a few utilities from BrowserTestUtils used in the browser mochitests that I didn't have time to replicate.
Posted patch Fix - v1 (obsolete) β€” β€” Splinter Review
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #9027413 - Flags: review?(geoff)
Posted patch Fix - v2 (obsolete) β€” β€” Splinter Review
(and now with linter fixed)
Attachment #9027413 - Attachment is obsolete: true
Attachment #9027413 - Flags: review?(geoff)
Attachment #9027414 - Flags: review?(geoff)
Comment on attachment 9027414 [details] [diff] [review]
Fix - v2

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

I'm giving this r+ without digging too deep because it's so similar to the Firefox version. I would like to see the tests even if we can't have them running yet. I can do a Try run.

My other concern is that we have other parts of primary UI such as the compose window that commands wouldn't run on. I'm not sure what this would look like, but my guess is a key in the manifest entry that specifies where commands should be active.

::: mail/components/extensions/jar.mn
@@ +8,5 @@
>  
>      content/messenger/parent/ext-addressBook.js    (parent/ext-addressBook.js)
>      content/messenger/parent/ext-browserAction.js  (parent/ext-browserAction.js)
>      content/messenger/parent/ext-composeAction.js  (parent/ext-composeAction.js)
> +    content/messenger/parent/ext-commands.js       (parent/ext-commands.js)

In order, please! :-)
Attachment #9027414 - Flags: review?(geoff) → review+
Posted patch Fix - v3 β€” β€” Splinter Review
Order changed. I don't really have something viable for tests yet. I mainly copied the m-c tests from https://searchfox.org/comm-central/search?q=&path=browser_ext_commands but quickly ran into the issue that a lot of the tests were using CustomizableUI helpers in head.js that would need to be ported. I was aiming for a more explorative approach in porting those, and for that I'd like to be able to run the tests locally.

Ok to check in anyway, or do you want tests first?
Attachment #9027414 - Attachment is obsolete: true
Flags: needinfo?(geoff)
Attachment #9029440 - Flags: review+
I've been using attachment 9024581 [details] [diff] [review] on mozilla-central to run mochitests locally. I don't think they should stop this landing, but we should at least try to get them going first. I can also do a Try run with tests.
Flags: needinfo?(geoff)

Comment 6

6 months ago
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/714714d80d62
WebExtension Commands API. r=darktrojan
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
I've landed this as I'm satisfied it passes the tests, (or would do if they were landed and mochitests worked). I had to add this line to ext-mail.json:

> "events": ["uninstall"],
Target Milestone: --- → Thunderbird 66.0
You need to log in before you can comment on or make changes to this bug.