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

Issue 2226323002: Resize DevTools target frames. (Closed)

Created:
4 years, 4 months ago by Eric Seckler
Modified:
4 years, 4 months ago
CC:
chromium-reviews, caseq+blink_chromium.org, jam, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, darin-cc_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org, dgozman
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Adds commands to get and set the frame size of a DevTools target. This is a first step towards more flexible screenshots (bit.ly/sized-screenshots): The size of the CompositorFrame sent from the renderer and the screenshot size are determined by the size of the RWHV. Combined with renderer-side Emulation overrides, this will allow us to obtain screenshots of a configurable size. We are planning on adding an additional Emulation override command that allows us to scale and position the page content, which will enable screenshots of a custom area of the page. Work-in-progress patch: https://codereview.chromium.org/2237433004/ BUG=625577 Committed: https://crrev.com/10bf0b24f8656aa2046ad92ecda2d6fc78e8f7f7 Cr-Commit-Position: refs/heads/master@{#413721}

Patch Set 1 #

Total comments: 4

Patch Set 2 : Rename, add tests, fix comments/style, add support in shell. #

Total comments: 6

Patch Set 3 : Don't expose WebContents through HeadlessWebContents. #

Total comments: 2

Patch Set 4 : move command to Emulation, pull out other fixes into separate patch. #

Total comments: 2

Patch Set 5 : No support for setFrameSize on Android. #

Total comments: 1

Patch Set 6 : Use MAYBE_ pattern to disable test on Android. #

Total comments: 4

Patch Set 7 : remove getFrameSize, rename to setVisibleSize. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -0 lines) Patch
M content/browser/devtools/protocol/devtools_protocol_browsertest.cc View 1 2 3 4 5 6 3 chunks +28 lines, -0 lines 0 comments Download
M content/browser/devtools/protocol/emulation_handler.h View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/devtools/protocol/emulation_handler.cc View 1 2 3 4 5 6 1 chunk +14 lines, -0 lines 0 comments Download
M headless/lib/headless_devtools_client_browsertest.cc View 1 2 3 4 5 6 2 chunks +18 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/browser_protocol.json View 1 2 3 4 5 6 1 chunk +9 lines, -0 lines 1 comment Download

Messages

Total messages: 48 (24 generated)
Sami
Seems to make sense from my point of view. https://codereview.chromium.org/2226323002/diff/1/third_party/WebKit/Source/core/inspector/browser_protocol.json File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2226323002/diff/1/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode4351 third_party/WebKit/Source/core/inspector/browser_protocol.json:4351: ...
4 years, 4 months ago (2016-08-09 16:45:44 UTC) #2
Eric Seckler
Sami + Alex, do you want to have another look before I send it out ...
4 years, 4 months ago (2016-08-10 11:12:27 UTC) #6
Sami
https://codereview.chromium.org/2226323002/diff/60001/headless/public/headless_web_contents.h File headless/public/headless_web_contents.h (right): https://codereview.chromium.org/2226323002/diff/60001/headless/public/headless_web_contents.h#newcode65 headless/public/headless_web_contents.h:65: virtual content::WebContents* GetWebContents() const = 0; Can we avoid ...
4 years, 4 months ago (2016-08-10 12:11:40 UTC) #9
Eric Seckler
https://codereview.chromium.org/2226323002/diff/60001/headless/public/headless_web_contents.h File headless/public/headless_web_contents.h (right): https://codereview.chromium.org/2226323002/diff/60001/headless/public/headless_web_contents.h#newcode65 headless/public/headless_web_contents.h:65: virtual content::WebContents* GetWebContents() const = 0; On 2016/08/10 12:11:40, ...
4 years, 4 months ago (2016-08-10 12:33:34 UTC) #10
Sami
lgtm. Thanks for adding the tests too.
4 years, 4 months ago (2016-08-10 12:35:17 UTC) #11
alex clarke (OOO till 29th)
https://codereview.chromium.org/2226323002/diff/60001/components/devtools_discovery/devtools_discovery_manager.cc File components/devtools_discovery/devtools_discovery_manager.cc (right): https://codereview.chromium.org/2226323002/diff/60001/components/devtools_discovery/devtools_discovery_manager.cc#newcode70 components/devtools_discovery/devtools_discovery_manager.cc:70: DevToolsDiscoveryManager::HandleCreateTargetCommand( Maybe mention in the patch comment that we ...
4 years, 4 months ago (2016-08-10 12:39:48 UTC) #12
Eric Seckler
Thanks! https://codereview.chromium.org/2226323002/diff/60001/components/devtools_discovery/devtools_discovery_manager.cc File components/devtools_discovery/devtools_discovery_manager.cc (right): https://codereview.chromium.org/2226323002/diff/60001/components/devtools_discovery/devtools_discovery_manager.cc#newcode70 components/devtools_discovery/devtools_discovery_manager.cc:70: DevToolsDiscoveryManager::HandleCreateTargetCommand( On 2016/08/10 12:39:48, alex clarke wrote: > ...
4 years, 4 months ago (2016-08-10 12:57:06 UTC) #14
Eric Seckler
+pfeldman (dgozman on cc) Hi Pavel, Sending this to you since you're familiar with the ...
4 years, 4 months ago (2016-08-10 16:52:14 UTC) #17
pfeldman
https://codereview.chromium.org/2226323002/diff/80001/third_party/WebKit/Source/core/inspector/browser_protocol.json File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2226323002/diff/80001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode4363 third_party/WebKit/Source/core/inspector/browser_protocol.json:4363: "name": "setFrameSize", This should go through the emulation domain ...
4 years, 4 months ago (2016-08-11 00:39:58 UTC) #18
Eric Seckler
https://codereview.chromium.org/2226323002/diff/80001/third_party/WebKit/Source/core/inspector/browser_protocol.json File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2226323002/diff/80001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode4363 third_party/WebKit/Source/core/inspector/browser_protocol.json:4363: "name": "setFrameSize", On 2016/08/11 00:39:58, pfeldman wrote: > This ...
4 years, 4 months ago (2016-08-11 14:20:16 UTC) #23
pfeldman
https://codereview.chromium.org/2226323002/diff/100001/third_party/WebKit/Source/core/inspector/browser_protocol.json File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2226323002/diff/100001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode761 third_party/WebKit/Source/core/inspector/browser_protocol.json:761: "name": "getFrameSize", Do you expect getFrameSize to be used ...
4 years, 4 months ago (2016-08-11 14:34:17 UTC) #24
Eric Seckler
https://codereview.chromium.org/2226323002/diff/100001/third_party/WebKit/Source/core/inspector/browser_protocol.json File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2226323002/diff/100001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode761 third_party/WebKit/Source/core/inspector/browser_protocol.json:761: "name": "getFrameSize", On 2016/08/11 14:34:17, pfeldman wrote: > Do ...
4 years, 4 months ago (2016-08-11 14:45:50 UTC) #25
Eric Seckler
FYI, RWHV does not support resizing on Android via setSize() and it seems infeasible to ...
4 years, 4 months ago (2016-08-12 11:15:30 UTC) #28
Eric Seckler
Pavel and Dmitry, I've added a test that demonstrates the use of the commands in ...
4 years, 4 months ago (2016-08-17 18:57:01 UTC) #29
Sami
Non-owner lgtm. https://codereview.chromium.org/2226323002/diff/120001/content/browser/devtools/protocol/devtools_protocol_browsertest.cc File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right): https://codereview.chromium.org/2226323002/diff/120001/content/browser/devtools/protocol/devtools_protocol_browsertest.cc#newcode857 content/browser/devtools/protocol/devtools_protocol_browsertest.cc:857: #if !defined(OS_ANDROID) nit: Please use the MAYBE_ ...
4 years, 4 months ago (2016-08-18 14:24:53 UTC) #30
dgozman
https://codereview.chromium.org/2226323002/diff/140001/third_party/WebKit/Source/core/inspector/browser_protocol.json File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2226323002/diff/140001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode761 third_party/WebKit/Source/core/inspector/browser_protocol.json:761: "name": "getFrameSize", As you discussed with Pavel, let's remove ...
4 years, 4 months ago (2016-08-18 20:16:25 UTC) #36
Eric Seckler
https://codereview.chromium.org/2226323002/diff/140001/third_party/WebKit/Source/core/inspector/browser_protocol.json File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2226323002/diff/140001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode770 third_party/WebKit/Source/core/inspector/browser_protocol.json:770: "name": "setFrameSize", On 2016/08/18 20:16:25, dgozman wrote: > I'm ...
4 years, 4 months ago (2016-08-18 22:04:40 UTC) #37
dgozman
https://codereview.chromium.org/2226323002/diff/140001/third_party/WebKit/Source/core/inspector/browser_protocol.json File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2226323002/diff/140001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode770 third_party/WebKit/Source/core/inspector/browser_protocol.json:770: "name": "setFrameSize", On 2016/08/18 22:04:40, Eric Seckler wrote: > ...
4 years, 4 months ago (2016-08-19 17:52:30 UTC) #38
Eric Seckler
Dmitry, Sami, PTAL :) On 2016/08/19 17:52:30, dgozman wrote: > https://codereview.chromium.org/2226323002/diff/140001/third_party/WebKit/Source/core/inspector/browser_protocol.json > File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): ...
4 years, 4 months ago (2016-08-22 10:00:38 UTC) #39
Sami
Looks good to me. https://codereview.chromium.org/2226323002/diff/160001/third_party/WebKit/Source/core/inspector/browser_protocol.json File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2226323002/diff/160001/third_party/WebKit/Source/core/inspector/browser_protocol.json#newcode764 third_party/WebKit/Source/core/inspector/browser_protocol.json:764: { "name": "width", "type": "integer", ...
4 years, 4 months ago (2016-08-22 13:42:12 UTC) #40
dgozman
lgtm
4 years, 4 months ago (2016-08-22 17:11:20 UTC) #41
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/2226323002/160001
4 years, 4 months ago (2016-08-23 09:46:13 UTC) #44
commit-bot: I haz the power
Committed patchset #7 (id:160001)
4 years, 4 months ago (2016-08-23 13:37:28 UTC) #46
commit-bot: I haz the power
4 years, 4 months ago (2016-08-23 13:39:35 UTC) #48
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/10bf0b24f8656aa2046ad92ecda2d6fc78e8f7f7
Cr-Commit-Position: refs/heads/master@{#413721}

Powered by Google App Engine
This is Rietveld 408576698