Move Loop button to the palette as default area

VERIFIED FIXED in Firefox 34

Status

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

unspecified
mozilla34
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox34+ verified)

Details

(Whiteboard: [loop-uplift])

Attachments

(1 attachment, 6 obsolete attachments)

[Tracking Requested - why for this release]:

In Fx34 the Loop toolbar button will be gradually enabled in the palette. See bug 1055319 for more details about this mechanism.
Flags: qe-verify+
Flags: firefox-backlog+
This bug needs to be tested with a clean profile.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Attachment #8503254 - Attachment is obsolete: true
Attachment #8503254 - Flags: review?(adam)
Attachment #8503257 - Flags: review?(jaws)
Blocks: 1073215
Attachment #8503257 - Attachment is obsolete: true
Attachment #8503257 - Flags: review?(jaws)
Attachment #8503266 - Flags: review?(jaws)
[Tracking Requested - why for this release]:
Target Milestone: --- → mozilla34
Comment on attachment 8503266 [details] [diff] [review]
Patch v1.1: move the Loop button to the palette by default

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

1:06 PM <mikedeboer> ahk. well, I think the loop button needs to move into a block that checks the prefs in CustomizableWidgets.jsm, because the palette is lazy loaded
1:07 PM <mikedeboer> so the code in browser-loop.js won't even work
Attachment #8503266 - Flags: review?(jaws)
This should do it... I wrapped the pref getters in a try...catch, because they were throwing in my local Aurora debug build, even though they're set in firefox.js.

I'd like to ask if someone could land this patch in case of an r+ or take it the rest of the way while I'm asleep. Thanks!
Attachment #8503266 - Attachment is obsolete: true
Attachment #8503447 - Flags: review?(jaws)
Attachment #8503447 - Flags: review?(adam)
This one will make all the tests still pass.
Attachment #8503447 - Attachment is obsolete: true
Attachment #8503447 - Flags: review?(jaws)
Attachment #8503447 - Flags: review?(adam)
Attachment #8503456 - Flags: review?(jaws)
Attachment #8503456 - Flags: review?(adam)
Comment on attachment 8503456 [details] [diff] [review]
Patch v1.3: move the Loop button to the palette by default

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

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

This should be conditional on softstart so reset defaults work. Are we doing that in this bug?

::: browser/components/loop/MozLoopService.jsm
@@ +1036,5 @@
>  
> +    let ticket;
> +    try {
> +      ticket = Services.prefs.getIntPref("loop.soft_start_ticket_number");
> +    } catch (ex) {}

This isn't necessary since the pref is in firefox.js and this code is in browser/.

(In reply to Mike de Boer [:mikedeboer] from comment #7)
> Created attachment 8503447 [details] [diff] [review]
> Patch v1.2: move the Loop button to the palette by default
> 
> This should do it... I wrapped the pref getters in a try...catch, because
> they were throwing in my local Aurora debug build, even though they're set
> in firefox.js.

I suspect your build has something weird going on.

@@ +1087,5 @@
>          // Hot diggity! It's our turn! Activate the service.
>          log.info("MozLoopService: Activating Loop via soft-start");
>          Services.prefs.setBoolPref("loop.throttled", false);
> +        if (buttonNode) {
> +          buttonNode.hidden = false;

So we're not adding the button to the navbar in this bug?

@@ +1105,5 @@
>      // ensure that these addresses aren't routable, the highest 8 bits must
>      // be "127" (reserved for localhost).
> +    let host;
> +    try {
> +      host = Services.prefs.getCharPref("loop.soft_start_hostname");

It's going to be a problem if this isn't set. This needs investigation.
Comment on attachment 8503456 [details] [diff] [review]
Patch v1.3: move the Loop button to the palette by default

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

r+ but I'm not sure what we should do with firefox.js, maybe we should adjust the #ifdef so that it is only enabled by default for Nightly or Aurora? We will want to get testing in on the soft start mechanism, and waiting for the EARLY_BETA_OR_EARLIER flag to become false may be too late to fix any bugs that crop up.

::: browser/base/content/browser-loop.js
@@ +99,5 @@
>        } else if (MozLoopService.doNotDisturb) {
>          state = "disabled";
>        }
> +      let toolbarButton = this.toolbarButton;
> +      if (toolbarButton && toolbarButton.node) {

Move this up to the top of the function and return early if !toolbarButton || !toolbarButton.node.

::: browser/components/loop/MozLoopService.jsm
@@ +1036,5 @@
>  
> +    let ticket;
> +    try {
> +      ticket = Services.prefs.getIntPref("loop.soft_start_ticket_number");
> +    } catch (ex) {}

It is failing because loop.soft_start_ticket_number and loop.soft_start_hostname is only defined for late Betas and Release.

If we want soft start to be enabled on the early betas then we will need to fix firefox.js.

@@ +1087,5 @@
>          // Hot diggity! It's our turn! Activate the service.
>          log.info("MozLoopService: Activating Loop via soft-start");
>          Services.prefs.setBoolPref("loop.throttled", false);
> +        if (buttonNode) {
> +          buttonNode.hidden = false;

As I understand it, 34 is only supposed to have the button appear in the palette after the soft start mechanism has decided it can appear. 35 is what will add it to the navbar.
Attachment #8503456 - Flags: review?(jaws) → review+
Comment on attachment 8503456 [details] [diff] [review]
Patch v1.3: move the Loop button to the palette by default

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

This looks functionally correct from a loop perspective. I agree with Jared's comments about wanting this to work in early beta, so we'll want to pull the conditionals out of firefox.js. r+ conditional on that change.

::: browser/components/loop/MozLoopService.jsm
@@ +1036,5 @@
>  
> +    let ticket;
> +    try {
> +      ticket = Services.prefs.getIntPref("loop.soft_start_ticket_number");
> +    } catch (ex) {}

Since we're basically landing this on Beta, I think the right way to handle this is to pull the conditionals out of firefox.js.

@@ +1105,5 @@
>      // ensure that these addresses aren't routable, the highest 8 bits must
>      // be "127" (reserved for localhost).
> +    let host;
> +    try {
> +      host = Services.prefs.getCharPref("loop.soft_start_hostname");

This is the same conditional problem as above -- fix it in firefox.js, and remove the try block here.
Attachment #8503456 - Flags: review?(adam) → review+
We will land this right after the merge to Beta since we only want this change on Beta. Anthony will test after it lands on Beta. We may want Anthony and Nils to work together to verify the move to customize with the throttle.

In the meantime we want to get this patch ready for landing and QA testing (post merge) and make sure it passes all the automated tests. If someone could update the patch based on review comments and push a new try as soon as possible (first thing Monday if not sooner), I'd really appreciate it. I know this is assigned to Mike, but I also know he'd appreciate help getting this ready to land and so would I.  Thanks!
Mike, Can you get this ready for landing & test?  (See my last comment - comment 13.)
Flags: needinfo?(mdeboer)
See Also: → 1081888
Patch with comments addressed and commit message updated. Ready for landing.

Filed bug 1081888 to backport some of the fixes here to Loop in general.
Attachment #8503456 - Attachment is obsolete: true
Attachment #8504013 - Flags: review+
Flags: needinfo?(mdeboer)
Try push with browser_UITour_availableTargets.js fixes: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f29b3ded85d9
Patch with bustage fix, ready to land
Attachment #8504013 - Attachment is obsolete: true
Attachment #8504148 - Flags: review+
This should be a try push against the new Beta: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b295b523f02e

Anthony/Pauly -- I think we're ready to land and test this.
Flags: needinfo?(paul.silaghi)
Flags: needinfo?(anthony.s.hughes)
Comment on attachment 8504148 [details] [diff] [review]
Patch v1.5: move the Loop button to the palette by default

Approval Request Comment
[Feature/regressing bug #]: Moving Loop button to Customize for Fx34 only
[User impact if declined]: Loop button would appear on toolbar, which we can't have for Fx34 release
[Describe test coverage new/current, TBPL]: tested locally, manually verified by QA after uplift
[Risks and why]: We need Fx34 to be a soft launch for Hello -- on the Customize menu, not on the toolbar
[String/UUID change made/needed]: No strings
Attachment #8504148 - Flags: approval-mozilla-beta?
Comment on attachment 8504148 [details] [diff] [review]
Patch v1.5: move the Loop button to the palette by default

Beta+

jesup - Please land ASAP so that this can make beta1.
Attachment #8504148 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(rjesup)
Since this is a beta-only landing, it's safe to resolve this bug and ready it for verification.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
To verify this using Beta and get past the throttling mechanism, you will need to flip the loop.throttled pref in about:config.  (The default is true;  you want to flip it to false.)  

If loop.throttled is true, there is a 90% chance that you won't see the Loop button.
Paul, please test this as part of 34b1 sign-off.
Flags: needinfo?(anthony.s.hughes)
QA Contact: anthony.s.hughes → paul.silaghi
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #24)
> To verify this using Beta and get past the throttling mechanism, you will
> need to flip the loop.throttled pref in about:config.  (The default is true;
> you want to flip it to false.)  
> 
> If loop.throttled is true, there is a 90% chance that you won't see the Loop
> button.
Verified fixed FF 34b1 Win 7, OS X 10.9.5
Status: RESOLVED → VERIFIED
Flags: needinfo?(paul.silaghi)
You need to log in before you can comment on or make changes to this bug.