1080948 - UITour: tell page when Email/Copy Link buttons are clicked

Status: RESOLVED FIXED

(Firefox :: Tours, defect)

Description

[Justin Dolske [:Dolske]]

The Hello tour page needs to know when the user clicks Email / Copy Link (after having created a room), so it can advance to the next step.

As with bug 1080947, the backend of this should probably involve Hello using existing message passing mechanisms to tell chrome (UITour) that one of the buttons was clicked, and then UITour can notify any existing tour pages. API TBD -- probably want the page to explicitly opt-in to receiving notifications/callbacks, but we could also just create and fire DOM events at the tour page.

Comment 1

[Justin Dolske [:Dolske]]

[Mike was out sick today but said he should be able to get it done tomorrow. If the plague strikes again, we can handoff to Matt (although he'll be flying).]

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: UITour: tell the page when a URL is copied or emailed

Hi Blair! Sorry for not using reviewboard yet... I'll switch over soon, promise!

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8537258 [details] [diff] [review]
Patch v1: UITour: tell the page when a URL is copied or emailed

Code LGTM but I haven't had a chance to review the test yet.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8537258 [details] [diff] [review]
Patch v1: UITour: tell the page when a URL is copied or emailed

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

r=me but if someone on the Loop team has a chance to look at stubCache today then it wouldn't hurt.

::: browser/components/loop/LoopRooms.jsm
@@ +553,5 @@
> +  stubCache: function(stub) {
> +    LoopRoomsInternal.rooms.clear();
> +    if (stub) {
> +      // Fill up the rooms cache with room objects provided in the `stub` Map.
> +      for (let [key, value] of stub.entries()) {

I don't think you can s/stub.entries()/stub/ but I may be wrong.

::: browser/components/loop/MozLoopAPI.jsm
@@ +754,1 @@
>      }

Nit: missing comma

::: browser/components/loop/content/shared/js/roomStore.js
@@ +284,5 @@
>       * @param  {sharedActions.CopyRoomUrl} actionData The action data.
>       */
>      copyRoomUrl: function(actionData) {
>        this._mozLoop.copyString(actionData.roomUrl);
> +      this._mozLoop.notifyUITour("Loop:URLCopied");

I think we should include "Room" in the notifications. e.g. Loop:RoomURLCopied

@@ +294,5 @@
>       * @param  {sharedActions.EmailRoomUrl} actionData The action data.
>       */
>      emailRoomUrl: function(actionData) {
>        loop.shared.utils.composeCallUrlEmail(actionData.roomUrl);
> +      this._mozLoop.notifyUITour("Loop:URLEmailed");

ditto

::: browser/modules/test/browser_UITour_loop.js
@@ +11,5 @@
>  
>  Components.utils.import("resource:///modules/UITour.jsm");
>  const { LoopRooms } = Components.utils.import("resource:///modules/loop/LoopRooms.jsm", {});
>  
> +function setupFakeRoom() {

Nit: move the helper to the bottom of the file near the other helper

@@ +111,5 @@
>        document.querySelector("#pinnedchats > chatbox").close();
>      });
>      LoopRooms.open("fakeTourRoom");
>    },
> +  function test_notifyLoopURLCopied(done) {

Nit: test_notifyRoomURLCopied

@@ +126,5 @@
> +          });
> +          chat.close();
> +          done();
> +        });
> +        chat.content.contentWindow.document.querySelector(".btn-copy").click();

I think this can be simplified to get the contentDocument from browser (aka. `content`) directly.

@@ +132,5 @@
> +    });
> +    setupFakeRoom();
> +    LoopRooms.open("fakeTourRoom");
> +  },
> +  function test_notifyLoopURLEmailed(done) {

Nit: test_notifyRoomURLEmailed

@@ +143,5 @@
> +        let composeEmailCalled = false;
> +
> +        gContentAPI.observe((event, params) => {
> +          is(event, "Loop:URLEmailed", "Check Loop:URLEmailed notification");
> +          ok(composeEmailCalled, "mozLoop.composeEmail should be called")

Missing semicolon

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8537258 [details] [diff] [review]
Patch v1: UITour: tell the page when a URL is copied or emailed

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

Regarding the stubCache, r=dmose with this comment somehow addressed.

::: browser/components/loop/LoopRooms.jsm
@@ +561,5 @@
> +    } else {
> +      // Restore the cache to not be stubbed anymore, but it'll need a refresh
> +      // from the server for sure.
> +      gDirty = true;
> +    }

Great to see more testability here. One thing I notice when reviewing the existing code is that there are some methods that (on the surface, at least) look like they should be checking gDirty before accessing this.rooms, but don't.

containsParticipant, and participantsCount look suspect, and I wouldn't be surprised if there are other more subtle cases. If you could do some combination of fixing the easy ones now and spinning off a bug to audit the rest, that would be much appreciated.

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Dan Mosedale (:dmose) - use needinfo? (not reading bugmail regularly) from comment #5)
> Great to see more testability here. One thing I notice when reviewing the
> existing code is that there are some methods that (on the surface, at least)
> look like they should be checking gDirty before accessing this.rooms, but
> don't.
> 
> containsParticipant, and participantsCount look suspect, and I wouldn't be
> surprised if there are other more subtle cases. If you could do some
> combination of fixing the easy ones now and spinning off a bug to audit the
> rest, that would be much appreciated.

Thanks for checking!
`containsParticipant` is an internal helper method used by `checkForParticipantsUpdate` and are used by `getAll` and `get` after receiving fresh data from the server, so that's _after_ properly checking the dirty flag.

The dirty (`gDirty`) flag is used to invalidate the cache at the point where we're not 100% sure that the current rooms list (cache) is not a mirror of the server state anymore. Room mutations (create/ update/ delete) are only recorded in the cache when the server returns OK, so at that point we're also sure that the cache is still in-sync.
When a push-notification comes in, we know that the rooms list was updated on the server side by another client (usually when someone joins a room with the standalone client), at which point we know we're not in-sync anymore. This is when we invalidate the cache and re-fetch the entire list of rooms and flip the dirty flag once they're in - by specifying the version parameter the server will return only the room objects that have actually changed, so no bandwidth is wasted here if all is well.
When the client logs in or out of FxA, we flip the dirty flag as well and fetch the whole list from the server, because the rooms owner identity changed.

So at this point I don't see an inconsistency in the code that requires a bug filed or any other follow-up. Do you agree?

Comment 7

[Mike de Boer [:mikedeboer]]

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=02b95ccfe8d9

Comment 8

[Marco Mucci [:MarcoM]]

Added to IT 37.2

Comment 9

[Mike de Boer [:mikedeboer]]

Before landing I had to convert 'browser_UITour_loop.js' line-ending from Win -> Unix - https://hg.mozilla.org/integration/fx-team/rev/94902e0d2120

Pushed to fx-team with comments addressed: https://hg.mozilla.org/integration/fx-team/rev/b36a41bb3c44

Comment 10

[Mike de Boer [:mikedeboer]]

That push busted Marionette, pushed a follow-up to fx-team:

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b36a41bb3c44
https://hg.mozilla.org/mozilla-central/rev/5a00907a4a6f

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 8537258 [details] [diff] [review]
Patch v1: UITour: tell the page when a URL is copied or emailed

[Triage Comment]

a+ (patch + followup fix) for Aurora+Beta. Needed for Fx35 Hello tour, no unusual risk.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/35dfc261d674
https://hg.mozilla.org/releases/mozilla-aurora/rev/32dd6121c205

Comment 14

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4354e0828136
https://hg.mozilla.org/releases/mozilla-beta/rev/c32afcb0913e