Error in Nightly when opening Hello menu via UITour

RESOLVED FIXED in Firefox 47

Status

Hello (Loop)
Client
P2
normal
Rank:
25
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: agibson, Assigned: standard8)

Tracking

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [btpp-active])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
STR:

1.) In Nightly 47.0a1 (2016-02-09), visit: https://www.mozilla.org/en-US/firefox/hello/
2.) Click on the blue button that says "Try Firefox Hello"

Expected results:

The Hello panel should open

Actual results:

The Hello panel does not open

Error in browser console:

"TypeError: aWindow.LoopUI.openCallPanel is not a function" UITour.jsm:1703:7
(Reporter)

Comment 1

2 years ago
Note: we're currently planning to use this functionality on the new redesigned product page for Hello, which is due to launch March 8.
(Reporter)

Updated

2 years ago
Blocks: 1240980

Updated

2 years ago
No longer blocks: 1240980
(Reporter)

Comment 2

2 years ago
Please don't remove this bug as a blocker :)
Blocks: 1240980
Is this to be uplifted, or out of scope?
Depends on: 1220630
Flags: needinfo?(ianb)
Priority: -- → P1
Whiteboard: [btpp-fix-now]
Can you update with priority and rank as we discussed?
Priority: P1 → --
Whiteboard: [btpp-fix-now]
(Assignee)

Comment 5

2 years ago
We changed the openCallPanel to something else. I think the best way for us to handle this is:

1) to add back the openCallPanel in the add-on (aliased to the new version) for now
2) and then do a patch for ui-tour an let that ride the trains.

Later we can remove the workaround in the add-on.

I think we should do it this way, as we've been looking for a lot of uplifts, and I'd rather keep things confined to the add-on if possible.
Rank: 10
Flags: needinfo?(ianb)
Priority: -- → P1
(Assignee)

Updated

2 years ago
Assignee: nobody → standard8
Created attachment 8718377 [details] [review]
[loop] Standard8:bug-1247255-tour > mozilla:master
(Assignee)

Comment 7

2 years ago
Comment on attachment 8718377 [details] [review]
[loop] Standard8:bug-1247255-tour > mozilla:master

This fixes the basic issue. I'm working on a patch for m-c to update the uitour code to the new API and hopefully add a test for it.
Attachment #8718377 - Flags: review?(mdeboer)
Attachment #8718377 - Flags: review?(edilee)
Attachment #8718377 - Flags: review?(mdeboer)
Attachment #8718377 - Flags: review?(edilee)
Attachment #8718377 - Flags: review+
(Assignee)

Comment 8

2 years ago
This is now fixed in the add-on which should get into nightly over the weekend:

https://github.com/mozilla/loop/commit/6ceea4148fea695c65f45ff23b1be1475ba16b45

There's a follow-up I want to do, so I'm going to leave this open until I have done that.
(Assignee)

Updated

2 years ago
Rank: 10 → 25
(Assignee)

Updated

2 years ago
Priority: P1 → P2
(Assignee)

Comment 9

2 years ago
Created attachment 8720694 [details] [diff] [review]
Update the UITour to use Loop's new API for opening the panel.

This moves us to use Loop's new API for the open panel method. I've also split out the test into two - since the main one doesn't work in e10s mode (and may go away soon since we've changed how the main tour works), it gives us coverage in case we break opening the panel again.
Attachment #8720694 - Flags: review?(MattN+bmo)
Comment on attachment 8720694 [details] [diff] [review]
Update the UITour to use Loop's new API for opening the panel.

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

::: browser/components/uitour/test/browser_UITour_loop_panel.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Nit: tests are PD by default so this isn't necessary anymore

@@ +14,5 @@
> +const { LoopAPI } = Cu.import("chrome://loop/content/modules/MozLoopAPI.jsm", {});
> +const { LoopRooms } = Cu.import("chrome://loop/content/modules/LoopRooms.jsm", {});
> +const { MozLoopServiceInternal } = Cu.import("chrome://loop/content/modules/MozLoopService.jsm", {});
> +
> +const FTU_VERSION = 1;

FTU_VERSION and MozLoopServiceInternal seem unused.

There seems to be a lot of other boilerplate that seems unused related to rooms. Do we need that if we're just checking if the panel opens?

@@ +21,5 @@
> +  UITourTest();
> +}
> +
> +var tests = [
> +  taskify(function* test_menu_show_hide() {

I would really prefer if new UITour tests were written the new way with `add_UITour_task` which is more other b-c tests and works better with e10s. See https://reviewboard.mozilla.org/r/32573/diff/4#3 for an example.

@@ +56,5 @@
> +  is(loopButton.hasAttribute("open"), false, "Loop button should know that the panel is closed");
> +}
> +
> +if (Services.prefs.getBoolPref("loop.enabled")) {
> +  loopButton = window.LoopUI.toolbarButton.node;

Do we still need to support this pref in tests?
Attachment #8720694 - Flags: review?(MattN+bmo) → feedback+
Whiteboard: [btpp-fix-now] → [btpp-active]
(Assignee)

Comment 11

2 years ago
(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #10)
> Comment on attachment 8720694 [details] [diff] [review]
> 
> There seems to be a lot of other boilerplate that seems unused related to
> rooms. Do we need that if we're just checking if the panel opens?

I thought we did, but re-checking what's happening, we don't.

> @@ +56,5 @@
> > +  is(loopButton.hasAttribute("open"), false, "Loop button should know that the panel is closed");
> > +}
> > +
> > +if (Services.prefs.getBoolPref("loop.enabled")) {
> > +  loopButton = window.LoopUI.toolbarButton.node;
> 
> Do we still need to support this pref in tests?

Yes, on ESR we're turning off Loop and using the pref to do it.
(Assignee)

Comment 12

2 years ago
Created attachment 8722165 [details] [diff] [review]
Update the UITour to use Loop's new API for opening the panel.

Updated patch.
Attachment #8722165 - Flags: review?(MattN+bmo)
(Assignee)

Updated

2 years ago
Attachment #8720694 - Attachment is obsolete: true
Comment on attachment 8722165 [details] [diff] [review]
Update the UITour to use Loop's new API for opening the panel.

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

Thanks
Attachment #8722165 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 14

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/bc194ea89e64784a573c4bb45dcd516beecd8c27
Bug 1247255 - Update the UITour to use Loop's new API for opening the panel. r=MattN
(Assignee)

Updated

2 years ago
Iteration: --- → 47.3 - Mar 7

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bc194ea89e64
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.