Mike Kaply [:mkaply] Assignee
	Description • 6 years ago

See bug 1459007 for context.

Currently, if a WebExtension has a sidebar we always open it on the install of the WebExtension

I think we should reconsider this decision.

In the uBlock case, they simply have a sidebar for debugging/logging, yet it opens up to a normal user.

It's up to the WebExtension to make the sidebar discoverable, it shouldn't be done by Firefox.

	Mike Conca [:mconca]
	Comment 1 • 6 years ago

The issue is that sidebars can't be opened programmatically, they can only be opened by user action. The rationale behind opening them on install was so that the user understood the extension included a sidebar.  This works and is desired by the vast majority of sidebar extensions.

I guess I view uBlock as the exception to the general case, and making every extension with a sidebar figure out a way to make themselves discoverable seems to penalize nearly every sidebar extension in order to account for the exception (albeit one with a very large install base).

Perhaps an alternative would be to add a manifest key called "installed_open" that defaults to true when not included, but could be set to false for extensions like uBlock.

	Mike Kaply [:mkaply] Assignee
	Comment 2 • 6 years ago

> The issue is that sidebars can't be opened programmatically, they can only be opened by user action.

I thought we provided a way for an add-on to open a sidebar if a user explicitly chose an action within the add-on, but it looks like we don't.

So we force users to go through our sidebar UI to access any sidebar?

It seems like a better fix would be to allow add-ons to have a way to open the sidebar if there was a user action within the add-on. Then they would be responsible for surfacing their own sidebar.

> Perhaps an alternative would be to add a manifest key called "installed_open" that defaults to true when not included, but could be set to false for extensions like uBlock.

That might work as well...

	Graham Perrin
	Comment 3 • 6 years ago

> … shouldn't be done by Firefox.

+1

The degree of annoyance when undoing the intrusion in fifteen or windows can be surprisingly high. 

I'm tempted to repeat this text now in comments 4, 
5, 
6, 
7, 
8, 
9, 
10, 
11, 
12, 
13, 
14, 
15, 
16, 
17 
and 18, but I suspect that doing so would bring to mind a word less gentle than "surprising". So I'll

	Mike Conca [:mconca]
	Comment 4 • 6 years ago

(In reply to Mike Kaply [:mkaply] from comment #2)
> > The issue is that sidebars can't be opened programmatically, they can only be opened by user action.
> 
> I thought we provided a way for an add-on to open a sidebar if a user
> explicitly chose an action within the add-on, but it looks like we don't.

We do.  sidebarAction.open() is available and can be used in response to a user clicking a browserAction, pageAction, context menu item, keyboard shortcut, or a bundled page.

The implication, though, is that every sidebar would need to include one of those items, when it may not make sense. There are a lot of sidebar extensions that are simply sidebars (they show Twitter, Facebook, chat, email, etc.) and want to simply be a sidebar selection inside of Firefox along with bookmarks, history, and sync'd tabs.

To be clear, I'm not against asking sidebar extensions to surface themselves, it's a reasonable request. It just seems like the current default behavior of showing the sidebar on install is what the majority of extensions want. So, to me, the problem is that the extensions that don't want this behavior have no choice, which is why offering an install-time choice to not be shown might be a better solution.

	Mike Kaply [:mkaply] Assignee
	Comment 5 • 6 years ago

> So, to me, the problem is that the extensions that don't want this behavior have no choice, which is why offering an install-time choice to not be shown might be a better solution.

Makes perfect sense. I'll see if I can write a quick patch.

	Mike Kaply [:mkaply] Assignee
	Comment 7 • 6 years ago

There is a try build for this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9174de2a4822f5e02c293eef65a863897fc9b5f0

	Shane Caraveo (:mixedpuppy)
	Comment 8 • 6 years ago
mozreview-review

Comment on attachment 8977043 [details]
Bug 1460910 - Allow sidebar to be closed at install.

https://reviewboard.mozilla.org/r/245142/#review251098

::: browser/components/extensions/schemas/sidebar_action.json:38
(Diff revision 1)
> +              },
> +              "open_at_install": {
> +                "type": "boolean",
> +                "optional": true,
> +                "default": true,
> +                "description": "Whether or not the sidebar is opened at install"

", default is <code>true</code>", or something like that

	Andrew Swan [:aswan]
	Comment 9 • 6 years ago
mozreview-review

Comment on attachment 8977043 [details]
Bug 1460910 - Allow sidebar to be closed at install.

https://reviewboard.mozilla.org/r/245142/#review251106

::: browser/components/extensions/parent/ext-sidebarAction.js:138
(Diff revision 1)
> -      if (install || SidebarUI.lastOpenedId == this.id) {
> +      if ((install || SidebarUI.lastOpenedId == this.id) &&
> +          this.extension.manifest.sidebar_action.open_at_install) {

This logic doesn't look right, shouldn't it be
`(install && open_at_install) || lastOpened == this`

	Mike Kaply [:mkaply] Assignee
	Comment 10 • 6 years ago
mozreview-review-reply

Comment on attachment 8977043 [details]
Bug 1460910 - Allow sidebar to be closed at install.

https://reviewboard.mozilla.org/r/245142/#review251106

> This logic doesn't look right, shouldn't it be
> `(install && open_at_install) || lastOpened == this`

Yes. I misread that this was completely under ADDON_INSTALL.

	Pulsebot
	Comment 12 • 6 years ago

Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/42abedd98732
Allow sidebar to be closed at install. r=mixedpuppy

	Mike Kaply [:mkaply] Assignee
	Comment 13 • 6 years ago

mconca:

I ended up using open_at_install

Hope you don't mind.

	Mike Conca [:mconca]
	Comment 14 • 6 years ago

(In reply to Mike Kaply [:mkaply] from comment #13)
> I ended up using open_at_install
> 
> Hope you don't mind.

What?!? And miss a prime opportunity to bikeshed the name of an optional keyword?!?  We'll need to carry on a public, back-and-forth discussion over the name for at least another handful of comments before I can be satisfied.

All kidding aside, mkaply, great job landing this. It'll be a nice enhancement for developers.

	Andreea Pavel [:apavel]
	Comment 15 • 6 years ago

Backed out for failing browser chrome at  browser/components/extensions/test/browser/test-oop-extensions/browser_ext_sidebarAction.js

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=42abedd987321bd8d3cc9d531c575e31e4283f1c

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=179239582&repo=autoland&lineNumber=2829

Backout: https://hg.mozilla.org/integration/autoland/rev/17efe13d07fafbf1539f86f2046c946b67f3a787

	fiveNinePlusR
	Comment 16 • 6 years ago

I am curious why it opens it for every single browser window when installed. A reasonable thing would be to open the sidebar if the extension wants it displayed on install in the window that initiated the addon installation.

	Mike Kaply [:mkaply] Assignee
	Comment 17 • 6 years ago

So addressing aswans last comment broke this (and I failed to retest).

The reason is because of the code here:

https://searchfox.org/mozilla-central/source/browser/components/extensions/parent/ext-sidebarAction.js#138

SidebarUI.lastOpenedId == this.id

Because this test is run repeatedly with the same extension, this value is true.

I'm wondering if we shoudl do something with lastOpenedId when an extension is installed.

Alternative is to use a different extensionID for the test.

	Mike Kaply [:mkaply] Assignee
	Comment 19 • 6 years ago

I've updated the patch to cover this case. During shutdown, if lastOpenedId is our ID, we should be resetting it.

	Andrew Swan [:aswan]
	Comment 20 • 6 years ago

(In reply to Mike Kaply [:mkaply] from comment #19)
> During shutdown, if lastOpenedId
> is our ID, we should be resetting it.

That doesn't sound right.  Isn't the point of that logic to restore the sidebar across browser sessions?
Did you mean "during uninstall" instead of "during shutdown"?

	Mike Kaply [:mkaply] Assignee
	Comment 21 • 6 years ago

> Isn't the point of that logic to restore the sidebar across browser sessions?
Did you mean "during uninstall" instead of "during shutdown"?

I thought that at first too, but I see no place where lastOpenedId is preserved across shutdowns.

And during on Shutdown, we close our own sidebar.

I honestly don't understand the point of lastOpenedId, and this comment in browser-sidebar makes it even less clear:

https://searchfox.org/mozilla-central/source/browser/base/content/browser-sidebar.js#33

  // lastOpenedId is set in show() but unlike currentID it's not cleared out on hide
  // and isn't persisted across windows

	Shane Caraveo (:mixedpuppy)
	Comment 22 • 6 years ago

mixedpuppy: aswan, mkaply: looks like it is fine to null lastOpenedId if the extension is being shutdown
mixedpuppy: lastOpenedId is used only to support SidebarUI.toggle
mixedpuppy: it is set during show to be the broadcaster id
mixedpuppy: the persistence (to reopen an open sidebar on startup) is handled in xul

additional note: lastOpenedId is also set on startup, based on the broadcaster id persisted via xul.

	Pulsebot
	Comment 24 • 6 years ago

Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/490a791ce6bd
Allow sidebar to be closed at install. r=mixedpuppy

	Natalia Csoregi [:nataliaCs]
	Comment 25 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/490a791ce6bd

	marius.santa
	Comment 26 • 6 years ago

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

	Mike Kaply [:mkaply] Assignee
	Comment 27 • 6 years ago

I don't think so. We have an automated test for this, so we should be good.

	Irene Smith
	Comment 28 • 6 years ago

Added open_at_install. The optional parameter has been added here:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/sidebar_action

and to the release notes.

	Mike Kaply [:mkaply] Assignee
	Comment 29 • 6 years ago

Content looks good. Formatting is off though. It's not using a fixed width font.