Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2612913002:
DevTools: Add feature to capture full-height screenshots (Closed)
|
Created:
3 years, 11 months ago by ahmetemirercin Modified:
3 years, 10 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Add feature to capture full-height screenshots
BUG=638281
Patch Set 1 #
Total comments: 19
Patch Set 2 : DevTools: Add feature to capture full-height screenshots #
Total comments: 11
Patch Set 3 : DevTools: Add feature to capture full-height screenshots #
Total comments: 7
Patch Set 4 : DevTools: Add feature to capture full-height screenshots #Patch Set 5 : nit #
Total comments: 2
Patch Set 6 : Rebase+setVisibleSize fix #Patch Set 7 : More readable #
Total comments: 2
Messages
Total messages: 20 (3 generated)
Description was changed from ========== DevTools: Add feature to capture full-height screenshots BUG= DevTools: Add feature to capture full-height screenshots ========== to ========== DevTools: Add feature to capture full-height screenshots BUG=638281 DevTools: Add feature to capture full-height screenshots ==========
ahmetemiremir@gmail.com changed reviewers: + dgozman@chromium.org, paulirish@chromium.org
Please take a look at this?
Message was sent while issue was closed.
Description was changed from ========== DevTools: Add feature to capture full-height screenshots BUG=638281 DevTools: Add feature to capture full-height screenshots ========== to ========== DevTools: Add feature to capture full-height screenshots BUG=638281 ==========
Message was sent while issue was closed.
Why is this one closed? Do you plan to proceed with this?
Message was sent while issue was closed.
On 2017/01/10 21:54:24, dgozman wrote: > Why is this one closed? Do you plan to proceed with this? Although I've heavily tested this patch, I thought it didn't meet the expectations because it's not reviewed and started to work on another patch which will enable getting screenshot of specified node (root node in the case of full-heights) via inspector protocol (which can also be used to add firefox's 'Screenshot Node' feature).Wdyt?
Message was sent while issue was closed.
On 2017/01/10 22:25:05, ahmetemirercin wrote: > On 2017/01/10 21:54:24, dgozman wrote: > > Why is this one closed? Do you plan to proceed with this? > > Although I've heavily tested this patch, I thought it didn't meet the > expectations because it's not reviewed Sorry for that, I was busy and then sick... and started to work on another patch > which will enable getting screenshot of specified node (root node in the case of > full-heights) via inspector protocol (which can also be used to add firefox's > 'Screenshot Node' feature).Wdyt? We discussed the 'screenshot node' a while ago, and conclusion was it will never be 100% correct, so having full-page correct screenshot is the way to go. Let's reopen this?
On 2017/01/10 22:29:38, dgozman wrote: > On 2017/01/10 22:25:05, ahmetemirercin wrote: > > On 2017/01/10 21:54:24, dgozman wrote: > > > Why is this one closed? Do you plan to proceed with this? > > > > Although I've heavily tested this patch, I thought it didn't meet the > > expectations because it's not reviewed > Sorry for that, I was busy and then sick... > > and started to work on another patch > > which will enable getting screenshot of specified node (root node in the case > of > > full-heights) via inspector protocol (which can also be used to add firefox's > > 'Screenshot Node' feature).Wdyt? > > We discussed the 'screenshot node' a while ago, and conclusion was it will never > be 100% correct, so having full-page correct screenshot is the way to go. Let's > reopen this? I hope you get better soon. Reopened it. Thanks.
This looks great! I applied it locally and played with it - works like a charm modulo viewport constraint (see below). https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:480: var pageWidth = Math.floor(visiblePageRect.width); visiblePageRect is cropped by available space. This makes it hard to take a screenshot of iPadPro, for example. If you move the logic into DeviceModeModel as suggested below, we can use pageWidth from there (equals to |screenSize.width - insets.left - insets.right|). https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:482: var executionContext = UI.context.flavor(SDK.ExecutionContext); Should instead use mainTarget.runtimeModel.defaultExecutionContext() https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:486: function evaluated(result) { Annotate with JSDoc please. https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:488: mainTarget.pageAgent().getLayoutMetrics((err, layoutViewport, visualViewport) => { Actually, we can expose content size through getLayoutMetrics as well. https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:488: mainTarget.pageAgent().getLayoutMetrics((err, layoutViewport, visualViewport) => { Let's put this method (which calls into agent) into DeviceModeModel. Then it can handle all modes (for example, I get exception when capturing full screenshot in responsive mode). https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:493: 0, 0, 0, this._model._device.mobile(), false, this._model.scale())); We should use current device scale factor applied (one from _device or set manually) instead of zero. https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:508: mainTarget.emulationAgent().setVisibleSize(Math.floor(visiblePageRect.width), Math.floor(visiblePageRect.height)); We should instead pass (0, 0) and add support on backend for (0, 0) to reset to default one. https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:510: UI.inspectorView.restore(); Don't need this. https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:532: link.href = URL.createObjectURL(blob); Instead of drawing to canvas, can we just do the following? var link = createElement('a'); link.href = 'data:image/png;base64,' + content; link.download = fileName + '.png'; link.click(); https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeWrapper.js (right): https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeWrapper.js:89: if (actionId === 'emulation.toggle-device-mode') { Put this under a switch as well.
@dgozman, I just updated the patch but it became a little bit different from your suggestions. If these are OK, I will add a test for 'setVisibleSize' and update 'protocol.json' accordingly. So waiting your review to go on. https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:480: var pageWidth = Math.floor(visiblePageRect.width); On 2017/01/11 01:19:25, dgozman wrote: > visiblePageRect is cropped by available space. This makes it hard to take a > screenshot of iPadPro, for example. If you move the logic into DeviceModeModel > as suggested below, we can use pageWidth from there (equals to |screenSize.width > - insets.left - insets.right|). After updating 'setVisibleSize' to handle width&height independently, there will be no need for this. https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:482: var executionContext = UI.context.flavor(SDK.ExecutionContext); On 2017/01/11 01:19:25, dgozman wrote: > Should instead use mainTarget.runtimeModel.defaultExecutionContext() Done. https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:486: function evaluated(result) { On 2017/01/11 01:19:25, dgozman wrote: > Annotate with JSDoc please. Done. https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:488: mainTarget.pageAgent().getLayoutMetrics((err, layoutViewport, visualViewport) => { On 2017/01/11 01:19:25, dgozman wrote: > Let's put this method (which calls into agent) into DeviceModeModel. Then it can > handle all modes (for example, I get exception when capturing full screenshot in > responsive mode). Checked that exception about responsive mode and fixed it with 'isMobile' in DeviceModeModel. Is there still need to move this to DMM? https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:493: 0, 0, 0, this._model._device.mobile(), false, this._model.scale())); On 2017/01/11 01:19:25, dgozman wrote: > We should use current device scale factor applied (one from _device or set > manually) instead of zero. Done. https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:508: mainTarget.emulationAgent().setVisibleSize(Math.floor(visiblePageRect.width), Math.floor(visiblePageRect.height)); On 2017/01/11 01:19:25, dgozman wrote: > We should instead pass (0, 0) and add support on backend for (0, 0) to reset to > default one. I think we can reset visible size from DeviceModeModel since we already know the original emulated page width/height and use (0,0) to change w&h independently.This will save us from fighting with pageWidth,visiblePageRect,inset...And seems like more functional than just reseting.What do you think? https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:510: UI.inspectorView.restore(); On 2017/01/11 01:19:25, dgozman wrote: > Don't need this. Done. https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:532: link.href = URL.createObjectURL(blob); On 2017/01/11 01:19:25, dgozman wrote: > Instead of drawing to canvas, can we just do the following? > > var link = createElement('a'); > link.href = 'data:image/png;base64,' + content; > link.download = fileName + '.png'; > link.click(); I tried this and because of data url size limits (afaik) it didn't work for huge snapshots (like mobile news sites) so used blob instead.
I took a closer look on what we can do on backend, and it seems that restoring there reliably would be a challenge. Let's just move all computations and agent work to model, and we'll be fine. https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/2612913002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:532: link.href = URL.createObjectURL(blob); On 2017/01/11 19:25:18, ahmetemirercin wrote: > On 2017/01/11 01:19:25, dgozman wrote: > > Instead of drawing to canvas, can we just do the following? > > > > var link = createElement('a'); > > link.href = 'data:image/png;base64,' + content; > > link.download = fileName + '.png'; > > link.click(); > > I tried this and because of data url size limits (afaik) it didn't work for huge > snapshots (like mobile news sites) so used blob instead. Oh, I see. That's unfortunate... Thanks for explanation! https://codereview.chromium.org/2612913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/2612913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:281: return false; Note this is incorrect for responsive mode. https://codereview.chromium.org/2612913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:534: this._emulatedPageWidth = pageWidth * scale; Use Size instead of width+height. https://codereview.chromium.org/2612913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:612: this._target.emulationAgent().setVisibleSize(this._emulatedPageWidth, this._emulatedPageHeight); If we reset not by passing zeroes, let's not change emulation_handler. Passing zero there should only reset to original. We don't really need it for "not changing the size". https://codereview.chromium.org/2612913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/2612913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:489: var scrollHeight = result.value; I still think we should expose this through getLayoutMetrics by adding one more output parameter |contentSize|. https://codereview.chromium.org/2612913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:490: mainTarget.pageAgent().getLayoutMetrics((err, layoutViewport, visualViewport) => { I still think this should be in DeviceModeModel. View should not operate emulationAgent() at all.
Although still testing it, just wanted to get a roughly review. https://codereview.chromium.org/2612913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/2612913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:281: return false; On 2017/01/12 21:49:35, dgozman wrote: > Note this is incorrect for responsive mode. Done. https://codereview.chromium.org/2612913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:534: this._emulatedPageWidth = pageWidth * scale; On 2017/01/12 21:49:35, dgozman wrote: > Use Size instead of width+height. Done. https://codereview.chromium.org/2612913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:612: this._target.emulationAgent().setVisibleSize(this._emulatedPageWidth, this._emulatedPageHeight); On 2017/01/12 21:49:35, dgozman wrote: > If we reset not by passing zeroes, let's not change emulation_handler. > > Passing zero there should only reset to original. We don't really need it for > "not changing the size". Done. https://codereview.chromium.org/2612913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/2612913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:489: var scrollHeight = result.value; On 2017/01/12 21:49:36, dgozman wrote: > I still think we should expose this through getLayoutMetrics by adding one more > output parameter |contentSize|. Done. https://codereview.chromium.org/2612913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:490: mainTarget.pageAgent().getLayoutMetrics((err, layoutViewport, visualViewport) => { On 2017/01/12 21:49:35, dgozman wrote: > I still think this should be in DeviceModeModel. View should not operate > emulationAgent() at all. Done. https://codereview.chromium.org/2612913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:509: mainTarget.emulationAgent().resetViewport(); This is also done according to comment above. https://codereview.chromium.org/2612913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorPageAgent.h (right): https://codereview.chromium.org/2612913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorPageAgent.h:140: std::unique_ptr<protocol::DOM::Rect>*) override; Just used DOM::Rect instead of inventing a new one like ContenSize. Maybe a lil bit ugly but it seems more clear to me.Is this OK for you?
Almost there! https://codereview.chromium.org/2612913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/2612913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:278: isMobile() { _isMobile https://codereview.chromium.org/2612913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:620: promises.push(this._target.emulationAgent().setDeviceMetricsOverride( nit: let's use params and invoke_setDeviceMetrricsOverride as above. This way we know what each parameter means. https://codereview.chromium.org/2612913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/2612913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:481: mainTarget.pageAgent().captureScreenshot(screenshotCaptured.bind(this)); Put this into model as well, and let it resetViewport() and resetVisibleSize() itself before calling back with screenshot content. Then we don't expose anything extra.
ptal https://codereview.chromium.org/2612913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/2612913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:278: isMobile() { On 2017/01/13 23:38:48, dgozman wrote: > _isMobile Done. https://codereview.chromium.org/2612913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:620: promises.push(this._target.emulationAgent().setDeviceMetricsOverride( On 2017/01/13 23:38:48, dgozman wrote: > nit: let's use params and invoke_setDeviceMetrricsOverride as above. This way we > know what each parameter means. Done. https://codereview.chromium.org/2612913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js (right): https://codereview.chromium.org/2612913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:481: mainTarget.pageAgent().captureScreenshot(screenshotCaptured.bind(this)); On 2017/01/13 23:38:48, dgozman wrote: > Put this into model as well, and let it resetViewport() and resetVisibleSize() > itself before calling back with screenshot content. Then we don't expose > anything extra. Done.
I played with this once again. See below for the only issue I've found. You'll also need to rebaseline, as it didn't apply cleanly. https://codereview.chromium.org/2612913002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/2612913002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:630: promises.push(this._target.emulationAgent().forceViewport(0, 0, visualViewport.scale)); This is using current scale, which produces cropped image if I manually zoom in (Shift+Drag on theverge.com for example). Let's instead force viewport which would capture whole content width. We can either calculate width for setVisibleSize (contentSize.width * viewport.scale * this._scale) or force different viewport's scale (contentSize.width / this._scale / this._emulatedPageSize). I prefer the first one unless it forces too much memory for large manual zoom.
please take a look at this. https://codereview.chromium.org/2612913002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/2612913002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:630: promises.push(this._target.emulationAgent().forceViewport(0, 0, visualViewport.scale)); On 2017/01/18 20:27:59, dgozman wrote: > This is using current scale, which produces cropped image if I manually zoom in > (Shift+Drag on http://theverge.com for example). Let's instead force viewport which > would capture whole content width. We can either calculate width for > setVisibleSize (contentSize.width * viewport.scale * this._scale) or force > different viewport's scale (contentSize.width / this._scale / > this._emulatedPageSize). > > I prefer the first one unless it forces too much memory for large manual zoom. > Second one, setting scale different from actual value makes site go back and forth which seems ugly and because of max frame size (afaik 16384) effect of zoom over memory remain limited. With these, I also prefered the first one. wdyt?
https://codereview.chromium.org/2612913002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js (right): https://codereview.chromium.org/2612913002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:632: promises.push(this._target.emulationAgent().forceViewport(0, 0, visualViewport.scale)); I tried this out: with 150% scale in dropdown and manual scale (Shift+Drag), the image is cropped. Now I think that we should ignore the viewport.scale entirely and force 1.0 since we account for the whole content size anyway. WDYT? https://codereview.chromium.org/2612913002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeModel.js:634: promises.push(this._target.emulationAgent().setVisibleSize(scaledPageSize.width, scaledPageSize.height)); You have to Math.floor() these to send integers.
Hi! Do you plan to land this? It would be very nice to have this feature in next release. If you don't have time, I can follow up and land it for you. WDYT?
On 2017/02/18 06:23:37, dgozman wrote: > Hi! Do you plan to land this? It would be very nice to have this feature in next > release. If you don't have time, I can follow up and land it for you. WDYT? Hi dgozman. Since I can't find any free time for this, I would be great If you can continue with this.
On 2017/02/21 21:30:06, ahmetemirercin wrote: > On 2017/02/18 06:23:37, dgozman wrote: > > Hi! Do you plan to land this? It would be very nice to have this feature in > next > > release. If you don't have time, I can follow up and land it for you. WDYT? > > Hi dgozman. Since I can't find any free time for this, I would be great If you > can continue with this. Thank you for contribution! I posted the patch based on this one (https://codereview.chromium.org/2702113006/), and will close this issue now. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
