Closed Bug 1023193 Opened 7 years ago Closed 6 years ago

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

Categories

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

defect

Tracking

(firefox34 verified)

VERIFIED FIXED
mozilla34
Tracking Status
firefox34 --- verified

People

(Reporter: standard8, Assigned: mikedeboer)

References

Details

(Whiteboard: [mozilla33 carry over])

Attachments

(1 file)

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.
Priority: -- → P1
Target Milestone: --- → mozilla33
Bug 1005065 should have done this, but missed this case.
Depends on: 1005065
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.
Assignee: standard8 → nobody
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
Assignee: nobody → standard8
Summary: Loop doesn't get automatically added to the toolbar for existing profiles → Loop doesn't get automatically added to the toolbar for existing profiles that have had toolbars customized
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.
likely happening with mike connely on the dependent bug - as needed in Fx team fix.  using this bug for loop team investigation/collaboration.
Whiteboard: p=0, investigation
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.
Whiteboard: p=0, investigation → [p=0][browser]
Assignee: standard8 → nobody
Whiteboard: [p=0][browser] → [p=?, depends on work required after bug 1023304][browser]
Blocks: 1030103
No longer blocks: 1030103
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.
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.
Whiteboard: [p=?, depends on work required after bug 1023304][browser] → --do_not_change-- [mozilla33 carry over]
Target Milestone: mozilla33 → mozilla34
Whiteboard: --do_not_change-- [mozilla33 carry over] → [mozilla33 carry over]
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>
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Hi Mike! Are you comfortable reviewing this? Thanks in advance!
Attachment #8464639 - Flags: review?(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.
Attachment #8464639 - Flags: review?(mconley) → review+
Thanks Mike!

Pushed to fx-team with comment addressed: https://hg.mozilla.org/integration/fx-team/rev/909162b20502
https://hg.mozilla.org/mozilla-central/rev/909162b20502
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Tracking for QA -- I'll add a smoketest for this to make sure it doesn't regress.
QA Contact: anthony.s.hughes
Whiteboard: [mozilla33 carry over] → [mozilla33 carry over][qa+]
Flags: qe-verify+
Whiteboard: [mozilla33 carry over][qa+] → [mozilla33 carry over]
Paul, can you please do some regression testing to verify this is fixed in Firefox 34?
Flags: needinfo?(paul.silaghi)
QA Contact: anthony.s.hughes → paul.silaghi
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 ?
Flags: needinfo?(paul.silaghi) → needinfo?(standard8)
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.
Flags: needinfo?(standard8)
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)
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.
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?
It seems to work fine when upgrading Nightly 2013-01-03 to latest. I'll wait for 34b1 build 2 and re-test.
Everything is fine in FF 34b1 build2.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.