Add link to Hangouts Remote Desktop in the It2Me section of the remoting app.

Add link to Hangouts Remote Desktop in the It2Me section of the remoting app.

BUG=237912
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203912

[Jamie, 2013-05-07 03:40:34]

I thought we were waiting for a UI review for this before landing it.

[Sergey Ulanov, 2013-05-07 18:01:05]

On 2013/05/07 03:40:34, Jamie wrote:
> I thought we were waiting for a UI review for this before landing it.

It looks like review will take some time and we can always change it later based
on the review. I uploaded what I have now - if you think it's necessary we can
wait for UI review before landing it.

[Jamie, 2013-05-07 18:24:27]

I think it's better to wait for the review, especially since there's some
cross-branding questions involved (should we a Hangouts button, for example,
instead of a link).

https://codereview.chromium.org/14803010/diff/2002/remoting/resources/remotin...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/14803010/diff/2002/remoting/resources/remotin...
remoting/resources/remoting_strings.grd:556: Want to help someone while having a
video chat with them too? Try <ph name="LINK_BEGIN">$1<ex>&lt;a
href=https://plus.google.com/hangouts&gt;</ex></ph> Remote Desktop app in Google
Hangouts<ph name="LINK_END">$2<ex>&lt;/a&gt;</ex></ph>.
I don't think "app" is necessary here.

https://codereview.chromium.org/14803010/diff/2002/remoting/webapp/main.html
File remoting/webapp/main.html (left):

https://codereview.chromium.org/14803010/diff/2002/remoting/webapp/main.html#...
remoting/webapp/main.html:138: </div>
It's hard to tell from the way codereview is rendering the diff, but have you
removed an enclosing <div> here? Is it necessary for this CL?

https://codereview.chromium.org/14803010/diff/2002/remoting/webapp/main.html
File remoting/webapp/main.html (right):

https://codereview.chromium.org/14803010/diff/2002/remoting/webapp/main.html#...
remoting/webapp/main.html:152: <p id="hangouts-invitation"
I don't think this node needs an id. It's not used anywhere.

[Sergey Ulanov, 2013-05-23 18:16:18]

Move the new message to the butter bar and added correct link to the hangouts
app.

https://codereview.chromium.org/14803010/diff/2002/remoting/resources/remotin...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/14803010/diff/2002/remoting/resources/remotin...
remoting/resources/remoting_strings.grd:556: Want to help someone while having a
video chat with them too? Try <ph name="LINK_BEGIN">$1<ex>&lt;a
href=https://plus.google.com/hangouts&gt;</ex></ph> Remote Desktop app in Google
Hangouts<ph name="LINK_END">$2<ex>&lt;/a&gt;</ex></ph>.
On 2013/05/07 18:24:27, Jamie wrote:
> I don't think "app" is necessary here.

Done.

https://codereview.chromium.org/14803010/diff/2002/remoting/webapp/main.html
File remoting/webapp/main.html (left):

https://codereview.chromium.org/14803010/diff/2002/remoting/webapp/main.html#...
remoting/webapp/main.html:138: </div>
On 2013/05/07 18:24:27, Jamie wrote:
> It's hard to tell from the way codereview is rendering the diff, but have you
> removed an enclosing <div> here? Is it necessary for this CL?

Yes, here I just removed a div that was wrapping another div. Reverted now
because it's not related to this CL.

https://codereview.chromium.org/14803010/diff/2002/remoting/webapp/main.html
File remoting/webapp/main.html (right):

https://codereview.chromium.org/14803010/diff/2002/remoting/webapp/main.html#...
remoting/webapp/main.html:152: <p id="hangouts-invitation"
On 2013/05/07 18:24:27, Jamie wrote:
> I don't think this node needs an id. It's not used anywhere.

Done.

[Jamie, 2013-05-23 20:31:46]

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
File remoting/webapp/butter_bar.js (right):

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:8: * to help improve the product.
Is this comment still correct?

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:27: var kHangoutsStorageKey =
'hangouts-notice-dismissed';
I realize this was my error originally, but by declaring these unscoped, we're
polluting the top-level 'window' namespace. I think it would make sense to make
the butter bar functions into a class and have these constants as members of it.

Alternatively, if you don't want to make this a class, you could just declare
them inside remoting.

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:37: * Hide the survey request and record some
basic information about the current
s/survey request/butter-bar/ (and similar elsewhere).

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:44: *     false if they clicked the close icon.
|optIn| doesn't really make sense as a parameter name for the Hangouts case, and
this comment is out-of-date. I suggest calling the parameter something like
|accepted| or invert the meaning and call it something like
|dismissedViaCloseIcon|.

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:74: false);
It's a shame to duplicate the action elements and this logic for the two
messages, and it doesn't really scale if we ever want to use this mechanism for
more messages.

I think a better design would be for this class just to manage a single
butter-bar message with one "yes please" link and one close icon. The storage
key can be a class member and you can have an |init| method that handles the
logic of picking which message to display, localizing the butter-bar UI with
that message and showing it.

One downside of that is that all messages need to conform to the same overall
structure w.r.t. having placeholders for the <a> and </a> elements. In
particular, you'd need to change the survey message so that the "Sign me up"
link is part of the message rather than a separate HTML element.

WDYT?

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:101:
chrome.storage.local.get(remoting.kIT2MeVisitedStorageKey, function(value) {
Optional: I'm not a big fan of anonymous functions, or nesting functions more
than two levels deep. Would you object to declaring an |onIT2MeVisitedLoaded|
function at the same level as |onSyncDataLoaded|?

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:129: }
This can be more simply expressed as three expressions of the form:

  element.hidden = (mode == xxx);

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/main.html
File remoting/webapp/main.html (right):

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/main.html...
remoting/webapp/main.html:158: </div>
Why has this block moved in the diff?

[Sergey Ulanov, 2013-05-29 23:36:11]

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
File remoting/webapp/butter_bar.js (right):

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:8: * to help improve the product.
On 2013/05/23 20:31:46, Jamie wrote:
> Is this comment still correct?

Done.

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:27: var kHangoutsStorageKey =
'hangouts-notice-dismissed';
On 2013/05/23 20:31:46, Jamie wrote:
> I realize this was my error originally, but by declaring these unscoped, we're
> polluting the top-level 'window' namespace. I think it would make sense to
make
> the butter bar functions into a class and have these constants as members of
it.
> 
> Alternatively, if you don't want to make this a class, you could just declare
> them inside remoting.

Done.

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:37: * Hide the survey request and record some
basic information about the current
On 2013/05/23 20:31:46, Jamie wrote:
> s/survey request/butter-bar/ (and similar elsewhere).

Done.

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:44: *     false if they clicked the close icon.
On 2013/05/23 20:31:46, Jamie wrote:
> |optIn| doesn't really make sense as a parameter name for the Hangouts case,
and
> this comment is out-of-date. I suggest calling the parameter something like
> |accepted| or invert the meaning and call it something like
> |dismissedViaCloseIcon|.

Done. Also changed the key that we use to store the result - let me know if it
is a problem.

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:74: false);
On 2013/05/23 20:31:46, Jamie wrote:
> It's a shame to duplicate the action elements and this logic for the two
> messages, and it doesn't really scale if we ever want to use this mechanism
for
> more messages.
> 
> I think a better design would be for this class just to manage a single
> butter-bar message with one "yes please" link and one close icon. The storage
> key can be a class member and you can have an |init| method that handles the
> logic of picking which message to display, localizing the butter-bar UI with
> that message and showing it.
> 
> One downside of that is that all messages need to conform to the same overall
> structure w.r.t. having placeholders for the <a> and </a> elements. In
> particular, you'd need to change the survey message so that the "Sign me up"
> link is part of the message rather than a separate HTML element.
> 
> WDYT?

Done.

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:101:
chrome.storage.local.get(remoting.kIT2MeVisitedStorageKey, function(value) {
On 2013/05/23 20:31:46, Jamie wrote:
> Optional: I'm not a big fan of anonymous functions, or nesting functions more
> than two levels deep. Would you object to declaring an |onIT2MeVisitedLoaded|
> function at the same level as |onSyncDataLoaded|?

Done

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:129: }
On 2013/05/23 20:31:46, Jamie wrote:
> This can be more simply expressed as three expressions of the form:
> 
>   element.hidden = (mode == xxx);

Removed most of this code.

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/main.html
File remoting/webapp/main.html (right):

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/main.html...
remoting/webapp/main.html:158: </div>
On 2013/05/23 20:31:46, Jamie wrote:
> Why has this block moved in the diff?

Moved it back. There is redundant <div> around the section-row above this one. I
removed it in the previous version of this CL, and then I didn't revert that
change properly.

[Jamie, 2013-05-30 01:10:16]

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
File remoting/webapp/butter_bar.js (right):

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:44: *     false if they clicked the close icon.
On 2013/05/29 23:36:11, Sergey Ulanov wrote:
> On 2013/05/23 20:31:46, Jamie wrote:
> > |optIn| doesn't really make sense as a parameter name for the Hangouts case,
> and
> > this comment is out-of-date. I suggest calling the parameter something like
> > |accepted| or invert the meaning and call it something like
> > |dismissedViaCloseIcon|.
> 
> Done. Also changed the key that we use to store the result - let me know if it
> is a problem.

It will mean that users who have previously dismissed the butter-bar will start
seeing it again, which is not ideal. I think we should keep the key as it is and
add a comment explaining why it's called what it is.

https://codereview.chromium.org/14803010/diff/23001/remoting/webapp/butter_ba...
File remoting/webapp/butter_bar.js (right):

https://codereview.chromium.org/14803010/diff/23001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:28: messageElement.innerHTML = message;
innerHTML makes me nervous :( The only other place we use it is in
l10n.localizeElementFromTag. Would it be possible to change this code to use
that? That way, if we want to sanitize the HTML (for example, whitelisting only
the tags we use) there's only one place to do it.

https://codereview.chromium.org/14803010/diff/23001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:40: /** @const */
I think these can all be made private (in which case they should end in an
underscore).

https://codereview.chromium.org/14803010/diff/23001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:101: new remoting.ButterBar(
I'm always skeptical of a 'new' statement that doesn't assign to anything. In
this case, I think it would be better to create a remoting.butterBar variable
somewhere (most likely in remoting.init, where most of our globals are created),
then have a non-static init method that selects the content to display.

[Sergey Ulanov, 2013-05-30 22:04:53]

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
File remoting/webapp/butter_bar.js (right):

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:44: *     false if they clicked the close icon.
On 2013/05/30 01:10:16, Jamie wrote:
> On 2013/05/29 23:36:11, Sergey Ulanov wrote:
> > On 2013/05/23 20:31:46, Jamie wrote:
> > > |optIn| doesn't really make sense as a parameter name for the Hangouts
case,
> > and
> > > this comment is out-of-date. I suggest calling the parameter something
like
> > > |accepted| or invert the meaning and call it something like
> > > |dismissedViaCloseIcon|.
> > 
> > Done. Also changed the key that we use to store the result - let me know if
it
> > is a problem.
> 
> It will mean that users who have previously dismissed the butter-bar will
start
> seeing it again, which is not ideal. I think we should keep the key as it is
and
> add a comment explaining why it's called what it is.

No, the value is not actually used in any way. We just verify the presence of
the value, but never check optIn/accepted attribute.

https://codereview.chromium.org/14803010/diff/23001/remoting/webapp/butter_ba...
File remoting/webapp/butter_bar.js (right):

https://codereview.chromium.org/14803010/diff/23001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:28: messageElement.innerHTML = message;
On 2013/05/30 01:10:17, Jamie wrote:
> innerHTML makes me nervous :( The only other place we use it is in
> l10n.localizeElementFromTag. Would it be possible to change this code to use
> that? That way, if we want to sanitize the HTML (for example, whitelisting
only
> the tags we use) there's only one place to do it.

The alternative is what I had before - have each message inserted in main.html
and show one of them depending on what's requested. I can revert back to that
approach.

It should be safe to use innerHTML as long as we control the inserted message.
In this case the message comes from resources and substitutions are hardcoded,
so it should be safe.

https://codereview.chromium.org/14803010/diff/23001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:40: /** @const */
On 2013/05/30 01:10:17, Jamie wrote:
> I think these can all be made private (in which case they should end in an
> underscore).

Done.

https://codereview.chromium.org/14803010/diff/23001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:101: new remoting.ButterBar(
On 2013/05/30 01:10:17, Jamie wrote:
> I'm always skeptical of a 'new' statement that doesn't assign to anything. In
> this case, I think it would be better to create a remoting.butterBar variable
> somewhere (most likely in remoting.init, where most of our globals are
created),
> then have a non-static init method that selects the content to display.

Done.

[Jamie, 2013-05-31 00:12:41]

lgtm, but please consider my suggestion of reusing localizeElementFromTag.

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
File remoting/webapp/butter_bar.js (right):

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:44: *     false if they clicked the close icon.
On 2013/05/30 22:04:54, Sergey Ulanov wrote:
> On 2013/05/30 01:10:16, Jamie wrote:
> > On 2013/05/29 23:36:11, Sergey Ulanov wrote:
> > > On 2013/05/23 20:31:46, Jamie wrote:
> > > > |optIn| doesn't really make sense as a parameter name for the Hangouts
> case,
> > > and
> > > > this comment is out-of-date. I suggest calling the parameter something
> like
> > > > |accepted| or invert the meaning and call it something like
> > > > |dismissedViaCloseIcon|.
> > > 
> > > Done. Also changed the key that we use to store the result - let me know
if
> it
> > > is a problem.
> > 
> > It will mean that users who have previously dismissed the butter-bar will
> start
> > seeing it again, which is not ideal. I think we should keep the key as it is
> and
> > add a comment explaining why it's called what it is.
> 
> No, the value is not actually used in any way. We just verify the presence of
> the value, but never check optIn/accepted attribute.

Ah yes, good point. Still, if we ever want to make use of it, we'd need to cope
with both variants if we change it. I still think that there's not a lot of
value in changing the name in storage, but if you feel strongly then I won't
insist. However, if you do change it, please add a comment in this function
explaining that some versions of the web-app use the "optIn" name.

https://codereview.chromium.org/14803010/diff/23001/remoting/webapp/butter_ba...
File remoting/webapp/butter_bar.js (right):

https://codereview.chromium.org/14803010/diff/23001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:28: messageElement.innerHTML = message;
On 2013/05/30 22:04:54, Sergey Ulanov wrote:
> On 2013/05/30 01:10:17, Jamie wrote:
> > innerHTML makes me nervous :( The only other place we use it is in
> > l10n.localizeElementFromTag. Would it be possible to change this code to use
> > that? That way, if we want to sanitize the HTML (for example, whitelisting
> only
> > the tags we use) there's only one place to do it.
> 
> The alternative is what I had before - have each message inserted in main.html
> and show one of them depending on what's requested. I can revert back to that
> approach.

I like this approach more--the HTML is simpler this way and I think it's cleaner
overall. However, I think you can reuse localizeElementFromTag if you pass in
the i18n-content tag and substitution parameters to the ctor instead of
localizing it beforehand.

> It should be safe to use innerHTML as long as we control the inserted message.
> In this case the message comes from resources and substitutions are hardcoded,
> so it should be safe.

Yes, I think it's probably safe enough as written here, but I'd like to minimize
the use as long as it doesn't make the code any messier than it needs to be.

[Sergey Ulanov, 2013-06-03 19:11:48]

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
File remoting/webapp/butter_bar.js (right):

https://codereview.chromium.org/14803010/diff/14001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:44: *     false if they clicked the close icon.
On 2013/05/31 00:12:41, Jamie wrote:
> On 2013/05/30 22:04:54, Sergey Ulanov wrote:
> > On 2013/05/30 01:10:16, Jamie wrote:
> > > On 2013/05/29 23:36:11, Sergey Ulanov wrote:
> > > > On 2013/05/23 20:31:46, Jamie wrote:
> > > > > |optIn| doesn't really make sense as a parameter name for the Hangouts
> > case,
> > > > and
> > > > > this comment is out-of-date. I suggest calling the parameter something
> > like
> > > > > |accepted| or invert the meaning and call it something like
> > > > > |dismissedViaCloseIcon|.
> > > > 
> > > > Done. Also changed the key that we use to store the result - let me know
> if
> > it
> > > > is a problem.
> > > 
> > > It will mean that users who have previously dismissed the butter-bar will
> > start
> > > seeing it again, which is not ideal. I think we should keep the key as it
is
> > and
> > > add a comment explaining why it's called what it is.
> > 
> > No, the value is not actually used in any way. We just verify the presence
of
> > the value, but never check optIn/accepted attribute.
> 
> Ah yes, good point. Still, if we ever want to make use of it, we'd need to
cope
> with both variants if we change it. I still think that there's not a lot of
> value in changing the name in storage, but if you feel strongly then I won't
> insist. However, if you do change it, please add a comment in this function
> explaining that some versions of the web-app use the "optIn" name.

Ok, renamed it back to optIn.

https://codereview.chromium.org/14803010/diff/23001/remoting/webapp/butter_ba...
File remoting/webapp/butter_bar.js (right):

https://codereview.chromium.org/14803010/diff/23001/remoting/webapp/butter_ba...
remoting/webapp/butter_bar.js:28: messageElement.innerHTML = message;
On 2013/05/31 00:12:42, Jamie wrote:
> On 2013/05/30 22:04:54, Sergey Ulanov wrote:
> > On 2013/05/30 01:10:17, Jamie wrote:
> > > innerHTML makes me nervous :( The only other place we use it is in
> > > l10n.localizeElementFromTag. Would it be possible to change this code to
use
> > > that? That way, if we want to sanitize the HTML (for example, whitelisting
> > only
> > > the tags we use) there's only one place to do it.
> > 
> > The alternative is what I had before - have each message inserted in
main.html
> > and show one of them depending on what's requested. I can revert back to
that
> > approach.
> 
> I like this approach more--the HTML is simpler this way and I think it's
cleaner
> overall. However, I think you can reuse localizeElementFromTag if you pass in
> the i18n-content tag and substitution parameters to the ctor instead of
> localizing it beforehand.
> 
> > It should be safe to use innerHTML as long as we control the inserted
message.
> > In this case the message comes from resources and substitutions are
hardcoded,
> > so it should be safe.
> 
> Yes, I think it's probably safe enough as written here, but I'd like to
minimize
> the use as long as it doesn't make the code any messier than it needs to be.

Done.

[commit-bot: I haz the power, 2013-06-03 19:12:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/14803010/11003

[commit-bot: I haz the power, 2013-06-04 10:39:36]

Change committed as 203912