Refactored web-app

Refactored web-app

BUG=None
TEST=Everything still works!


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107673

[Jamie, 2011-10-27 20:41:36]

Here's the web-app refactoring CL. The main goal was to reduce the size of
remoting.js. I've done this by:

* Creating a HostSession class analogous to ClientSession.
* Pulling out code into self-contained modules corresponding to the host and
client modes of the app, as well as a couple of other pieces of self-contained
logic.
* Moving code that was logically part of the OAuth2 and DebugLog classes into
them.

I've also done some basic clean-up:

* Minimize what's exported into the remoting namespace. Quite a few functions
have been made private (and invisible outside their defining anonymous
functions).
* Be more consistent with naming.
* Add JSDoc comments to all functions and global variables.
* Make everything lint and jscompiler clean.

There's scope for plenty more clean-up, but I wanted to get these disruptive
changes done first.

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/debug_l...
File remoting/webapp/me2mom/debug_log.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/debug_l...
remoting/webapp/me2mom/debug_log.js:18: * @param {Element} statsElement The HTML
div to which to update stats.
I've extended DebugLog to manage the stats panel as well.

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/host_se...
File remoting/webapp/me2mom/host_session.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/host_se...
remoting/webapp/me2mom/host_session.js:31: // must be kept in sync.
I decided against having a type mapping like we have in ClientSession because
there are already lots of things we have to keep in sync between the C++ and JS
worlds, and one more didn't seem like a big deal.

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/oauth2.js
File remoting/webapp/me2mom/oauth2.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/oauth2....
remoting/webapp/me2mom/oauth2.js:33: remoting.OAuth2.prototype.KEY_EMAIL_ =
'remoting-email';
This class now handles retrieving and saving the email address.

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/remotin...
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/remotin...
remoting/webapp/me2mom/remoting.js:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
The diffs are not very useful for this file. Basically, I've added nothing, but
I have renamed a couple of things, and reordered to be more consistent with the
new files (public stuff, then private stuff).

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/wcs_loa...
File remoting/webapp/me2mom/wcs_loader.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/wcs_loa...
remoting/webapp/me2mom/wcs_loader.js:146: this.onReady_ = function(success) {};
This fixes a jscompiler error; it has nothing to do with the refactoring.

[garykac, 2011-10-27 22:32:39]

LGTM

I made a few nit comments, but nothing serious and they can be done in a
separate CL (if you agree).

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/client_...
File remoting/webapp/me2mom/client_screen.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/client_...
remoting/webapp/me2mom/client_screen.js:32: remoting.scaleToFit = false;
nit: I assume these should have underscores at the end.

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/debug_l...
File remoting/webapp/me2mom/debug_log.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/debug_l...
remoting/webapp/me2mom/debug_log.js:18: * @param {Element} statsElement The HTML
div to which to update stats.
On 2011/10/27 20:41:36, Jamie wrote:
> I've extended DebugLog to manage the stats panel as well.

OK

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/host_sc...
File remoting/webapp/me2mom/host_screen.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/host_sc...
remoting/webapp/me2mom/host_screen.js:183: var timerRunning_ = false;
I was contemplatingpulling these timer routines into a 'timer.js' file.

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/host_se...
File remoting/webapp/me2mom/host_session.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/host_se...
remoting/webapp/me2mom/host_session.js:31: // must be kept in sync.
On 2011/10/27 20:41:36, Jamie wrote:
> I decided against having a type mapping like we have in ClientSession because
> there are already lots of things we have to keep in sync between the C++ and
JS
> worlds, and one more didn't seem like a big deal.

OK

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/modal.js
File remoting/webapp/me2mom/modal.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/modal.j...
remoting/webapp/me2mom/modal.js:6: * @fileoverview
nit: something like 'appmode.js' sounds more descriptive than 'modal.js'

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/oauth2.js
File remoting/webapp/me2mom/oauth2.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/oauth2....
remoting/webapp/me2mom/oauth2.js:33: remoting.OAuth2.prototype.KEY_EMAIL_ =
'remoting-email';
On 2011/10/27 20:41:36, Jamie wrote:
> This class now handles retrieving and saving the email address.

OK

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/remotin...
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/remotin...
remoting/webapp/me2mom/remoting.js:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/10/27 20:41:36, Jamie wrote:
> The diffs are not very useful for this file. Basically, I've added nothing,
but
> I have renamed a couple of things, and reordered to be more consistent with
the
> new files (public stuff, then private stuff).

OK

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/wcs_loa...
File remoting/webapp/me2mom/wcs_loader.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/wcs_loa...
remoting/webapp/me2mom/wcs_loader.js:146: this.onReady_ = function(success) {};
On 2011/10/27 20:41:36, Jamie wrote:
> This fixes a jscompiler error; it has nothing to do with the refactoring.

OK

[Jamie, 2011-10-27 22:51:37]

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/client_...
File remoting/webapp/me2mom/client_screen.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/client_...
remoting/webapp/me2mom/client_screen.js:32: remoting.scaleToFit = false;
On 2011/10/27 22:32:39, garykac wrote:
> nit: I assume these should have underscores at the end.

No, anything exported to the remoting namespace is public and doesn't have
underscores. Now, whether or not they would need to be exported in a perfect
system is another question.

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/host_sc...
File remoting/webapp/me2mom/host_screen.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/host_sc...
remoting/webapp/me2mom/host_screen.js:183: var timerRunning_ = false;
On 2011/10/27 22:32:39, garykac wrote:
> I was contemplatingpulling these timer routines into a 'timer.js' file.

I think in order to do that, more of them would need exposing in the namespace,
as they're quite tightly coupled with the Share/Cancel functionality. Since this
file is still quite manageable, I think I'll leave them here.

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/modal.js
File remoting/webapp/me2mom/modal.js (right):

http://codereview.chromium.org/8416007/diff/14/remoting/webapp/me2mom/modal.j...
remoting/webapp/me2mom/modal.js:6: * @fileoverview
On 2011/10/27 22:32:39, garykac wrote:
> nit: something like 'appmode.js' sounds more descriptive than 'modal.js'

Done. I went with ui_mode.js, since the relevant tag in html is data-ui-mode.

[commit-bot: I haz the power, 2011-10-28 00:35:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/8416007/16

[commit-bot: I haz the power, 2011-10-28 00:39:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/8416007/17

[commit-bot: I haz the power, 2011-10-28 01:29:02]

Change committed as 107673