Implement bump-scroll browser-test.

Implement bump-scroll browser-test.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288185

[Jamie, 2014-08-02 00:31:44]

Patch-set 1 just moves the existing full-screen browser test into a file of its
own. The reset of the CL is probably best reviewed as a delta from patch-set 1,
although there was quite a big time-gap, so there are a few bogus diffs in there
pulled from trunk.

Note that this browser test doesn't work because of crbug.com/399839, but I've
verified that it works if the PIN is not reset at the start of the test.

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
File remoting/webapp/browser_test/browser_test.js (left):

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:327:
}).catch(function(errorMessage){
As discussed, jscompile is not smart enough to recognize this as valid JS.

[kelvinp, 2014-08-04 16:31:48]

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
File remoting/webapp/browser_test/browser_test.js (left):

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:327:
}).catch(function(errorMessage){
On 2014/08/02 00:31:44, Jamie wrote:
> As discussed, jscompile is not smart enough to recognize this as valid JS.

Acknowledged.

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
File remoting/webapp/browser_test/bump_scroll_browser_test.js (right):

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
remoting/webapp/browser_test/bump_scroll_browser_test.js:49:
this.noScrollWindowed.bind(this)
Nit: Indent

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
remoting/webapp/browser_test/bump_scroll_browser_test.js:78: window.innerWidth +
10;
Where does 10px come from?  Does it represent the width of some on screen
elements?

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
remoting/webapp/browser_test/bump_scroll_browser_test.js:98: var expectedTop =
heightFraction == 0.0 ? 0 :
I am a bit confused about how this work.  Say if widthFraction is 1.0 and
heightFraction is 0.5.
expectedTop would be -10 and expectedLeft would be undefined.

Would the test fails as the expectedLeft should not be undefined?

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
remoting/webapp/browser_test/bump_scroll_browser_test.js:222:
reject(Error('Incorrect scroll detected: (' +
Practically, Error and new Error has the same effect.  However, for readability
and consistency.  Since Error is a constructor, I prefer putting the new
keyboard before it so that it is clear that it is a constructor.

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
remoting/webapp/browser_test/bump_scroll_browser_test.js:235: return new
Promise(verifyPluginPosition);
Since verifyPluginPosition doesn't have any async wait.
You can just return Promise.reject(new Error(...)) or Promise.resolve() in the
branches.  In this case, you won't need to wrap the function with the promise
constructor.

[Jamie, 2014-08-05 19:54:18]

PTAL.

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
File remoting/webapp/browser_test/bump_scroll_browser_test.js (right):

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
remoting/webapp/browser_test/bump_scroll_browser_test.js:49:
this.noScrollWindowed.bind(this)
On 2014/08/04 16:31:47, kelvinp wrote:
> Nit: Indent

Done.

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
remoting/webapp/browser_test/bump_scroll_browser_test.js:78: window.innerWidth +
10;
On 2014/08/04 16:31:47, kelvinp wrote:
> Where does 10px come from?  Does it represent the width of some on screen
> elements?

It's arbitrary, but positive. The idea is to fake the situation where the host
desktop is larger than the client window. I've moved it into a constant with a
comment.

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
remoting/webapp/browser_test/bump_scroll_browser_test.js:98: var expectedTop =
heightFraction == 0.0 ? 0 :
On 2014/08/04 16:31:47, kelvinp wrote:
> I am a bit confused about how this work.  Say if widthFraction is 1.0 and
> heightFraction is 0.5.
> expectedTop would be -10 and expectedLeft would be undefined.
> 
> Would the test fails as the expectedLeft should not be undefined?

undefined means "I don't care what value this has", although now I think of it,
it's always used to mean "this value should not change", which is a stricter
guarantee. I've changed it to be interpreted that way.

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
remoting/webapp/browser_test/bump_scroll_browser_test.js:222:
reject(Error('Incorrect scroll detected: (' +
On 2014/08/04 16:31:47, kelvinp wrote:
> Practically, Error and new Error has the same effect.  However, for
readability
> and consistency.  Since Error is a constructor, I prefer putting the new
> keyboard before it so that it is clear that it is a constructor.

Done.

https://codereview.chromium.org/421433002/diff/80001/remoting/webapp/browser_...
remoting/webapp/browser_test/bump_scroll_browser_test.js:235: return new
Promise(verifyPluginPosition);
On 2014/08/04 16:31:47, kelvinp wrote:
> Since verifyPluginPosition doesn't have any async wait.
> You can just return Promise.reject(new Error(...)) or Promise.resolve() in the
> branches.  In this case, you won't need to wrap the function with the promise
> constructor.

Done.

[kelvinp, 2014-08-07 16:58:38]

lgtm

[Jamie, 2014-08-07 17:03:59]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2014-08-07 17:07:57]

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

[commit-bot: I haz the power, 2014-08-07 17:17:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/38621)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/43822)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-07 17:19:16]

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

[commit-bot: I haz the power, 2014-08-07 17:19:17]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/38627)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Jamie, 2014-08-07 17:58:54]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2014-08-07 18:09:17]

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

[commit-bot: I haz the power, 2014-08-08 01:02:32]

Change committed as 288185