Fix scroll-bar behaviour.

Fix scroll-bar behaviour.

For apps v2, the default, "no scroll-bar" behaviour is overridden until a connection to the host is made.

For both apps v1 and apps v2, the in-session behaviour is corrected to work-around crbug.com/240772

BUG=252796, 240772
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215705

[Jamie, 2013-08-03 00:02:18]

ptal

[Wez, 2013-08-03 00:19:51]

https://codereview.chromium.org/22006002/diff/1/remoting/webapp/client_sessio...
File remoting/webapp/client_session.js (right):

https://codereview.chromium.org/22006002/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:158: * bar visibility correctly.
Why does this not need calling again whenever shrink-to-fit changes?

https://codereview.chromium.org/22006002/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:166: var kScrollBarWidth = 16;
Is this defined in our CSS?

https://codereview.chromium.org/22006002/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:168: !this.shrinkToFit_ && window.innerHeight
< this.plugin.desktopHeight;
nit: Might be clearer to just skip this logic entirely in the srhink-to-fit
case.

[Jamie, 2013-08-03 00:36:15]

ptal

https://codereview.chromium.org/22006002/diff/1/remoting/webapp/client_sessio...
File remoting/webapp/client_session.js (right):

https://codereview.chromium.org/22006002/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:158: * bar visibility correctly.
On 2013/08/03 00:19:51, Wez wrote:
> Why does this not need calling again whenever shrink-to-fit changes?

It does. I've updated the comment.

https://codereview.chromium.org/22006002/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:166: var kScrollBarWidth = 16;
On 2013/08/03 00:19:51, Wez wrote:
> Is this defined in our CSS?

Yes.

https://codereview.chromium.org/22006002/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:168: !this.shrinkToFit_ && window.innerHeight
< this.plugin.desktopHeight;
On 2013/08/03 00:19:51, Wez wrote:
> nit: Might be clearer to just skip this logic entirely in the srhink-to-fit
> case.

I think it's clearer this way. The variables are called needs*Scroll, which is
trivially false if shrink-to-fit is enabled.

[Wez, 2013-08-03 00:47:18]

https://codereview.chromium.org/22006002/diff/1/remoting/webapp/client_sessio...
File remoting/webapp/client_session.js (right):

https://codereview.chromium.org/22006002/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:158: * bar visibility correctly.
On 2013/08/03 00:36:16, Jamie wrote:
> On 2013/08/03 00:19:51, Wez wrote:
> > Why does this not need calling again whenever shrink-to-fit changes?
> 
> It does. I've updated the comment.

In this case, name it updateScrollbarVisibility, to match updateDimensions?

https://codereview.chromium.org/22006002/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:168: !this.shrinkToFit_ && window.innerHeight
< this.plugin.desktopHeight;
On 2013/08/03 00:36:16, Jamie wrote:
> On 2013/08/03 00:19:51, Wez wrote:
> > nit: Might be clearer to just skip this logic entirely in the srhink-to-fit
> > case.
> 
> I think it's clearer this way. The variables are called needs*Scroll, which is
> trivially false if shrink-to-fit is enabled.

Yes, exactly. So why not:

needsHScroll = false;
needsVScroll = false;
if (!this.shrinkToFit_) {
  <complicated logic goes here!>
}
<mess with styles>

https://codereview.chromium.org/22006002/diff/5002/remoting/webapp/client_ses...
File remoting/webapp/client_session.js (right):

https://codereview.chromium.org/22006002/diff/5002/remoting/webapp/client_ses...
remoting/webapp/client_session.js:954: this.setScrollbarVisibility();
nit: Add a comment e.g. "Show or hide the scrollbars dependent upon the new
size."

[Jamie, 2013-08-03 01:35:54]

ptal. I've added a call to updateScrollbarVisibility from onDesktopSizeChanged,
which obviates the need for one when the session is established.

https://codereview.chromium.org/22006002/diff/1/remoting/webapp/client_sessio...
File remoting/webapp/client_session.js (right):

https://codereview.chromium.org/22006002/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:158: * bar visibility correctly.
On 2013/08/03 00:47:18, Wez wrote:
> On 2013/08/03 00:36:16, Jamie wrote:
> > On 2013/08/03 00:19:51, Wez wrote:
> > > Why does this not need calling again whenever shrink-to-fit changes?
> > 
> > It does. I've updated the comment.
> 
> In this case, name it updateScrollbarVisibility, to match updateDimensions?

Done.

https://codereview.chromium.org/22006002/diff/1/remoting/webapp/client_sessio...
remoting/webapp/client_session.js:168: !this.shrinkToFit_ && window.innerHeight
< this.plugin.desktopHeight;
On 2013/08/03 00:47:18, Wez wrote:
> On 2013/08/03 00:36:16, Jamie wrote:
> > On 2013/08/03 00:19:51, Wez wrote:
> > > nit: Might be clearer to just skip this logic entirely in the
srhink-to-fit
> > > case.
> > 
> > I think it's clearer this way. The variables are called needs*Scroll, which
is
> > trivially false if shrink-to-fit is enabled.
> 
> Yes, exactly. So why not:
> 
> needsHScroll = false;
> needsVScroll = false;
> if (!this.shrinkToFit_) {
>   <complicated logic goes here!>
> }
> <mess with styles>

Done.

https://codereview.chromium.org/22006002/diff/5002/remoting/webapp/client_ses...
File remoting/webapp/client_session.js (right):

https://codereview.chromium.org/22006002/diff/5002/remoting/webapp/client_ses...
remoting/webapp/client_session.js:954: this.setScrollbarVisibility();
On 2013/08/03 00:47:18, Wez wrote:
> nit: Add a comment e.g. "Show or hide the scrollbars dependent upon the new
> size."

This feels like a classic "Add 1 to x" style comment. I don't think it adds
anything to readability.

[Wez, 2013-08-03 02:48:23]

On 2013/08/03 01:35:54, Jamie wrote:
> ptal. I've added a call to updateScrollbarVisibility from
onDesktopSizeChanged,
> which obviates the need for one when the session is established.

Did you mean to remove that call, then?

[Wez, 2013-08-03 02:48:48]

LGTM w/ logging issue addressed.

https://codereview.chromium.org/22006002/diff/14001/remoting/webapp/client_se...
File remoting/webapp/client_session.js (right):

https://codereview.chromium.org/22006002/diff/14001/remoting/webapp/client_se...
remoting/webapp/client_session.js:168: window.innerHeight + "; plugin height = "
+ this.plugin.desktopHeight);
Won't this result in a lot of logging, and potentially a performance hit,
whenever the user resizes the window?

[Jamie, 2013-08-05 18:33:05]

https://codereview.chromium.org/22006002/diff/14001/remoting/webapp/client_se...
File remoting/webapp/client_session.js (right):

https://codereview.chromium.org/22006002/diff/14001/remoting/webapp/client_se...
remoting/webapp/client_session.js:168: window.innerHeight + "; plugin height = "
+ this.plugin.desktopHeight);
On 2013/08/03 02:48:49, Wez wrote:
> Won't this result in a lot of logging, and potentially a performance hit,
> whenever the user resizes the window?

Left-over debugging; removed.

[commit-bot: I haz the power, 2013-08-05 18:33:26]

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

[commit-bot: I haz the power, 2013-08-05 22:14:17]

Change committed as 215705