Move hello message from It2MeService to It2MeChannel

Move hello message from It2MeService to It2MeChannel

Currently, Hangouts sends a hello message to the webapp before establishing a long-lived connection.
After this CL, Hangouts will establish the connection first and then send a hello message.  If the
webapp is not installed, Hangouts will receive a disconnect event on the port.  If the webapp is
installed, Hangouts will receive a hello response with the list of supported features.

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

[Jamie, 2014-08-15 19:11:46]

Please avoid long lines in CL descriptions; the tool doesn't handle them very
well.

https://codereview.chromium.org/473073002/diff/20001/remoting/webapp/backgrou...
File remoting/webapp/background/it2me_helper_channel.js (right):

https://codereview.chromium.org/473073002/diff/20001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:156: method: message.method +
'Response',
I think that helloResponse would be better declared explicitly in the
HangoutMessageTypes enum. That way, the enum documents all the messages that can
be sent, including replies.

https://codereview.chromium.org/473073002/diff/20001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:158:
base.values(remoting.It2MeHelperChannel.HangoutMessageTypes)
I don't think that requiring a 1-1 mapping of features to messages is the right
approach. Better to have a separate HangoutFeatures enum. For example, it's not
really necessary to advertise that we support the "hello" feature, since that's
the mechanism by which features are advertised. Similarly, "connect" and
"disconnect" are really separate messages offered by the same feature, which
might be called something like "RemoteAssistance". Or you could take the view
that they're fundamental to the operation of the extension and so don't need to
be advertised (you could call the enum HangoutOptionalFeatures to make it clear
that it's used to future-proofing against features we might add later on).

https://codereview.chromium.org/473073002/diff/20001/remoting/webapp/backgrou...
File remoting/webapp/background/it2me_service.js (left):

https://codereview.chromium.org/473073002/diff/20001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:38: this.onMessageExternalRef_ =
this.onMessageExternal_.bind(this);
Did you intend to remove this? There's nothing in the CL description to suggest
it.

[kelvinp, 2014-08-15 21:28:33]

PTAL

https://codereview.chromium.org/473073002/diff/20001/remoting/webapp/backgrou...
File remoting/webapp/background/it2me_helper_channel.js (right):

https://codereview.chromium.org/473073002/diff/20001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:156: method: message.method +
'Response',
On 2014/08/15 19:11:46, Jamie wrote:
> I think that helloResponse would be better declared explicitly in the
> HangoutMessageTypes enum. That way, the enum documents all the messages that
can
> be sent, including replies.

Done.

https://codereview.chromium.org/473073002/diff/20001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:158:
base.values(remoting.It2MeHelperChannel.HangoutMessageTypes)
On 2014/08/15 19:11:46, Jamie wrote:
> I don't think that requiring a 1-1 mapping of features to messages is the
right
> approach. Better to have a separate HangoutFeatures enum. For example, it's
not
> really necessary to advertise that we support the "hello" feature, since
that's
> the mechanism by which features are advertised. Similarly, "connect" and
> "disconnect" are really separate messages offered by the same feature, which
> might be called something like "RemoteAssistance". Or you could take the view
> that they're fundamental to the operation of the extension and so don't need
to
> be advertised (you could call the enum HangoutOptionalFeatures to make it
clear
> that it's used to future-proofing against features we might add later on).

Done.

https://codereview.chromium.org/473073002/diff/20001/remoting/webapp/backgrou...
File remoting/webapp/background/it2me_service.js (left):

https://codereview.chromium.org/473073002/diff/20001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:38: this.onMessageExternalRef_ =
this.onMessageExternal_.bind(this);
On 2014/08/15 19:11:46, Jamie wrote:
> Did you intend to remove this? There's nothing in the CL description to
suggest
> it.

Yes, it is intended. Currently, the only external message exposed by
it2meService is the hello message.  Since we are moving it to the it2meChannel,
it is no longer required.

[Jamie, 2014-08-15 21:57:32]

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/backgrou...
File remoting/webapp/background/it2me_helper_channel.js (right):

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:175: method:
MessageTypes.ERROR,
My comment about appending "Response" was just in relation to the hello message.
I do prefer a dedicated error response, but I don't think it belongs in this CL.
I think the changes needed to support it are more far-reaching; specifically,
there's no longer any need to include an "error" field in every message.

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:285: });
As above, I like this change, but it doesn't really relate to what this CL is
doing, and should be kept separate.

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/unittest...
File remoting/webapp/unittests/it2me_helper_channel_unittest.js (right):

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/unittest...
remoting/webapp/unittests/it2me_helper_channel_unittest.js:71:
QUnit.equal(message.method, 'helloResponse');
remoting.It2MeHelperChannel.HangoutMessageTypes.HELLO_RESPONSE

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/unittest...
remoting/webapp/unittests/it2me_helper_channel_unittest.js:72:
QUnit.ok(message.supportedFeatures instanceof Array);
I think it might be worth checking that it's equal to some permutation of
remoting.It2MeHelperChannel.Features.

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/unittest...
remoting/webapp/unittests/it2me_helper_channel_unittest.js:138: });
Is this related to this CL?

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/unittest...
File remoting/webapp/unittests/it2me_service_unittest.js (right):

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/unittest...
remoting/webapp/unittests/it2me_service_unittest.js:100: // verify that Hangouts
can receive messages from webapp 1
I remember asking for this change in another CL. Has there been some sort of
rebase confusion?

[kelvinp, 2014-08-15 23:25:01]

I have pulled my changes to clean up the error reporting interface in a separate
change.
 
PTAL

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/backgrou...
File remoting/webapp/background/it2me_helper_channel.js (right):

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:175: method:
MessageTypes.ERROR,
On 2014/08/15 21:57:31, Jamie wrote:
> My comment about appending "Response" was just in relation to the hello
message.
> I do prefer a dedicated error response, but I don't think it belongs in this
CL.
> I think the changes needed to support it are more far-reaching; specifically,
> there's no longer any need to include an "error" field in every message.

I will make the change in error in later CL's

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/unittest...
File remoting/webapp/unittests/it2me_helper_channel_unittest.js (right):

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/unittest...
remoting/webapp/unittests/it2me_helper_channel_unittest.js:71:
QUnit.equal(message.method, 'helloResponse');
On 2014/08/15 21:57:31, Jamie wrote:
> remoting.It2MeHelperChannel.HangoutMessageTypes.HELLO_RESPONSE

Done.

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/unittest...
remoting/webapp/unittests/it2me_helper_channel_unittest.js:72:
QUnit.ok(message.supportedFeatures instanceof Array);
On 2014/08/15 21:57:31, Jamie wrote:
> I think it might be worth checking that it's equal to some permutation of
> remoting.It2MeHelperChannel.Features.

Done.

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/unittest...
remoting/webapp/unittests/it2me_helper_channel_unittest.js:138: });
On 2014/08/15 21:57:32, Jamie wrote:
> Is this related to this CL?

Done.

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/unittest...
File remoting/webapp/unittests/it2me_service_unittest.js (right):

https://codereview.chromium.org/473073002/diff/80001/remoting/webapp/unittest...
remoting/webapp/unittests/it2me_service_unittest.js:100: // verify that Hangouts
can receive messages from webapp 1
On 2014/08/15 21:57:32, Jamie wrote:
> I remember asking for this change in another CL. Has there been some sort of
> rebase confusion?

No, I am just trying address your feedback since I am modifying the same files. 
I have reverted the changes and will send out a separate CL to address them.

[Jamie, 2014-08-15 23:30:12]

LGTM with optional test improvement.

https://codereview.chromium.org/473073002/diff/100001/remoting/webapp/unittes...
File remoting/webapp/unittests/it2me_helper_channel_unittest.js (right):

https://codereview.chromium.org/473073002/diff/100001/remoting/webapp/unittes...
remoting/webapp/unittests/it2me_helper_channel_unittest.js:70:
supportedFeatures: base.values(remoting.It2MeHelperChannel.Features)
This is testing something stricter than it needs to: namely that the order
matches. In practise, I can't see that being a problem, and if the test fails,
it will at least fail safe. Unless I've missed some more serious failure mode, I
think this is fine, but if it's easy to make the test order-agnostic, it's
probably worthwhile.

[kelvinp, 2014-08-15 23:38:26]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-08-15 23:41:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kelvinp@chromium.org/473073002/100001

[commit-bot: I haz the power, 2014-08-16 04:48:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[commit-bot: I haz the power, 2014-08-16 08:08:40]

Committed patchset #3 (100001) as 290127