[Chromoting] Change Application.Delegate to proper subclass of Application.

[Chromoting] Change Application.Delegate to proper subclass of Application.

App Delegate conversion:
* Removed Application.Delegate
* Convert DesktopRemoting and AppRemoting to be subclasses of Application
* Add ApplicationInterface so that jscompile can verify all required overrides are provided.

Access to SessionConnector was a big motivation for having the apps as a subclass:
* Create the SessionConnector in the constructor rather than in the getter.
* Remove some references to remoting.app.getSessionConnector

Test updates:
* Updated base.inherits unittests to verify calling superclass methods with arguments.
* Removed desktopDelegateForTesting since we can use remoting.app for that now.

BUG=465878
Committed: https://crrev.com/a22c2029f25b596648a64d0e41f354c9f6c1c0b0
Cr-Commit-Position: refs/heads/master@{#322437}

[garykac, 2015-03-23 18:44:21]

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

[garykac, 2015-03-23 18:44:23]

PTAL.  I recommend starting with application.js, and then desktop_remoting.js

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/application.js (left):

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:121:
remoting.Application.prototype.onConnected = function(connectionInfo) {
This has been moved a bit further down so that it is next to onSessionFinished_
and updateStatistics_.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:138: this.delegate_.handleDisconnected();
These redirects are no longer needed. Subclass will override if needed.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:166: // TODO(garykac): Check if this can
be initialized in the ctor.
Recent changes make this easy to move into the constructor now.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:67: new
remoting.DefaultSessionConnectorFactory();
Moved here from start() since this is required to create the sessionconnector.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:79: this.sessionConnector_ =
remoting.SessionConnector.factory.createConnector(
Moved here from getter.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/desktop_remoting.js (left):

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_remoting.js:155:
remoting.DesktopRemoting.prototype.getApplicationName = function() {
These were moved earlier in the file so to match the order specified in the
ApplicationInterface (with getters and simple methods first).

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_remoting.js:387:
remoting.desktopDelegateForTesting = null;
No need for this anymore. We can simply use remoting.app and cast it.

[garykac, 2015-03-24 19:53:59]

ping!

[Jamie, 2015-03-24 21:57:06]

I haven't finished reviewing, but since my first comment calls into question
whether or not this is a good change, I wanted to send it out anyway.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/application.js (left):

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:103: this.delegate_.handleExit();
I'm concerned about a loss of readability here. With the old code, when I see a
call to remoting.application.exit(), it's immediately obvious where I need to
look for that method (ie, here) and when I get here, it's immediately obvious
that the delegate might be doing additional work. With the new design, it's not
obvious that the delegate-as-was is involved at all, which makes it much harder
to reason about the run-time behaviour of the application. This is one reason
why I think a delegate model, for all its failings, is a better design in this
case.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:107: /** @return {string} */
AFAICT, these functions are essentially boilerplate to provide something
equivalent to pure virtual functions in Javascript. As such, I would move them
to the end of the file so they don't get in the way of understanding the rest of
it.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:109: throw "Subclass must override";
base.assert would be more appropriate here.

[Jamie, 2015-03-25 20:00:40]

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/js/app_remoting.js (right):

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/app_remoting.js:64:
remoting.AppRemoting.prototype.runApplicationUrl = function() {
This shouldn't be a public member, I don't think. Public methods on this class
should all be accessed through remoting.app, and runApplicationUrl is not a
generic property of all applications. It doesn't need to be this CL, but
consider either making this private or moving it to a more suitable home.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/application.js (left):

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:103: this.delegate_.handleExit();
On 2015/03/24 21:57:05, Jamie wrote:
> I'm concerned about a loss of readability here. With the old code, when I see
a
> call to remoting.application.exit(), it's immediately obvious where I need to
> look for that method (ie, here) and when I get here, it's immediately obvious
> that the delegate might be doing additional work. With the new design, it's
not
> obvious that the delegate-as-was is involved at all, which makes it much
harder
> to reason about the run-time behaviour of the application. This is one reason
> why I think a delegate model, for all its failings, is a better design in this
> case.

As discussed off-line, please refactor so that all methods are either pure
virtual or final (at least notionally--Javascript doesn't have a concept of
either).

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:67: new
remoting.DefaultSessionConnectorFactory();
On 2015/03/23 18:44:22, garykac wrote:
> Moved here from start() since this is required to create the sessionconnector.

The purpose of these factories is to allow test code to substitute mocks for the
client plugin and session connector. Since the Application is constructed from a
load handler, moving them here makes that more difficult.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:179:
remoting.Application.prototype.onDisconnected = function() {
Under the new "pure-virtual or final" model, I think most of these onX methods
should be part of the interface, correct? Also, there's a discrepancy between
public and private visibility among them. In general, I think we tend to make
onX methods private because they are registered by the class itself as
callbacks. I don't know if that approach makes sense here, because I'm not sure
where they are called from (and in any case, changing this is probably not part
of this CL).

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_remoting.js:40: * Required for
remoting.ApplicationInterface interface.
I don't think you need this annotation on every method--it's implied by
@override (though it would be nice if the syntax was something like @override
{remoting.ApplicationInterface} to make it explicit).

[garykac, 2015-03-26 01:41:58]

ptal

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/js/app_remoting.js (right):

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/app_remoting.js:64:
remoting.AppRemoting.prototype.runApplicationUrl = function() {
On 2015/03/25 20:00:39, Jamie wrote:
> This shouldn't be a public member, I don't think. Public methods on this class
> should all be accessed through remoting.app, and runApplicationUrl is not a
> generic property of all applications. It doesn't need to be this CL, but
> consider either making this private or moving it to a more suitable home.

Done.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/application.js (left):

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:103: this.delegate_.handleExit();
On 2015/03/25 20:00:39, Jamie wrote:
> On 2015/03/24 21:57:05, Jamie wrote:
> > I'm concerned about a loss of readability here. With the old code, when I
see
> a
> > call to remoting.application.exit(), it's immediately obvious where I need
to
> > look for that method (ie, here) and when I get here, it's immediately
obvious
> > that the delegate might be doing additional work. With the new design, it's
> not
> > obvious that the delegate-as-was is involved at all, which makes it much
> harder
> > to reason about the run-time behaviour of the application. This is one
reason
> > why I think a delegate model, for all its failings, is a better design in
this
> > case.
> 
> As discussed off-line, please refactor so that all methods are either pure
> virtual or final (at least notionally--Javascript doesn't have a concept of
> either).

Done.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:67: new
remoting.DefaultSessionConnectorFactory();
On 2015/03/25 20:00:39, Jamie wrote:
> On 2015/03/23 18:44:22, garykac wrote:
> > Moved here from start() since this is required to create the
sessionconnector.
> 
> The purpose of these factories is to allow test code to substitute mocks for
the
> client plugin and session connector. Since the Application is constructed from
a
> load handler, moving them here makes that more difficult.

Previously, they were setup in the start() function, which was also called in
the load handler (just after the app was created). So moving them here does not
change the testability.

Fixing the problem you point out (tests being able to override) should be in a
followup cl.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:107: /** @return {string} */
On 2015/03/24 21:57:05, Jamie wrote:
> AFAICT, these functions are essentially boilerplate to provide something
> equivalent to pure virtual functions in Javascript. As such, I would move them
> to the end of the file so they don't get in the way of understanding the rest
of
> it.

Done.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:109: throw "Subclass must override";
On 2015/03/24 21:57:05, Jamie wrote:
> base.assert would be more appropriate here.

Done.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:179:
remoting.Application.prototype.onDisconnected = function() {
On 2015/03/25 20:00:39, Jamie wrote:
> Under the new "pure-virtual or final" model, I think most of these onX methods
> should be part of the interface, correct? Also, there's a discrepancy between
> public and private visibility among them. In general, I think we tend to make
> onX methods private because they are registered by the class itself as
> callbacks. I don't know if that approach makes sense here, because I'm not
sure
> where they are called from (and in any case, changing this is probably not
part
> of this CL).

All of them except getApplicationName are now @protected.

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/1016373003/diff/20001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/desktop_remoting.js:40: * Required for
remoting.ApplicationInterface interface.
On 2015/03/25 20:00:39, Jamie wrote:
> I don't think you need this annotation on every method--it's implied by
> @override (though it would be nice if the syntax was something like @override
> {remoting.ApplicationInterface} to make it explicit).

@override {type} works, but the type isn't validated by jscompile. However, it
does look better and is more concise.

[Jamie, 2015-03-26 01:54:30]

lgtm

https://codereview.chromium.org/1016373003/diff/80001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/js/app_remoting.js (right):

https://codereview.chromium.org/1016373003/diff/80001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/app_remoting.js:205: * @param
{remoting.ConnectionInfo} connectionInfo
Do you need the @param declarating? I thought @override had the effect of
importing all the type declarations from the parent class?

Also other instances in this file.

https://codereview.chromium.org/1016373003/diff/80001/remoting/webapp/base/js...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/1016373003/diff/80001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:81: remoting.Application.prototype.exit_
= function() {
Having both exit_ and quit is confusing. Perhaps exitApplication should be
public, then you can get rid of the latter.

[garykac, 2015-03-26 16:37:55]

The CQ bit was checked by garykac@chromium.org

[garykac, 2015-03-26 16:37:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1016373003/#ps100001
(title: "Rename exit_ -> closeMainWindow_")

[garykac, 2015-03-26 16:38:11]

https://codereview.chromium.org/1016373003/diff/80001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/js/app_remoting.js (right):

https://codereview.chromium.org/1016373003/diff/80001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/app_remoting.js:205: * @param
{remoting.ConnectionInfo} connectionInfo
On 2015/03/26 01:54:30, Jamie wrote:
> Do you need the @param declarating? I thought @override had the effect of
> importing all the type declarations from the parent class?
> 
> Also other instances in this file.

@param is still required and jscompile will verify that the signature matches
the base class signature.

Curiously, @return is optional and will be taken from the base class method if
not provided.

These type checks happen whether or not @override is specified.

https://codereview.chromium.org/1016373003/diff/80001/remoting/webapp/base/js...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/1016373003/diff/80001/remoting/webapp/base/js...
remoting/webapp/base/js/application.js:81: remoting.Application.prototype.exit_
= function() {
On 2015/03/26 01:54:30, Jamie wrote:
> Having both exit_ and quit is confusing. Perhaps exitApplication should be
> public, then you can get rid of the latter.

Changed name to closeMainWindow_ since I like having the functions in
ApplicationInterface protected.

getApplicationName is the only non-protected interface function, but I might
change that to make it consistent with the other functions.

[commit-bot: I haz the power, 2015-03-26 16:38:22]

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

[commit-bot: I haz the power, 2015-03-26 16:41:30]

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

[commit-bot: I haz the power, 2015-03-26 16:41:31]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[garykac, 2015-03-26 17:10:59]

The CQ bit was checked by garykac@chromium.org

[garykac, 2015-03-26 17:10:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1016373003/#ps110001
(title: "sync/merge")

[commit-bot: I haz the power, 2015-03-26 17:11:26]

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

[commit-bot: I haz the power, 2015-03-26 18:35:04]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2015-03-26 18:35:59]

Patchset 7 (id:??) landed as
https://crrev.com/a22c2029f25b596648a64d0e41f354c9f6c1c0b0
Cr-Commit-Position: refs/heads/master@{#322437}