NiklasGollenstede / unload-tabs

WebExtension that unloads tabs and prevents tabs from loading
https://addons.mozilla.org/addon/unload-tabs/
Mozilla Public License 2.0
69 stars 13 forks source link

Show "Unload Tree" context menu command in the context menu on the Tree Style Tab sidebar #31

Closed piroor closed 5 years ago

piroor commented 5 years ago

On TST 3.0.12 and later, notification messages are not delivered for private windows by default, due to security reasons. I think that these changes should make this addon compatible to TST 3.0.12 and later.

For more details, please see the updated API document: https://github.com/piroor/treestyletab/wiki/API-for-other-addons#information-in-private-windows https://github.com/piroor/treestyletab/wiki/API-for-other-addons#when-permissions-for-your-addon-are-changed

NiklasGollenstede commented 5 years ago

First of all, thanks for looking out for breaking changes in depending add-ons, I do appreciate it!

Do I see it correctly that you made two changes (in the past year) that affect this extension?:

If I got that correctly, I have two questions:

piroor commented 5 years ago

Thank you for comments. Sorry, I actually didn't run this modified version locally. I retry to create a PR.

piroor commented 5 years ago

I've updated this PR.

It is necessary (or recommended for forward compatibility) to list `listeningTypes: [ 'ready', 'fake-contextMenu-click', ... ].

You right, only permissions-changed was necessary. I listed others just for a reminder.

When 'permissions-changed' gets fired, the integrating extension needs to send the 'register-self' message again. (To re-apply styles, but why?)

I thought that this re-registering was required on a scenario:

  1. TST and Unload Tabs are installed, and both are allowed on private windows.
  2. The checkbox in the column "Notify Messages from Private Windows" for Unload Tabs is turned on.

However, finally I realized that re-registering is unnecessary for this addon. If the registered stylesheet is generated based on tabs, it needed to be re-registered because visibility of tabs in private windows can be changed. But Unload Tabs doesn't do that, it just register a fixed stylesheet.

So the initially proposed changes are completely disappeared. Instead I've added a small change to install "Unload Tree" context menu command for the new context menu on the sidebar for Firefox 64 and later. How about the change?

NiklasGollenstede commented 5 years ago

Ah, sorry for the delay. You had me lost there for a while, I didn't know that browser.menus.overrideContext() was a thing and didn't get what you were doing with the combination of tab and sidebar with a native Firefox API there. Cool stuff. Given that, your PR is definitely on the right track!

Thanks for your effort, I'll merge this now and fix some stuff (e.g. awaiting a promise, unregistering the added menu) myself. I appreciate that, even though yours is clearly different, you stuck with the coding style of this repo.


If the registered stylesheet is generated based on tabs, it needed to be re-registered [once permission for private tabs is granted] because visibility of tabs in private windows can be changed.

This makes sense. I didn't consider that since, as you said, UnloadTab's CSS is static. I think it would be good to make this a bit more explicit in your documentation, e.g. change

If your addon injects additional style rules to TST's sidebar, you may need to re-inject it when permissions are changed

to

If your addon injects additional style rules, that are based on individual tabs, to TST's sidebar, you may need to re-inject it when permissions are changed

under https://github.com/piroor/treestyletab/wiki/API-for-other-addons#when-permissions-for-your-addon-are-changed