Closed Bug 855683 Opened 7 years ago Closed 6 years ago

Current UX breaks Jetpack ToolbarButton (and therefore the Gecko Profiler addon)

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: snorp, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M6])

Attachments

(1 file, 2 obsolete files)

I get the following error when enabling the Gecko Profiler addon in today's UX nightly:

Timestamp: 3/28/13 10:03:28 AM
Error: geckoprofiler: An exception occurred.
TypeError: tb.insertItem is not a function
resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/toolbarbutton/lib/toolbarbutton.js 106
Traceback (most recent call last):
  File "resource://gre/modules/NetUtil.jsm", line 137, in 
    aCallback(pipe.inputStream, aStatusCode, aRequest);
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/net/url.js", line 49, in 
    resolve(data);
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 142, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 36, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 142, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 36, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 142, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 36, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 142, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 36, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 142, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 36, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 142, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 122, in then
    else result.then(resolved, rejected)
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 36, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 54, in effort
    try { return f(options) }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 142, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 36, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 54, in effort
    try { return f(options) }
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/addon/runner.js", line 91, in onLocalizationReady
    run(options);
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/addon/runner.js", line 135, in run
    quit: exit
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/geckoprofiler/lib/main.js", line 2104, in 
    appWrapper.init();
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/geckoprofiler/lib/main.js", line 1712, in ff_init
    tbb = toolbarButton.ToolbarButton(tbbOptions);
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/toolbarbutton/lib/toolbarbutton.js", line 130, in ToolbarButton
    var tracker = winUtils.WindowTracker(delegate);
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/deprecated/window-utils.js", line 57, in WindowTracker
    return new WindowTracker(delegate);
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/deprecated/window-utils.js", line 64, in WindowTracker
    this._regWindow(window);
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/deprecated/window-utils.js", line 90, in _regWindow
    this._delegate.onTrack(window);
  File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/toolbarbutton/lib/toolbarbutton.js", line 106, in 
    tb.insertItem(options.id, b4, null, false);
I just hit bug 844055 with Gecko Profiler. What other add-ons hit this?
This happened since a recent merge of m-c into UX. We saw this too on the Jamun branch when we merged it this morning.

Anyway, we thought it to be a good idea to ask Erik for a bit of background! Please?
Flags: needinfo?(evold)
(In reply to Mike de Boer [:mikedeboer] from comment #2)
> This happened since a recent merge of m-c into UX. We saw this too on the
> Jamun branch when we merged it this morning.
> 
> Anyway, we thought it to be a good idea to ask Erik for a bit of background!
> Please?

Do you know what commit caused this?
Flags: needinfo?(evold)
(In reply to Mike de Boer [:mikedeboer] from comment #2)
> This happened since a recent merge of m-c into UX. We saw this too on the
> Jamun branch when we merged it this morning.
> 
> Anyway, we thought it to be a good idea to ask Erik for a bit of background!
> Please?

This is actually older, AFAICT. The jetpack tests for tb.insertItem on jamun have been failing for a while longer (since April 21st at least; TBPL doesn't have data for before then. :-( )
OK, so this is caused by Australis' new toolbar XBL bindings, which don't have an insertItem method. Mike, what should be the goal here? What kind of API should jetpack be using to add a toolbar button? Can we reimplement insertItem, or should we change jetpack to use a different API to insert its widgets? I suspect the latter, but requesting needinfo to be sure.
Flags: needinfo?(mconley)
Blair can probably comment more on this (so needinfo'ing him), but yes - the insertItem function is not available in our simplified customizable toolbar binding.

What add-ons should do instead is use the createWidget function. Blair, do we have a document lying around detailing the properties that can / should be passed to createWidget? I seem to recall seeing a wiki page with that information, but I cannot find it.
Flags: needinfo?(mconley) → needinfo?(bmcbride)
Seems like a small price to pay for a nice backwards compatibility win. 

The only funny bit will be that adding items to toolbars is now global, whereas the insertItem() API is per-window. insertItem(() will end up being a wrapper around our new APIs in CustomizableUI. In theory, I think multiple calls should just work without any unintended side-effects.
Flags: needinfo?(bmcbride)
Attached patch WIP patch (obsolete) — Splinter Review
So I'm trying to fix this up, and it's going alright, but it seems add-on sdk widgets (yeah, not confusing at all) get added to the addon-bar. Then when in my new insertItem implementation I try to use the customization API (specifically, addWidgetToArea), it breaks because the onWidgetAdded handler goes through each node using getWidgetNode, which in turn only looks in gPalette (AFAICT, that's only API-added items?), and the desired toolbar's toolbox's palette. That doesn't work, probably because (I guess?) the item isn't in the navigator-toolbox palette - it's on the addon-bar. I know that the addon-bar is going away, but I'm just trying to figure out what the best way forward here would be...

(the guesses / AFAICT are here because I haven't yet figured out a way to pause the jetpack tests, so I'm stuck with trial and error and dump() debugging)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Mike, can you give me a clue or two here?
Flags: needinfo?(mconley)
In order for CustomizableUI.jsm to know about your widget, it's necessary that you register widget with it, via createWidget.

Remember that CustomizableUI is global, and the whole purpose of it is supposed to be the go-to module for adding stuff to *all windows simultaneously*. So createWidget doesn't create a XUL node, but it does tell CustomizableUI that at some point, those XUL nodes *will* need to be created. This is why addWidgetToArea is breaking for you - it doesn't know about this widget because it was never registered (and it's not the old XUL style).

I think I would, however, like Blair to comment more on exactly what the relationships is between Jetpack's notion of widgets and CustomizableUI's notion of widgets. Because they seem disjoint, but I heard somebody quote Blair as saying that this all should "more or less just work".

So, to sum up, the question is: How do Jetpack widgets get turned into CustomizableUI widgets?

So I'm redirecting this needinfo to Blair.
Flags: needinfo?(mconley) → needinfo?(bmcbride)
TBH, I think we have two problems. :-)

One is Jetpack. Jetpack should (IMHO) just be updated to use our API directly. All the jetpack consumers will Just Work. They're switching API's under the jetpack hood but that won't matter. But (again, IMHO) that's a separate bug.

Here, we're breaking existing Jetpack tests. That's because we're not implementing insertItem. insertItem is probably used by other add-ons, and it's relatively easy to implement (I should hope). I think we should do that (, too).

I'll defer to Blair and you on a decision on this, but that's how I interpret the situation. :-)
(In reply to Mike Conley (:mconley) from comment #10)
> I think I would, however, like Blair to comment more on exactly what the
> relationships is between Jetpack's notion of widgets and CustomizableUI's
> notion of widgets. Because they seem disjoint, but I heard somebody quote
> Blair as saying that this all should "more or less just work".

Ah, I think there may be some misunderstanding here. To CustomizableUI, current Jetpack widgets are nothing special - they're just normal XUL widgets, just like anything hard-coded in browser.xul or overlayed on browser.xul by a normal add-on. So nothing special has to happen for Jetpack widgets to work - CustomizableUI already supports it seamlessly. ie, if a XUL widget is in a toolbar or toolbox palette (which is different from CustomizableUI's internal palette, which holds widgets created via the new API), CustomizableUI will know about it.

The only wrinkles in Jetpack widgets being supported are:
* New functionality we've added that would be good if Jetpack adapted to, such as the overflow panel (which should still just work, but introduces opportunities for improvements)
* APIs that we've removed, but Jetpack uses. Such as insertItem() - that's an easy fix, we should just add that API back (as a wrapper around CustomizableUI's APIs).



In the future, Jetpack can be updated to use our new APIs to create widgets (which I imagine will be updated to be like the APIs Jetpack provides to add-ons). But, there's no requirement to do that.
Flags: needinfo?(bmcbride)
(In reply to :Gijs Kruitbosch from comment #8)
> it breaks because the onWidgetAdded handler
> goes through each node using getWidgetNode, which in turn only looks in
> gPalette (AFAICT, that's only API-added items?)

Yes.

>, and the desired toolbar's
> toolbox's palette.

Yes.

> That doesn't work, probably because (I guess?) the item
> isn't in the navigator-toolbox palette - it's on the addon-bar.

It looks anywhere in the document. So if its in the add-on bar, it should find it. It has to look at the toolbox palette separately because that's removed from the DOM on startup, so getElementByID doesn't work.
Comment on attachment 747381 [details] [diff] [review]
WIP patch

Review of attachment 747381 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/content/toolbar.xml
@@ +75,5 @@
> +            pos = beforeInfo.position;
> +          }
> +
> +          let w = CustomizableUI.getWidget(aId);
> +          CustomizableUI.addWidgetToArea(aId, this.id, pos);

You don't need the call to getWidget() - that's for widgets created via CustomizableUI.createWidget(). Perhaps we need to make this type of thing clearer somehow. CustomizableUI.getWidgetNode() is what you'd use to get the node.
(In reply to :Gijs Kruitbosch from comment #11)
> One is Jetpack. Jetpack should (IMHO) just be updated to use our API
> directly. All the jetpack consumers will Just Work. They're switching API's
> under the jetpack hood but that won't matter. But (again, IMHO) that's a
> separate bug.

Yes, separate bug/issue, and can be done at any time (or not at all, but I would prefer it did).


> Here, we're breaking existing Jetpack tests. That's because we're not
> implementing insertItem. insertItem is probably used by other add-ons, and
> it's relatively easy to implement (I should hope). I think we should do that
> (, too).

Correct.
(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #12)
> (In reply to Mike Conley (:mconley) from comment #10)
> > I think I would, however, like Blair to comment more on exactly what the
> > relationships is between Jetpack's notion of widgets and CustomizableUI's
> > notion of widgets. Because they seem disjoint, but I heard somebody quote
> > Blair as saying that this all should "more or less just work".
> 
> Ah, I think there may be some misunderstanding here. To CustomizableUI,
> current Jetpack widgets are nothing special - they're just normal XUL
> widgets, just like anything hard-coded in browser.xul or overlayed on
> browser.xul by a normal add-on. So nothing special has to happen for Jetpack
> widgets to work - CustomizableUI already supports it seamlessly. ie, if a
> XUL widget is in a toolbar or toolbox palette (which is different from
> CustomizableUI's internal palette, which holds widgets created via the new
> API), CustomizableUI will know about it.
> 
> The only wrinkles in Jetpack widgets being supported are:
> * New functionality we've added that would be good if Jetpack adapted to,
> such as the overflow panel (which should still just work, but introduces
> opportunities for improvements)
> * APIs that we've removed, but Jetpack uses. Such as insertItem() - that's
> an easy fix, we should just add that API back (as a wrapper around
> CustomizableUI's APIs).
> 
> 
> 
> In the future, Jetpack can be updated to use our new APIs to create widgets
> (which I imagine will be updated to be like the APIs Jetpack provides to
> add-ons). But, there's no requirement to do that.

Ah, OK - I get it now. I was assuming that Jetpack *must* use the new widget API for some reason. Thanks for clearing it up. :D
(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #13)
> > That doesn't work, probably because (I guess?) the item
> > isn't in the navigator-toolbox palette - it's on the addon-bar.
> 
> It looks anywhere in the document. So if its in the add-on bar, it should
> find it. It has to look at the toolbox palette separately because that's
> removed from the DOM on startup, so getElementByID doesn't work.

http://hg.mozilla.org/projects/ux/file/7c407cb365a0/browser/components/customizableui/src/CustomizableUI.jsm#l741

seems to check if there's a customization target and if the toolbox is in the ancestor chain of the node. Neither of those are, I believe, going to be true for the add-on bar?
(In reply to :Gijs Kruitbosch from comment #17)
> (In reply to Blair McBride [:Unfocused] (Limited availability.) from comment
> #13)
> > > That doesn't work, probably because (I guess?) the item
> > > isn't in the navigator-toolbox palette - it's on the addon-bar.
> > 
> > It looks anywhere in the document. So if its in the add-on bar, it should
> > find it. It has to look at the toolbox palette separately because that's
> > removed from the DOM on startup, so getElementByID doesn't work.
> 
> http://hg.mozilla.org/projects/ux/file/7c407cb365a0/browser/components/
> customizableui/src/CustomizableUI.jsm#l741
> 
> seems to check if there's a customization target and if the toolbox is in
> the ancestor chain of the node. Neither of those are, I believe, going to be
> true for the add-on bar?

Huh, I was wrong about the toolbox. But the customization target is unlikely to work out. Besides, we're getting rid of the addon-bar, which will completely break the test (and probably bits of the SDK)... I asked the jetpack team to discuss this and they will hopefully come up with a plan.
Flags: needinfo?(zer0)
(In reply to :Gijs Kruitbosch from comment #18)
>  Besides, we're getting rid of the addon-bar, which will
> completely break the test (and probably bits of the SDK)... I asked the
> jetpack team to discuss this and they will hopefully come up with a plan.

And now we have bug 872209.
Flags: needinfo?(zer0)
(In reply to :Gijs Kruitbosch from comment #18)
> But the customization target is unlikely
> to work out.

Will need to make it inherit from our new toolbar XBL binding.
No longer blocks: 770135
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [Australis:M5]
Version: unspecified → Trunk
(this may depend on the add-on bar having a customization target)

So this fixes some bugs, but it doesn't get us all the way. Now when you install the profiler add-on, it works, but after a restart its toolbarbutton is gone and the JS console features errors that we couldn't find the widget to move. The jetpack widget tests also still break.

I imagine that this is because the widget API uses insertItem on the fly and the nodes we expect to find aren't in the DOM when the window loads. It also seems to be heavily using currentset et al. to figure out where to insert the nodes. I imagine that properly fixing this will require adapting jetpack's widget code to know/care about our API. :-(
Attachment #747381 - Attachment is obsolete: true
Attachment #749929 - Flags: review?(mconley)
Comment on attachment 749929 [details] [diff] [review]
Implement insertItem, fix our querySelector calls, fix appending to the end of a toolbar

Review of attachment 749929 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/content/toolbar.xml
@@ +62,5 @@
> +        <parameter name="aBeforeElt"/>
> +        <parameter name="aWrapper"/>
> +        <body><![CDATA[
> +          if (aWrapper) {
> +            // Unsupported, as widgets are now cross-window

We should probably Cu.reportError instead of just failing silently.

@@ +67,5 @@
> +            return null;
> +          }
> +
> +          // Hack, the customizable UI code makes this be the last position
> +          let pos = "unknown";

null is probably better than "unknown"

@@ +71,5 @@
> +          let pos = "unknown";
> +          if (aBeforeElt) {
> +            let beforeInfo = CustomizableUI.getPlacementOfWidget(aBeforeElt.id);
> +            if (beforeInfo.area != this.id) {
> +              dump("Fail fail fail fail fail");

Cu.reportError instead.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +619,5 @@
> +      let nextNode;
> +      if (nextNodeId) {
> +        nextNode = container.querySelector(idToSelector(nextNodeId));
> +      }
> +      if (nextNode) {

According to https://developer.mozilla.org/en/docs/DOM/Node.insertBefore, if nextNode is null, it automatically appends to the end of the list.

So this can probably be simplified, like:

let nextNode = nextNodeId ? container.querySelector(idToSelector(nextNodeId))
                          : null;
container.insertBefore(widgetNode, nextNode);

@@ +675,5 @@
>          ERROR("Widget not found, unable to move");
>          continue;
>        }
>  
> +      let nextNode;

Same as above.

@@ +1788,5 @@
>  }
>  
> +// When IDs contain special characters, we need to escape them for use with querySelector:
> +function idToSelector(aId) {
> +  return "#" + aId.replace(/[ @\[\]:-]/g, '\\$&');

Good call - I completely discounted these cases.
Attachment #749929 - Flags: review?(mconley)
Comment on attachment 749929 [details] [diff] [review]
Implement insertItem, fix our querySelector calls, fix appending to the end of a toolbar

Review of attachment 749929 [details] [diff] [review]:
-----------------------------------------------------------------

As per comment 22.
Attachment #749929 - Flags: review-
Attached patch Better patchSplinter Review
This addresses the review comments, I hope.

I've also made onWidgetMoved use the getWidgetNode API rather than only using ownerDocument, so that it works if custom code (say, the SDK) uses insertItem with a node from the palette, which is therefore not found from the toolbar's ownerDocument... This fixes the profiler add-on completely for me. I'll check how this fares against the SDK tests later today, but I expect it to help there, too.

Finally, I've used http://mathiasbynens.be/notes/css-escapes to complete the list of crazy CSS stuff that gets escaped (although most of it shouldn't be in IDs in the first place... and I wonder if we don't have an API for this kind of thing somewhere?)
Attachment #749929 - Attachment is obsolete: true
Attachment #750376 - Flags: review?(mconley)
Comment on attachment 750376 [details] [diff] [review]
Better patch

Review of attachment 750376 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1000,4 @@
>        aPosition = 0;
>      } else if (aPosition > placements.length - 1) {
>        aPosition = placements.length - 1;
>      }

Looking at this again, not sure if my block should be placements.length - 1, or if the existing block should not be decrementing placements.length for position use. Surely it's valid to insert as the last item in a toolbar, and that'd need aPosition == placements.length, wouldn't it?
OK, so sadly this doesn't fix the SDK widget tests, although it does improve the situation there quite a bit. I'll need to look into those more. In the meantime, as this fixes a real use case, assuming the patch is OK I'd like to land it while we work to fix the tests.
Comment on attachment 750376 [details] [diff] [review]
Better patch

Review of attachment 750376 [details] [diff] [review]:
-----------------------------------------------------------------

Looks right to me, and I'm so glad to have the profiler add-on working again!

Yes, let's land this.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1000,4 @@
>        aPosition = 0;
>      } else if (aPosition > placements.length - 1) {
>        aPosition = placements.length - 1;
>      }

aPosition == placements.length makes sense to me.
Attachment #750376 - Flags: review?(mconley) → review+
Comment on attachment 750376 [details] [diff] [review]
Better patch

Checked in with == placements.length. I suspect some of the generic fixes that went into the add-on bar patch on bug 872209 will further help the SDK tests, but I haven't checked, will do that tomorrow. Leaving this un-fixed-on-ux for now for that reason.
Attachment #750376 - Flags: checkin+
What's left to do with this bug?
(In reply to Mike Conley (:mconley) from comment #29)
> What's left to do with this bug?

The widget tests are still failing, we just can't see it because the selection test for private browsing windows times out first, and the testing tool the add-on SDK uses doesn't recover from timeouts, so we can't see those failures. I was leaving it open to deal with them; dealing with them requires a decision on the future of the add-ons bar. Whatever we do with it, though, we will also need to adjust the SDK and/or its tests.
Bumping to M6 because we're not going to have a decision on the add-on bar in this milestone.
Whiteboard: [Australis:M5] → [Australis:M6]
I'm going to go ahead and mark this fixed as the toolbarbutton is working, AFAICT (tested this morning), in multiple windows, also when disabling/re-enabling the add-on. Erik/Matteo, if you do still find issues, can you please file followup bugs and CC me?

Separately, the widget tests are still failing but we still don't have final decisions regarding where we're going with the add-on bar, so this isn't something we can fix definitively for now; we should probably file a new bug if/when we still have issues with it after implementing our migration work for the add-on bar, if/when necessary.
Whiteboard: [Australis:M6] → [Australis:M6][fixed-in-ux]
Hmm something is wrong still.  Adding the button to a toolbar seems to work fine now, but as soon as I customize the position of the button then it no longer appears in new windows.
https://hg.mozilla.org/mozilla-central/rev/83d32b7c1dda
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M6][fixed-in-ux] → [Australis:M6]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.