Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(328)

Issue 2896763002: Implement window management devtools commands for headless. (Closed)

Created:
3 years, 7 months ago by jzfeng
Modified:
3 years, 7 months ago
Reviewers:
Sami, Eric Seckler
CC:
chromium-reviews, devtools-reviews_chromium.org, pfeldman, mac-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Implement window management devtools commands for headless. These commands are used by chromedriver to change window bounds (size, position) and state (normal, minimized, maximized, fullscreen). 1. add window state to HeadlessWebContentsImpl to track window states change. 2. add window id to HeadlessWebContentsImpl as an unique id used by the commands. 3. window size doesn't change when its state changes. BUG=604324 Review-Url: https://codereview.chromium.org/2896763002 Cr-Commit-Position: refs/heads/master@{#474601} Committed: https://chromium.googlesource.com/chromium/src/+/cbecd589a9ca1e66959de5bc3b6016ff1adb5d21

Patch Set 1 #

Patch Set 2 : nit #

Patch Set 3 : nit #

Total comments: 12

Patch Set 4 : map 1 tab to 1 window and remove screen_size #

Total comments: 6

Patch Set 5 : nit and rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+353 lines, -0 lines) Patch
M headless/BUILD.gn View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M headless/lib/browser/headless_browser_impl.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M headless/lib/browser/headless_browser_impl.cc View 1 2 3 4 1 chunk +13 lines, -0 lines 0 comments Download
M headless/lib/browser/headless_devtools_client_impl.h View 3 chunks +3 lines, -0 lines 0 comments Download
M headless/lib/browser/headless_devtools_client_impl.cc View 2 chunks +5 lines, -0 lines 0 comments Download
M headless/lib/browser/headless_devtools_manager_delegate.h View 1 chunk +9 lines, -0 lines 0 comments Download
M headless/lib/browser/headless_devtools_manager_delegate.cc View 1 2 3 5 chunks +140 lines, -0 lines 0 comments Download
M headless/lib/browser/headless_web_contents_impl.h View 1 2 3 4 2 chunks +9 lines, -0 lines 0 comments Download
M headless/lib/browser/headless_web_contents_impl.cc View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M headless/lib/headless_devtools_client_browsertest.cc View 1 2 3 2 chunks +160 lines, -0 lines 0 comments Download
M headless/public/headless_devtools_client.h View 2 chunks +4 lines, -0 lines 0 comments Download
M headless/test/headless_browser_test.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M headless/test/headless_browser_test.cc View 1 2 3 4 3 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 38 (22 generated)
jzfeng
PTAL. Thanks!
3 years, 7 months ago (2017-05-22 05:14:46 UTC) #7
Sami
Looks great! +Eric, could you make sure mapping all the tabs to a single window ...
3 years, 7 months ago (2017-05-22 16:30:57 UTC) #11
Eric Seckler
On 2017/05/22 16:30:57, Sami wrote: > Looks great! +Eric, could you make sure mapping all ...
3 years, 7 months ago (2017-05-22 19:13:40 UTC) #13
jzfeng
Thanks for the feedback! We can discuss a bit more before I actually change the ...
3 years, 7 months ago (2017-05-23 07:33:13 UTC) #14
Eric Seckler
On 2017/05/23 07:33:13, jzfeng wrote: > Thanks for the feedback! > > We can discuss ...
3 years, 7 months ago (2017-05-23 08:23:59 UTC) #15
Sami
I think I'm also leaning toward having a separate window per WebContents. That's the least ...
3 years, 7 months ago (2017-05-23 16:59:51 UTC) #16
jzfeng
Thanks for the explanation! I have changed to 1:1 mapping and track the window state ...
3 years, 7 months ago (2017-05-24 05:05:39 UTC) #21
Sami
lgtm assuming Eric is happy :) https://codereview.chromium.org/2896763002/diff/60001/headless/lib/browser/headless_browser_impl.h File headless/lib/browser/headless_browser_impl.h (right): https://codereview.chromium.org/2896763002/diff/60001/headless/lib/browser/headless_browser_impl.h#newcode75 headless/lib/browser/headless_browser_impl.h:75: HeadlessWebContentsImpl* GetWebContentsForWindowId(int window_id); ...
3 years, 7 months ago (2017-05-24 17:50:54 UTC) #24
Eric Seckler
Yup, looks great! Thank you! lgtm :)
3 years, 7 months ago (2017-05-24 19:11:29 UTC) #25
jzfeng
Thanks for the review! https://codereview.chromium.org/2896763002/diff/60001/headless/lib/browser/headless_browser_impl.h File headless/lib/browser/headless_browser_impl.h (right): https://codereview.chromium.org/2896763002/diff/60001/headless/lib/browser/headless_browser_impl.h#newcode75 headless/lib/browser/headless_browser_impl.h:75: HeadlessWebContentsImpl* GetWebContentsForWindowId(int window_id); On 2017/05/24 ...
3 years, 7 months ago (2017-05-25 01:10:51 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2896763002/80001
3 years, 7 months ago (2017-05-25 01:12:02 UTC) #29
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/302650)
3 years, 7 months ago (2017-05-25 02:59:39 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2896763002/80001
3 years, 7 months ago (2017-05-25 07:36:09 UTC) #33
commit-bot: I haz the power
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/cbecd589a9ca1e66959de5bc3b6016ff1adb5d21
3 years, 7 months ago (2017-05-25 07:40:51 UTC) #36
findit-for-me
Findit (https://goo.gl/kROfz5) identified this CL at revision 474601 as the culprit for failures in the ...
3 years, 7 months ago (2017-05-25 09:00:52 UTC) #37
Eric Seckler
3 years, 7 months ago (2017-05-25 09:51:08 UTC) #38
Message was sent while issue was closed.
On 2017/05/25 09:00:52, findit-for-me wrote:
> Findit (https://goo.gl/kROfz5) identified this CL at revision 474601 as the
> culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

Looks like this breaks headless_browsertests on mac, because RWHVMac only
supports setting the position for popups [1]. I'll send a patch to disable the
tests on Mac for now, no need to revert this one.

If we want to fix this, we should be setting the bounds of the actual window on
Mac (probably should also do this for aura), e.g. via
WebContentsView::GetTopLevelNativeWindow() [2]. But that requires that all
WebContents have their own top-level window on Mac, which they probably do not
right now.

[1]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
[2]
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...

Powered by Google App Engine
This is Rietveld 408576698