Scroll-bar browser test.

Scroll-bar browser test.

This doesn't yet do the most interesting test--whether or not scroll-bars
work correctly when connected to a host--but that should be simple to add
once a ClientPlugin mock is implemented.

Committed: https://crrev.com/7689f9b0abe0278459e79eaebc39dd5eddbd3c85
Cr-Commit-Position: refs/heads/master@{#294290}

[Jamie, 2014-09-09 00:45:44]

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

[Jamie, 2014-09-09 00:45:44]

ptal

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/html/tem...
File remoting/webapp/html/template_main.html (right):

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/html/tem...
remoting/webapp/html/template_main.html:47: <div class="inset home-screen
full-height"
The content of the scroller needs to be full-height because I couldn't find a
better method of detecting scroll-bars that doing a hit-test near the edge of
the scroller <div> and seeing whether it returns the scroller or its content. If
the content is not full-height, the bottom hit-test will (almost) always return
the scroller. If you can think of a better approach, I'd love to hear it.

[kelvinp, 2014-09-10 00:02:51]

The approach to test for scroll bars is creative.  
Just some comments regarding retrieving the height of the contents.

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/scrollbar_browser_test.js (right):

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/scrollbar_browser_test.js:22: this.kScrollBarWidth_
= 16;
According to style guide, constants should be named LIKE_THIS.
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Naming

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/scrollbar_browser_test.js:29:
marker.getBoundingClientRect().top + this.kBorderWidth_;
why not just get the clientHeight(content + padding) of the home-screen div?
e.g. document.querySelector('.home-screen').clientHeight

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/scrollbar_browser_test.js:31: // The width of the
content is computed from the width of a <section> (590px)
Is it a typo of 690px?

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/scrollbar_browser_test.js:34: // the horizontal
space required by the header depends on the length of the
Would the test break if the clientWidth of the contents are changed? Would
document.querySelector('.home-screen').clientWidth (content + padding) work
better?

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/scrollbar_browser_test.js:50: .then(function(value)
{ return browserTest.pass(value); },
Nit: 
.then(browserTest.pass, browserTest.fail)

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/html/tem...
File remoting/webapp/html/template_main.html (right):

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/html/tem...
remoting/webapp/html/template_main.html:58: <div id="bottom-marker"></div>
This should not be required.  See comments above.

[Jamie, 2014-09-10 00:40:50]

PTAL

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/scrollbar_browser_test.js (right):

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/scrollbar_browser_test.js:22: this.kScrollBarWidth_
= 16;
On 2014/09/10 00:02:51, kelvinp wrote:
> According to style guide, constants should be named LIKE_THIS.
> https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Naming

Yuck :( Done

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/scrollbar_browser_test.js:29:
marker.getBoundingClientRect().top + this.kBorderWidth_;
On 2014/09/10 00:02:51, kelvinp wrote:
> why not just get the clientHeight(content + padding) of the home-screen div?
> e.g. document.querySelector('.home-screen').clientHeight

There are a couple of problems with this. Firstly, the home-screen div is marked
as full-height because otherwise my scroll-bar detection doesn't work
(elementFromPoint returns the scroller both if the content div is larger than
the scroller and if it's smaller). Secondly, I'd have to include the title-bar
in all the measurements.

I'm not particularly happy with needing a <div> to measure this, but I think
it's better than the alternatives I came up with. We could instead take the
bottom of the preceding element, but that feels more likely to be flaky as new
elements are added; at least with the approach I've taken there's a comment in
the source to guide people when adding new elements.

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/scrollbar_browser_test.js:31: // The width of the
content is computed from the width of a <section> (590px)
On 2014/09/10 00:02:51, kelvinp wrote:
> Is it a typo of 690px?

Yes; done.

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/scrollbar_browser_test.js:34: // the horizontal
space required by the header depends on the length of the
On 2014/09/10 00:02:51, kelvinp wrote:
> Would the test break if the clientWidth of the contents are changed? Would
> document.querySelector('.home-screen').clientWidth (content + padding) work
> better?

Yes, the test will break in that case. I could measure the div to get rid of the
hard-coded 690px, but I can't see any easy way of getting rid of the hard-coded
margin width (short of parsing the CSS). Since the tests are very sensitive
(just 1px difference between test cases), any change to the CSS that affects
this will result in a test failure so I think it will fail safe, but if you can
think of a better approach, I'm open to changing it.

https://codereview.chromium.org/552923002/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/scrollbar_browser_test.js:50: .then(function(value)
{ return browserTest.pass(value); },
On 2014/09/10 00:02:51, kelvinp wrote:
> Nit: 
> .then(browserTest.pass, browserTest.fail)

Done.

[kelvinp, 2014-09-10 23:35:47]

lgtm

[Jamie, 2014-09-11 00:07:30]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2014-09-11 00:13:38]

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

[commit-bot: I haz the power, 2014-09-11 01:52:58]

Committed patchset #4 (id:60001) as 10ca062a1fb7f6ca5ed66797547816fb580c7aea

[commit-bot: I haz the power, 2014-09-11 03:09:04]

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