fix testWindowMaximize bug

fix testWindowMaximize bug

Add sleep to SetWindowBounds to give chrome ui enough time to respond to devtools command.

BUG=chromedriver:1779
Review-Url: https://codereview.chromium.org/2836023003
Cr-Commit-Position: refs/heads/master@{#467893}
Committed: https://chromium.googlesource.com/chromium/src/+/be8ffd36e6592c835e7c2c183ec87f1d44b52faa

[jzfeng, 2017-04-25 02:37:55]

jzfeng@chromium.org changed reviewers:
+ jochen@chromium.org, stgao@chromium.org

[jzfeng, 2017-04-25 02:37:55]

I managed to reproduce the bug on my mac. And verified the patch can fix the bug
locally.

PTAL.

Thanks!

[jzfeng, 2017-04-25 02:41:03]

The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 02:41:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 02:53:58]

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

[commit-bot: I haz the power, 2017-04-25 02:53:59]

Dry run: This issue passed the CQ dry run.

[stgao, 2017-04-25 04:06:48]

stgao@chromium.org changed reviewers:
+ johnchen@chromium.org

[stgao, 2017-04-25 04:06:48]

+johnchen who is the new owner of Chromedriver

https://codereview.chromium.org/2836023003/diff/1/chrome/test/chromedriver/ch...
File chrome/test/chromedriver/chrome/chrome_desktop_impl.cc (right):

https://codereview.chromium.org/2836023003/diff/1/chrome/test/chromedriver/ch...
chrome/test/chromedriver/chrome/chrome_desktop_impl.cc:387:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
I would not suggest a sleep like this, because it could make tests flaky due to
timing issue. Is there some deterministic solution for this?

[jzfeng, 2017-04-25 04:16:25]

Description was changed from

==========
fix testWindowMaximize bug

Add sleep to SetWindowBounds to give chrome ui enough time to respond to
devtools command.

BUG=chromedriver:1779
==========

to

==========
fix testWindowMaximize bug

Add sleep to SetWindowBounds to give chrome ui enough time to respond to
devtools command.

BUG=chromedriver:1779
==========

[jzfeng, 2017-04-25 04:16:25]

jzfeng@chromium.org changed reviewers:
- jochen@chromium.org

[jzfeng, 2017-04-25 04:39:42]

https://codereview.chromium.org/2836023003/diff/1/chrome/test/chromedriver/ch...
File chrome/test/chromedriver/chrome/chrome_desktop_impl.cc (right):

https://codereview.chromium.org/2836023003/diff/1/chrome/test/chromedriver/ch...
chrome/test/chromedriver/chrome/chrome_desktop_impl.cc:387:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
On 2017/04/25 04:06:48, stgao(ping after 24h) wrote:
> I would not suggest a sleep like this, because it could make tests flaky due
to
> timing issue. Is there some deterministic solution for this?

I'm afraid not, since it is an ui event and the chrome ui never expect there
could be a window bound change before the ui has already finished the previous
change. Even the browser test for the devtools command use some sleep strategy.

[stgao, 2017-04-25 16:59:40]

https://codereview.chromium.org/2836023003/diff/1/chrome/test/chromedriver/ch...
File chrome/test/chromedriver/chrome/chrome_desktop_impl.cc (right):

https://codereview.chromium.org/2836023003/diff/1/chrome/test/chromedriver/ch...
chrome/test/chromedriver/chrome/chrome_desktop_impl.cc:387:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
On 2017/04/25 04:39:41, jzfeng wrote:
> On 2017/04/25 04:06:48, stgao(ping after 24h) wrote:
> > I would not suggest a sleep like this, because it could make tests flaky due
> to
> > timing issue. Is there some deterministic solution for this?
> 
> I'm afraid not, since it is an ui event and the chrome ui never expect there
> could be a window bound change before the ui has already finished the previous
> change. Even the browser test for the devtools command use some sleep
strategy.

Is it possible to force the ui event to happen or wait for it to complete?

Before we had a trick for such case in the renderer process. After the intended
devtools command, send another effect-less devtools command to the same renderer
process so that the intended one will complete before the effect-less one
return. However, I'm not sure if it works for the Browser.setWindowBounds
command.

[jzfeng, 2017-04-25 23:36:23]

> Is it possible to force the ui event to happen or wait for it to complete?
> 
> Before we had a trick for such case in the renderer process. After the
intended
> devtools command, send another effect-less devtools command to the same
renderer
> process so that the intended one will complete before the effect-less one
> return. However, I'm not sure if it works for the Browser.setWindowBounds
> command.

There is no such simple command yet and I don't think it is easy to implement in
chrome. 
The only way I can think of is sending a Browser.getWindowBounds command every 
several millisecondsto check whether the bounds has changed as intended . 
And we can return once the check succeed. WDYT?

Thanks!

[stgao, 2017-04-26 00:52:42]

On 2017/04/25 23:36:23, jzfeng wrote:
> > Is it possible to force the ui event to happen or wait for it to complete?
> > 
> > Before we had a trick for such case in the renderer process. After the
> intended
> > devtools command, send another effect-less devtools command to the same
> renderer
> > process so that the intended one will complete before the effect-less one
> > return. However, I'm not sure if it works for the Browser.setWindowBounds
> > command.
> 
> There is no such simple command yet and I don't think it is easy to implement
in
> chrome. 
> The only way I can think of is sending a Browser.getWindowBounds command every

> several millisecondsto check whether the bounds has changed as intended . 
> And we can return once the check succeed. WDYT?
> 
> Thanks!

As johnchen@ is the owner of Chromedriver, his approval is more important than
my comment :)

[johnchen, 2017-04-26 01:48:18]

On 2017/04/26 00:52:42, stgao(ping after 24h) wrote:
> On 2017/04/25 23:36:23, jzfeng wrote:
> > > Is it possible to force the ui event to happen or wait for it to complete?
> > > 
> > > Before we had a trick for such case in the renderer process. After the
> > intended
> > > devtools command, send another effect-less devtools command to the same
> > renderer
> > > process so that the intended one will complete before the effect-less one
> > > return. However, I'm not sure if it works for the Browser.setWindowBounds
> > > command.
> > 
> > There is no such simple command yet and I don't think it is easy to
implement
> in
> > chrome. 
> > The only way I can think of is sending a Browser.getWindowBounds command
every
> 
> > several millisecondsto check whether the bounds has changed as intended . 
> > And we can return once the check succeed. WDYT?
> > 
> > Thanks!
> 
> As johnchen@ is the owner of Chromedriver, his approval is more important than
> my comment :)

I agree with stgao@ that a simple Sleep can be problematic. You risk either
having a sleep time that is too short, and the window doesn't reach the right
state, or too much wait and needlessly wasting time.

I like the idea of repeatedly sending Browser.getWindowBounds command until the
windows gets into the right state, with a timeout and return error after too
many retries.

[jzfeng, 2017-04-26 07:03:27]

Implemented the retry logic and add more error logging for each case.

I have to change the test case in testWindowPosition, since chrome ui doesn't
allow the window width to be below 300. You may try resizing the window manually
to verify such limitation.

PTAL.

[jzfeng, 2017-04-26 07:05:48]

On 2017/04/26 07:03:27, jzfeng wrote:
> Implemented the retry logic and add more error logging for each case.
> 
> I have to change the test case in testWindowPosition, since chrome ui doesn't
> allow the window width to be below 300. You may try resizing the window
manually
> to verify such limitation.
> 
> PTAL.

To clarify: by "below 300", I mean as small as 300.

[jzfeng, 2017-04-26 07:36:21]

The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 07:36:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jzfeng, 2017-04-26 07:42:16]

On 2017/04/26 07:05:48, jzfeng wrote:
> On 2017/04/26 07:03:27, jzfeng wrote:
> > Implemented the retry logic and add more error logging for each case.
> > 
> > I have to change the test case in testWindowPosition, since chrome ui
doesn't
> > allow the window width to be below 300. You may try resizing the window
> manually
> > to verify such limitation.
> > 
> > PTAL.
> 
> To clarify: by "below 300", I mean as small as 300.

Also change testMaximizedWindow, because on Linux, the default window can't be
moved to (100, 200), since it is too high. So change the size first.

[commit-bot: I haz the power, 2017-04-26 07:52:33]

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

[commit-bot: I haz the power, 2017-04-26 07:52:35]

Dry run: This issue passed the CQ dry run.

[johnchen, 2017-04-26 17:10:47]

I don't fully understand the changes to ChromeDesktopImpl::SetWindowBounds.
Previously, the code verifies that the window is in "normal" state, and call
SetWindowState first if necessary, before attempting to set size. This check has
been removed. Is it turned out to be unnecessary?

I didn't realize that Chrome doesn't always set the window size and position to
the exact value requested by the client. Previously, trying to set window size
to (1,1) would succeed, with the window set to the minimum allowed size. After
this change, such a command would result in an error, even though the window is
still set to the minimum size. This could be a breaking change.

The old extension-based window resize code doesn't have any Sleep. Do you know
why it is necessary when resizing is done through DevTools?

[jzfeng, 2017-04-27 04:28:12]

On 2017/04/26 17:10:47, johnchen wrote:
> I don't fully understand the changes to ChromeDesktopImpl::SetWindowBounds.
> Previously, the code verifies that the window is in "normal" state, and call
> SetWindowState first if necessary, before attempting to set size. This check
has
> been removed. Is it turned out to be unnecessary?

This check is necessary. It has been moved to
ChromeDesktopImpl::SetWindowPosition 
and ChromeDesktopImpl::SetWindowSize. 
Previously, both SetWindowState and SetWindowBounds calls the same devtools
command
and requires sleep. So I merged these two methods into the new SetWindowBounds
method.

> I didn't realize that Chrome doesn't always set the window size and position
to
> the exact value requested by the client. Previously, trying to set window size
> to (1,1) would succeed, with the window set to the minimum allowed size. After
> this change, such a command would result in an error, even though the window
is
> still set to the minimum size. This could be a breaking change.

Changed to not report error when size and position are not set exactly as
intended.


> 
> The old extension-based window resize code doesn't have any Sleep. Do you know
> why it is necessary when resizing is done through DevTools?

The underlying methods called by extension and devtools are the same. 
The difference is that devtools does more check than the extension, 
such as resizing can only be performed when the window state is normal.
While the extension always try to call the underlying methods, and leave
the outcome unknown. (This would cause a mess, if the client try to send
a lot of window management command sequentially and expect each command
to take effect.)
 
Take testWindowMaximize on mac as an example. 
Though the extension seems to work. It actually doesn't. 
After the window is maximized, then SetWindowSize and SetWindowPosition 
are called. The assertEquals check that the size and position are 
correct. However, the internal window state never changes back from 
maximized to normal. The error is not captured since chromedriver 
doesn't have a IsMaximized() method.

As a conclusion, the old extension-based window resize code also need
the sleep code ideally.

PS:
On my Linux workstation, running the python tests using the extension api,
I found: 
1) testWindowPosition failed with incorrect position.
2) by changing a bit of the code (as shown in the patch), 
   testWindowMaximize failed with incorrect size.
While the code using devtools passes all the tests.

To reproduce the problem, just do a git cl patch and change 
kBrowserWindowDevtoolsBuildNo in session_commands.cc to a very large number
to switch back to the extension api.

[johnchen, 2017-04-27 15:58:23]

Thanks for the explanations. LGTM.

Shuotao: Do you have any additional concerns?

[stgao, 2017-04-28 01:22:18]

lgtm

[jzfeng, 2017-04-28 05:04:11]

The CQ bit was checked by jzfeng@chromium.org

[jzfeng, 2017-04-28 05:04:12]

Thanks for the review!

[commit-bot: I haz the power, 2017-04-28 05:04:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 05:19:37]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493355851536700,
"parent_rev": "93637728a91bf34e608f0c281febaf06af44fe30", "commit_rev":
"be8ffd36e6592c835e7c2c183ec87f1d44b52faa"}

[commit-bot: I haz the power, 2017-04-28 05:20:09]

Description was changed from

==========
fix testWindowMaximize bug

Add sleep to SetWindowBounds to give chrome ui enough time to respond to
devtools command.

BUG=chromedriver:1779
==========

to

==========
fix testWindowMaximize bug

Add sleep to SetWindowBounds to give chrome ui enough time to respond to
devtools command.

BUG=chromedriver:1779

Review-Url: https://codereview.chromium.org/2836023003
Cr-Commit-Position: refs/heads/master@{#467893}
Committed:
https://chromium.googlesource.com/chromium/src/+/be8ffd36e6592c835e7c2c183ec8...
==========

[commit-bot: I haz the power, 2017-04-28 05:20:11]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/be8ffd36e6592c835e7c2c183ec8...