1125181 - Hello should put the conversation (aka room) name in the <title>

Status: VERIFIED FIXED

(Hello (Loop) :: Client, enhancement, P2)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I've set up all links to open in a new tab. When I open a Hello Link which has been named by the sender, a new tab opens with the title of the tab as "Firefox Hello". I see the name of that conversation in the page itself, right of the conversation window. It would be great to have that title in the page's title too so that awesomebar can index it and the other party doesn't have to send the link to me the next time.

Comment 2

[Mark Banner (:standard8)]

To do this in a way that makes most sense for the user (and for development) probably depends on bug 1114563 so that we can get the room name before the room is entered (and hence have the title updated then).

Comment 4

[Mark Banner (:standard8)]

Sevaan, any suggested wording/order here? I think its probably "<brand name> - <conversation name>"?

Comment 5

[Sevaan Franks [:sevaan]]

I think it should be "<conversation name> - <brand name>" to keep consistent with how Mozilla titles other pages.

Comment 6

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1125181-Hello-should-put-the-conversation-aka-ro.patch

Hi,
would you mind take a look to the patch?
Regards,
Mai

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8634643 [details] [diff] [review]
0001-Bug-1125181-Hello-should-put-the-conversation-aka-ro.patch

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

I provided the rest of the necessary feedback on IRC.

::: browser/components/loop/standalone/content/l10n/en-US/loop.properties
@@ +135,5 @@
>  ## LOCALIZATION_NOTE(standalone_title_with_status): {{clientShortname}} will be
>  ## replaced by the brand name and {{currentStatus}} will be replaced
>  ## by the current call status (Connecting, Ringing, etc.)
>  standalone_title_with_status={{clientShortname}} — {{currentStatus}}
> +standalone_title_with_room_name={{romName}} - {{clientShortname}}

Well, with this typo it didn't work, now did it?

Comment 8

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1125181-Hello-should-put-the-conversation-aka-ro.patch

updated

Comment 9

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1125181-Hello-should-put-the-conversation-aka-ro.patch

Updated pr, bad rebase, sorry

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8634683 [details] [diff] [review]
0001-Bug-1125181-Hello-should-put-the-conversation-aka-ro.patch

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

Well, this touches the correct files, you found the correct room state to check for - so I have no feedback there :)

The place where you decided to set the title and the way you attempt it could use a bit of work:

 - Please move the operation to the `StandaloneRoomView` components, inside the `componentWillUpdate` method to be precise; the activeRoomStore is part of that component and whenever we get data in from the store, the state of the component changes, because they're linked together. The `componentWillUpdate` method is called whenever the state object of the component is about to be updated and that's the place where we can put additional logic. (See the ROOM_STATES.MEDIA_WAIT check, for example.)
 - There is a special `DocumentTitleMixin` that we use and adds a `setTitle` method to a component. Please use that, instead of `document.title = x`.

Comment 11

[Mike de Boer [:mikedeboer]]

Oh! And don't forget to add test coverage here! :)

Comment 12

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1125181-Hello-should-put-the-conversation-aka-ro.patch

Hi, 
would you mind reviewing the patch?
Regards

Comment 13

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1125181-Hello-should-put-the-conversation-aka-ro.patch

Missing file on the previous attachment

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8635971 [details] [diff] [review]
0001-Bug-1125181-Hello-should-put-the-conversation-aka-ro.patch

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

I provided quite a bit of feedback, but please don't worry about that. If there's anything I said that you don't understand or agree with, please let me know!

::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
@@ +318,5 @@
>       * @param  {Object} nextProps (Unused)
>       * @param  {Object} nextState Next state object.
>       */
>      componentWillUpdate: function(nextProps, nextState) {
> +      if (this.state.roomState === ROOM_STATES.READY) {

Please change this to:
```js
if (this.state.roomState !== ROOM_STATES.READY &&
    nextState.roomState === ROOM_STATES.READY) {
```

@@ +321,5 @@
>      componentWillUpdate: function(nextProps, nextState) {
> +      if (this.state.roomState === ROOM_STATES.READY) {
> +        var roomName = this.state.roomName;
> +        this.setTitle(mozL10n.get("standalone_title_with_room_name", {
> +          roomName: roomName,

Please make this simply `nextState.roomName || this.state.roomName` and you can remove the variable declaration above.

::: browser/components/loop/standalone/content/l10n/en-US/loop.properties
@@ +134,5 @@
>  
>  ## LOCALIZATION_NOTE(standalone_title_with_status): {{clientShortname}} will be
>  ## replaced by the brand name and {{currentStatus}} will be replaced
>  ## by the current call status (Connecting, Ringing, etc.)
> +standalone_title_with_room_name={{roomName}} - {{clientShortname}}

Three points here:

1. Can you move the LOCALIZATION_NOTE above here to the string it belongs to (standalone_title_with_status)?
2. Can you use the same dash as the string below (—)?
3. Can you add a new LOCALIZATION_NOTE for this string too? You can model it after the one you're supposed to move down in 1). Localization notes are used to provide a bit of metadata for translators' tools so that translations can be assured to be more accurate.

::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
@@ +42,5 @@
> +    sandbox.stub(navigator.mozL10n, "get", function(key, args) {
> +        switch(key) {
> +          case "standalone_title_with_room_name":
> +            return args.roomName;
> +          default: return "";

Please change `return "";` to `return key;` and put it on its own line.

@@ +44,5 @@
> +          case "standalone_title_with_room_name":
> +            return args.roomName;
> +          default: return "";
> +        }
> +      });

Can you fix the indentation of this block?

@@ +102,5 @@
> +          addEventListener: function() {},
> +          document: { addEventListener: function(){} },
> +          setTimeout: function(callback) { callback(); }
> +        };
> +        loop.shared.mixins.setRootObject(fakeWindow);

Does moving this to the first beforeEach() in this file break things horribly?
If it works, please move it there.

@@ +103,5 @@
> +          document: { addEventListener: function(){} },
> +          setTimeout: function(callback) { callback(); }
> +        };
> +        loop.shared.mixins.setRootObject(fakeWindow);
> +        activeRoomStore.setStoreState({roomName: "fakeName", roomState: ROOM_STATES.READY});

Well, this'll need to change if you make this the changes I suggested earlier.

Comment 15

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1125181-Hello-should-put-the-conversation-aka-ro.patch

Updated the pr with your comments, 
regards

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8636337 [details] [diff] [review]
0001-Bug-1125181-Hello-should-put-the-conversation-aka-ro.patch

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

\o/ Looks awesome to me! Ship it!

(Please attach the final patch with the two nits fixed, before you request checkin.)

::: browser/components/loop/standalone/content/l10n/en-US/loop.properties
@@ +137,5 @@
>  ## by the current call status (Connecting, Ringing, etc.)
>  standalone_title_with_status={{clientShortname}} — {{currentStatus}}
> +## LOCALIZATION_NOTE(standalone_title_with_room_name): {{roomName}} will be replaced
> +## by the name of the conversation and {{clientShortname}} will be
> +## replaced by the brand name

nit: missing dot.

::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
@@ +105,5 @@
>          sinon.match.instanceOf(sharedActions.SetupStreamElements));
>      }
>  
>      describe("#componentWillUpdate", function() {
> +      it("should set document.title to roomName and branch when the READY state is dispatched", function() {

s/branch/brand name/

Comment 17

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1125181-Hello-should-put-the-conversation-aka-ro.patch

Nits fixed

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Can we get a try run for this (or a reason why no try run is needed) before asking for checkin-needed?

Comment 19

[Marina Rodríguez [:mai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3211e081be19

Comment 20

[Marina Rodríguez [:mai]]

Sorry, I'm a novice at launching gecko patchs.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 22

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1125181-Hello-should-put-the-conversation-aka-ro.patch

Updated patch

Comment 23

[Pulsebot]

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

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e29a16b9eee0

Comment 25

[Bogdan Maris, Desktop Test Engineering [OUT until 12 Jan]]

I verified that the name of the conversation is correctly displayed in the title bar, 
<Conversation name> - <Firefox Hello> using calls in Firefox 42 beta 4 across platforms (Windows 7 64-bit, Mac OS X 10.10.5 and Ubuntu 12.04 32-bit).