973074 - Gecko Profiler can't be removed from the toolbar

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Justin Dolske [:Dolske]]

Not sure if this is a problem with Firefox, the Addon SDK, or the addon itself.

1) Have Gecko Profiler installed (I have 1.12.21), button is in navbar
2) Right-click, select "Remove from Toolbar"
3) Button correctly disappears
4) Open a new window
5) Button has returned, it's in both the new and old window.

Comment 1

[:Gijs (he/him)]

This is either the gecko profiler's or its toolbarbutton's library's fault. I can't tell 100% for sure which because the browser debugger keeps crashing (mostly itself, but sometimes the calling process) when I try to figure it out, and after 3 restarts I'm getting bored of trying to debug what all these unfamiliar bits of code are doing.

It seems, however, upon casual inspection, that the toolbarbutton library just assumes that if the add-on's options specify a toolbar, and a new window appears, it should always add the toolbarbutton to that window. If it can't find it somewhere else in the window, it'll put it in the toolbar specified by the calling add-on. See https://github.com/erikvold/addon-pathfinder/blob/master/lib/ui/toolbarbutton.js#L56 . Assuming I'm reading the code correctly, it shouldn't be doing that.

Comment 2

[Justin Dolske [:Dolske]]

So, sounds like this needs a fix to the toolbarbutton library (and addons using it will need to update to the newer version)?

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Justin Dolske [:Dolske] from comment #2)
> So, sounds like this needs a fix to the toolbarbutton library (and addons
> using it will need to update to the newer version)?

I thought there was a bug about making sure the toolbarbutton module works as it is in Australis, so that all of those add-ons don't have to update?

Also there are restartless add-ons in the wild that use the same method of adding buttons that would require the same fix, so updating the module didn't make sense.

Matteo or Gijs do you recall this bug #? I can't seem to find it atm.

Comment 4

[:Gijs (he/him)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #3)
> (In reply to Justin Dolske [:Dolske] from comment #2)
> > So, sounds like this needs a fix to the toolbarbutton library (and addons
> > using it will need to update to the newer version)?
> 
> I thought there was a bug about making sure the toolbarbutton module works
> as it is in Australis, so that all of those add-ons don't have to update?
> 
> Also there are restartless add-ons in the wild that use the same method of
> adding buttons that would require the same fix, so updating the module
> didn't make sense.
> 
> Matteo or Gijs do you recall this bug #? I can't seem to find it atm.

It was marked WFM when stuff seemed to work.

I don't know if this particular bug is the library's or the add-on's fault, because I don't understand how the options in the toolbarbutton library are meant to work and if the arguments passed to moveTo are causing this or if it's just brokenness inside the library.

Either way, this isn't something Australis can fix for the library or add-on. The bug existed pre-Australis already - install the add-on, customize, remove the button, restart the browser, and voila, the button is back again.

In other words, if the add-on re-adds the button automatically in its code, there's nothing we can do about that.

Note that similar issues existed with the widget library (bug 854226) until I fixed the library.

Comment 5

[Justin Dolske [:Dolske]]

Untracking from Australis because this seems like a clear bug in the library. It would be great to have a fix soon, though, so that add-on authors who will need to update to the fixed library have time to do so.

Comment 6

[Jorge Villalobos [:jorgev] (he/him)]

I marked versions up to 1.12.21 as incompatible with 29 and above. Nothing else to do here.

Comment 7

[Justin Dolske [:Dolske]]

Did this get fixed in the library it was using? Or do we need a bug specifically for that?

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #4)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #3)
> > (In reply to Justin Dolske [:Dolske] from comment #2)
> > > So, sounds like this needs a fix to the toolbarbutton library (and addons
> > > using it will need to update to the newer version)?
> > 
> > I thought there was a bug about making sure the toolbarbutton module works
> > as it is in Australis, so that all of those add-ons don't have to update?
> > 
> > Also there are restartless add-ons in the wild that use the same method of
> > adding buttons that would require the same fix, so updating the module
> > didn't make sense.
> > 
> > Matteo or Gijs do you recall this bug #? I can't seem to find it atm.
> 
> It was marked WFM when stuff seemed to work.
> 
> I don't know if this particular bug is the library's or the add-on's fault,
> because I don't understand how the options in the toolbarbutton library are
> meant to work and if the arguments passed to moveTo are causing this or if
> it's just brokenness inside the library.
> 
> Either way, this isn't something Australis can fix for the library or
> add-on. The bug existed pre-Australis already - install the add-on,
> customize, remove the button, restart the browser, and voila, the button is
> back again.
> 
> In other words, if the add-on re-adds the button automatically in its code,
> there's nothing we can do about that.

This would be a fault on the add-on devs side then, my docs for the toolbarbutton module made it clear that the best UX would be to check the `loadReason` and only move the button on install, after that the customization code should take care of the rest.  I provide moveTo because there are other cases where one may want to use that.

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Justin Dolske [:Dolske] from comment #5)
> Untracking from Australis because this seems like a clear bug in the
> library. 

This doesn't seem accurate to me, it's more likely a fault with the add-on code that uses it afaict from this thread.

> It would be great to have a fix soon, though, so that add-on
> authors who will need to update to the fixed library have time to do so.

It sounds like the add-on dev should be checking that the `require('sdk/self').loadReason == "install"` before calling moveTo.

I don't see any issue with the library atm.

Comment 10

[Justin Dolske [:Dolske]]

Ok, thanks for the clarification!