Closed Bug 1215570 Opened 4 years ago Closed 4 years ago

Update the room's context whenever the domain/tab changes whilst tab sharing

Categories

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

defect

Tracking

(firefox45 verified)

VERIFIED FIXED
mozilla45
Iteration:
45.3 - Dec 14
Tracking Status
firefox45 --- verified

People

(Reporter: standard8, Assigned: mancas)

References

Details

(Whiteboard: [web sharing])

User Story

Acceptance criteria:

- A change is defined as: a user navigates to a different domain, or a user changes to a tab which is a different domain.
- When in a tab share, if a change is detected, then the updated context is given to the room and saved to the server. If the room name hasn't been set then it gets updated in the panel and the conversation window (bug 1214582).

Notes:

- Standalone changes are NOT part of this bug.
- Inserting into text chat is NOT part of this bug (but may need to be considered).
- We'll need to be careful only to update the context once - i.e. we don't want to update the context for the domain/title, then get the favicon, then update again.

Attachments

(2 files, 5 obsolete files)

As part of the user journey rework, we need to update a room context whenever the page/tab changes whilst tab sharing. This depends on bug 1214215 as it won't make much sense to add until we've done that.

See the user story for more detail.
Rank: 18
Blocks: 1215612
How immediately should the context be updated when navigating on the same tab? The only thing we have immediately on location change is the url, so no title or description. This can be done with gBrowser.addProgressListener({onLocationChange}).

For "different domain," is that the full domain with subdomains or eTLD+1/base domain?

And even if the domain is the same, different pages would have different titles, e.g., different bugs on bugzilla.

If we only handle tab changes, there's the existing TabSelect listener via addBrowserSharingListener that could trigger mozLoop.getSelectedTabMetadata to update the context.
Assignee: nobody → edilee
Attached video v1 video
Attachment #8681141 - Flags: ui-review?(sfranks)
Comment on attachment 8681141 [details]
v1 video

Awesome.
Attachment #8681141 - Flags: ui-review?(sfranks) → ui-review+
Hey Ed, do you mind to upload a wip patch to start thinking how to solve bug 1215612? In the meantime, I'm going to implement the css rules to fit the mockup
Flags: needinfo?(edilee)
Attached patch wip (obsolete) — Splinter Review
Flags: needinfo?(edilee)
Iteration: --- → 45.2 - Nov 30
Assignee: edilee → nobody
Assignee: nobody → b.mcb
Status: NEW → ASSIGNED
Mark, after apply Ed's wip patch and test it, I've noted that in some websites the DOMTitleChange is received several times e.g booking.com

Also I've added a few asserts in the test to ensure the getMetadata function is called. Do we want to update the title in the same domain? I mean, when the user navigates through a website,
when he click an url, the actions throws a title change event because the title is change, so from my point of view the context is changing. wdyt?
Attachment #8683154 - Attachment is obsolete: true
Attachment #8688911 - Flags: feedback?(standard8)
(In reply to Manuel Casas Barrado [:mancas] from comment #6)
> Created attachment 8688911 [details] [diff] [review]
> Update the room's context whenever the domain/tab changes whilst tab sharing.
> 
> Mark, after apply Ed's wip patch and test it, I've noted that in some
> websites the DOMTitleChange is received several times e.g booking.com

We should probably add a check somewhere to ensure that we don't update the context if the title hasn't changed. If the title is changing frequently as a page loads, then maybe we should implement something like a one second hold-off to save flooding the server.

> Also I've added a few asserts in the test to ensure the getMetadata function
> is called. Do we want to update the title in the same domain? I mean, when
> the user navigates through a website,
> when he click an url, the actions throws a title change event because the
> title is change, so from my point of view the context is changing. wdyt?

Sevaan's already had something thinking about context updates, so he's best to ask about this.
Flags: needinfo?(sfranks)
Comment on attachment 8688911 [details] [diff] [review]
Update the room's context whenever the domain/tab changes whilst tab sharing.

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

::: browser/components/loop/test/shared/activeRoomStore_test.js
@@ +1590,5 @@
>          type: "browser"
>        }));
>  
>        sinon.assert.calledOnce(fakeSdkDriver.startScreenShare);
> +      sinon.assert.calledOnce(fakeMozLoop.getSelectedTabMetadata);

These should in be separate it() statements so that each thing a function does has its own statement, and its easy to see from reading just the "it"s everything that a function should be doing.

Additionally, you'll also need a test or two for the callback to check that the action is dispatched as expected.
Attachment #8688911 - Flags: feedback?(standard8)
> > Also I've added a few asserts in the test to ensure the getMetadata function
> > is called. Do we want to update the title in the same domain? I mean, when
> > the user navigates through a website,
> > when he click an url, the actions throws a title change event because the
> > title is change, so from my point of view the context is changing. wdyt?
> 
> Sevaan's already had something thinking about context updates, so he's best
> to ask about this.

Yes, let's update the title on any new page load. But a tile only gets displayed the first time the domain changes.
Flags: needinfo?(sfranks)
Sevaan I'm not sure if I'm understanding you. What you mean is if we are in the same domain, even if we navigate to other page, the conversation window title doesn't change?

It's no sense for me. Imagine this scenario: I open a new conversation in this bug so the title is "Bug 1215570 bla bla..". Now I navigate to other bug, e.g 1136991. A context change message appears in my chat because of the page load event, but the conversation title is still pointing to the other bug so it's a bit confusing, isn't it?
Flags: needinfo?(sfranks)
Hey Manu,

Not quite.

The conversation title changes on every new page load to match the title of the page.
A tile, however, only appears in chat with

(In reply to Manuel Casas Barrado [:mancas] from comment #10)
> Sevaan I'm not sure if I'm understanding you. What you mean is if we are in
> the same domain, even if we navigate to other page, the conversation window
> title doesn't change?

That's incorrect. The page conversation title should change with every new pageload, to reflect the title of the page being displayed.

> It's no sense for me. Imagine this scenario: I open a new conversation in
> this bug so the title is "Bug 1215570 bla bla..". Now I navigate to other
> bug, e.g 1136991. A context change message appears in my chat because of the
> page load event, but the conversation title is still pointing to the other
> bug so it's a bit confusing, isn't it?

You have it reversed...the conversation title should change with each bug you navigate to, but the message in chat only shows up once, when you first land on bugzilla.mozilla.org. When you surf to another different domain, then that tile shows up in chat (not replacing the first tile, but as a new tile in the history of the chat).

Does that make sense?
Flags: needinfo?(sfranks)
Ok Sevaan. It has more sense now, although what you're talking about will be done in bug 1215612. So let's change the title of the conversation window every time the page loads, taking into account the multiple events we could receive in order to avoid flooding the server.

Mark, I realize that when I visit booking.com, the first two or three events I receive, don't have the favicon. So if we implement any mechanism to avoid flooding the server, perhaps we couldn't recover the favicon at first time. A simple solution could be, trying to recover the favicon when we render every room entry. wdyt?
Flags: needinfo?(standard8)
(In reply to Manuel Casas Barrado [:mancas] from comment #12)
> Mark, I realize that when I visit booking.com, the first two or three events
> I receive, don't have the favicon. So if we implement any mechanism to avoid
> flooding the server, perhaps we couldn't recover the favicon at first time.
> A simple solution could be, trying to recover the favicon when we render
> every room entry. wdyt?

Why not do the flood prevention the other way around - don't update the room until x milliseconds have elapsed? We might not want to do something differently for the tiles themselves - but I think for the context as saved on the server, it wouldn't hurt for a slight delay. We'll probably need to test it to see how it feels.

For the favicon, we're going to need to save/send it for the context tile, but also it will need to be saved in the context - otherwise, the remote parties are going to be needing to load that image every time, and that opens up tracking possibilities.
Flags: needinfo?(standard8)
Flood prevention first approach. After receive a DOMTitlteChanged, we update the room context after 300ms have elapsed.

I'd like to test it but I can't due to the error I get when trying to share tabs, also commented on IRC. In the meantime, would you mind to take a look at the approach and give me some feedback?
Attachment #8691899 - Flags: feedback?(standard8)
Attachment #8688911 - Attachment is obsolete: true
I've updated the patch to fit the new folder structure. Also the unit test is fixed.
Attachment #8693522 - Flags: feedback?(standard8)
Attachment #8691899 - Attachment is obsolete: true
Attachment #8691899 - Flags: feedback?(standard8)
Comment on attachment 8693522 [details] [diff] [review]
Update the rooms context whenever the domain/tab changes whilst tab sharing

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

Sorry for the delay. In general looks good, though I've not tested it yet. There's a few comments in-line of things to consider.

::: browser/extensions/loop/content/panels/js/roomStore.js
@@ +489,5 @@
>  
>          var roomData = {};
>          var context = result.decryptedContext;
>          var oldRoomName = context.roomName;
> +        var newRoomName = (actionData.newRoomName || "").trim();

I think we should have protection in this function against doing unnecessary updates, e.g. a person reloads the page they are sharing and hence we'd go do the update a second time.

::: browser/extensions/loop/content/shared/js/activeRoomStore.js
@@ +937,5 @@
> +            newRoomURL: meta.url,
> +            roomToken: this.getStoreState().roomToken
> +          }));
> +          updateContextTimer = null;
> +        }.bind(this), 500);

I've not tested this, but this looks like it'll only update once every half second with the *first* data received, not the latest.

I think this would be reasonable. The other way to do it would be to delay updates until we've completed the server update for the previous time - though we'd still probably want to rate limit that a bit, in case of fast server responses.

::: browser/extensions/loop/test/shared/activeRoomStore_test.js
@@ +1610,5 @@
>          }
>        });
>      });
> +
> +    it("should request the new metadata when the browser being shared change", function() {

We should have a second test here, for checking that if two come in during the timeout, then the second one gets the data sent.
Attachment #8693522 - Flags: feedback?(standard8) → feedback+
Iteration: 45.2 - Nov 30 → 45.3 - Dec 14
Priority: P2 → P1
(In reply to Mark Banner (:standard8) from comment #16)
> Comment on attachment 8693522 [details] [diff] [review]
> Update the rooms context whenever the domain/tab changes whilst tab sharing
> 
> Review of attachment 8693522 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay. In general looks good, though I've not tested it yet.
> There's a few comments in-line of things to consider.
> 
> ::: browser/extensions/loop/content/panels/js/roomStore.js
> @@ +489,5 @@
> >  
> >          var roomData = {};
> >          var context = result.decryptedContext;
> >          var oldRoomName = context.roomName;
> > +        var newRoomName = (actionData.newRoomName || "").trim();
> 
> I think we should have protection in this function against doing unnecessary
> updates, e.g. a person reloads the page they are sharing and hence we'd go
> do the update a second time.

We've already a mechanism to avoid this behaviour. It check if the room name has changed and if the old and the updated url are the same.

> // When no properties have been set on the roomData object, there's nothing
> // to save.
> if (!Object.getOwnPropertyNames(roomData).length) {
 
> ::: browser/extensions/loop/content/shared/js/activeRoomStore.js
> @@ +937,5 @@
> > +            newRoomURL: meta.url,
> > +            roomToken: this.getStoreState().roomToken
> > +          }));
> > +          updateContextTimer = null;
> > +        }.bind(this), 500);
> 
> I've not tested this, but this looks like it'll only update once every half
> second with the *first* data received, not the latest.

Hmmm, if a new request arrives, the old one is discarded. We're clearing the timeout and setting it again with the new data right?

> I think this would be reasonable. The other way to do it would be to delay
> updates until we've completed the server update for the previous time -
> though we'd still probably want to rate limit that a bit, in case of fast
> server responses.

I like the idea of having a queue of requests. However, from my point of view there is no need for update the context in this edge situation because the update it's always the same.

> ::: browser/extensions/loop/test/shared/activeRoomStore_test.js
> @@ +1610,5 @@
> >          }
> >        });
> >      });
> > +
> > +    it("should request the new metadata when the browser being shared change", function() {
> 
> We should have a second test here, for checking that if two come in during
> the timeout, then the second one gets the data sent.

Test added, please review it

Thanks!
Attachment #8693522 - Attachment is obsolete: true
Comment on attachment 8694102 [details] [diff] [review]
Update the rooms context whenever the domain/tab changes whilst tab sharing

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

Ok, looks good. This will need a bitrot fix, and there's one small nit to address.

::: browser/extensions/loop/bootstrap.js
@@ +573,5 @@
>          return removed;
>        },
>  
>        /**
> +       * Boradcast 'BrowserSwitch' event.

nit: Broadcast (spelling)
Attachment #8694102 - Flags: review?(standard8) → review+
Thanks for the review Mark. All comments addressed and patch ready to be landed.
Attachment #8696686 - Flags: review+
Attachment #8694102 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/06a9506d70b9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Flags: qe-verify+
QA Contact: bogdan.maris
I did some exploratory testing using latest Nightly 45.0a1 and 46.0a1 across platforms (Windows 7 64-bit, Mac OS X 10.11.1 and Ubuntu 14.04 32-bit) around this fix and I found one new issue (bug 1233425) that does not seem major and will be dealt separately. Marking this as verified fixed based on my testing.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.