Closed Bug 616419 Opened 9 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED INVALID
Tracking Status
blocking2.0 --- -

People

(Reporter: dietrich, Unassigned)

References

Details

(Whiteboard: [addon bar])

Attachments

(2 files, 1 obsolete file)

Followup to bug 598923, for non-restartless add-ons.
Blocking, since the original blocked.
blocking2.0: --- → betaN+
Assignee: nobody → dietrich
Whiteboard: [addon bar]
Assignee: dietrich → 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?
Attached patch WIP - no tests (obsolete) — Splinter Review
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?
Attachment #495993 - Flags: feedback?(dietrich)
(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.
(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.
(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.
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 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.
Attachment #495993 - Flags: feedback?(dietrich) → feedback+
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.
(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
(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 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.
tested manually with this addon
(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.
(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.
(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.
Oh, that's all in the add-ons code and the add-ons manager wouldn't know about it
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.
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.
blocking2.0: betaN+ → ?
(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.
(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?
Nope, it's the 'collapsed' attribute all over right now.
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 #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-.
blocking2.0: ? → -
(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?
Whiteboard: [addon bar] → [addon bar][firebug-p1]
Whiteboard: [addon bar][firebug-p1] → [addon bar]
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.
Just to let you know: don't use setToolbarVisibility, it fails on WinXP.
(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.
I mean: we tried to use it in Firebug but we got Bug 633611 for our efforts.
Assignee: ddahl → nobody
The add-on bar was removed in Firefox 29.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.