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

RESOLVED INVALID

Status

()

Firefox
Toolbars and Customization
RESOLVED INVALID
7 years ago
3 years ago

People

(Reporter: dietrich, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [addon bar])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
Followup to bug 598923, for non-restartless add-ons.
(Reporter)

Comment 1

7 years ago
Blocking, since the original blocked.
blocking2.0: --- → betaN+
(Reporter)

Updated

7 years ago
Assignee: nobody → dietrich
Whiteboard: [addon bar]
Blocks: 598923
(Reporter)

Updated

7 years ago
Assignee: dietrich → ddahl

Comment 2

7 years ago
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

7 years ago
Created attachment 495993 [details] [diff] [review]
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?
Attachment #495993 - Flags: feedback?(dietrich)

Comment 4

7 years ago
(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

7 years ago
(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

7 years ago
(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

7 years ago
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.
(Reporter)

Comment 8

7 years ago
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+
(Reporter)

Comment 9

7 years ago
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

7 years ago
(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

Comment 12

7 years ago
(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

7 years ago
Created attachment 496230 [details] [diff] [review]
v 0.2 manual test passes for traditional extension
Attachment #495993 - Attachment is obsolete: true

Comment 14

7 years ago
Created attachment 496232 [details]
testAddon - traditional

tested manually with this addon

Comment 15

7 years ago
(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.

Comment 17

7 years ago
(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.

Comment 22

7 years ago
(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.

Comment 24

7 years ago
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?
(Reporter)

Comment 25

7 years ago
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: ? → -

Comment 26

6 years ago
(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]

Updated

6 years ago
Whiteboard: [addon bar][firebug-p1] → [addon bar]

Comment 27

6 years ago
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

6 years ago
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.

Comment 30

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

Updated

4 years ago
Assignee: ddahl → nobody
The add-on bar was removed in Firefox 29.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.