|
|
Created:
3 years, 9 months ago by jzfeng Modified:
3 years, 8 months ago 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. |
Descriptionadd 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 #Messages
Total messages: 107 (63 generated)
Description was changed from ========== add a new set of commands to resize and position windows BUG=604324 ========== to ========== add a new set of commands to resize and position windows changes made in content/DEPS is temporary to make the presubmit check pass. BUG=604324 ==========
jzfeng@chromium.org changed reviewers: + dgozman@chromium.org, samuong@chromium.org
Hi all, I found a way to implement the new commands using views::Widget. Currently it violates the rule in content/DEPS. I can make the handling logic into ChromeDevToolsManagerDelegate if needed. Thanks!
Description was changed from ========== add a new set of commands to resize and position windows changes made in content/DEPS is temporary to make the presubmit check pass. BUG=604324 ========== to ========== add a new set of commands to resize and position windows BUG=604324 ==========
change the DEPS in content/browser/devtools instead. PTAL.
Some high-level comments below. Overall, we need a more complete proposal which takes into account all platforms, maybe multiple browser windows, etc. > I can make the handling logic into ChromeDevToolsManagerDelegate if needed. That's the right way. https://codereview.chromium.org/2734123004/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2734123004/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:637: auto* widget = views::Widget::GetWidgetForNativeWindow( I believe there are not ui::views on MacOS/Android/iOS. We should use cross-platform abstractions instead, or implement this for all the platforms (which I'd really like to avoid). https://codereview.chromium.org/2734123004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2734123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:528: "name": "maximizeWindow", I'd like to see a document or description in the bug of where this is heading. What else do we lack for ChromeDriver? What are the usecases? It's hard to understand how we should structure this without full picture. https://codereview.chromium.org/2734123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:533: "name": "minimizeWindow", - I think we need new domain "Browser" which will control the browser window. - There could be multiple browser windows. Which one we are talking about here? Should we model after extension API you are using currently? https://codereview.chromium.org/2734123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:553: "description": "Set position and size of the desktop window.", Is this supposed to work on mobile devices?
Thanks for the high level feedback! I added all the information to a doc: https://docs.google.com/a/chromium.org/document/d/1iDSRQzYiF6xYKp2PuA8kBmghK7.... This is still a draft version. But we can discuss base on it. PTAL. Platforms tested: linux/window/mac. https://codereview.chromium.org/2734123004/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2734123004/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:637: auto* widget = views::Widget::GetWidgetForNativeWindow( On 2017/03/08 19:50:11, dgozman wrote: > I believe there are not ui::views on MacOS/Android/iOS. We should use > cross-platform abstractions instead, or implement this for all the platforms > (which I'd really like to avoid). I changed to use BrowserWindow, which also works for MacOS, and does exactly the chrome extension does. I feel it is ok to leave Android/iOS as unimplemented. TODO: I will also implment a SetWindowFullscreen method for BrowserWindow in the CL. https://codereview.chromium.org/2734123004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2734123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:528: "name": "maximizeWindow", On 2017/03/08 19:50:11, dgozman wrote: > I'd like to see a document or description in the bug of where this is heading. > What else do we lack for ChromeDriver? What are the usecases? It's hard to > understand how we should structure this without full picture. I added a doc https://docs.google.com/a/chromium.org/document/d/1iDSRQzYiF6xYKp2PuA8kBmghK7... to explain the full picture. https://codereview.chromium.org/2734123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:533: "name": "minimizeWindow", On 2017/03/08 19:50:12, dgozman wrote: > - I think we need new domain "Browser" which will control the browser window. > - There could be multiple browser windows. Which one we are talking about here? > Should we model after extension API you are using currently? I don't know what will be different for multiple browser windows. The intended behavior is to resize the browser window that contains the page. I will create the new domain later in this CL. Keep using the page domain just for testing. https://codereview.chromium.org/2734123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:553: "description": "Set position and size of the desktop window.", On 2017/03/08 19:50:12, dgozman wrote: > Is this supposed to work on mobile devices? No, it isn't.
Jianzhou, thanks for putting together the doc! I'd like to leave a comment there, but I don't have permission. Are you able to grant me access? https://codereview.chromium.org/2734123004/diff/20001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2734123004/diff/20001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:637: auto* widget = views::Widget::GetWidgetForNativeWindow( On 2017/03/13 09:09:15, jzfeng wrote: > On 2017/03/08 19:50:11, dgozman wrote: > > I believe there are not ui::views on MacOS/Android/iOS. We should use > > cross-platform abstractions instead, or implement this for all the platforms > > (which I'd really like to avoid). > > I changed to use BrowserWindow, which also works for MacOS, and does exactly the > chrome extension does. I feel it is ok to leave Android/iOS as unimplemented. > > TODO: I will also implment a SetWindowFullscreen method for BrowserWindow in the > CL. From ChromeDriver's point of view, I think it's OK if you leave out Android support. That's not really a regression since we don't support that right now. However I'm not sure what Dmitry's opinion is on adding an incomplete command to DevTools. For iOS, do we care about it? I thought they were using Apple's WebKit's DevTools?
Hi Dmitry, Would you mind taking a look? Thanks! On 2017/03/13 at 17:23:00, samuong wrote: > Jianzhou, thanks for putting together the doc! I'd like to leave a comment there, but I don't have permission. Are you able to grant me access? Sure. Done. > > https://codereview.chromium.org/2734123004/diff/20001/content/browser/devtool... > File content/browser/devtools/protocol/page_handler.cc (right): > > https://codereview.chromium.org/2734123004/diff/20001/content/browser/devtool... > content/browser/devtools/protocol/page_handler.cc:637: auto* widget = views::Widget::GetWidgetForNativeWindow( > On 2017/03/13 09:09:15, jzfeng wrote: > > On 2017/03/08 19:50:11, dgozman wrote: > > > I believe there are not ui::views on MacOS/Android/iOS. We should use > > > cross-platform abstractions instead, or implement this for all the platforms > > > (which I'd really like to avoid). > > > > I changed to use BrowserWindow, which also works for MacOS, and does exactly the > > chrome extension does. I feel it is ok to leave Android/iOS as unimplemented. > > > > TODO: I will also implment a SetWindowFullscreen method for BrowserWindow in the > > CL. > > From ChromeDriver's point of view, I think it's OK if you leave out Android support. That's not really a regression since we don't support that right now. However I'm not sure what Dmitry's opinion is on adding an incomplete command to DevTools. > > For iOS, do we care about it? I thought they were using Apple's WebKit's DevTools? I don't know how devtools behave on iOS either.
Description was changed from ========== add a new set of commands to resize and position windows BUG=604324 ========== to ========== add a new set of commands to resize and position windows https://docs.google.com/a/chromium.org/document/d/1iDSRQzYiF6xYKp2PuA8kBmghK7... BUG=604324 ==========
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jzfeng@chromium.org changed reviewers: - samuong@chromium.org
Hi Dmitry, Would you mind taking a look? Thanks!
samuong@chromium.org changed reviewers: + samuong@chromium.org
hi jianzhou, i seem to have lost access to your doc again, are you able to add me back?
Description was changed from ========== add a new set of commands to resize and position windows https://docs.google.com/a/chromium.org/document/d/1iDSRQzYiF6xYKp2PuA8kBmghK7... BUG=604324 ========== to ========== add a new set of commands to resize and position windows https://docs.google.com/a/google.com/document/d/1iDSRQzYiF6xYKp2PuA8kBmghK7Ar... BUG=604324 ==========
Sorry for long delay! This overall looks good. Using the BrowserWindow is definitely a right way. I believe we don't need to get it from native window (see below). I propose the following plan: - Add "UI" domain in browser_protocol.json. - Expose this through /devtools/browser websocket endpoint instead of a page, because it is about whole browser. See how BrowserDevToolsAgentHost is created to trace the root of it. Ask me if you need more details. - Apply to all BrowserWindows instead of taking one from the page. We can iterate on this and report all the windows instead so client chooses which one to resize. - See below for fullscreen. https://codereview.chromium.org/2734123004/diff/40001/content/browser/devtool... File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2734123004/diff/40001/content/browser/devtool... content/browser/devtools/protocol/page_handler.cc:606: Response PageHandler::SetWindowFullscreen(Maybe<bool> fullscreen) { Is this method an equivalent of fullscreen api, or it just makes the window go fullscreen (I believe you can do that manually on mac)? In the first cast it belongs to the page domain, in the second it belongs to the browser domain. https://codereview.chromium.org/2734123004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2734123004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:542: { "name": "fullscreen", "type": "boolean", "optional": true} Let's not make it optional. https://codereview.chromium.org/2734123004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:549: { "name": "left", "type": "integer", "optional": true, "description": "The offset from the left edge of the screen to move the window to in pixels."}, Why optionals here?
Thanks for the review! Before changing the code. I have some questions to clarify (see below). On 2017/03/16 at 01:46:00, dgozman wrote: > Sorry for long delay! > > This overall looks good. Using the BrowserWindow is definitely a right way. I believe we don't need to get it from native window (see below). > > I propose the following plan: > - Add "UI" domain in browser_protocol.json. This step is easy to implement. > - Expose this through /devtools/browser websocket endpoint instead of a page, because it is about whole browser. See how BrowserDevToolsAgentHost is created to trace the root of it. Ask me if you need more details. So, this step requires user to send UI domain devtools commands to ws://localhost:9222/devtools/browser, and then ChromeDevToolsManagerDelegate can handle them. But I don't find a way to get all the BrowserWindows from ChromeDevToolsManagerDelegate. Could you explain more? > - Apply to all BrowserWindows instead of taking one from the page. We can iterate on this and report all the windows instead so client chooses which one to resize. In this case Chromedriver need to: 1) Get all the BrowserWindows, assume each window has an id. (devtool command: UI.getBrowserWindows, return a list of ids) 2) Select the BrowserWindow the current page belongs to. (I don't see a clear way of doing this. Maybe a better way is adding a getBrowserWindow method to page domain, which returns the id of the required BrowserWindow, so that (1) and (2) can be merged.) 3) Resize that BrowserWindow. (devtool command: UI.resize(id)) > - See below for fullscreen. > > https://codereview.chromium.org/2734123004/diff/40001/content/browser/devtool... > File content/browser/devtools/protocol/page_handler.cc (right): > > https://codereview.chromium.org/2734123004/diff/40001/content/browser/devtool... > content/browser/devtools/protocol/page_handler.cc:606: Response PageHandler::SetWindowFullscreen(Maybe<bool> fullscreen) { > Is this method an equivalent of fullscreen api, or it just makes the window go fullscreen (I believe you can do that manually on mac)? In the first cast it belongs to the page domain, in the second it belongs to the browser domain. I'm not sure about the difference between these two. I guess the second one? I assume it should be equivalent to press F11 on linux and click the green button on mac. > > https://codereview.chromium.org/2734123004/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): > > https://codereview.chromium.org/2734123004/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/inspector/browser_protocol.json:542: { "name": "fullscreen", "type": "boolean", "optional": true} > Let's not make it optional. Sure. I can do that. > > https://codereview.chromium.org/2734123004/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/inspector/browser_protocol.json:549: { "name": "left", "type": "integer", "optional": true, "description": "The offset from the left edge of the screen to move the window to in pixels."}, > Why optionals here? They are optional here since chromedriver let user to either setPosition or setSize (https://cs.chromium.org/chromium/src/chrome/test/chromedriver/chrome/automati...), without changing the other. I can split this method into 2 if you feel it is better.
> So, this step requires user to send UI domain devtools commands to > ws://localhost:9222/devtools/browser, and then ChromeDevToolsManagerDelegate can > handle them. > But I don't find a way to get all the BrowserWindows from > ChromeDevToolsManagerDelegate. Could you explain more? Take a look at chrome/browser/ui/browser_list_observer.h. You can get BrowserWindow through Browser::window() call. > In this case Chromedriver need to: > 1) Get all the BrowserWindows, assume each window has an id. (devtool command: > UI.getBrowserWindows, return a list of ids) > 2) Select the BrowserWindow the current page belongs to. (I don't see a clear > way of doing this. Maybe a better way is adding a getBrowserWindow method to > page domain, which returns the id of the required BrowserWindow, so that (1) and > (2) can be merged.) > 3) Resize that BrowserWindow. (devtool command: UI.resize(id)) I am not sure about "current page" you refer to. What is that? Do you start the browser yourself? Note that resizing browser window affects more than one page, so if you have two "current pages" it's impossible to resize separately. I think the easiest way for you would be to create a browser window and open a page in it? Or just resize all of them? > I'm not sure about the difference between these two. I guess the second one? I > assume it should be equivalent to press F11 on linux and click the green button > on mac. Alright, that's a window fullscreen, not page fullscreen. Sounds good. > They are optional here since chromedriver let user to either setPosition or > setSize > (https://cs.chromium.org/chromium/src/chrome/test/chromedriver/chrome/automati...), > without changing the other. I can split this method into 2 if you feel it is > better. Thanks for explanation, optionals are good in this case. Just mention that in a description.
On 2017/03/16 at 16:03:43, dgozman wrote: > Take a look at chrome/browser/ui/browser_list_observer.h. You can get BrowserWindow through Browser::window() call. Cool! Looks like we can manage all the windows by making ChromeDevToolsManagerDelegate : public BrowserListObserver and adding BrowserList::AddObserver(this); to ChromeDevToolsManagerDelegate(). > I am not sure about "current page" you refer to. What is that? Do you start the browser yourself? Note that resizing browser window affects more than one page, so if you have two "current pages" it's impossible to resize separately. I think the easiest way for you would be to create a browser window and open a page in it? Or just resize all of them? > I read more about the code in chromedriver. From what I can understand: (1) As shown in the webdriver spec, the window to manipulate is the operating system window corresponding to the "current top-level browsing context". (2) The "current top-level browsing context" has a unique window handle (a string value), which is the id field of a DevToolsAgentHost, retrieved from localhost:9222/json. (3) The handle of the "current top-level browsing context" is initialized as the first tab after launching chrome. Then users can get all the handles (i.e., id of DevToolsAgentHosts) from localhost:9222/json, and switch to one of them. At any time, there is only one "current" handle. So basically, giving a DevToolsAgentHost id, chromedriver should be able to manipulate the browser window that contains the web_contents, which is inspected by the DevToolsAgentHost. So it may make more sense to send the devtools commands to webSocketDebuggerUrl of the DevToolsAgentHost. Could we get the list of BrowserWindows using the method above and then select the BrowserWindow that contains the web_contents to do window management? The way to select the browserwindow could be check: BrowserWindow::GetNativeWindow() == web_contents-> GetTopLevelNativeWindow(). Or maybe there are better ways? > > They are optional here since chromedriver let user to either setPosition or > > setSize > > (https://cs.chromium.org/chromium/src/chrome/test/chromedriver/chrome/automati...), > > without changing the other. I can split this method into 2 if you feel it is > > better. > Thanks for explanation, optionals are good in this case. Just mention that in a description. Sure. Will do.
> I read more about the code in chromedriver. From what I can understand: > (1) As shown in the webdriver spec, the window to manipulate is the operating > system window corresponding to the "current top-level browsing context". > (2) The "current top-level browsing context" has a unique window handle (a > string value), which is the id field of a DevToolsAgentHost, retrieved from > localhost:9222/json. > (3) The handle of the "current top-level browsing context" is initialized as the > first tab after launching chrome. Then users can get all the handles (i.e., id > of DevToolsAgentHosts) from localhost:9222/json, and switch to one of them. At > any time, there is only one "current" handle. > > So basically, giving a DevToolsAgentHost id, chromedriver should be able to > manipulate the browser window that contains the web_contents, which is inspected > by the DevToolsAgentHost. > So it may make more sense to send the devtools commands to webSocketDebuggerUrl > of the DevToolsAgentHost. Not really. Web browser (such as Chrome) operates on windows and tabs inside. We should not design it's API based on one client's abstractions. Let's instead handle that in ChromeDriver. > Could we get the list of BrowserWindows using the method above and then select > the BrowserWindow that contains the web_contents to do window management? Yeah, that would be the way to go. For example, we can have a method to resolve from TargetId (the id from /json/list) to a BrowserWindow instance. > The way to select the browserwindow could be check: > BrowserWindow::GetNativeWindow() == web_contents-> GetTopLevelNativeWindow(). Or > maybe there are better ways? Look for example at FindInspectedBrowserAndTabIndex here: https://cs.chromium.org/chromium/src/chrome/browser/devtools/devtools_window....
Description was changed from ========== add a new set of commands to resize and position windows https://docs.google.com/a/google.com/document/d/1iDSRQzYiF6xYKp2PuA8kBmghK7Ar... BUG=604324 ========== to ========== add a new set of commands to resize and position windows https://docs.google.com/a/google.com/document/d/1iDSRQzYiF6xYKp2PuA8kBmghK7Ar... Devtools side, add a new UI domain and methods: method: getWindowFromTarget, param: targetID, return: windowID. method: maximizeWindow, param: windowID method: miniimizeWindow, param: windowID method: maximizeWindow, param: windowID method: setWindowFullscreen, param: windowID, fullscreen (bool) method: setWindowBounds, param: windowID, left, top, width, height method: getWindowBounds, param: windowID, return: left, top, width, height BUG=604324 ==========
Thanks for the information! Added the methods as discussed. PTAL.
dgozman@chromium.org changed reviewers: + pfeldman@chromium.org
I think this shapes up in the right direction. The changes to BrowserWindow seems unnecessary (see the comment below), but the protocol is good. We'll also need tests of some kind. +pfeldman for more protocol https://codereview.chromium.org/2734123004/diff/120001/chrome/browser/devtool... File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/120001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:73: DevToolsAgentHost::GetForId(target_id); if (!host) CreateErrorResponse(id, "No target with given id") https://codereview.chromium.org/2734123004/diff/120001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:191: window->SetFullscreen(fullscreen); Why not just call window->GetExclusiveAccessContext() and operate on it? https://codereview.chromium.org/2734123004/diff/120001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:243: if (domain == chrome::devtools::UI::kName) if (method.find("UI.") == 0) https://codereview.chromium.org/2734123004/diff/120001/content/browser/devtoo... File content/browser/devtools/protocol/ui_handler.cc (right): https://codereview.chromium.org/2734123004/diff/120001/content/browser/devtoo... content/browser/devtools/protocol/ui_handler.cc:18: Response UIHandler::GetWindowFromTarget(const String& targetId, int* windowId) { It's sad that we have to do a stub here - sorry for that. We'll try to solve that on our side. https://codereview.chromium.org/2734123004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2734123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:4564: { "name": "targetId", "type": "string", "description": "Devtools agent host id." } $ref: Target.TargetID https://codereview.chromium.org/2734123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:4567: { "name": "windowId", "type": "integer", "description": "Browser window id." } Let's have a defined type WindowID (see TargetID for example). https://codereview.chromium.org/2734123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:4579: "description": "Maximize the browser window.", Minimize https://codereview.chromium.org/2734123004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/inspector_protocol_config.json (right): https://codereview.chromium.org/2734123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/inspector_protocol_config.json:101: "domain": "UI", Remove this - we don't need UI in core/inspector.
Description was changed from ========== add a new set of commands to resize and position windows https://docs.google.com/a/google.com/document/d/1iDSRQzYiF6xYKp2PuA8kBmghK7Ar... Devtools side, add a new UI domain and methods: method: getWindowFromTarget, param: targetID, return: windowID. method: maximizeWindow, param: windowID method: miniimizeWindow, param: windowID method: maximizeWindow, param: windowID method: setWindowFullscreen, param: windowID, fullscreen (bool) method: setWindowBounds, param: windowID, left, top, width, height method: getWindowBounds, param: windowID, return: left, top, width, height BUG=604324 ========== to ========== add a new set of commands to resize and position windows https://docs.google.com/a/google.com/document/d/1iDSRQzYiF6xYKp2PuA8kBmghK7Ar... Devtools side, add a new UI domain and methods: method: getWindowFromTarget, param: targetID, return: windowID. method: maximizeWindow, param: windowID method: minimizeWindow, param: windowID method: setWindowFullscreen, param: windowID, fullscreen (bool) method: setWindowBounds, param: windowID, left, top, width, height method: getWindowBounds, param: windowID, return: left, top, width, height BUG=604324 ==========
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Thanks for the review! So I need to add tests to devtools_sanity_browsertest.cc, right?
https://codereview.chromium.org/2734123004/diff/120001/chrome/browser/devtool... File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/120001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:73: DevToolsAgentHost::GetForId(target_id); On 2017/03/20 at 22:00:48, dgozman wrote: > if (!host) > CreateErrorResponse(id, "No target with given id") Done. https://codereview.chromium.org/2734123004/diff/120001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:191: window->SetFullscreen(fullscreen); On 2017/03/20 at 22:00:48, dgozman wrote: > Why not just call window->GetExclusiveAccessContext() and operate on it? Good point! Done. https://codereview.chromium.org/2734123004/diff/120001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:243: if (domain == chrome::devtools::UI::kName) On 2017/03/20 at 22:00:48, dgozman wrote: > if (method.find("UI.") == 0) Done. https://codereview.chromium.org/2734123004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2734123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:4564: { "name": "targetId", "type": "string", "description": "Devtools agent host id." } On 2017/03/20 at 22:00:48, dgozman wrote: > $ref: Target.TargetID Done. https://codereview.chromium.org/2734123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:4567: { "name": "windowId", "type": "integer", "description": "Browser window id." } On 2017/03/20 at 22:00:48, dgozman wrote: > Let's have a defined type WindowID (see TargetID for example). Done. https://codereview.chromium.org/2734123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:4579: "description": "Maximize the browser window.", On 2017/03/20 at 22:00:48, dgozman wrote: > Minimize Done. https://codereview.chromium.org/2734123004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/inspector_protocol_config.json (right): https://codereview.chromium.org/2734123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/inspector_protocol_config.json:101: "domain": "UI", On 2017/03/20 at 22:00:48, dgozman wrote: > Remove this - we don't need UI in core/inspector. Done.
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Overall looks good, a couple of protocol and code organization recommendations below. https://codereview.chromium.org/2734123004/diff/160001/chrome/browser/devtool... File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/160001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:52: std::unique_ptr<base::DictionaryValue> HandleUICommand( dgozman: is this time to instantiate inspector_protocol in chrome/? But for now, for better readability, do you mind introducing method per command with the same signature? GetWindowForTarget GetWindowBounds GetWindowBounds etc. https://codereview.chromium.org/2734123004/diff/160001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:68: content::WebContents* web_contents = host->GetWebContents(); This is nullable, no need to run getIndexOfWebContents when it is null. https://codereview.chromium.org/2734123004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2734123004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:4556: "domain": "UI", Let's rename this to Browser, we might have more Browser-specific requests we'll handle on the chrome/ layer. https://codereview.chromium.org/2734123004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:4567: "name": "getWindowFromTarget", getWindowForTarget https://codereview.chromium.org/2734123004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:4577: "name": "maximizeWindow", If minimized/maximized state is mutually exclusive with the bounds, they could belong to the same "Bounds" struct: top, left, width, height numbers could contain actual values (even for the maximized/minimized states), while 'maximized' and 'minimized' booleans would be optional. Remove this method and the next one, only use setWindowBounds for all of these. You can also return this bounds object from getWindowForTarget to save on a call.
Thanks for the comments! See the response below. https://codereview.chromium.org/2734123004/diff/160001/chrome/browser/devtool... File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/160001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:52: std::unique_ptr<base::DictionaryValue> HandleUICommand( On 2017/03/21 at 17:36:02, pfeldman wrote: > dgozman: is this time to instantiate inspector_protocol in chrome/? But for now, for better readability, do you mind introducing method per command with the same signature? > > GetWindowForTarget > GetWindowBounds > GetWindowBounds > > etc. Done. https://codereview.chromium.org/2734123004/diff/160001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:68: content::WebContents* web_contents = host->GetWebContents(); On 2017/03/21 at 17:36:02, pfeldman wrote: > This is nullable, no need to run getIndexOfWebContents when it is null. Done. https://codereview.chromium.org/2734123004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2734123004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:4556: "domain": "UI", On 2017/03/21 at 17:36:02, pfeldman wrote: > Let's rename this to Browser, we might have more Browser-specific requests we'll handle on the chrome/ layer. Done. https://codereview.chromium.org/2734123004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:4567: "name": "getWindowFromTarget", On 2017/03/21 at 17:36:02, pfeldman wrote: > getWindowForTarget Done. https://codereview.chromium.org/2734123004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:4577: "name": "maximizeWindow", On 2017/03/21 at 17:36:02, pfeldman wrote: > If minimized/maximized state is mutually exclusive with the bounds, they could belong to the same "Bounds" struct: top, left, width, height numbers could contain actual values (even for the maximized/minimized states), while 'maximized' and 'minimized' booleans would be optional. Remove this method and the next one, only use setWindowBounds for all of these. > > You can also return this bounds object from getWindowForTarget to save on a call. Make the method interface the same as chrome.windows.update https://developer.chrome.com/extensions/windows#method-update. And reuse the logic in https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_... PTAL.
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A couple of comments, but looks good overall. https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:44: BrowserWindow* GetBrowserWindow(int window_id) { Place all these functions into a single anonymous namespace (or mark them all as static) to avoid naming conflicts in future. namespace { ... } // namespace https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:90: return DevToolsProtocol::CreateErrorResponse( style: when |if| body takes more than one line, wrap it in braces. https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:101: std::unique_ptr<base::DictionaryValue> result( You don't have to use MakeUnique when you have a variable of specific type: std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue()) MakeUnique is useful for example with auto or for temporaries: auto result = MakeUnique<...>(); foo(MakeUnique<...>()); https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:158: window->Restore(); Is it ok to call Restore when not maximized nor minimized? https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:163: if (window->IsMinimized()) || window->IsMaximized() ? https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:255: return HandleBrowserCommand(id, method, params).release(); Let's only do this if |agent_host->GetType() == DevToolsAgentHost::kTypeBrowser|. https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... File chrome/browser/devtools/devtools_protocol.cc (right): https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... chrome/browser/devtools/devtools_protocol.cc:47: response->SetInteger(kIdParam, command_id); Nice catch! Thanks. https://codereview.chromium.org/2734123004/diff/180001/content/browser/devtoo... File content/browser/devtools/browser_devtools_agent_host.cc (right): https://codereview.chromium.org/2734123004/diff/180001/content/browser/devtoo... content/browser/devtools/browser_devtools_agent_host.cc:55: session->AddHandler(base::WrapUnique(new protocol::BrowserHandler())); IIRC, things should just work now without BrowserHandler and content/browser/devtools/protocol/browser.{h,cc}. https://codereview.chromium.org/2734123004/diff/180001/content/browser/devtoo... File content/browser/devtools/protocol/browser_handler.h (right): https://codereview.chromium.org/2734123004/diff/180001/content/browser/devtoo... content/browser/devtools/protocol/browser_handler.h:17: // This class implements reversed tethering handler. Wrong comment :-) https://codereview.chromium.org/2734123004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2734123004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:4600: { "name": "bounds", "$ref": "Bounds", "description": "New window bounds. The 'minimized', 'maximized' and 'fullscreen' states cannot be combined with 'left', 'top', 'width' or 'height'." } ... Leaves unspecified fields unchanged.
Thanks for the comments. I also try adding some browser tests. But I am not sure if it is the correct way. PTAL. https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:44: BrowserWindow* GetBrowserWindow(int window_id) { On 2017/03/22 at 21:28:34, dgozman wrote: > Place all these functions into a single anonymous namespace (or mark them all as static) to avoid naming conflicts in future. > > namespace { > > ... > > } // namespace Done. https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:90: return DevToolsProtocol::CreateErrorResponse( On 2017/03/22 at 21:28:34, dgozman wrote: > style: when |if| body takes more than one line, wrap it in braces. Thanks for the tip! Done. https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:101: std::unique_ptr<base::DictionaryValue> result( On 2017/03/22 at 21:28:34, dgozman wrote: > You don't have to use MakeUnique when you have a variable of specific type: > > std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue()) > > MakeUnique is useful for example with auto or for temporaries: > auto result = MakeUnique<...>(); > foo(MakeUnique<...>()); The explanation is very helpful! Done. https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:158: window->Restore(); On 2017/03/22 at 21:28:34, dgozman wrote: > Is it ok to call Restore when not maximized nor minimized? Changed to restore only when the window is maximized. I don't find restore useful when the window is minimized on linux. Maybe it is not true for other platform. https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:163: if (window->IsMinimized()) On 2017/03/22 at 21:28:34, dgozman wrote: > || window->IsMaximized() ? Done. https://codereview.chromium.org/2734123004/diff/180001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:255: return HandleBrowserCommand(id, method, params).release(); On 2017/03/22 at 21:28:34, dgozman wrote: > Let's only do this if |agent_host->GetType() == DevToolsAgentHost::kTypeBrowser|. Done. https://codereview.chromium.org/2734123004/diff/180001/content/browser/devtoo... File content/browser/devtools/browser_devtools_agent_host.cc (right): https://codereview.chromium.org/2734123004/diff/180001/content/browser/devtoo... content/browser/devtools/browser_devtools_agent_host.cc:55: session->AddHandler(base::WrapUnique(new protocol::BrowserHandler())); On 2017/03/22 at 21:28:34, dgozman wrote: > IIRC, things should just work now without BrowserHandler and content/browser/devtools/protocol/browser.{h,cc}. Yeah, it works. I forget what lead me to add all these files. https://codereview.chromium.org/2734123004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2734123004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:4600: { "name": "bounds", "$ref": "Bounds", "description": "New window bounds. The 'minimized', 'maximized' and 'fullscreen' states cannot be combined with 'left', 'top', 'width' or 'height'." } On 2017/03/22 at 21:28:34, dgozman wrote: > ... Leaves unspecified fields unchanged. Done.
lgtm modulo comments. Pavel, are you fine with this as well? https://codereview.chromium.org/2734123004/diff/220001/chrome/browser/devtool... File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/220001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:182: } else if (window->IsMaximized()) { || window->IsMinimized() https://codereview.chromium.org/2734123004/diff/220001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:190: bounds = window->GetRestoredBounds(); If minimize/maximize are synchronous, this should never happen (because we don't allow to set bounds together with minimizing), so let's just use GetBounds(). If it's async, we should extract the bounds before manipulating the state to be on the safe side. https://codereview.chromium.org/2734123004/diff/220001/chrome/browser/devtool... File chrome/browser/devtools/chrome_devtools_manager_delegate.h (right): https://codereview.chromium.org/2734123004/diff/220001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.h:29: static std::unique_ptr<base::DictionaryValue> GetWindowForTarget( Move this to private, and add a friend class DevToolsManagerDelegateTest. https://codereview.chromium.org/2734123004/diff/220001/content/browser/devtoo... File content/browser/devtools/protocol_config.json (right): https://codereview.chromium.org/2734123004/diff/220001/content/browser/devtoo... content/browser/devtools/protocol_config.json:13: "domain": "Browser" I think we don't even need this.
looks good to me as well.
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== add a new set of commands to resize and position windows https://docs.google.com/a/google.com/document/d/1iDSRQzYiF6xYKp2PuA8kBmghK7Ar... Devtools side, add a new UI domain and methods: method: getWindowFromTarget, param: targetID, return: windowID. method: maximizeWindow, param: windowID method: minimizeWindow, param: windowID method: setWindowFullscreen, param: windowID, fullscreen (bool) method: setWindowBounds, param: windowID, left, top, width, height method: getWindowBounds, param: windowID, return: left, top, width, height BUG=604324 ========== to ========== add a new set of commands to resize and position windows https://docs.google.com/a/google.com/document/d/1iDSRQzYiF6xYKp2PuA8kBmghK7Ar... 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 ==========
The CQ bit was checked by jzfeng@chromium.org
Thanks for the review! https://codereview.chromium.org/2734123004/diff/220001/chrome/browser/devtool... File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/220001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:182: } else if (window->IsMaximized()) { On 2017/03/23 at 22:56:01, dgozman wrote: > || window->IsMinimized() Done. https://codereview.chromium.org/2734123004/diff/220001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:190: bounds = window->GetRestoredBounds(); On 2017/03/23 at 22:56:01, dgozman wrote: > If minimize/maximize are synchronous, this should never happen (because we don't allow to set bounds together with minimizing), so let's just use GetBounds(). > > If it's async, we should extract the bounds before manipulating the state to be on the safe side. Done. Looks like it is async on linux. So I moved the section before manipulating window state. https://codereview.chromium.org/2734123004/diff/220001/chrome/browser/devtool... File chrome/browser/devtools/chrome_devtools_manager_delegate.h (right): https://codereview.chromium.org/2734123004/diff/220001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.h:29: static std::unique_ptr<base::DictionaryValue> GetWindowForTarget( On 2017/03/23 at 22:56:01, dgozman wrote: > Move this to private, and add a friend class DevToolsManagerDelegateTest. Done. https://codereview.chromium.org/2734123004/diff/220001/content/browser/devtoo... File content/browser/devtools/protocol_config.json (right): https://codereview.chromium.org/2734123004/diff/220001/content/browser/devtoo... content/browser/devtools/protocol_config.json:13: "domain": "Browser" On 2017/03/23 at 22:56:01, dgozman wrote: > I think we don't even need this. Done.
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2734123004/#ps260001 (title: "remove unused include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
What's the status of this?
I tried to submit the patch last week, and found some of the newly added browser tests failed on linux_chromium_asan_rel_ng, mac_chromium_rel_ng, win_chromium_rel_ng, etc. I have been working on setting up my mac and windows machine to fix these tests. Currently the rest tests to fix are on mac, which change the window state from fullscreen to others. In this case, the manual test works as intended, so I just need to find a way to let the tests pass. Within a day or two, I will let you to take another look. Thanks! On Tue, Apr 4, 2017 at 4:25 AM, <dgozman@chromium.org> wrote: > What's the status of this? > > https://codereview.chromium.org/2734123004/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
I tried to submit the patch last week, and found some of the newly added browser tests failed on linux_chromium_asan_rel_ng, mac_chromium_rel_ng, win_chromium_rel_ng, etc. I have been working on setting up my mac and windows machine to fix these tests. Currently the rest tests to fix are on mac, which change the window state from fullscreen to others. In this case, the manual test works as intended, so I just need to find a way to let the tests pass. Within a day or two, I will let you to take another look. Thanks! On Tue, Apr 4, 2017 at 4:25 AM, <dgozman@chromium.org> wrote: > What's the status of this? > > https://codereview.chromium.org/2734123004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Hi Dmitry, I made some changes to fix the browser tests on all platforms, especially on Mac. Thanks!
On 2017/04/05 at 04:46:18, jzfeng wrote: > Hi Dmitry, > > I made some changes to fix the browser tests on all platforms, especially on Mac. > > Thanks! PTAL.
https://codereview.chromium.org/2734123004/diff/600001/chrome/browser/devtool... File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/600001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:89: if (window->IsMaximized() || retry_attempts <= 0) Retrying is a bad practice. Let's ask for help from mac people (see comment below). https://codereview.chromium.org/2734123004/diff/600001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:227: // GetRestoredBounds behave differently on Mac. Get the actual restored What's the difference? Let's ask someone from chrome/browser/ui/cocoa/OWNERS (e.g. thakis@) to shed the light? Maybe we should fix it instead.
https://codereview.chromium.org/2734123004/diff/600001/chrome/browser/devtool... File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/600001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:89: if (window->IsMaximized() || retry_attempts <= 0) On 2017/04/05 at 17:57:23, dgozman wrote: > Retrying is a bad practice. Let's ask for help from mac people (see comment below). Sure, will do. Windows also has such problem, if ExitFullscreen or Restore is called, then SetBounds sometimes get ignored. Would it be better to always post a delayed task instead of retrying? The delay time need to be large enough. https://codereview.chromium.org/2734123004/diff/600001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:227: // GetRestoredBounds behave differently on Mac. Get the actual restored On 2017/04/05 at 17:57:23, dgozman wrote: > What's the difference? Let's ask someone from chrome/browser/ui/cocoa/OWNERS (e.g. thakis@) to shed the light? Maybe we should fix it instead. In mac, GetRestoredBounds is always exactly the same as GetBounds, even when the window is maximized. See code here: https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/browser_window_c.... While on linux, for a maximized window, GetRestoredBounds return the bounds of the window before it is maximized.
Hi Dmitry, I asked tapted@, who is the Mac person in Sydney. From what he had explained, it seems that: 1) On Mac, users can resize a maximized browser window as they like and still keep it maximized. So there's no meaningful RestoredBounds for a maximized window on Mac. 2) The place where we need the retry logic now is because these use cases are not supported by ui. Such as, in fullscreen mode, the user can only exit fullscreen before they can resize the window or change the browser window to other state. (To avoid the retry logic, we may need some observer logic to get notification when the window state transition has finished. This option is complicated to implement.) Hence, would it make more sense to let the devtools command to only support the same set of use cases as the chrome ui? In this way, the retry logic can be avoided. WDYT? Thanks!
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/06 at 05:35:26, jzfeng wrote: > Hi Dmitry, > > I asked tapted@, who is the Mac person in Sydney. From what he had explained, it seems that: > > 1) On Mac, users can resize a maximized browser window as they like and still keep it maximized. So there's no meaningful RestoredBounds for a maximized window on Mac. > 2) The place where we need the retry logic now is because these use cases are not supported by ui. Such as, in fullscreen mode, the user can only exit fullscreen before they can resize the window or change the browser window to other state. (To avoid the retry logic, we may need some observer logic to get notification when the window state transition has finished. This option is complicated to implement.) > > Hence, would it make more sense to let the devtools command to only support the same set of use cases as the chrome ui? > > In this way, the retry logic can be avoided. WDYT? > > Thanks! Change the code as described in the previous email. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I definitely like the new approach - mirroring automation after the real user scenarios is very robust. https://codereview.chromium.org/2734123004/diff/620001/chrome/browser/devtool... File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/620001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:207: "Browser window is in fullscreen mode, can not be minimized, " To resize fullscreen window, restore it to normal state first. https://codereview.chromium.org/2734123004/diff/620001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:221: "or resized."); To resize minimized window, restore it to normal state first. https://codereview.chromium.org/2734123004/diff/620001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:228: // current window is normal or maximized. I find this code hard to read now. Let's structure it either by current window state or the new window state: if (window_state == "fullscreen") { if (minimized || maximized) error; enterFullScreen(); return; } if (window_state == "minimized") { if (fullscreen || maximized) error; minimize(); return; } if (window_state == "maximized") { if (fullscreen || minimized) error; maximize(); return; } if (window_state == "normal") { if (fullscreen) { if (set_bounds) error; exitFullScreen(); } if (minimized || maximized) { if (set_bounds) error; restore(); } if (set_bounds) setBounds(); } return success;
Cool! I've re-organized the logic in sedbounds as suggested. PTAL. Thanks! https://codereview.chromium.org/2734123004/diff/620001/chrome/browser/devtool... File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right): https://codereview.chromium.org/2734123004/diff/620001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:207: "Browser window is in fullscreen mode, can not be minimized, " On 2017/04/06 at 21:56:32, dgozman wrote: > To resize fullscreen window, restore it to normal state first. Done. https://codereview.chromium.org/2734123004/diff/620001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:221: "or resized."); On 2017/04/06 at 21:56:32, dgozman wrote: > To resize minimized window, restore it to normal state first. Done. https://codereview.chromium.org/2734123004/diff/620001/chrome/browser/devtool... chrome/browser/devtools/chrome_devtools_manager_delegate.cc:228: // current window is normal or maximized. On 2017/04/06 at 21:56:32, dgozman wrote: > I find this code hard to read now. Let's structure it either by current window state or the new window state: > > if (window_state == "fullscreen") { > if (minimized || maximized) error; > enterFullScreen(); > return; > } > > if (window_state == "minimized") { > if (fullscreen || maximized) error; > minimize(); > return; > } > > if (window_state == "maximized") { > if (fullscreen || minimized) error; > maximize(); > return; > } > > if (window_state == "normal") { > if (fullscreen) { > if (set_bounds) error; > exitFullScreen(); > } > if (minimized || maximized) { > if (set_bounds) error; > restore(); > } > if (set_bounds) > setBounds(); > } > > return success; Done.
The CQ bit was checked by jzfeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by jzfeng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...)
The CQ bit was checked by jzfeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2734123004/#ps680001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 680001, "attempt_start_ts": 1491612416951110, "parent_rev": "edcc7d75605ea804bf7a2d9a79903fdb4eed6491", "commit_rev": "24a8ad434aab59c8c8a08ba02d934ca5fdfdc7dd"}
Message was sent while issue was closed.
Description was changed from ========== add a new set of commands to resize and position windows https://docs.google.com/a/google.com/document/d/1iDSRQzYiF6xYKp2PuA8kBmghK7Ar... 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 ========== to ========== add a new set of commands to resize and position windows https://docs.google.com/a/google.com/document/d/1iDSRQzYiF6xYKp2PuA8kBmghK7Ar... 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/+/24a8ad434aab59c8c8a08ba02d93... ==========
Message was sent while issue was closed.
Committed patchset #35 (id:680001) as https://chromium.googlesource.com/chromium/src/+/24a8ad434aab59c8c8a08ba02d93...
Message was sent while issue was closed.
Description was changed from ========== add a new set of commands to resize and position windows https://docs.google.com/a/google.com/document/d/1iDSRQzYiF6xYKp2PuA8kBmghK7Ar... 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/+/24a8ad434aab59c8c8a08ba02d93... ========== to ========== add a new set of commands to resize and position windows https://docs.google.com/document/d/1Q0kuHgU1d-sj8gePjEU9nHtlQzz5rNvvGH8fCJ3iB... 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/+/24a8ad434aab59c8c8a08ba02d93... ==========
Message was sent while issue was closed.
A revert of this CL (patchset #35 id:680001) has been created in https://codereview.chromium.org/2812643003/ by jzfeng@chromium.org. The reason for reverting is: It cause flaky browser tests on mac10.10 https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.10%20Tests....
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.... |