Interfaceify ClientPlugin in preparation for mocking it.

Interfaceify ClientPlugin in preparation for mocking it.

Various browser tests currently have to make a connection to a host in order to test
features that only make sense in that context (eg, bump-scroll). These tests would
be more robust if the we could mock the client plugin so that a real connection isn't
needed.

For the most part, I've changed only what I had to to make it possible to mock these
classes; there's a lot more clean-up that could be done, but I don't want to make it
hard to merge things back into trunk.

Committed: https://crrev.com/611d14ec5320ddee23aff092bafa9536ec6f2b83
Cr-Commit-Position: refs/heads/master@{#296001}

[Jamie, 2014-09-12 23:40:38]

jamiewalch@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Jamie, 2014-09-12 23:40:38]

ptal

[Jamie, 2014-09-15 21:35:47]

Ping.

[Sergey Ulanov, 2014-09-16 02:02:51]

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/client_p...
File remoting/webapp/client_plugin_interface.js (right):

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/client_p...
remoting/webapp/client_plugin_interface.js:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
year
remove (c)

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/client_p...
remoting/webapp/client_plugin_interface.js:19: remoting.ClientPluginInterface =
function() {};
IMO it would be better to call abstract interface ClientPlugin and the
implementation ClientPluginImpl or something like that - that's the style that's
normally used in chromium (using interface instead of implementation should not
be penalized by requiring to type longer name).

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/remoting.js
File remoting/webapp/remoting.js (right):

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/remoting...
remoting/webapp/remoting.js:44: 
remove this empty line?

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/session_...
File remoting/webapp/session_connector_interface.js (right):

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/session_...
remoting/webapp/session_connector_interface.js:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
year

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/session_...
remoting/webapp/session_connector_interface.js:18:
remoting.SessionConnectorInterface = function() {};
Same comment as for ClientPluginInterface. Ideally interface name should be
shorted than implementation

[Jamie, 2014-09-16 16:23:21]

ptal

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/client_p...
File remoting/webapp/client_plugin_interface.js (right):

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/client_p...
remoting/webapp/client_plugin_interface.js:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2014/09/16 02:02:51, Sergey Ulanov wrote:
> year
> remove (c)

Done.

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/client_p...
remoting/webapp/client_plugin_interface.js:19: remoting.ClientPluginInterface =
function() {};
On 2014/09/16 02:02:51, Sergey Ulanov wrote:
> IMO it would be better to call abstract interface ClientPlugin and the
> implementation ClientPluginImpl or something like that - that's the style
that's
> normally used in chromium (using interface instead of implementation should
not
> be penalized by requiring to type longer name).

I agree. The problem is that we're expecting a merge back to trunk and I don't
want to make that any more difficult than it needs to be. I've filed
crbug.com/414774 and added a TODO to clean this up.

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/remoting.js
File remoting/webapp/remoting.js (right):

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/remoting...
remoting/webapp/remoting.js:44: 
On 2014/09/16 02:02:51, Sergey Ulanov wrote:
> remove this empty line?

Done.

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/session_...
File remoting/webapp/session_connector_interface.js (right):

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/session_...
remoting/webapp/session_connector_interface.js:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2014/09/16 02:02:51, Sergey Ulanov wrote:
> year

Done.

https://codereview.chromium.org/552403004/diff/40001/remoting/webapp/session_...
remoting/webapp/session_connector_interface.js:18:
remoting.SessionConnectorInterface = function() {};
On 2014/09/16 02:02:51, Sergey Ulanov wrote:
> Same comment as for ClientPluginInterface. Ideally interface name should be
> shorted than implementation

Agreed; see above.

[Jamie, 2014-09-19 23:55:44]

PTAL. I've renamed the interfaces and files as suggested. Since the review tool
doesn't do a good job of showing the diffs, I've added comments to highlight the
non-mechanical changes I made as a result.

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/client_p...
File remoting/webapp/client_plugin.js (right):

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/client_p...
remoting/webapp/client_plugin.js:274: remoting.ClientPlugin.Feature = {
This enum was defined on the implementation in the previous CL; I think it makes
more sense on the interface.

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/client_p...
remoting/webapp/client_plugin.js:310:
remoting.ClientPluginFactory.prototype.preloadPlugin = function() {};
This was previously a static member of the implementation.

[Sergey Ulanov, 2014-09-20 00:35:25]

lgtm

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/client_p...
File remoting/webapp/client_plugin.js (right):

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/client_p...
remoting/webapp/client_plugin.js:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
this is not a new file, so keep 2012 here?

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/client_p...
remoting/webapp/client_plugin.js:7: * Interface abstracting the ClientPlugin
functionality.
Please update this comment - it's confusing after you renamed ClientPlugin to
ClientPluginImpl.

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/client_p...
File remoting/webapp/client_plugin_impl.js (right):

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/client_p...
remoting/webapp/client_plugin_impl.js:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
2014 - this is a new file

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/session_...
File remoting/webapp/session_connector.js (right):

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/session_...
remoting/webapp/session_connector.js:7: * Interface abstracting the
SessionConnector functionality.
Now that the implementation is called SessionConnectorImpl this comment looks
confusing.

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/session_...
File remoting/webapp/session_connector_impl.js (right):

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/session_...
remoting/webapp/session_connector_impl.js:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
Technically this is a new file, so I guess it should be 2014, and keep 2013 in
session_connector.h

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/session_...
remoting/webapp/session_connector_impl.js:7: * Connect set-up state machine for
Me2Me and IT2Me
update this comment please. E.g. something like "Default SessionConnector" would
be sufficint.

[Jamie, 2014-09-20 00:54:22]

FYI. I've left the dates as-is, since I think the age of a file should refer
more to its contents than its name (ie, you wouldn't update a copyright date for
a simple copy, which is what this is, for the most part). LMK if you still
disagree and I'll update the dates as you suggest.

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/client_p...
File remoting/webapp/client_plugin.js (right):

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/client_p...
remoting/webapp/client_plugin.js:7: * Interface abstracting the ClientPlugin
functionality.
On 2014/09/20 00:35:24, Sergey Ulanov wrote:
> Please update this comment - it's confusing after you renamed ClientPlugin to
> ClientPluginImpl.

Done.

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/session_...
File remoting/webapp/session_connector.js (right):

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/session_...
remoting/webapp/session_connector.js:7: * Interface abstracting the
SessionConnector functionality.
On 2014/09/20 00:35:25, Sergey Ulanov wrote:
> Now that the implementation is called SessionConnectorImpl this comment looks
> confusing.

Done.

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/session_...
File remoting/webapp/session_connector_impl.js (right):

https://codereview.chromium.org/552403004/diff/80001/remoting/webapp/session_...
remoting/webapp/session_connector_impl.js:7: * Connect set-up state machine for
Me2Me and IT2Me
On 2014/09/20 00:35:25, Sergey Ulanov wrote:
> update this comment please. E.g. something like "Default SessionConnector"
would
> be sufficint.

Done.

[Jamie, 2014-09-20 00:54:26]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2014-09-20 00:54:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/552403004/80001

[commit-bot: I haz the power, 2014-09-20 02:56:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-20 02:56:20]

Exceeded time limit waiting for builds to trigger.

[Jamie, 2014-09-22 17:06:26]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2014-09-22 17:07:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/552403004/80001

[commit-bot: I haz the power, 2014-09-22 17:08:52]

Committed patchset #5 (id:80001) as b3f57fb7f8376de68c097aba29034bc4550ac39a

[commit-bot: I haz the power, 2014-09-22 17:09:36]

Patchset 5 (id:??) landed as
https://crrev.com/611d14ec5320ddee23aff092bafa9536ec6f2b83
Cr-Commit-Position: refs/heads/master@{#296001}