965264 - Opening a new window shows original provider buttons, not updated ones

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Mark Banner (:standard8)]

STR:

1. Have a provider with a default button (e.g. Talkilla)
2. Sign-in, so that the icon changes from the default
3. Open a new window

=> The new window has the default icons, not the new icons seen in step 2 (original window stays unchanged).

Comment 1

[Florian Quèze [:florian]]

Attachment: Fix

This patch fixes the problem and seems harmless, but I'm not completely sure why it's needed.

I think the code at http://hg.mozilla.org/mozilla-central/annotate/25c361f6661f/browser/base/content/browser-social.js#l1016 :
1016   onWidgetAdded: function(aWidgetId, aArea, aPosition) {
1017     let origin = this._getNodeOrigin(aWidgetId);
1018     if (origin)
1019       SocialStatus.updateButton(origin);
1020   },

was expected to do exactly what my patch is doing. I added a dump in there and couldn't see it called. Shane, do you know when this code is called, and if we expect it to be called when opening a new window?

Comment 2

[Shane Caraveo (:mixedpuppy)]

Maybe @mconley knows why onWidgetAdded is not called per window.  Assuming that it is not supposed to be called per window I'd give my r+, but lets see what he says first.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

It's not called per window because onWidgetAdded is fired when a widget is added to an area. An area is something that exists globally... it's like this abstract thing. *waves hands*.

A window might have a node registered to represent that area. If so, CustomizableUI does the job of making sure that node accurately represents that area.

So I guess that's my hand-wavey answer - onWidgetAdded is not called per window because the widget is really only added once.

onWidgetBeforeDOMChange and onWidgetAfterDOMChange are, however, fired per DOM change, as you'd expect - so those fire per-window.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8367979 [details] [diff] [review]
Fix

call updateButton from the loop in ToolbarHelper.populatePalette, that should fix both the status and marks  button.  I'd r+ with that change.

Comment 5

[Florian Quèze [:florian]]

Attachment: Patch v2

SocialMarks doesn't have an updateButton method. I added one and tried to do something that makes sense, but I'm not fully satisfied with the changes in this patch (I was tempted to change ToolbarHelper to take only one parameter: either SocialStatus or SocialMarks; and use the methods there directly, which would avoid the issues with the value of 'this').

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8370159 [details] [diff] [review]
Patch v2

Sorry, I think the original patch is simpler and gets the job done, rather than forcing marks into the same model when it is probably unnecessary.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8367979 [details] [diff] [review]
Fix

Removing the 'obsolete' flag, as we are actually moving forward with this patch.

Comment 8

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/e94372b0ea08

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e94372b0ea08

Comment 10

[Paul Silaghi, QA [:pauly]]

Reproduced in Nightly 2014-01-27.
Verified fixed 30.0a1 (2014-03-10), Win 7 x64.
Note that the widget also changes in private browsing mode.