Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1737733002:
[DevTools] Handle emulated device scale factor and original device scale factor properly. (Closed)
|
Created:
4 years, 10 months ago by oshima Modified:
4 years, 9 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, jam, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, pfeldman, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle emulated device scale factor and original device scale factor properly in use-zoom-for-dsf mode.
* The zoom level is set to the original device scale factor (m_compositorDeviceScaleFactorOverride).
* Sets the page's dsf to emulated dsf / original dsf so that device pixel ratio becomes emulated dsf.
* Explicitly call ScreenMetricsEmuation::Apply so that RenderWidget can tell if it's in emulation mode.
* Remove DevToolsHost.convertLengthForEmbedder because it's no longer necessary.
BUG=584709
TEST=RenderViewImplScaleFactorTest.ScreenMetricsEmulation*
(existing ScreenMetricsEmulation has been migrated to RenderViewImplScakeFactirTest.ScreenMetricsEmulationWithOriginalDSF1)
also tested manually
Committed: https://crrev.com/50872a70a931c93798061e5976b86499123fa1d0
Cr-Commit-Position: refs/heads/master@{#379277}
Patch Set 1 : #
Total comments: 10
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 8
Patch Set 5 : #Patch Set 6 : add comment #
Total comments: 3
Patch Set 7 : #Messages
Total messages: 68 (39 generated)
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737733002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737733002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737733002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737733002/220001
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #3 (id:180001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:200001) has been deleted
Description was changed from ========== Merge branch 'dev_tools_2' into dev_tools_3 DevTools BUG= patch from issue 1732893002 at patchset 60001 (http://crrev.com/1732893002#ps60001) ========== to ========== Handle emulated device scale factor and original device scale factor properly. * The zoom level is set to the original device scale factor (m_compositorDeviceScaleFactorOverride). * Sets the page's dsf to emulated dsf / original dsf so that device pixel ratio becomes emulated dsf. BUG=584709 ==========
oshima@chromium.org changed reviewers: + dgozman@chromium.org
Patchset #2 (id:240001) has been deleted
Patchset #2 (id:260001) has been deleted
Description was changed from ========== Handle emulated device scale factor and original device scale factor properly. * The zoom level is set to the original device scale factor (m_compositorDeviceScaleFactorOverride). * Sets the page's dsf to emulated dsf / original dsf so that device pixel ratio becomes emulated dsf. BUG=584709 ========== to ========== Handle emulated device scale factor and original device scale factor properly. * The zoom level is set to the original device scale factor (m_compositorDeviceScaleFactorOverride). * Sets the page's dsf to emulated dsf / original dsf so that device pixel ratio becomes emulated dsf. BUG=584709 TEST=RenderViewImplScaleFactorTest.ScreenMetricsEmulation* (existing ScreenMetricsEmulation has been migrated to RenderViewImplScakeFactirTest.ScreenMetricsEmulationWithOriginalDSF1) also tested manually ==========
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737733002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737733002/280001
Description was changed from ========== Handle emulated device scale factor and original device scale factor properly. * The zoom level is set to the original device scale factor (m_compositorDeviceScaleFactorOverride). * Sets the page's dsf to emulated dsf / original dsf so that device pixel ratio becomes emulated dsf. BUG=584709 TEST=RenderViewImplScaleFactorTest.ScreenMetricsEmulation* (existing ScreenMetricsEmulation has been migrated to RenderViewImplScakeFactirTest.ScreenMetricsEmulationWithOriginalDSF1) also tested manually ========== to ========== Handle emulated device scale factor and original device scale factor properly in use-zoom-for-dsf mode. * The zoom level is set to the original device scale factor (m_compositorDeviceScaleFactorOverride). * Sets the page's dsf to emulated dsf / original dsf so that device pixel ratio becomes emulated dsf. BUG=584709 TEST=RenderViewImplScaleFactorTest.ScreenMetricsEmulation* (existing ScreenMetricsEmulation has been migrated to RenderViewImplScakeFactirTest.ScreenMetricsEmulationWithOriginalDSF1) also tested manually ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Hi, seems like I sent this prematualy, sorry about that. I'll send you when it's really ready.
Patchset #1 (id:220001) has been deleted
Description was changed from ========== Handle emulated device scale factor and original device scale factor properly in use-zoom-for-dsf mode. * The zoom level is set to the original device scale factor (m_compositorDeviceScaleFactorOverride). * Sets the page's dsf to emulated dsf / original dsf so that device pixel ratio becomes emulated dsf. BUG=584709 TEST=RenderViewImplScaleFactorTest.ScreenMetricsEmulation* (existing ScreenMetricsEmulation has been migrated to RenderViewImplScakeFactirTest.ScreenMetricsEmulationWithOriginalDSF1) also tested manually ========== to ========== Handle emulated device scale factor and original device scale factor properly in use-zoom-for-dsf mode. * The zoom level is set to the original device scale factor (m_compositorDeviceScaleFactorOverride). * Sets the page's dsf to emulated dsf / original dsf so that device pixel ratio becomes emulated dsf. * Explicitly call ScreenMetricsEmuation::Apply so that RenderWidget can tell if it's in emulation mode. BUG=584709 TEST=RenderViewImplScaleFactorTest.ScreenMetricsEmulation* (existing ScreenMetricsEmulation has been migrated to RenderViewImplScakeFactirTest.ScreenMetricsEmulationWithOriginalDSF1) also tested manually ==========
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Patchset #2 (id:300001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737733002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737733002/320001
Good that we now know what to fix. Let's clean it up a little bit to make less confusing? https://codereview.chromium.org/1737733002/diff/280001/content/renderer/devto... File content/renderer/devtools/render_widget_screen_metrics_emulator.cc (right): https://codereview.chromium.org/1737733002/diff/280001/content/renderer/devto... content/renderer/devtools/render_widget_screen_metrics_emulator.cc:25: // Apply(); Why this change? https://codereview.chromium.org/1737733002/diff/280001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1737733002/diff/280001/content/renderer/rende... content/renderer/render_widget.cc:1486: compositor_->SetPaintedDeviceScaleFactor(GetOriginalDeviceScaleFactor()); These calls start to look messy. When do I use |device_scale_factor_| and when do I use original one? It's unclear. Can we clean this up a little bit? For example, having |device_scale_factor_| and |screenInfo_.deviceScaleFactor| sounds strange. Killing the former could simplify quite a few places, and maybe then we could use a |painted_device_scale_factor| or |compositor_device_scale_factor| to make everything clear? https://codereview.chromium.org/1737733002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/InspectedPagePlaceholder.js (right): https://codereview.chromium.org/1737733002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/InspectedPagePlaceholder.js:81: var windowToViewportRatio = 1000 / DevToolsHost.convertLengthForEmbedder(1000); Looks like this compensates for similar call to convertLengthForEmbedder in devtools.js file. Just remove it altogether.
https://codereview.chromium.org/1737733002/diff/280001/content/renderer/devto... File content/renderer/devtools/render_widget_screen_metrics_emulator.cc (right): https://codereview.chromium.org/1737733002/diff/280001/content/renderer/devto... content/renderer/devtools/render_widget_screen_metrics_emulator.cc:25: // Apply(); On 2016/02/29 20:26:20, dgozman wrote: > Why this change? I updated the description. This is to be able to check if RenderViewImpl is in emulation mode. (The scoped_ptr is set after ctor is finished) https://codereview.chromium.org/1737733002/diff/280001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1737733002/diff/280001/content/renderer/rende... content/renderer/render_widget.cc:1486: compositor_->SetPaintedDeviceScaleFactor(GetOriginalDeviceScaleFactor()); On 2016/02/29 20:26:20, dgozman wrote: > These calls start to look messy. When do I use |device_scale_factor_| and when > do I use original one? It's unclear. Can we clean this up a little bit? For > example, having |device_scale_factor_| and |screenInfo_.deviceScaleFactor| > sounds strange. Killing the former could simplify quite a few places, and maybe > then we could use a |painted_device_scale_factor| or > |compositor_device_scale_factor| to make everything clear? Compositor should always use the original scale factor. It may not clear because the compositor's scale_factor_ here can be overridden by setCompositorDeviceScaleFactorOverride. I think we can make it clearer by always using the original scale factor to compositor here, and then explicitly set the emulated scale factor to WebView in RenderWidgetScreenMetricsEmulator::Apply. (as we briefly discussed offline last week) What do you think? I can also look into killing device_scale_factor_ but let me do this refactoring in a separate CL because I need to merge this to 50 and I want to keep the change not to affect the regular code path (for use-zoom-for-dsf is disabled) https://codereview.chromium.org/1737733002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/InspectedPagePlaceholder.js (right): https://codereview.chromium.org/1737733002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/InspectedPagePlaceholder.js:81: var windowToViewportRatio = 1000 / DevToolsHost.convertLengthForEmbedder(1000); On 2016/02/29 20:26:20, dgozman wrote: > Looks like this compensates for similar call to convertLengthForEmbedder in > devtools.js file. Just remove it altogether. setInspectedPageBounds is also used by browser, which uses DIP, to update the bounds, so we can't simply remove it. The issue is that the same function is called from browser and renderer, which now uses different coordinate system. How about creating a separate API for a browser (like setInspectedPageBoundsInDIP)?
https://codereview.chromium.org/1737733002/diff/280001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1737733002/diff/280001/content/renderer/rende... content/renderer/render_widget.cc:1486: compositor_->SetPaintedDeviceScaleFactor(GetOriginalDeviceScaleFactor()); On 2016/02/29 21:28:03, oshima wrote: > On 2016/02/29 20:26:20, dgozman wrote: > > These calls start to look messy. When do I use |device_scale_factor_| and when > > do I use original one? It's unclear. Can we clean this up a little bit? For > > example, having |device_scale_factor_| and |screenInfo_.deviceScaleFactor| > > sounds strange. Killing the former could simplify quite a few places, and > maybe > > then we could use a |painted_device_scale_factor| or > > |compositor_device_scale_factor| to make everything clear? > > Compositor should always use the original scale factor. It may not clear because > the compositor's > scale_factor_ here can be overridden by setCompositorDeviceScaleFactorOverride. > > I think we can make it clearer by always using the original scale factor to > compositor here, and > then explicitly set the emulated scale factor to WebView in > RenderWidgetScreenMetricsEmulator::Apply. > (as we briefly discussed offline last week) What do you think? > > I can also look into killing device_scale_factor_ but let me do this refactoring > in a separate CL because > I need to merge this to 50 and I want to keep the change not to affect the > regular code path (for use-zoom-for-dsf is disabled) Are you sure about merging this into 50? Sounds scary. Perhaps, we can bake the new mode for one more release (and just disable it in 50)? Since it's a refactoring, users won't notice. https://codereview.chromium.org/1737733002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/InspectedPagePlaceholder.js (right): https://codereview.chromium.org/1737733002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/InspectedPagePlaceholder.js:81: var windowToViewportRatio = 1000 / DevToolsHost.convertLengthForEmbedder(1000); On 2016/02/29 21:28:03, oshima wrote: > On 2016/02/29 20:26:20, dgozman wrote: > > Looks like this compensates for similar call to convertLengthForEmbedder in > > devtools.js file. Just remove it altogether. > > setInspectedPageBounds is also used by browser, which uses DIP, to update the > bounds, so we can't simply remove it. > The issue is that the same function is called from browser and renderer, which > now uses different coordinate system. > > How about creating a separate API for a browser (like > setInspectedPageBoundsInDIP)? Not sure I'm following you. setInspectedPageBounds is only used from the devtools' renderer and goes directly to the browser window. It should talk in whatever units browser ui understands (I think that's DIP). There is no other usage of this function or DevToolsHost.convertLengthForEmbedder.
Uploaded new patch. https://codereview.chromium.org/1737733002/diff/280001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1737733002/diff/280001/content/renderer/rende... content/renderer/render_widget.cc:1486: compositor_->SetPaintedDeviceScaleFactor(GetOriginalDeviceScaleFactor()); On 2016/02/29 21:38:41, dgozman wrote: > On 2016/02/29 21:28:03, oshima wrote: > > On 2016/02/29 20:26:20, dgozman wrote: > > > These calls start to look messy. When do I use |device_scale_factor_| and > when > > > do I use original one? It's unclear. Can we clean this up a little bit? For > > > example, having |device_scale_factor_| and |screenInfo_.deviceScaleFactor| > > > sounds strange. Killing the former could simplify quite a few places, and > > maybe > > > then we could use a |painted_device_scale_factor| or > > > |compositor_device_scale_factor| to make everything clear? > > > > Compositor should always use the original scale factor. It may not clear > because > > the compositor's > > scale_factor_ here can be overridden by > setCompositorDeviceScaleFactorOverride. > > > > I think we can make it clearer by always using the original scale factor to > > compositor here, and > > then explicitly set the emulated scale factor to WebView in > > RenderWidgetScreenMetricsEmulator::Apply. > > (as we briefly discussed offline last week) What do you think? > > > > I can also look into killing device_scale_factor_ but let me do this > refactoring > > in a separate CL because > > I need to merge this to 50 and I want to keep the change not to affect the > > regular code path (for use-zoom-for-dsf is disabled) > > Are you sure about merging this into 50? Sounds scary. Perhaps, we can bake the > new mode for one more release (and just disable it in 50)? Since it's a > refactoring, users won't notice. This one isn't refactoreing, and we need this to merge to 50 for ChromeOS. That's why I made this change specific to this mode (This is enabled only on ChroemOS and disabled on all other platforms). In worst case, we still have an option to disable it in 50 by the flag (enable-use-zoom-for-dsf). https://codereview.chromium.org/1737733002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/emulation/InspectedPagePlaceholder.js (right): https://codereview.chromium.org/1737733002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/emulation/InspectedPagePlaceholder.js:81: var windowToViewportRatio = 1000 / DevToolsHost.convertLengthForEmbedder(1000); On 2016/02/29 21:38:41, dgozman wrote: > On 2016/02/29 21:28:03, oshima wrote: > > On 2016/02/29 20:26:20, dgozman wrote: > > > Looks like this compensates for similar call to convertLengthForEmbedder in > > > devtools.js file. Just remove it altogether. > > > > setInspectedPageBounds is also used by browser, which uses DIP, to update the > > bounds, so we can't simply remove it. > > The issue is that the same function is called from browser and renderer, which > > now uses different coordinate system. > > > > How about creating a separate API for a browser (like > > setInspectedPageBoundsInDIP)? > > Not sure I'm following you. setInspectedPageBounds is only used from the > devtools' renderer and goes directly to the browser window. It should talk in > whatever units browser ui understands (I think that's DIP). > > There is no other usage of this function or > DevToolsHost.convertLengthForEmbedder. Sorry I seem to have wrong memory. I somehow thought the devtool layout is controlled by browser process. Done.
ping? Let me know if you want me to work on refactoring CL now.
Sorry, this one slipped from my radar. Now I got used to GetOriginalDeviceScaleFactor(), but I'm worried about WebViewImpl's code. I'll also apply and play with this tomorrow. https://codereview.chromium.org/1737733002/diff/360001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1737733002/diff/360001/content/renderer/rende... content/renderer/render_widget.cc:591: screen_metrics_emulator_->Apply(); Similarly, you'll have to decouple Apply from ChangeEmulationParams as well. https://codereview.chromium.org/1737733002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/DevToolsHost.idl (left): https://codereview.chromium.org/1737733002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/DevToolsHost.idl:39: float convertLengthForEmbedder(float length); Remove this from externs.js as well. https://codereview.chromium.org/1737733002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1737733002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3059: if (m_compositorDeviceScaleFactorOverride) { style: 4 spaces in blink https://codereview.chromium.org/1737733002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3060: page()->setDeviceScaleFactor(m_zoomFactorForDeviceScaleFactor / m_compositorDeviceScaleFactorOverride); This is surprising. I thought in new mode device_scale_factor would always be one. This makes emulation conceptually different from the regular case, which I'd like to avoid. Also, do you plan to remove device scale factor from page eventually? This would block it.
https://codereview.chromium.org/1737733002/diff/360001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1737733002/diff/360001/content/renderer/rende... content/renderer/render_widget.cc:591: screen_metrics_emulator_->Apply(); On 2016/03/02 01:19:56, dgozman wrote: > Similarly, you'll have to decouple Apply from ChangeEmulationParams as well. The instance is already set so it's not necessary. I'm happy to change if you think that's better. Please let me know. https://codereview.chromium.org/1737733002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/DevToolsHost.idl (left): https://codereview.chromium.org/1737733002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/DevToolsHost.idl:39: float convertLengthForEmbedder(float length); On 2016/03/02 01:19:56, dgozman wrote: > Remove this from externs.js as well. Done. https://codereview.chromium.org/1737733002/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1737733002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3059: if (m_compositorDeviceScaleFactorOverride) { On 2016/03/02 01:19:56, dgozman wrote: > style: 4 spaces in blink Done. https://codereview.chromium.org/1737733002/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3060: page()->setDeviceScaleFactor(m_zoomFactorForDeviceScaleFactor / m_compositorDeviceScaleFactorOverride); On 2016/03/02 01:19:56, dgozman wrote: > This is surprising. I thought in new mode device_scale_factor would always be > one. This makes emulation conceptually different from the regular case, which > I'd like to avoid. > Also, do you plan to remove device scale factor from page eventually? This would > block it. DPR (Device Pixel Ratio), which emulator sets, is different from Device Scale Factor. DPR is computed by "Device Scale Factor set in Page" * ZoomFactor. In new mode, compositor's device scale factor is always one, and instead it'll apply zoom factor to blink side. The above is to get the produce DPR in new mode. This behaves exactly same as old mode. And my new test should cover both new and old mode. (ChromeOS runs with new mode, while other platforms runs with old mode) Once the migration is completed, we should simply have "emulated DPR", which does not take zoom factor into account.
emulation lgtm
Description was changed from ========== Handle emulated device scale factor and original device scale factor properly in use-zoom-for-dsf mode. * The zoom level is set to the original device scale factor (m_compositorDeviceScaleFactorOverride). * Sets the page's dsf to emulated dsf / original dsf so that device pixel ratio becomes emulated dsf. * Explicitly call ScreenMetricsEmuation::Apply so that RenderWidget can tell if it's in emulation mode. BUG=584709 TEST=RenderViewImplScaleFactorTest.ScreenMetricsEmulation* (existing ScreenMetricsEmulation has been migrated to RenderViewImplScakeFactirTest.ScreenMetricsEmulationWithOriginalDSF1) also tested manually ========== to ========== Handle emulated device scale factor and original device scale factor properly in use-zoom-for-dsf mode. * The zoom level is set to the original device scale factor (m_compositorDeviceScaleFactorOverride). * Sets the page's dsf to emulated dsf / original dsf so that device pixel ratio becomes emulated dsf. * Explicitly call ScreenMetricsEmuation::Apply so that RenderWidget can tell if it's in emulation mode. * Remove DevToolsHost.convertLengthForEmbedder because it's no longer necessary. BUG=584709 TEST=RenderViewImplScaleFactorTest.ScreenMetricsEmulation* (existing ScreenMetricsEmulation has been migrated to RenderViewImplScakeFactirTest.ScreenMetricsEmulationWithOriginalDSF1) also tested manually ==========
oshima@chromium.org changed reviewers: + kinuko@chromium.org, tkent@chromium.org
tkent -> third_party/WebKit owner kinuko -> content/ owner
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737733002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737733002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, assuming that emulation logic lgt dgozman https://codereview.chromium.org/1737733002/diff/400001/content/renderer/rende... File content/renderer/render_view_browsertest.cc (right): https://codereview.chromium.org/1737733002/diff/400001/content/renderer/rende... content/renderer/render_view_browsertest.cc:2673: TEST_F(RenderViewImplScaleFactorTest, ScreenMetricsEmulationWithOriginalDSF1) { DSF1 / DSF2 comes from SetDeviceScaleFactor 1.f and 2.f, respectively? (Looked a bit cryptic at first look)
https://codereview.chromium.org/1737733002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/DevToolsHost.cpp (left): https://codereview.chromium.org/1737733002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/DevToolsHost.cpp:167: return m_frontendFrame ? m_frontendFrame->pageZoomFactor() : 1; The original code implies m_fontendFrame can be nullptr. Is it ok to remove the nullptr check?
https://codereview.chromium.org/1737733002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/DevToolsHost.cpp (left): https://codereview.chromium.org/1737733002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/DevToolsHost.cpp:167: return m_frontendFrame ? m_frontendFrame->pageZoomFactor() : 1; On 2016/03/03 13:05:55, tkent wrote: > The original code implies m_fontendFrame can be nullptr. Is it ok to remove the > nullptr check? Good point. Done.
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737733002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737733002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by oshima@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/1737733002/#ps420001 (title: " ")
The CQ bit was unchecked by oshima@chromium.org
lgtm
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1737733002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737733002/420001
Message was sent while issue was closed.
Description was changed from ========== Handle emulated device scale factor and original device scale factor properly in use-zoom-for-dsf mode. * The zoom level is set to the original device scale factor (m_compositorDeviceScaleFactorOverride). * Sets the page's dsf to emulated dsf / original dsf so that device pixel ratio becomes emulated dsf. * Explicitly call ScreenMetricsEmuation::Apply so that RenderWidget can tell if it's in emulation mode. * Remove DevToolsHost.convertLengthForEmbedder because it's no longer necessary. BUG=584709 TEST=RenderViewImplScaleFactorTest.ScreenMetricsEmulation* (existing ScreenMetricsEmulation has been migrated to RenderViewImplScakeFactirTest.ScreenMetricsEmulationWithOriginalDSF1) also tested manually ========== to ========== Handle emulated device scale factor and original device scale factor properly in use-zoom-for-dsf mode. * The zoom level is set to the original device scale factor (m_compositorDeviceScaleFactorOverride). * Sets the page's dsf to emulated dsf / original dsf so that device pixel ratio becomes emulated dsf. * Explicitly call ScreenMetricsEmuation::Apply so that RenderWidget can tell if it's in emulation mode. * Remove DevToolsHost.convertLengthForEmbedder because it's no longer necessary. BUG=584709 TEST=RenderViewImplScaleFactorTest.ScreenMetricsEmulation* (existing ScreenMetricsEmulation has been migrated to RenderViewImplScakeFactirTest.ScreenMetricsEmulationWithOriginalDSF1) also tested manually ==========
Message was sent while issue was closed.
Committed patchset #7 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== Handle emulated device scale factor and original device scale factor properly in use-zoom-for-dsf mode. * The zoom level is set to the original device scale factor (m_compositorDeviceScaleFactorOverride). * Sets the page's dsf to emulated dsf / original dsf so that device pixel ratio becomes emulated dsf. * Explicitly call ScreenMetricsEmuation::Apply so that RenderWidget can tell if it's in emulation mode. * Remove DevToolsHost.convertLengthForEmbedder because it's no longer necessary. BUG=584709 TEST=RenderViewImplScaleFactorTest.ScreenMetricsEmulation* (existing ScreenMetricsEmulation has been migrated to RenderViewImplScakeFactirTest.ScreenMetricsEmulationWithOriginalDSF1) also tested manually ========== to ========== Handle emulated device scale factor and original device scale factor properly in use-zoom-for-dsf mode. * The zoom level is set to the original device scale factor (m_compositorDeviceScaleFactorOverride). * Sets the page's dsf to emulated dsf / original dsf so that device pixel ratio becomes emulated dsf. * Explicitly call ScreenMetricsEmuation::Apply so that RenderWidget can tell if it's in emulation mode. * Remove DevToolsHost.convertLengthForEmbedder because it's no longer necessary. BUG=584709 TEST=RenderViewImplScaleFactorTest.ScreenMetricsEmulation* (existing ScreenMetricsEmulation has been migrated to RenderViewImplScakeFactirTest.ScreenMetricsEmulationWithOriginalDSF1) also tested manually Committed: https://crrev.com/50872a70a931c93798061e5976b86499123fa1d0 Cr-Commit-Position: refs/heads/master@{#379277} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/50872a70a931c93798061e5976b86499123fa1d0 Cr-Commit-Position: refs/heads/master@{#379277} |
