1023193 - Loop doesn't get automatically added to the toolbar for existing profiles that have had toolbars customized

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mark Banner (:standard8)]

Somehow missed this previously - the loop button isn't getting added to the toolbar for existing profiles. It does get added for new profiles.

I'll dig into it this afternoon to at least try and figure out what needs to happen.

Comment 1

[Mark Banner (:standard8)]

Bug 1005065 should have done this, but missed this case.

Comment 2

[Mark Banner (:standard8)]

Ok, if I'm reading this line correctly:

http://hg.mozilla.org/mozilla-central/annotate/9dc0ffca10f4/browser/components/customizableui/src/CustomizableUI.jsm#l272

Then there's no pre-existing functionality in the customizable UI to upgrade a user's "currentset" of icons when we change the "defaultset".

To fix this bug, we'd need to write this code.

Comment 3

[Mark Banner (:standard8)]

I've just been talking to Gijs on irc. He has suggested that we need to move from a hard-coded XUL button to a CUI.createWidget. Docs on this are here:

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/CustomizableUI.jsm/API-provided_widgets
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/CustomizableUI.jsm

See also http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/src/CustomizableWidgets.jsm#336

Comment 4

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

I don't believe that just switching to an API-created widget will fix this issue - the "New e10s window" button, for example, suffered the same issue as the Loop button, and was only in the menu panel by default in uncustomized profiles.

Comment 5

[:shell escalante]

likely happening with mike connely on the dependent bug - as needed in Fx team fix.  using this bug for loop team investigation/collaboration.

Comment 6

[Mark Banner (:standard8)]

I took a look at implementing a customisable widget instead of the xul button as part of bug 1011392.

Our "requirements" for the Loop panel are roughly:

- It has an iframe, that is only created once when the user opens the panel
- Since only one panel can be visible at any one time, the iframe is shared across locations and different windows.

The main issues that I remember coming up against were:

- There wasn't enough notifications on customisable widgets to support moving an iframe as part of a panel from one dom location to another.

This is what SharedFrame currently does for us.

- Using a "custom" widget type, I generally found it was creating the widget on startup, as it was creating the actual button, although I could probably attach a panel to it, creating the iframe at that stage was still too early.


There may already be ways around these - moving to a customisable widget originally seemed easy (and that it may have fixed some of our issues), but having already had a patch for bug 1011392 almost complete, I decided to complete that as it seemed easier.

Comment 7

[Romain Testard [:RT]]

Is there an update on this one? 
It seems we should have this one fixed for internal promotion or at least communicate the procedure for existing nightly users with customized toolbars.

Comment 8

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

I believe what we need to do is get bug 1023304 selected during the Firefox backlog triage. If gavin, chad, or madhava know how important bug 1023304 is to Loop, then I think it's more likely to get selected.

I'll put a needinfo in that bug to see if we can get it queued.

Comment 9

[Mark Banner (:standard8)]

From Mike Conley, this is what should be needed to fix this bug, now that bu 1023304 is fixed:

* Switch the Loop button from a xul button in browser.xul to a button defined in CustomizableWidget
* Set "introducedInVersion: 1" on the button
* Bump kVersion in CustomisableUI.jsm from 0 to 1.

<quote>and that _should_ kick it off</quote>

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: make a customizableWidget out the Loop toolbar button

Hi Mike! Are you comfortable reviewing this? Thanks in advance!

Comment 11

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

Comment on attachment 8464639 [details] [diff] [review]
Patch v1: make a customizableWidget out the Loop toolbar button

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

So this works, so long as you remove the underscore before _introducedInVersion. When I manually edited the code to not have the underscore, everything worked as expected.

See also my note about the const.

r=me with those things fixed.

::: browser/components/customizableui/CustomizableUI.jsm
@@ +51,5 @@
>  /**
>   * The current version. We can use this to auto-add new default widgets as necessary.
>   * (would be const but isn't because of testing purposes)
>   */
> +let kVersion = 1;

Not your doing, but I just noticed - I guess this should be a constant instead?

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +904,5 @@
> +    // XXX Bug 1013989 will provide a label for the button
> +    label: "loop-call-button.label",
> +    tooltiptext: "loop-call-button.tooltiptext",
> +    defaultArea: CustomizableUI.AREA_NAVBAR,
> +    _introducedInVersion: 1,

Ah, this doesn't need to be underscored here - it gets translated to the underscored property in the generated widget.

Comment 12

[Mike de Boer [:mikedeboer]]

Thanks Mike!

Pushed to fx-team with comment addressed: https://hg.mozilla.org/integration/fx-team/rev/909162b20502

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/909162b20502

Comment 14

[u279076]

Tracking for QA -- I'll add a smoketest for this to make sure it doesn't regress.

Comment 15

[u279076]

Paul, can you please do some regression testing to verify this is fixed in Firefox 34?

Comment 16

[Paul Silaghi, QA [:pauly]]

I'm not sure exactly what this bug covers, but here is what I did:
1. Start FF 20b1 with a new profile
2. Pave over install of FF 34b1
3. Start FF 34b1 with the same profile
AR: No Loop button
Thoughts ?

Comment 17

[Maire Reavy [:mreavy]]

You're seeing the throttle code in effect in Beta.  With loop.throttled set to true (which it is in Beta), you have a 90% chance of not seeing the Loop button.  If you flip loop.throttled to "true", you should see it.

Comment 18

[Paul Silaghi, QA [:pauly]]

loop.throttled is set to FALSE (I'm using 34b1 build 1, bug 1081192 didn't make it here, it will enter only in build 2, if I understood correctly)

Comment 19

[Adam Roach [:abr]]

Bug 1081192 moved the button from the default toolbar to the palette for Beta 34. Appearing in the toolbar will happen for 35; although it will be similarly throttled.

Comment 20

[Maire Reavy [:mreavy]]

ah, my apologies, Pauly.  You're right loop.throttled wouldn't be a factor until build 2.

Can you test this first on Nightly and tell us what you see?

Comment 21

[Paul Silaghi, QA [:pauly]]

It seems to work fine when upgrading Nightly 2013-01-03 to latest. I'll wait for 34b1 build 2 and re-test.

Comment 22

[Paul Silaghi, QA [:pauly]]

Everything is fine in FF 34b1 build2.