|
|
Created:
4 years, 2 months ago by Eric Seckler Modified:
4 years, 1 month ago Reviewers:
dgozman 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, kinuko+watch, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiondevtools: Adds Page.getLayoutMetrics.
This allows e.g. headless embedders to determine the position/dimension/scale of layout and visual viewport.
BUG=546953
Committed: https://crrev.com/10f7e12361ee5e1eeb6955f0bc27eb3c182dd412
Cr-Commit-Position: refs/heads/master@{#428985}
Patch Set 1 #Patch Set 2 : refactor commands to Page.getLayoutMetrics. #
Total comments: 6
Patch Set 3 : address comments. #
Messages
Total messages: 24 (14 generated)
Description was changed from ========== devtools: Adds Emulation.getLayoutViewport and getVisualViewport. ========== to ========== devtools: Adds Emulation.getLayoutViewport and getVisualViewport. BUG=546953 ==========
Description was changed from ========== devtools: Adds Emulation.getLayoutViewport and getVisualViewport. BUG=546953 ========== to ========== devtools: Adds Emulation.getLayoutViewport and getVisualViewport. This allows e.g. headless embedders to figure out what contents are shown in the two viewports. BUG=546953 ==========
eseckler@chromium.org changed reviewers: + dgozman@chromium.org
Dmitry, PTAL :) I'm adding methods for both layout and visual viewport, because both are interesting for headless embedders. The naming also separates them a little more from forceViewport, which doesn't actually change the layout or visual viewport. Thanks!
On 2016/10/21 11:22:00, Eric Seckler wrote: > Dmitry, PTAL :) > > I'm adding methods for both layout and visual viewport, because both are > interesting for headless embedders. The naming also separates them a little more > from forceViewport, which doesn't actually change the layout or visual viewport. > > Thanks! I find it strange that getVisualViewport does not return the forced viewport. Is there any usecase for it to behave this way? Also, isn't layout viewport accessible to javascript (via window.innerWidth/innerHeight/scrollX/scrollY)? I'd prefer to not introduce methods which can be replaced by Runtime.evaluate call.
On 2016/10/21 20:29:23, dgozman wrote: > On 2016/10/21 11:22:00, Eric Seckler wrote: > > Dmitry, PTAL :) > > > > I'm adding methods for both layout and visual viewport, because both are > > interesting for headless embedders. The naming also separates them a little > more > > from forceViewport, which doesn't actually change the layout or visual > viewport. > > > > Thanks! > > I find it strange that getVisualViewport does not return the forced viewport. Is > there any usecase for it to behave this way? > > Also, isn't layout viewport accessible to javascript (via > window.innerWidth/innerHeight/scrollX/scrollY)? I'd prefer to not introduce > methods which can be replaced by Runtime.evaluate call. window.innerWidth/Height/.. change when the visual viewport scales. At the moment, it's neither possible to get the layout nor visual viewport reliably via JS. In the future, there'll be the VisualViewport API, which gives us pretty much what getVisualViewport would provide. (That won't change that you can't figure out the layout viewport, though, at least for the foreseeable future.) I guess we could use the experimental visual viewport api via Runtime.evaluate - then we'd only need getLayoutViewport. The viewport set by forceViewport is not visible to the page, so it's neither the visual nor layout viewport (just what's actually visible in the RenderView). I think we said that we don't need a better for this, since only the DevTools client can set it (and therefore knows its values). The use case of these functions is to obtain the viewports that are used to layout the page elements. That can be independent of forcing a viewport, but doesn't have to. For example, before setting a forced viewport, you might want to intersect the forced viewport text with the current layout viewport (if you only wanted to draw what's inside the layout viewport). Shall we go ahead only with getLayoutViewport for now?
On 2016/10/21 22:03:21, Eric Seckler wrote: > On 2016/10/21 20:29:23, dgozman wrote: > > On 2016/10/21 11:22:00, Eric Seckler wrote: > > > Dmitry, PTAL :) > > > > > > I'm adding methods for both layout and visual viewport, because both are > > > interesting for headless embedders. The naming also separates them a little > > more > > > from forceViewport, which doesn't actually change the layout or visual > > viewport. > > > > > > Thanks! > > > > I find it strange that getVisualViewport does not return the forced viewport. > Is > > there any usecase for it to behave this way? > > > > Also, isn't layout viewport accessible to javascript (via > > window.innerWidth/innerHeight/scrollX/scrollY)? I'd prefer to not introduce > > methods which can be replaced by Runtime.evaluate call. > > window.innerWidth/Height/.. change when the visual viewport scales. At the > moment, it's neither possible to get the layout nor visual viewport reliably via > JS. In the future, there'll be the VisualViewport API, which gives us pretty > much what getVisualViewport would provide. (That won't change that you can't > figure out the layout viewport, though, at least for the foreseeable future.) > > I guess we could use the experimental visual viewport api via Runtime.evaluate - > then we'd only need getLayoutViewport. > > The viewport set by forceViewport is not visible to the page, so it's neither > the visual nor layout viewport (just what's actually visible in the RenderView). > I think we said that we don't need a better for this, since only the DevTools > client can set it (and therefore knows its values). > > The use case of these functions is to obtain the viewports that are used to > layout the page elements. That can be independent of forcing a viewport, but > doesn't have to. For example, before setting a forced viewport, you might want > to intersect the forced viewport text with the current layout viewport (if you > only wanted to draw what's inside the layout viewport). > > Shall we go ahead only with getLayoutViewport for now? Thanks for explanations! We can expose both viewports if waiting for VisualViewport API will take long. Let's go with single Page.getLayoutMetrics which will return all the values together. This will also avoid the confusion between forceViewport and getXXXViewport interaction. Also, we only put tests in devtools_protocol_browsertest if they require some C++ setup/interaction. Otherwise, layout test will be better. In this case, it should be LayoutTests/inspector-protocol/page/get-layout-metrics.html.
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by eseckler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== devtools: Adds Emulation.getLayoutViewport and getVisualViewport. This allows e.g. headless embedders to figure out what contents are shown in the two viewports. BUG=546953 ========== to ========== devtools: Adds Page.getLayoutMetrics. This allows e.g. headless embedders to determine the position/dimension/scale of layout and visual viewport. BUG=546953 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/24 18:10:36, dgozman wrote: > On 2016/10/21 22:03:21, Eric Seckler wrote: > > On 2016/10/21 20:29:23, dgozman wrote: > > > On 2016/10/21 11:22:00, Eric Seckler wrote: > > > > Dmitry, PTAL :) > > > > > > > > I'm adding methods for both layout and visual viewport, because both are > > > > interesting for headless embedders. The naming also separates them a > little > > > more > > > > from forceViewport, which doesn't actually change the layout or visual > > > viewport. > > > > > > > > Thanks! > > > > > > I find it strange that getVisualViewport does not return the forced > viewport. > > Is > > > there any usecase for it to behave this way? > > > > > > Also, isn't layout viewport accessible to javascript (via > > > window.innerWidth/innerHeight/scrollX/scrollY)? I'd prefer to not introduce > > > methods which can be replaced by Runtime.evaluate call. > > > > window.innerWidth/Height/.. change when the visual viewport scales. At the > > moment, it's neither possible to get the layout nor visual viewport reliably > via > > JS. In the future, there'll be the VisualViewport API, which gives us pretty > > much what getVisualViewport would provide. (That won't change that you can't > > figure out the layout viewport, though, at least for the foreseeable future.) > > > > I guess we could use the experimental visual viewport api via Runtime.evaluate > - > > then we'd only need getLayoutViewport. > > > > The viewport set by forceViewport is not visible to the page, so it's neither > > the visual nor layout viewport (just what's actually visible in the > RenderView). > > I think we said that we don't need a better for this, since only the DevTools > > client can set it (and therefore knows its values). > > > > The use case of these functions is to obtain the viewports that are used to > > layout the page elements. That can be independent of forcing a viewport, but > > doesn't have to. For example, before setting a forced viewport, you might want > > to intersect the forced viewport text with the current layout viewport (if you > > only wanted to draw what's inside the layout viewport). > > > > Shall we go ahead only with getLayoutViewport for now? > > Thanks for explanations! We can expose both viewports if waiting for > VisualViewport API will take long. > Let's go with single Page.getLayoutMetrics which will return all the values > together. This will also avoid the confusion between forceViewport and > getXXXViewport interaction. > > Also, we only put tests in devtools_protocol_browsertest if they require some > C++ setup/interaction. Otherwise, layout test will be better. In this case, it > should be LayoutTests/inspector-protocol/page/get-layout-metrics.html. Thanks, sounds good. PTAL :)
lgtm https://codereview.chromium.org/2438023003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector-protocol/page/get-layout-metrics-expected.txt (right): https://codereview.chromium.org/2438023003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector-protocol/page/get-layout-metrics-expected.txt:5: clientWidth : 785 Could this flake on different platforms due to different scrollbar size? https://codereview.chromium.org/2438023003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2438023003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:211: { "name": "scale", "type": "number", "description": "Scale (as used in viewport <meta> tag)." } This one is not necessarily from <meta> tag: user's pinch-zoom and @viewport css rule both affect it. https://codereview.chromium.org/2438023003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2438023003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:7: #include "core/dom/Document.h" Please revert changes in this file :-)
https://codereview.chromium.org/2438023003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector-protocol/page/get-layout-metrics-expected.txt (right): https://codereview.chromium.org/2438023003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector-protocol/page/get-layout-metrics-expected.txt:5: clientWidth : 785 On 2016/10/25 18:14:55, dgozman wrote: > Could this flake on different platforms due to different scrollbar size? Not really, all platforms have the same size mock scrollbars. It would need different expectations if it was run on android (b/c of different window size), but android only runs smoke layout tests AFAIK. https://codereview.chromium.org/2438023003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2438023003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/browser_protocol.json:211: { "name": "scale", "type": "number", "description": "Scale (as used in viewport <meta> tag)." } On 2016/10/25 18:14:55, dgozman wrote: > This one is not necessarily from <meta> tag: user's pinch-zoom and @viewport css > rule both affect it. Hehe, okay, that's not what I meant :) I was thinking of the unit, rather than the value. Clarified (I hope). https://codereview.chromium.org/2438023003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/InspectorEmulationAgent.cpp (right): https://codereview.chromium.org/2438023003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/InspectorEmulationAgent.cpp:7: #include "core/dom/Document.h" On 2016/10/25 18:14:55, dgozman wrote: > Please revert changes in this file :-) Done.
The CQ bit was checked by eseckler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2438023003/#ps80001 (title: "address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== devtools: Adds Page.getLayoutMetrics. This allows e.g. headless embedders to determine the position/dimension/scale of layout and visual viewport. BUG=546953 ========== to ========== devtools: Adds Page.getLayoutMetrics. This allows e.g. headless embedders to determine the position/dimension/scale of layout and visual viewport. BUG=546953 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== devtools: Adds Page.getLayoutMetrics. This allows e.g. headless embedders to determine the position/dimension/scale of layout and visual viewport. BUG=546953 ========== to ========== devtools: Adds Page.getLayoutMetrics. This allows e.g. headless embedders to determine the position/dimension/scale of layout and visual viewport. BUG=546953 Committed: https://crrev.com/10f7e12361ee5e1eeb6955f0bc27eb3c182dd412 Cr-Commit-Position: refs/heads/master@{#428985} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/10f7e12361ee5e1eeb6955f0bc27eb3c182dd412 Cr-Commit-Position: refs/heads/master@{#428985} |