1487008 - Toolbar buttons WebExtensions API

Status: RESOLVED FIXED

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

Description

[Geoff Lankow (:darktrojan)]

I've begun working on an API so extensions can add buttons to toolbars. It'd be the Thunderbird equivalent of browserAction. We have a lot of toolbars, which makes life difficult. Currently what I have is a different name for each different button: messengerAction, composeAction, etc., but I think it'd work better to be more like Firefox's contextMenus API, where in code you create any number of menu items and specify which menu(s) to add them to.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I think we should definitely support browserAction to the main toolbar, for compatibility. I'd be carefully with allowing any number of items, in Firefox/Chrome I suspect this was done to avoid an overflow of buttons by a single add-on. I can imagine cases where more than one per toolbar would make sense, but we shouldn't allow an unlimited number of buttons.

This would indeed be a great API to add, it would get us a step closer to making the gecko profiler add-on work.

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1487008-webext-actions-1.diff

I've created "browser"_action for the 3-pane tab and compose_action for the compose window. It should be easy from here to add more actions for other toolbars like the message display toolbar.

ExtensionPopups.jsm is more or less the same as the one in browser/components/extensions, but we don't have CustomizableUI so I've had to hack it a bit. There's one or two bits still missing, like the browser_style stylesheets.

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9008612 [details] [diff] [review]
1487008-webext-actions-1.diff

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

This looks great overall, I like that it is so easy to add new _actions.

::: mail/themes/shared/mail/messengercompose.css
@@ +10,5 @@
>    --lwt-background-tiling: no-repeat;
>  }
>  
> +toolbarbutton[data-extensionid] > .toolbarbutton-icon {
> +  list-style-image: var(--webextension-toolbar-image);

These styles probably need some additions for the default icon, similar to how it is in the m-c code. You might have that planned though.

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: 1487008-webext-actions-2.diff

I'm not convinced this is perfect, but it's good enough to use.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9009802 [details] [diff] [review]
1487008-webext-actions-2.diff

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

Fabulous, looking forward to getting this one in. Some minor comments:

::: mail/components/extensions/ExtensionPopups.jsm
@@ +50,5 @@
> +//   if (AppConstants.platform === "win") {
> +//     stylesheets.push("chrome://browser/content/extension-win-panel.css");
> +//   }
> +//   return stylesheets;
> +// });

Can you remove the commented out code? If we need this later we can still add it in, by then we'd probably have to sync with browser's ExtensionPopups.jsm anyway.

::: mail/components/extensions/ExtensionToolbarButtons.jsm
@@ +195,5 @@
> +  }
> +
> +  // Update the toolbar button |node| with the tab context data
> +  // in |tabData|.
> +  updateButton(node, tabData, sync = false) {

This file contains a mix of undocumented functions, fully documented functions, and such where there is just a simple comment. Can you consider making them all full jsdoc comments?

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9009802 [details] [diff] [review]
1487008-webext-actions-2.diff

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

::: mail/components/extensions/ExtensionPopups.jsm
@@ +25,5 @@
> +const {
> +  makeWidgetId,
> +} = ExtensionCommon;
> +
> +

nit: double empty line

@@ +513,5 @@
> +    }
> +
> +    this.attached = true;
> +
> +

nir: here too double empty line

::: mail/components/extensions/schemas/browserAction.json
@@ +50,5 @@
> +    ]
> +  },
> +  {
> +    "namespace": "browserAction",
> +    "description": "Use browser actions to put icons in the main browser toolbar, to the right of the address bar. In addition to its icon, a browser action can also have a tooltip, a badge, and a popup.",

Hmm. Might need some updates :)

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: 1487008-webext-actions-3.diff

This is more or less what I'd intended to post last time (but didn't for some reason). Asking for review again because of the large number of changes.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9011726 [details] [diff] [review]
1487008-webext-actions-3.diff

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

Looks good to me, thanks for the update!

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: 1487008-webext-actions-4.diff

Rebased to avoid dependency on bug 1469238.

Comment 10

[Jorg K (CEST = GMT+2)]

Sorry, I have no indication whether this could possibly break anything:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ddfa17b95d8d90d7fcbabcd61c1f185881181f48

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/630fc01f8e33
Toolbar buttons WebExtensions API. r=philipp

Comment 12

[[e-mail]]

Comment on attachment 9013178 [details] [diff] [review]
1487008-webext-actions-4.diff

>+      // Sets the context information for context menus.
>+      mm.loadFrameScript("chrome://browser/content/content.js", true, true);
Do you actually ship this file? I don't see it in my build.

Comment 13

[Jorg K (CEST = GMT+2)]

Seems to be a browser file:
https://searchfox.org/comm-central/search?q=content.js&case=false&regexp=false&path=
mozilla/browser/base/content/content.js

mozilla/browser/base/jar.mn
65 content/browser/content.js (content/content.js)

mozilla/browser/base/content/browser.js
1845 mm.loadFrameScript("chrome://browser/content/content.js", true, true);