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

Issue 2732923002: Add fromSurface optional parameter to devtools Page.CaptureScreenshot (Closed)

Created:
3 years, 9 months ago by dvallet
Modified:
3 years, 9 months ago
CC:
chromium-reviews, jam, darin-cc_chromium.org, devtools-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add fromSurface optional parameter to devtools Page.CaptureScreenshot This allows headless screenshots to work more consistently in all platforms by getting screenshots from the surface rather than the view. It also fixes screenshots not working properly in headless Mac, due to screen dependency. BUG=687407 Review-Url: https://codereview.chromium.org/2732923002 Cr-Commit-Position: refs/heads/master@{#456309} Committed: https://chromium.googlesource.com/chromium/src/+/b8f3abee06225b664de730b8cd2f7e61897f80ee

Patch Set 1 #

Patch Set 2 : nit change method name #

Total comments: 6

Patch Set 3 : Added fromSurface optional parameter to Page.CaptureScreenshot #

Patch Set 4 : Remove switches include #

Total comments: 11

Patch Set 5 : Use only one Screenshot callback #

Patch Set 6 : Marked fromSurface parameter as experimental #

Total comments: 5

Patch Set 7 : Remove PendingSnapshotFromSurfaceMap #

Total comments: 2

Patch Set 8 : added from_surface parameter to GetSnapshotFromBrowser #

Total comments: 4

Patch Set 9 : nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+100 lines, -26 lines) Patch
M content/browser/devtools/protocol/page_handler.h View 1 2 3 4 2 chunks +5 lines, -0 lines 0 comments Download
M content/browser/devtools/protocol/page_handler.cc View 1 2 3 4 5 6 7 3 chunks +7 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.h View 1 2 3 4 5 6 7 8 4 chunks +18 lines, -6 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.cc View 1 2 3 4 5 6 7 8 2 chunks +65 lines, -15 lines 0 comments Download
M headless/app/headless_shell.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M headless/lib/headless_web_contents_browsertest.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/inspector/browser_protocol.json View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/devtools/front_end/sdk/ScreenCaptureModel.js View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 46 (30 generated)
dvallet
PTAL, I've confirmed that this patch works on Mac, Windows, Linux. Currently I don't set ...
3 years, 9 months ago (2017-03-06 05:11:01 UTC) #5
dvallet
PTAL, I've confirmed that this patch works on Mac, Windows, Linux. Currently I don't set ...
3 years, 9 months ago (2017-03-06 05:11:02 UTC) #6
Eric Seckler
+pfeldman Looks good, thanks! Some comments below :) https://codereview.chromium.org/2732923002/diff/20001/content/browser/devtools/protocol/page_handler.cc File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2732923002/diff/20001/content/browser/devtools/protocol/page_handler.cc#newcode90 content/browser/devtools/protocol/page_handler.cc:90: std::string ...
3 years, 9 months ago (2017-03-06 07:32:53 UTC) #14
pfeldman
+dgozman is your guy!
3 years, 9 months ago (2017-03-06 18:00:04 UTC) #16
dgozman
Let's introduce an optional parameter "fromSurface" (defaulting to false) to the Page.captureScreenshot method and pass ...
3 years, 9 months ago (2017-03-06 18:45:34 UTC) #17
dvallet
PTAL https://codereview.chromium.org/2732923002/diff/20001/content/browser/devtools/protocol/page_handler.cc File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2732923002/diff/20001/content/browser/devtools/protocol/page_handler.cc#newcode90 content/browser/devtools/protocol/page_handler.cc:90: std::string EncodeBitmap(const SkBitmap& bitmap, On 2017/03/06 07:32:53, Eric ...
3 years, 9 months ago (2017-03-08 02:26:14 UTC) #21
dgozman
https://codereview.chromium.org/2732923002/diff/60001/content/browser/devtools/protocol/page_handler.cc File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2732923002/diff/60001/content/browser/devtools/protocol/page_handler.cc#newcode612 content/browser/devtools/protocol/page_handler.cc:612: base::Bind(&PageHandler::ScreenshotFromSurfaceEncoded, You can bind directly to callback: base::Bind(&CaptureScreenshotCallback::sendSuccess, base::Passed(std::move(callback))) ...
3 years, 9 months ago (2017-03-08 19:21:37 UTC) #24
dvallet
https://codereview.chromium.org/2732923002/diff/60001/content/browser/devtools/protocol/page_handler.cc File content/browser/devtools/protocol/page_handler.cc (right): https://codereview.chromium.org/2732923002/diff/60001/content/browser/devtools/protocol/page_handler.cc#newcode612 content/browser/devtools/protocol/page_handler.cc:612: base::Bind(&PageHandler::ScreenshotFromSurfaceEncoded, On 2017/03/08 19:21:36, dgozman wrote: > You can ...
3 years, 9 months ago (2017-03-09 06:19:47 UTC) #27
Eric Seckler
lgtm Thanks! :) https://codereview.chromium.org/2732923002/diff/60001/content/browser/renderer_host/render_widget_host_impl.cc File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/60001/content/browser/renderer_host/render_widget_host_impl.cc#newcode2411 content/browser/renderer_host/render_widget_host_impl.cc:2411: static const int kMaxRetries = 5; ...
3 years, 9 months ago (2017-03-09 09:55:22 UTC) #30
dvallet
Thanks for the comments! https://codereview.chromium.org/2732923002/diff/100001/content/browser/renderer_host/render_widget_host_impl.cc File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/100001/content/browser/renderer_host/render_widget_host_impl.cc#newcode2428 content/browser/renderer_host/render_widget_host_impl.cc:2428: it->second.Run(gfx::Image::CreateFrom1xBitmap(bitmap)); On 2017/03/09 09:55:22, Eric ...
3 years, 9 months ago (2017-03-09 22:43:52 UTC) #31
dgozman
Almost there! https://codereview.chromium.org/2732923002/diff/120001/content/browser/renderer_host/render_widget_host_impl.h File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2732923002/diff/120001/content/browser/renderer_host/render_widget_host_impl.h#newcode211 content/browser/renderer_host/render_widget_host_impl.h:211: void GetSnapshotFromBrowser(const GetSnapshotFromBrowserCallback& callback); Let's just add ...
3 years, 9 months ago (2017-03-09 23:17:36 UTC) #33
dvallet
https://codereview.chromium.org/2732923002/diff/100001/content/browser/renderer_host/render_widget_host_impl.cc File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/100001/content/browser/renderer_host/render_widget_host_impl.cc#newcode2428 content/browser/renderer_host/render_widget_host_impl.cc:2428: it->second.Run(gfx::Image::CreateFrom1xBitmap(bitmap)); On 2017/03/09 22:43:52, dvallet wrote: > On 2017/03/09 ...
3 years, 9 months ago (2017-03-10 00:07:27 UTC) #34
dgozman
lgtm https://codereview.chromium.org/2732923002/diff/140001/content/browser/renderer_host/render_widget_host_impl.cc File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/140001/content/browser/renderer_host/render_widget_host_impl.cc#newcode2402 content/browser/renderer_host/render_widget_host_impl.cc:2402: gfx::Image image = gfx::Image(); nit: it's unnecessary to ...
3 years, 9 months ago (2017-03-10 21:28:17 UTC) #39
dvallet
https://codereview.chromium.org/2732923002/diff/140001/content/browser/renderer_host/render_widget_host_impl.cc File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2732923002/diff/140001/content/browser/renderer_host/render_widget_host_impl.cc#newcode2402 content/browser/renderer_host/render_widget_host_impl.cc:2402: gfx::Image image = gfx::Image(); On 2017/03/10 at 21:28:17, dgozman ...
3 years, 9 months ago (2017-03-12 22:35:34 UTC) #40
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/2732923002/160001
3 years, 9 months ago (2017-03-12 22:37:05 UTC) #43
commit-bot: I haz the power
3 years, 9 months ago (2017-03-13 00:05:29 UTC) #46
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/b8f3abee06225b664de730b8cd2f...

Powered by Google App Engine
This is Rietveld 408576698