616419 - add-on bar should be made visible when non-restartless add-ons are installed and add to it

Status: RESOLVED INVALID

(Firefox :: Toolbars and Customization, defect)

Description

[Dietrich Ayala (:dietrich)]

Followup to bug 598923, for non-restartless add-ons.

Comment 1

[Dietrich Ayala (:dietrich)]

Blocking, since the original blocked.

Comment 2

[David Dahl :ddahl]

I assume I want to run a check for the count of the toolbar's currentSet after the overlay is loaded on restart - if it is greater than x I will want to show the addonBar. at what point in the startup sequence should I run this check?

Comment 3

[David Dahl :ddahl]

Attachment: WIP - no tests

i'm wondering if there is more trickiness here... - also, will the test need to install an addon and restart? I thought we could not do that with mochitest? in that case do we depend on a mozmill test?

Comment 4

[Adam Kowalczyk]

(In reply to comment #3)
> i'm wondering if there is more trickiness here...

Unfortunately, some (most?) extensions insert their elements asynchronously which makes checking in delayedStartup() unreliable. And I am not sure if they have a choice, last time I checked I couldn't get it to work directly on window load because it interfered with toolbar initialization.

Comment 5

[David Dahl :ddahl]

(In reply to comment #4)
> (In reply to comment #3)
> > i'm wondering if there is more trickiness here...
> 
> Unfortunately, some (most?) extensions insert their elements asynchronously
> which makes checking in delayedStartup() unreliable.

hmm. If theses are "normal" extensions, then they have overlays - at some point (maybe not delayed startup final-ui-startup maybe?) they will have to be loaded with the rest of the chrome.

Comment 6

[Adam Kowalczyk]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> hmm. If theses are "normal" extensions, then they have overlays - at some point
> (maybe not delayed startup final-ui-startup maybe?) they will have to be loaded
> with the rest of the chrome.

Yes, overlays will have been loaded by the time load event is fired. But you never overlay the toolbar directly, you overlay the toolbarpalette and then programmatically insert the button into the toolbar. See these articles for how it looks from the extension's perspective:

https://developer.mozilla.org/en/Code_snippets/Toolbar#Adding_button_by_default
http://blog.pearlcrescent.com/archives/24

Maybe doing it asynchronously isn't necessary any more, as some extensions seem to do fine with doing it synchronously on window load. Maybe it has never been really necessary, I don't know. However, the fact is plenty of extensions use async, including heavyweights such as Adblock Plus.

Comment 7

[Adam Kowalczyk]

Some extensions do it like this:

toolbar.insertItem("button-id", beforeElement, null, false);
toolbar.setAttribute("currentset", toolbar.currentSet);
document.persist(toolbar.id, "currentset");

Others do it like this:

var newset = toolbar.currentSet.concat(',button-id');
toolbar.currentSet = newset;
toolbar.setAttribute('currentset', newset);
document.persist(toolbar.id, 'currentset');
BrowserToolboxCustomizeDone(true);

I think plugging into both insertItem() and BrowserToolboxCustomizeDone() should let you avoid race conditions.

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 495993 [details] [diff] [review]
WIP - no tests

Approach looks ok for a first-cut. I think you should test this manually with both types overlay and add-on-window-load add-ons to confirm that it covers those cases.

WRT to automation, ping Henrik about a mozmill test. If mozmill is in continuous integration now, then you should write one of those. Otherwise, set the in-litmus flag so we can get manual test coverage.

Comment 9

[Dietrich Ayala (:dietrich)]

I wonder if we should send a CustomizationOccurred event that things like this can listen to.

Also, we only want to do this post-install of add-ons. You should find out if there's a way to determine that from the Add-ons Manager. Otherwise, you could do something like storing the set in a pref, and comparing.

Comment 10

[David Dahl :ddahl]

(In reply to comment #9)
> I wonder if we should send a CustomizationOccurred event that things like this
> can listen to.
> 
> Also, we only want to do this post-install of add-ons. You should find out if
> there's a way to determine that from the Add-ons Manager.

CC'd mossop on this question

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #10)
> (In reply to comment #9)
> > I wonder if we should send a CustomizationOccurred event that things like this
> > can listen to.
> > 
> > Also, we only want to do this post-install of add-ons. You should find out if
> > there's a way to determine that from the Add-ons Manager.
> 
> CC'd mossop on this question

I can't think of an easy one right now, but we could just send out the appropriate onInstalled etc. events when the add-ons manager detects changes at startup, I think a component of the application can be loaded early enough to listen to that. No good for extensions I guess though

Comment 12

[Adam Kowalczyk]

(In reply to comment #9)
> I wonder if we should send a CustomizationOccurred event that things like this
> can listen to.

This would be very useful for extensions as well. For toolbar items that are stateful and need to be dynamically updated, there is no good way for detecting when they are added. This has now become more troublesome since all kinds of status indicators that used to be permanent on the status bar are now customizable.

Comment 13

[David Dahl :ddahl]

Attachment: v 0.2 manual test passes for traditional extension

Comment 14

[David Dahl :ddahl]

Attachment: testAddon - traditional

tested manually with this addon

Comment 15

[David Dahl :ddahl]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > I wonder if we should send a CustomizationOccurred event that things like this
> > > can listen to.
> > > 
> > > Also, we only want to do this post-install of add-ons. You should find out if
> > > there's a way to determine that from the Add-ons Manager.
> > 
> > CC'd mossop on this question
> 
> I can't think of an easy one right now, but we could just send out the
> appropriate onInstalled etc. events when the add-ons manager detects changes at
> startup, I think a component of the application can be loaded early enough to
> listen to that. No good for extensions I guess though

In which case, should we file a bug or include the CustomizationOccurred event here? What bit of code would handle a notifyObservers() call in this case? How would we know of each instance of the addonBar being customized without a DOMMutation Listener, which may be to resource intensive, no?

I think a pref would only help us on each startup, which seems to be working with this patch as is.

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #15)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > I wonder if we should send a CustomizationOccurred event that things like this
> > > > can listen to.
> > > > 
> > > > Also, we only want to do this post-install of add-ons. You should find out if
> > > > there's a way to determine that from the Add-ons Manager.
> > > 
> > > CC'd mossop on this question
> > 
> > I can't think of an easy one right now, but we could just send out the
> > appropriate onInstalled etc. events when the add-ons manager detects changes at
> > startup, I think a component of the application can be loaded early enough to
> > listen to that. No good for extensions I guess though
> 
> In which case, should we file a bug or include the CustomizationOccurred event
> here? What bit of code would handle a notifyObservers() call in this case? How
> would we know of each instance of the addonBar being customized without a
> DOMMutation Listener, which may be to resource intensive, no?
> 
> I think a pref would only help us on each startup, which seems to be working
> with this patch as is.

What I suggested would help you detect that on startup Firefox installed/uninstalled some extensions, but you say you already have the startup case working so maybe I'm not sure what you're looking for anymore.

Comment 17

[David Dahl :ddahl]

(In reply to comment #16)

> What I suggested would help you detect that on startup Firefox
> installed/uninstalled some extensions, but you say you already have the startup
> case working so maybe I'm not sure what you're looking for anymore.

ok, thanks. It sounds like there are some corner cases where an extension has an option for adding or removing a button to/from the addon-bar, in which case, the addon-bar might not be displayed/hidden.

Comment 18

[Dave Townsend [:mossop]]

Oh, that's all in the add-ons code and the add-ons manager wouldn't know about it

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 496230 [details] [diff] [review]
v 0.2 manual test passes for traditional extension

>+  onDelayedStartup: function AML_onDelayedStartup() {
>+    if (this.getAddonBarItemCount() > 0) {
>+      setToolbarVisibility(this.addonBar, true);
>+    }
>+    else {
>+      setToolbarVisibility(this.addonBar, false);
>+    }
>   }

This doesn't do what we need it to do, i.e. it shows the toolbar when hiding it manually and restarting without installing anything.

Comment 20

[Dão Gottwald [:dao]]

I also don't think this should block. Traditional extensions can call setToolbarVisibility at the same time they manually append the item. The add-on bar works just like a normal toolbar here.

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #15)
> How
> would we know of each instance of the addonBar being customized without a
> DOMMutation Listener, which may be to resource intensive, no?

Yep, we can't use that.

Comment 22

[David Dahl :ddahl]

(In reply to comment #19)
> Comment on attachment 496230 [details] [diff] [review]

> This doesn't do what we need it to do, i.e. it shows the toolbar when hiding it
> manually and restarting without installing anything.

Is there a pref or object attribute I can check as well in case of a manually-hidden addon-bar in order to do the right thing here?

Comment 23

[Dão Gottwald [:dao]]

Nope, it's the 'collapsed' attribute all over right now.

Comment 24

[David Dahl :ddahl]

There is one case we can test for here: if a user has buttons on the addon-bar and the addon-bar is collapsed, we can leave it collapsed. That leaves us with the case where the addon-bar was collapsed manually and has no addon buttons added to it (for whatever reason).

perhaps a pref is needed here that is set by the menuitem?

Comment 25

[Dietrich Ayala (:dietrich)]

comment #20 makes sense, we shouldn't need to do the extra work here, since it's super simple for add-ons to do it. blocking-.

Comment 26

[John J. Barton]

(In reply to comment #20)
> I also don't think this should block. Traditional extensions can call
> setToolbarVisibility at the same time they manually append the item. The add-on
> bar works just like a normal toolbar here.

Can some one explain what this means?

Comment 27

[bah.doo.bah]

This should have an about:config entry to enable the user to choose if the add-on bar should automatically display itself.

Some add-ons stubbornly place themselves in the bar and I don't want to see the bar.

Comment 28

[John J. Barton]

Just to let you know: don't use setToolbarVisibility, it fails on WinXP.

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #28)
> Just to let you know: don't use setToolbarVisibility, it fails on WinXP.

What does this mean? setToolbarVisibility does nothing that would fail on XP. In fact, we call it and run tests with it on XP.

Comment 30

[John J. Barton]

I mean: we tried to use it in Firebug but we got Bug 633611 for our efforts.

Comment 31

[Matthew N. [:MattN]]

The add-on bar was removed in Firefox 29.