Allow full-screen mode when not connected, and remember the previous setting.

Allow full-screen mode when not connected, and remember the previous setting.

This CL also includes a small refactoring of the desktop- vs. app-remoting split.

BUG=432361
Committed: https://crrev.com/5da90dafdac536c66494da0fb86129a6a55709cf
Cr-Commit-Position: refs/heads/master@{#313749}

[Jamie, 2015-01-23 22:37:24]

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

[Jamie, 2015-01-23 22:37:25]

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/app_l...
File remoting/webapp/crd/js/app_launcher.js (right):

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/app_l...
remoting/webapp/crd/js/app_launcher.js:112: var state = values[START_FULLSCREEN]
? 'fullscreen' : 'normal';
As discussed, this currently doesn't work due to crbug.com/51587.

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

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/deskt...
remoting/webapp/crd/js/desktop_remoting.js:76: remoting.fullscreen = new
remoting.FullscreenAppsV2();
Moving this here feels cleaner because it allows the code that uses
remoting.fullscreen to be placed immediately after its creation, rather than
relying on the fact that initGlobalObjects creates it.

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/event...
File remoting/webapp/crd/js/event_handlers.js (left):

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/event...
remoting/webapp/crd/js/event_handlers.js:17:
remoting.fullscreen.addListener(remoting.onResize);
This needed to be moved because remoting.fullscreen is now created later than it
was previously. I think the new location makes more sense, since it's only
needed for CRD.

[Jamie, 2015-01-23 22:40:06]

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

[Jamie, 2015-01-23 22:40:07]

Kelvin: PTAL
Gary: FYI

[kelvinp, 2015-01-23 23:39:58]

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/app_l...
File remoting/webapp/crd/js/app_launcher.js (right):

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/app_l...
remoting/webapp/crd/js/app_launcher.js:112: var state = values[START_FULLSCREEN]
? 'fullscreen' : 'normal';
On 2015/01/23 22:37:24, Jamie wrote:
> As discussed, this currently doesn't work due to crbug.com/51587.

Should we wait for the apps team before making this change as they may come back
with by design and then we would need to do the workaround instead of this code?

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/clien...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/clien...
remoting/webapp/crd/js/client_session.js:615:
remoting.fullscreen.removeListener(this.callOnFullScreenChanged_);
Rant: It is weird that sometimes the clientSession hooks its own fullscreen
listener like here, but sometimes via a proxy remoting.onResize().
IMHO, if clientSession needs to be shared among LG (which does't care about
fullscreen)  and chromoting.  It will be better to have the owning object of
client session to handle the event hooking.

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/event...
File remoting/webapp/crd/js/event_handlers.js (left):

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/event...
remoting/webapp/crd/js/event_handlers.js:17:
remoting.fullscreen.addListener(remoting.onResize);
On 2015/01/23 22:37:24, Jamie wrote:
> This needed to be moved because remoting.fullscreen is now created later than
it
> was previously. I think the new location makes more sense, since it's only
> needed for CRD.

I think we should also move both handlers (window.resize and fullscreen) at the
same time, otherwise it would be hard to follow the comments when it says 'both'
events.

If we are moving the handlers anyways, can we simply move it to the place when
remoting.clientSession is set.  However, looking at the code, it is probably
hard to do that now, as remoting.clientSession is currently set in one place but
tear down in another, which would require some refactoring.

[Jamie, 2015-01-24 00:46:35]

PTAL

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/app_l...
File remoting/webapp/crd/js/app_launcher.js (right):

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/app_l...
remoting/webapp/crd/js/app_launcher.js:112: var state = values[START_FULLSCREEN]
? 'fullscreen' : 'normal';
On 2015/01/23 23:39:58, kelvinp wrote:
> On 2015/01/23 22:37:24, Jamie wrote:
> > As discussed, this currently doesn't work due to crbug.com/51587.
> 
> Should we wait for the apps team before making this change as they may come
back
> with by design and then we would need to do the workaround instead of this
code?
> 

I decided to go ahead and implement the workaround anyway, since there isn't
much value to this CL without it (just enabling the full-screen button before
you connect to a host).

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/clien...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/clien...
remoting/webapp/crd/js/client_session.js:615:
remoting.fullscreen.removeListener(this.callOnFullScreenChanged_);
On 2015/01/23 23:39:58, kelvinp wrote:
> Rant: It is weird that sometimes the clientSession hooks its own fullscreen
> listener like here, but sometimes via a proxy remoting.onResize().
> IMHO, if clientSession needs to be shared among LG (which does't care about
> fullscreen)  and chromoting.  It will be better to have the owning object of
> client session to handle the event hooking.

We are going to have to disentangle full-screen from this class, but we only
need it for bump-scroll support (which is also not used for app-remoting), so I
think we'll move both together in some clever way that Gary will come up with :)

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/event...
File remoting/webapp/crd/js/event_handlers.js (left):

https://codereview.chromium.org/875583002/diff/1/remoting/webapp/crd/js/event...
remoting/webapp/crd/js/event_handlers.js:17:
remoting.fullscreen.addListener(remoting.onResize);
On 2015/01/23 23:39:58, kelvinp wrote:
> On 2015/01/23 22:37:24, Jamie wrote:
> > This needed to be moved because remoting.fullscreen is now created later
than
> it
> > was previously. I think the new location makes more sense, since it's only
> > needed for CRD.
> 
> I think we should also move both handlers (window.resize and fullscreen) at
the
> same time, otherwise it would be hard to follow the comments when it says
'both'
> events.
> 
> If we are moving the handlers anyways, can we simply move it to the place when
> remoting.clientSession is set.  However, looking at the code, it is probably
> hard to do that now, as remoting.clientSession is currently set in one place
but
> tear down in another, which would require some refactoring.

Done. I've also moved the two remaining lines out of this method, since they're
apps v1 only, and deleted the method entirely.

[Jamie, 2015-01-26 22:11:56]

Ping Kelvin

[kelvinp, 2015-01-26 23:54:39]

lgtm

[Jamie, 2015-01-26 23:55:51]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2015-01-26 23:56:29]

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

[commit-bot: I haz the power, 2015-01-26 23:59:36]

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

[commit-bot: I haz the power, 2015-01-26 23:59:37]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)

[Jamie, 2015-01-29 18:10:06]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2015-01-29 18:11:35]

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

[commit-bot: I haz the power, 2015-01-29 19:03:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-29 19:03:48]

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