Requiem for client_screen.js

Requiem for client_screen.js

client_screen.js currently defines a few global functions and a few 'private'
functions that are called in application.js and desktop_connected_view.js

This CL
1. Moves onClientStateChange_, updateStatistics_ into application.js as
   they are called from there and their implementation depends on application.js.
2. Moves declaration of globals clientSession and desktopConnectedView
   into application.js
3. Moves remoting.disconnect into remoting.Application.
4. Moves onResize and onVisibilityChanged into desktop_connected_view.js
5. Removes client_screen.js

BUG=461995
Committed: https://crrev.com/975f1f41b5e28426de9c3ee6d56f26c4fdfbd618
Cr-Commit-Position: refs/heads/master@{#319195}

[kelvinp, 2015-02-25 22:41:39]

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

[kelvinp, 2015-02-25 22:44:38]

PTAL

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/base/js/appl...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/base/js/appl...
remoting/webapp/base/js/application.js:231:
remoting.Application.prototype.onClientStateChange_ = function(state) {
Moved from client_screen.js.

[kelvinp, 2015-03-03 22:53:11]

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

[kelvinp, 2015-03-03 22:53:12]

PTAL

[Jamie, 2015-03-04 01:06:55]

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/app_remoting...
File remoting/webapp/app_remoting/js/app_remoting.js (right):

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/app_remoting...
remoting/webapp/app_remoting/js/app_remoting.js:147: remoting.app.disconnect);
bind?

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/base/js/appl...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/base/js/appl...
remoting/webapp/base/js/application.js:126: // TODO(garykac): Make clientSession
a member var of Application.
Move this TODO up to where remoting.clientSession is declared?

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/base/js/appl...
remoting/webapp/base/js/application.js:256: remoting.clientSession.cleanup();
Not for this CL, but I think cleanup() pre-dates base.Disposable. I think it
should probably be called dispose() now.

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_connected_view.js:320: var fullscreen = /** @type
{chrome.Event} */ (remoting.fullscreen);
remoting.fullscreen is not a chrome.Event, so you shouldn't cast it like this
(it would cause a runtime error if, for example, base.ChromeEventHook was ever
updated to use the hasListener property). The cleanest option is probably to add
an MiscEventHook class similar to ChromeEventHook (in fact, ChromeEventHook
could just be a typedef) that accepts a parameter of type {addListener:Function,
removeListener:Function}. If that doesn't appeal, then you could define
FullscreenEventHook in fullscreen.js. WDYT?

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_connected_view.js:328: new
base.ChromeEventHook(fullscreen, this.onResize_.bind(this)),
I think it would be clearer not to hook fullscreen here, but to explicitly call
onResize_ from onFullScreenChanged_. You can also move the comment about why
that's necessary to the call-site (and so ignore my next comment).

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_connected_view.js:330:
this.onVisibilityChanged_.bind(this), false),
This hook is unrelated to the comment above. It would be better moved to the
bottom with its own comment (or just a blank line if you don't feel it warrants
one).

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
File remoting/webapp/crd/js/desktop_remoting.js (left):

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_remoting.js:109:
remoting.fullscreen.addListener(remoting.onResize);
This was previously specific to CRD. If there are no negative ramifications to
moving it to shared code, then I'm all for it, but I wanted to call it out.

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_remoting.js:100:
window.addEventListener('unload', remoting.app.disconnect, false);
bind?

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_remoting.js:231:
remoting.hangoutSessionEvents.raiseEvent(
Can we get rid of this now?

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/toolb...
File remoting/webapp/crd/js/toolbar.js (right):

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/toolb...
remoting/webapp/crd/js/toolbar.js:66:
registerEventListener('toolbar-disconnect', 'click', remoting.app.disconnect);
bind?

[kelvinp, 2015-03-04 21:02:21]

PTAL

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/app_remoting...
File remoting/webapp/app_remoting/js/app_remoting.js (right):

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/app_remoting...
remoting/webapp/app_remoting/js/app_remoting.js:147: remoting.app.disconnect);
On 2015/03/04 01:06:54, Jamie wrote:
> bind?

Done.

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/base/js/appl...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/base/js/appl...
remoting/webapp/base/js/application.js:126: // TODO(garykac): Make clientSession
a member var of Application.
On 2015/03/04 01:06:54, Jamie wrote:
> Move this TODO up to where remoting.clientSession is declared?

Done.

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/base/js/appl...
remoting/webapp/base/js/application.js:256: remoting.clientSession.cleanup();
On 2015/03/04 01:06:54, Jamie wrote:
> Not for this CL, but I think cleanup() pre-dates base.Disposable. I think it
> should probably be called dispose() now.

Acknowledged.

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_connected_view.js:320: var fullscreen = /** @type
{chrome.Event} */ (remoting.fullscreen);
On 2015/03/04 01:06:54, Jamie wrote:
> remoting.fullscreen is not a chrome.Event, so you shouldn't cast it like this
> (it would cause a runtime error if, for example, base.ChromeEventHook was ever
> updated to use the hasListener property). The cleanest option is probably to
add
> an MiscEventHook class similar to ChromeEventHook (in fact, ChromeEventHook
> could just be a typedef) that accepts a parameter of type
{addListener:Function,
> removeListener:Function}. If that doesn't appeal, then you could define
> FullscreenEventHook in fullscreen.js. WDYT?

Good point.  I will create FullscreenEventHook.
I think the long term solution is to change the event hooking interface in
fullScreen.js to match other events listeners, so I am reluctant to create a
shared component MiscEventHook.

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_connected_view.js:328: new
base.ChromeEventHook(fullscreen, this.onResize_.bind(this)),
On 2015/03/04 01:06:54, Jamie wrote:
> I think it would be clearer not to hook fullscreen here, but to explicitly
call
> onResize_ from onFullScreenChanged_. You can also move the comment about why
> that's necessary to the call-site (and so ignore my next comment).

I like this approach better as well :)

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_connected_view.js:330:
this.onVisibilityChanged_.bind(this), false),
On 2015/03/04 01:06:54, Jamie wrote:
> This hook is unrelated to the comment above. It would be better moved to the
> bottom with its own comment (or just a blank line if you don't feel it
warrants
> one).

Done.

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
File remoting/webapp/crd/js/desktop_remoting.js (left):

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_remoting.js:109:
remoting.fullscreen.addListener(remoting.onResize);
On 2015/03/04 01:06:54, Jamie wrote:
> This was previously specific to CRD. If there are no negative ramifications to
> moving it to shared code, then I'm all for it, but I wanted to call it out.

This is a good point.  Looking glass should be find as it is not possible of
resizing a looking glass window or changing its fullscreen status.
desktop_connected_view.js, as the name suggests, is meant to be for desktop
remoting only when the refactor completes.  Looking glass will eventually have
an equivalent page of app_connected_view.js

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_remoting.js:100:
window.addEventListener('unload', remoting.app.disconnect, false);
On 2015/03/04 01:06:54, Jamie wrote:
> bind?

Done.

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_remoting.js:231:
remoting.hangoutSessionEvents.raiseEvent(
On 2015/03/04 01:06:54, Jamie wrote:
> Can we get rid of this now?

This will be gone after a rebase as it is already removed in
https://codereview.chromium.org/973333002/
Was avoiding the rebase lest it will make the CL harder :)

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/toolb...
File remoting/webapp/crd/js/toolbar.js (right):

https://codereview.chromium.org/952353002/diff/1/remoting/webapp/crd/js/toolb...
remoting/webapp/crd/js/toolbar.js:66:
registerEventListener('toolbar-disconnect', 'click', remoting.app.disconnect);
On 2015/03/04 01:06:54, Jamie wrote:
> bind?

Done.

[Jamie, 2015-03-04 22:00:52]

lgtm

https://codereview.chromium.org/952353002/diff/20001/remoting/webapp/crd/js/f...
File remoting/webapp/crd/js/fullscreen.js (right):

https://codereview.chromium.org/952353002/diff/20001/remoting/webapp/crd/js/f...
remoting/webapp/crd/js/fullscreen.js:61: this.src_ = remoting.fullscreen;
Add @private annotations for these members.

[kelvinp, 2015-03-05 01:23:02]

FYI

[kelvinp, 2015-03-05 01:23:06]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2015-03-05 01:23:07]

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/952353002/#ps60001 (title:
"Rebase + Ready for Check-in")

[commit-bot: I haz the power, 2015-03-05 01:24:18]

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

[commit-bot: I haz the power, 2015-03-05 02:32:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-05 02:32:50]

Patchset 4 (id:??) landed as
https://crrev.com/975f1f41b5e28426de9c3ee6d56f26c4fdfbd618
Cr-Commit-Position: refs/heads/master@{#319195}