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

Issue 2734123004: add a new set of commands to resize and position windows (Closed)

Created:
3 years, 9 months ago by jzfeng
Modified:
3 years, 8 months ago
Reviewers:
samuong, dgozman, pfeldman
CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, darin-cc_chromium.org, devtools-reviews_chromium.org, jam, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman, pfeldman+blink_chromium.org, samuong
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

add a new set of commands to resize and position windows https://docs.google.com/document/d/1Q0kuHgU1d-sj8gePjEU9nHtlQzz5rNvvGH8fCJ3iBq4/edit?usp=sharing Devtools side, add a new UI domain and methods: method: getWindowForTarget, param: targetID, return: windowID, bounds. method: setWindowBounds, param: windowID, bounds method: getWindowBounds, param: windowID, return: bounds bounds object: left, top, width, height, window_state (minimized, maximized, fullscreen, normal) BUG=604324 Review-Url: https://codereview.chromium.org/2734123004 Cr-Commit-Position: refs/heads/master@{#463110} Committed: https://chromium.googlesource.com/chromium/src/+/24a8ad434aab59c8c8a08ba02d934ca5fdfdc7dd

Patch Set 1 #

Patch Set 2 : move DEPS include down to content/browser/devtools #

Total comments: 9

Patch Set 3 : use browser window to support linux/window/mac #

Total comments: 1

Patch Set 4 : implement set fullscreen #

Patch Set 5 : rebase #

Patch Set 6 : make test compile #

Total comments: 2

Patch Set 7 : add methods to UI domain #

Total comments: 15

Patch Set 8 : fix code #

Patch Set 9 : fix layout test #

Total comments: 10

Patch Set 10 : modify protocol #

Total comments: 18

Patch Set 11 : remove unused files #

Patch Set 12 : add tests #

Total comments: 8

Patch Set 13 : minor fix #

Patch Set 14 : remove unused include #

Patch Set 15 : imporve tests error management #

Patch Set 16 : fix linux_chromium_asan_rel_ng #

Patch Set 17 : try another fix #

Patch Set 18 : test #

Patch Set 19 : test2 #

Patch Set 20 : fix memory leak try 2 #

Patch Set 21 : fix windows and mac #

Patch Set 22 : increase timeout #

Patch Set 23 : update test case for mac #

Patch Set 24 : fix MaximizedToNormalWindow on Mac #

Patch Set 25 : fix fullscreen test on mac #

Patch Set 26 : debug #

Patch Set 27 : test #

Patch Set 28 : test2 #

Patch Set 29 : test3 #

Patch Set 30 : test4 #

Patch Set 31 : fix all tests #

Total comments: 4

Patch Set 32 : disallow certain use cases #

Total comments: 6

Patch Set 33 : re-orgnize the logic to be more readable #

Patch Set 34 : fix typo #

Patch Set 35 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+523 lines, -7 lines) Patch
M chrome/browser/devtools/chrome_devtools_manager_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 3 chunks +16 lines, -0 lines 0 comments Download
M chrome/browser/devtools/chrome_devtools_manager_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 6 chunks +225 lines, -3 lines 0 comments Download
M chrome/browser/devtools/devtools_protocol.h View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/devtools/devtools_protocol.cc View 1 2 3 4 5 6 7 3 chunks +15 lines, -3 lines 0 comments Download
M chrome/browser/devtools/devtools_sanity_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 4 chunks +201 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/inspector/schema-get-domains-matches-agents-expected.txt View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/browser_protocol.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 2 chunks +61 lines, -1 line 0 comments Download

Messages

Total messages: 107 (63 generated)
jzfeng
Hi all, I found a way to implement the new commands using views::Widget. Currently it ...
3 years, 9 months ago (2017-03-08 06:32:17 UTC) #3
jzfeng
change the DEPS in content/browser/devtools instead. PTAL.
3 years, 9 months ago (2017-03-08 06:48:50 UTC) #5
dgozman
Some high-level comments below. Overall, we need a more complete proposal which takes into account ...
3 years, 9 months ago (2017-03-08 19:50:12 UTC) #6
jzfeng
Thanks for the high level feedback! I added all the information to a doc: https://docs.google.com/a/chromium.org/document/d/1iDSRQzYiF6xYKp2PuA8kBmghK7Ar0OZXQZFsaUBNgKw/edit?usp=sharing. ...
3 years, 9 months ago (2017-03-13 09:09:15 UTC) #7
samuong
Jianzhou, thanks for putting together the doc! I'd like to leave a comment there, but ...
3 years, 9 months ago (2017-03-13 17:23:00 UTC) #8
jzfeng
Hi Dmitry, Would you mind taking a look? Thanks! On 2017/03/13 at 17:23:00, samuong wrote: ...
3 years, 9 months ago (2017-03-14 06:36:10 UTC) #9
jzfeng
Hi Dmitry, Would you mind taking a look? Thanks!
3 years, 9 months ago (2017-03-15 11:24:40 UTC) #24
samuong
hi jianzhou, i seem to have lost access to your doc again, are you able ...
3 years, 9 months ago (2017-03-15 17:19:42 UTC) #26
dgozman
Sorry for long delay! This overall looks good. Using the BrowserWindow is definitely a right ...
3 years, 9 months ago (2017-03-16 01:46:00 UTC) #28
jzfeng
Thanks for the review! Before changing the code. I have some questions to clarify (see ...
3 years, 9 months ago (2017-03-16 07:16:40 UTC) #29
dgozman
> So, this step requires user to send UI domain devtools commands to > ws://localhost:9222/devtools/browser, ...
3 years, 9 months ago (2017-03-16 16:03:43 UTC) #30
jzfeng
On 2017/03/16 at 16:03:43, dgozman wrote: > Take a look at chrome/browser/ui/browser_list_observer.h. You can get ...
3 years, 9 months ago (2017-03-17 08:54:33 UTC) #31
dgozman
> I read more about the code in chromedriver. From what I can understand: > ...
3 years, 9 months ago (2017-03-17 17:56:02 UTC) #32
jzfeng
Thanks for the information! Added the methods as discussed. PTAL.
3 years, 9 months ago (2017-03-20 07:52:39 UTC) #34
dgozman
I think this shapes up in the right direction. The changes to BrowserWindow seems unnecessary ...
3 years, 9 months ago (2017-03-20 22:00:49 UTC) #36
jzfeng
Thanks for the review! So I need to add tests to devtools_sanity_browsertest.cc, right?
3 years, 9 months ago (2017-03-21 09:13:06 UTC) #42
jzfeng
https://codereview.chromium.org/2734123004/diff/120001/chrome/browser/devtools/chrome_devtools_manager_delegate.cc File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/120001/chrome/browser/devtools/chrome_devtools_manager_delegate.cc#newcode73 chrome/browser/devtools/chrome_devtools_manager_delegate.cc:73: DevToolsAgentHost::GetForId(target_id); On 2017/03/20 at 22:00:48, dgozman wrote: > if ...
3 years, 9 months ago (2017-03-21 09:13:32 UTC) #43
pfeldman
Overall looks good, a couple of protocol and code organization recommendations below. https://codereview.chromium.org/2734123004/diff/160001/chrome/browser/devtools/chrome_devtools_manager_delegate.cc File chrome/browser/devtools/chrome_devtools_manager_delegate.cc ...
3 years, 9 months ago (2017-03-21 17:36:02 UTC) #48
jzfeng
Thanks for the comments! See the response below. https://codereview.chromium.org/2734123004/diff/160001/chrome/browser/devtools/chrome_devtools_manager_delegate.cc File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/160001/chrome/browser/devtools/chrome_devtools_manager_delegate.cc#newcode52 chrome/browser/devtools/chrome_devtools_manager_delegate.cc:52: std::unique_ptr<base::DictionaryValue> ...
3 years, 9 months ago (2017-03-22 06:56:10 UTC) #49
dgozman
A couple of comments, but looks good overall. https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtools/chrome_devtools_manager_delegate.cc File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtools/chrome_devtools_manager_delegate.cc#newcode44 chrome/browser/devtools/chrome_devtools_manager_delegate.cc:44: BrowserWindow* ...
3 years, 9 months ago (2017-03-22 21:28:35 UTC) #54
jzfeng
Thanks for the comments. I also try adding some browser tests. But I am not ...
3 years, 9 months ago (2017-03-23 06:58:25 UTC) #55
dgozman
lgtm modulo comments. Pavel, are you fine with this as well? https://codereview.chromium.org/2734123004/diff/220001/chrome/browser/devtools/chrome_devtools_manager_delegate.cc File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): ...
3 years, 9 months ago (2017-03-23 22:56:01 UTC) #56
pfeldman
looks good to me as well.
3 years, 9 months ago (2017-03-24 00:17:48 UTC) #57
jzfeng
Thanks for the review! https://codereview.chromium.org/2734123004/diff/220001/chrome/browser/devtools/chrome_devtools_manager_delegate.cc File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/220001/chrome/browser/devtools/chrome_devtools_manager_delegate.cc#newcode182 chrome/browser/devtools/chrome_devtools_manager_delegate.cc:182: } else if (window->IsMaximized()) { ...
3 years, 9 months ago (2017-03-24 02:24:40 UTC) #62
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/2734123004/260001
3 years, 9 months ago (2017-03-24 02:24:58 UTC) #64
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/413952)
3 years, 9 months ago (2017-03-24 03:31:30 UTC) #66
dgozman
What's the status of this?
3 years, 8 months ago (2017-04-03 18:25:28 UTC) #71
jzfeng
I tried to submit the patch last week, and found some of the newly added ...
3 years, 8 months ago (2017-04-04 00:42:21 UTC) #72
jzfeng
I tried to submit the patch last week, and found some of the newly added ...
3 years, 8 months ago (2017-04-04 00:42:21 UTC) #73
jzfeng
Hi Dmitry, I made some changes to fix the browser tests on all platforms, especially ...
3 years, 8 months ago (2017-04-05 04:46:18 UTC) #78
jzfeng
On 2017/04/05 at 04:46:18, jzfeng wrote: > Hi Dmitry, > > I made some changes ...
3 years, 8 months ago (2017-04-05 04:46:56 UTC) #79
dgozman
https://codereview.chromium.org/2734123004/diff/600001/chrome/browser/devtools/chrome_devtools_manager_delegate.cc File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/600001/chrome/browser/devtools/chrome_devtools_manager_delegate.cc#newcode89 chrome/browser/devtools/chrome_devtools_manager_delegate.cc:89: if (window->IsMaximized() || retry_attempts <= 0) Retrying is a ...
3 years, 8 months ago (2017-04-05 17:57:23 UTC) #80
jzfeng
https://codereview.chromium.org/2734123004/diff/600001/chrome/browser/devtools/chrome_devtools_manager_delegate.cc File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/600001/chrome/browser/devtools/chrome_devtools_manager_delegate.cc#newcode89 chrome/browser/devtools/chrome_devtools_manager_delegate.cc:89: if (window->IsMaximized() || retry_attempts <= 0) On 2017/04/05 at ...
3 years, 8 months ago (2017-04-06 01:36:14 UTC) #81
jzfeng
Hi Dmitry, I asked tapted@, who is the Mac person in Sydney. From what he ...
3 years, 8 months ago (2017-04-06 05:35:26 UTC) #82
jzfeng
On 2017/04/06 at 05:35:26, jzfeng wrote: > Hi Dmitry, > > I asked tapted@, who ...
3 years, 8 months ago (2017-04-06 07:29:58 UTC) #85
dgozman
I definitely like the new approach - mirroring automation after the real user scenarios is ...
3 years, 8 months ago (2017-04-06 21:56:32 UTC) #88
jzfeng
Cool! I've re-organized the logic in sedbounds as suggested. PTAL. Thanks! https://codereview.chromium.org/2734123004/diff/620001/chrome/browser/devtools/chrome_devtools_manager_delegate.cc File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): ...
3 years, 8 months ago (2017-04-07 04:18:57 UTC) #89
dgozman
lgtm
3 years, 8 months ago (2017-04-07 17:12:39 UTC) #94
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/2734123004/660001
3 years, 8 months ago (2017-04-07 23:21:26 UTC) #96
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/406069)
3 years, 8 months ago (2017-04-07 23:30:15 UTC) #98
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/2734123004/680001
3 years, 8 months ago (2017-04-08 00:47:23 UTC) #101
commit-bot: I haz the power
Committed patchset #35 (id:680001) as https://chromium.googlesource.com/chromium/src/+/24a8ad434aab59c8c8a08ba02d934ca5fdfdc7dd
3 years, 8 months ago (2017-04-08 02:31:25 UTC) #104
jzfeng
A revert of this CL (patchset #35 id:680001) has been created in https://codereview.chromium.org/2812643003/ by jzfeng@chromium.org. ...
3 years, 8 months ago (2017-04-10 17:07:51 UTC) #106
hongchan
3 years, 8 months ago (2017-04-10 17:23:58 UTC) #107
Message was sent while issue was closed.
A revert of this CL (patchset #35 id:680001) has been created in
https://codereview.chromium.org/2809733002/ by hongchan@chromium.org.

The reason for reverting is: This CL is flaky on Mac10.10 builder:
https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.10%20Tests....

Powered by Google App Engine
This is Rietveld 408576698