Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
347143002:
DevTools: Screencast view scaling (Closed)
|
Created:
6 years, 6 months ago by vkuzkokov Modified:
6 years, 5 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionDevTools: Screencast view scaling
We request the image of max size avaliable but resize canvas to match the size of actual image returned from device.
Depends on https://codereview.chromium.org/351633002/
BUG=387557
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177113
Patch Set 1 #Patch Set 2 : Cleanup debug output #
Total comments: 7
Patch Set 3 : Allow screencast to show more pixels then the device has #Patch Set 4 : Fixed request size on high DPI clients #Patch Set 5 : Removed comment #
Total comments: 8
Patch Set 6 : Accept viewport in DIP (not device CSS px) #Patch Set 7 : Renamed variables #Patch Set 8 : Added units to variable names #
Total comments: 4
Patch Set 9 : Some simplifications #Patch Set 10 : Cleanup #
Total comments: 7
Patch Set 11 : Used new ScreencastFrameMetadata params. Fixed zoom. getNodeForLocation works expects CSS coords now #
Total comments: 8
Patch Set 12 : Used getBoundingClientRect #Patch Set 13 : Small fixes #
Total comments: 6
Patch Set 14 : #Patch Set 15 : Removed unused variable #
Messages
Total messages: 19 (0 generated)
lgtm https://codereview.chromium.org/347143002/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/ScreencastView.js (right): https://codereview.chromium.org/347143002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/ScreencastView.js:155: console.error(event.data.data); Lets remove this message. https://codereview.chromium.org/347143002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/ScreencastView.js:180: this._imageZoom = 1;// this._imageElement.naturalWidth ? this._canvasElement.offsetWidth / this._imageElement.naturalWidth : 1; Please remove this. https://codereview.chromium.org/347143002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/ScreencastView.js:562: if (!model) { if (!model || !this._viewport)
https://codereview.chromium.org/347143002/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/ScreencastView.js (right): https://codereview.chromium.org/347143002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/ScreencastView.js:180: this._imageZoom = 1;// this._imageElement.naturalWidth ? this._canvasElement.offsetWidth / this._imageElement.naturalWidth : 1; On 2014/06/20 17:59:52, pfeldman wrote: > Please remove this. Note that we also need to make this work with devicePixelRatio > 1 on the client.
https://codereview.chromium.org/347143002/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/ScreencastView.js (right): https://codereview.chromium.org/347143002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/ScreencastView.js:155: console.error(event.data.data); On 2014/06/20 17:59:52, pfeldman wrote: > Lets remove this message. Done. https://codereview.chromium.org/347143002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/ScreencastView.js:180: this._imageZoom = 1;// this._imageElement.naturalWidth ? this._canvasElement.offsetWidth / this._imageElement.naturalWidth : 1; On 2014/06/23 06:44:38, pfeldman wrote: > On 2014/06/20 17:59:52, pfeldman wrote: > > Please remove this. > > Note that we also need to make this work with devicePixelRatio > 1 on the > client. Done. https://codereview.chromium.org/347143002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/ScreencastView.js:562: if (!model) { On 2014/06/20 17:59:52, pfeldman wrote: > if (!model || !this._viewport) Done.
https://codereview.chromium.org/347143002/diff/70001/Source/devtools/front_en... File Source/devtools/front_end/ScreencastView.js (right): https://codereview.chromium.org/347143002/diff/70001/Source/devtools/front_en... Source/devtools/front_end/ScreencastView.js:164: var screenWidthDIP = Math.round(this._viewport.width * this._pageScaleFactor); Can we send screen size DIP from the backend instead? https://codereview.chromium.org/347143002/diff/70001/Source/devtools/front_en... Source/devtools/front_end/ScreencastView.js:171: this._imageZoom = Math.min(dimensions.width / screenWidthDIP, dimensions.height / screenHeightDIP) * window.devicePixelRatio / this._screenZoom; This value has nothing to do with the imageZoom, right? https://codereview.chromium.org/347143002/diff/70001/Source/devtools/front_en... Source/devtools/front_end/ScreencastView.js:180: this._viewportElement.style.width = screenWidthDIP / window.devicePixelRatio * this._screenZoom + bordersSize + "px"; This is again confusing - why would we divide screenWidthDIP by devicePixelRatio? Screen width DIP is DIP, viewport's style is DIP, why dividing my dpr? https://codereview.chromium.org/347143002/diff/70001/Source/devtools/front_en... Source/devtools/front_end/ScreencastView.js:278: this.highlightDOMNode(node, this._inspectModeConfig); I fixed it in a separate patch.
https://codereview.chromium.org/347143002/diff/70001/Source/devtools/front_en... File Source/devtools/front_end/ScreencastView.js (right): https://codereview.chromium.org/347143002/diff/70001/Source/devtools/front_en... Source/devtools/front_end/ScreencastView.js:164: var screenWidthDIP = Math.round(this._viewport.width * this._pageScaleFactor); On 2014/06/23 12:43:12, pfeldman wrote: > Can we send screen size DIP from the backend instead? Backend patch: https://codereview.chromium.org/351633002 https://codereview.chromium.org/347143002/diff/70001/Source/devtools/front_en... Source/devtools/front_end/ScreencastView.js:171: this._imageZoom = Math.min(dimensions.width / screenWidthDIP, dimensions.height / screenHeightDIP) * window.devicePixelRatio / this._screenZoom; On 2014/06/23 12:43:12, pfeldman wrote: > This value has nothing to do with the imageZoom, right? Multiplication by _imageZoom translates pixels of the image into real pixels on screen. https://codereview.chromium.org/347143002/diff/70001/Source/devtools/front_en... Source/devtools/front_end/ScreencastView.js:180: this._viewportElement.style.width = screenWidthDIP / window.devicePixelRatio * this._screenZoom + bordersSize + "px"; On 2014/06/23 12:43:12, pfeldman wrote: > This is again confusing - why would we divide screenWidthDIP by > devicePixelRatio? Screen width DIP is DIP, viewport's style is DIP, why dividing > my dpr? Perhaps, the ordering was unfortunate. screenWidthDIP * this._screenZoom / window.devicePixelRatio is better. DIP of device is translated into real pixels of the client and then into DIP of the client. https://codereview.chromium.org/347143002/diff/70001/Source/devtools/front_en... Source/devtools/front_end/ScreencastView.js:278: this.highlightDOMNode(node, this._inspectModeConfig); On 2014/06/23 12:43:12, pfeldman wrote: > I fixed it in a separate patch. Removed fix from this patch
https://codereview.chromium.org/347143002/diff/130001/Source/devtools/front_e... File Source/devtools/front_end/ScreencastView.js (right): https://codereview.chromium.org/347143002/diff/130001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:166: var deviceHeightDIP = Math.round(this._viewport.height + totalBarHeightDIP); Lets pass device dimensions from the backend instead. https://codereview.chromium.org/347143002/diff/130001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:283: node.highlight(this._inspectModeConfig); Roll back
https://codereview.chromium.org/347143002/diff/130001/Source/devtools/front_e... File Source/devtools/front_end/ScreencastView.js (right): https://codereview.chromium.org/347143002/diff/130001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:166: var deviceHeightDIP = Math.round(this._viewport.height + totalBarHeightDIP); On 2014/06/24 16:24:59, pfeldman wrote: > Lets pass device dimensions from the backend instead. Done. https://codereview.chromium.org/347143002/diff/130001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:283: node.highlight(this._inspectModeConfig); On 2014/06/24 16:24:59, pfeldman wrote: > Roll back Done.
https://codereview.chromium.org/347143002/diff/170001/Source/devtools/front_e... File Source/devtools/front_end/ScreencastView.js (right): https://codereview.chromium.org/347143002/diff/170001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:132: dimensions.width *= WebInspector.zoomManager.zoomFactor() * window.devicePixelRatio; window.devicePixelRatio already includes zoom factor. https://codereview.chromium.org/347143002/diff/170001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:165: var deviceWidthDIP = metadata.device.width not: semicolon https://codereview.chromium.org/347143002/diff/170001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:166: var deviceHeightDIP = metadata.device.height ditto https://codereview.chromium.org/347143002/diff/170001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:646: this._context.scale(1 / window.devicePixelRatio, 1 / window.devicePixelRatio); This may be wrong with zoom != 1. Please double check. https://codereview.chromium.org/347143002/diff/170001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:649: this._imageElement.naturalWidth * this._imageZoom * window.devicePixelRatio, Again, devicePixelRatio includes zoom.
https://codereview.chromium.org/347143002/diff/170001/Source/devtools/front_e... File Source/devtools/front_end/ScreencastView.js (right): https://codereview.chromium.org/347143002/diff/170001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:132: dimensions.width *= WebInspector.zoomManager.zoomFactor() * window.devicePixelRatio; On 2014/06/25 13:31:35, dgozman wrote: > window.devicePixelRatio already includes zoom factor. Done. https://codereview.chromium.org/347143002/diff/170001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:646: this._context.scale(1 / window.devicePixelRatio, 1 / window.devicePixelRatio); On 2014/06/25 13:31:35, dgozman wrote: > This may be wrong with zoom != 1. Please double check. Done.
https://codereview.chromium.org/347143002/diff/190001/Source/core/inspector/I... File Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/347143002/diff/190001/Source/core/inspector/I... Source/core/inspector/InspectorDOMAgent.cpp:1416: m_document->renderView()->hitTest(request, result); Why not m_document->frame()->contentRenderer()->hitTest? https://codereview.chromium.org/347143002/diff/190001/Source/devtools/front_e... File Source/devtools/front_end/ScreencastView.js (right): https://codereview.chromium.org/347143002/diff/190001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:168: //if (this._imageZoom < 1.01 / window.devicePixelRatio) Dead code. https://codereview.chromium.org/347143002/diff/190001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:170: this._screenZoom = this._imageElement.naturalWidth * this._imageZoom / metadata.deviceWidth; Let's call this something like |imageCssToDeviceDipScale|. https://codereview.chromium.org/347143002/diff/190001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:643: this._imageElement.naturalWidth * this._imageZoom * window.devicePixelRatio, Why scale context and then multiply everything by the same devicePixelRatio? https://codereview.chromium.org/347143002/diff/190001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:809: var height = Math.floor(this.element.offsetHeight - bordersSize - gutterSize - WebInspector.ScreencastView._navBarHeight); Ensure that canvas has exactly integer width/height properties.
https://codereview.chromium.org/347143002/diff/190001/Source/core/inspector/I... File Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/347143002/diff/190001/Source/core/inspector/I... Source/core/inspector/InspectorDOMAgent.cpp:1416: m_document->renderView()->hitTest(request, result); On 2014/06/26 11:34:21, dgozman wrote: > Why not m_document->frame()->contentRenderer()->hitTest? OK. https://codereview.chromium.org/347143002/diff/190001/Source/devtools/front_e... File Source/devtools/front_end/ScreencastView.js (right): https://codereview.chromium.org/347143002/diff/190001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:168: //if (this._imageZoom < 1.01 / window.devicePixelRatio) On 2014/06/26 11:34:21, dgozman wrote: > Dead code. Forgot to uncomment https://codereview.chromium.org/347143002/diff/190001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:643: this._imageElement.naturalWidth * this._imageZoom * window.devicePixelRatio, On 2014/06/26 11:34:21, dgozman wrote: > Why scale context and then multiply everything by the same devicePixelRatio? No reason
lgtm https://codereview.chromium.org/347143002/diff/230001/Source/devtools/front_e... File Source/devtools/front_end/ScreencastView.js (right): https://codereview.chromium.org/347143002/diff/230001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:481: var zoom = this._canvasElement.offsetWidth / this._deviceWidth; This seems to be equal to _screenZoom. https://codereview.chromium.org/347143002/diff/230001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:571: var scale = this._canvasElement.offsetWidth * this._pageScaleFactor / this._deviceWidth; this is |_screenZoom * _pageScaleFactor| https://codereview.chromium.org/347143002/diff/230001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:606: this._context.fillRect(0, 0, this._canvasElement.offsetWidth, this._screenOffsetTop * this._screenZoom); Switch from offsetWidth/Height to the getBoundingClientRect through this function.
https://codereview.chromium.org/347143002/diff/230001/Source/devtools/front_e... File Source/devtools/front_end/ScreencastView.js (right): https://codereview.chromium.org/347143002/diff/230001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:481: var zoom = this._canvasElement.offsetWidth / this._deviceWidth; On 2014/06/27 12:07:22, dgozman wrote: > This seems to be equal to _screenZoom. It is. Replaced and inlined https://codereview.chromium.org/347143002/diff/230001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:571: var scale = this._canvasElement.offsetWidth * this._pageScaleFactor / this._deviceWidth; On 2014/06/27 12:07:22, dgozman wrote: > this is |_screenZoom * _pageScaleFactor| Replaced and inlined https://codereview.chromium.org/347143002/diff/230001/Source/devtools/front_e... Source/devtools/front_end/ScreencastView.js:606: this._context.fillRect(0, 0, this._canvasElement.offsetWidth, this._screenOffsetTop * this._screenZoom); On 2014/06/27 12:07:22, dgozman wrote: > Switch from offsetWidth/Height to the getBoundingClientRect through this > function. Done.
The CQ bit was checked by vkuzkokov@chromium.org
The CQ bit was unchecked by vkuzkokov@chromium.org
The CQ bit was checked by vkuzkokov@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vkuzkokov@chromium.org/347143002/270001
Message was sent while issue was closed.
Change committed as 177113 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
