Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16136004:
Add graphics layers for pinch virtual viewport. (Closed)
|
Created:
7 years, 6 months ago by wjmaclean Modified:
7 years, 6 months ago CC:
blink-reviews, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, aelias_OOO_until_Jul13, Ian Vollick Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionAdd graphics layers for pinch virtual viewport.
When --enable-pinch-virtual-viewport is specified, adds both a clip layer and a
scroll layer to support the pinch virtual viewport.
BUG=none
Patch Set 1 #
Total comments: 12
Patch Set 2 : Revised as per comments #
Total comments: 6
Messages
Total messages: 12 (0 generated)
CL to add required layers for pinch virtual viewport as per design doc.
+trchen, aelias, jamesr If you're planning to apply the page scale onto m_scrollLayer directly, can you update the layer tree diagram in the shared doc to not have an explicit page scale layer? https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... Source/core/rendering/RenderLayerCompositor.cpp:2404: if (isMainFrame() && m_renderView->document()->settings()->pinchVirtualViewportEnabled()) { Can you add a outline diagram in a comment here of what this hierarchy is supposed to look like in terms of member variables and parent/child relationships for both virtual viewport enabled and disabled? https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... Source/core/rendering/RenderLayerCompositor.cpp:2422: } else Can you add braces for this else to be in line with https://codereview.chromium.org/15747011/ when it lands? https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... Source/core/rendering/RenderLayerCompositor.cpp:2762: info.addMember(m_pinchVirtualViewportClipLayer, "pinch virtual viewport clip layer"); For consistency, maybe this should be pinchVirtualClipLayer and pinchVirtualViewportScrollLayer?
I'd like us to have an explicit "page scale layer", otherwise the order that scroll and page scale should be applied on the same layer is nonobvious. Secondly, I think we should with the terminology used in the doc of "inner viewport" and "outer viewport", as that seems to be the terms that everyone finds clearest. The ones you're adding here would be "inner clip" and "inner scroll". Maybe you can rename the existing clip and scroll layers to "outer" as part of this change.
Thanks for the quick turn around ... I'll get a revised patch up first thing in the morning will all your suggestions addressed. >> enne@ - If you're planning to apply the page scale onto m_scrollLayer directly, can you update the layer tree diagram in the shared doc to not have an explicit page scale layer? >> aelias@ - I'd like us to have an explicit "page scale layer", otherwise the order that scroll and page scale should be applied on the same layer is nonobvious. I'll add the explicit page-scale layer. I missed it in the doc (it wasn't in colour like the other new layers, though that's no excuse!) >> aelias@ - Secondly, I think we should with the terminology used in the doc of "inner viewport" and "outer viewport", as that seems to be the terms that everyone finds clearest. The ones you're adding here would be "inner clip" and "inner scroll". Maybe you can rename the existing clip and scroll layers to "outer" as part of this change. Will do. https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... Source/core/rendering/RenderLayerCompositor.cpp:2404: if (isMainFrame() && m_renderView->document()->settings()->pinchVirtualViewportEnabled()) { On 2013/05/29 19:57:10, enne wrote: > Can you add a outline diagram in a comment here of what this hierarchy is > supposed to look like in terms of member variables and parent/child > relationships for both virtual viewport enabled and disabled? Will do. https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... Source/core/rendering/RenderLayerCompositor.cpp:2422: } else On 2013/05/29 19:57:10, enne wrote: > Can you add braces for this else to be in line with > https://codereview.chromium.org/15747011/ when it lands? Will do. https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... Source/core/rendering/RenderLayerCompositor.cpp:2762: info.addMember(m_pinchVirtualViewportClipLayer, "pinch virtual viewport clip layer"); On 2013/05/29 19:57:10, enne wrote: > For consistency, maybe this should be pinchVirtualClipLayer and > pinchVirtualViewportScrollLayer? I'll rename everything to be inner/out as in the document, as per aelias' suggestion.
https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... Source/core/rendering/RenderLayerCompositor.cpp:1122: FloatSize visibleContentSize = frameView->unscaledVisibleContentSize(); FloatSize unscaledVisibleContentSize = ... https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... Source/core/rendering/RenderLayerCompositor.cpp:1124: if (m_pinchVirtualViewportClipLayer) { FloatSize visibleContentSize = unscaledVisibleContentSize; visibleContentSize.scale(frameView->visibleContentScaleFactor()); https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... Source/core/rendering/RenderLayerCompositor.cpp:2421: m_pinchVirtualViewportScrollLayer->addChild(m_rootContentLayer.get()); This doesn't look right. According to enne's design doc, the inner(pinch zoom) viewport is supposed in a higher hierarchy than the outer(main frame) viewport. And I'm still not convinced why this must belong to RLC. I weakly feel pinch zoom should be a WebWidget concept, but I can be convinced otherwise. Either way, at least we should make a separate helper class for it.
PTAL https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... Source/core/rendering/RenderLayerCompositor.cpp:1122: FloatSize visibleContentSize = frameView->unscaledVisibleContentSize(); On 2013/05/29 21:32:56, trchen wrote: > FloatSize unscaledVisibleContentSize = ... Done. https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... Source/core/rendering/RenderLayerCompositor.cpp:1124: if (m_pinchVirtualViewportClipLayer) { On 2013/05/29 21:32:56, trchen wrote: > FloatSize visibleContentSize = unscaledVisibleContentSize; > visibleContentSize.scale(frameView->visibleContentScaleFactor()); Done. https://codereview.chromium.org/16136004/diff/1/Source/core/rendering/RenderL... Source/core/rendering/RenderLayerCompositor.cpp:2421: m_pinchVirtualViewportScrollLayer->addChild(m_rootContentLayer.get()); On 2013/05/29 21:32:56, trchen wrote: > This doesn't look right. According to enne's design doc, the inner(pinch zoom) > viewport is supposed in a higher hierarchy than the outer(main frame) viewport. > > And I'm still not convinced why this must belong to RLC. I weakly feel pinch > zoom should be a WebWidget concept, but I can be convinced otherwise. Either > way, at least we should make a separate helper class for it. The layer named m_pinchVirtualViewportScrollLayer here is the outer viewport, so I think this matches the doc.
https://codereview.chromium.org/16136004/diff/8001/Source/core/rendering/Rend... File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/16136004/diff/8001/Source/core/rendering/Rend... Source/core/rendering/RenderLayerCompositor.cpp:2214: // FIXME: When the pinch virtual viewport is enabled, do we really want This is wrong, but this patch doesn't make it any more wrong. Can you remove the question marks and more definitively say that this is wrong were there to be page scale? On the upside, the overhang layers are only for Mac, where we never have non-1 page scale. https://codereview.chromium.org/16136004/diff/8001/Source/core/rendering/Rend... Source/core/rendering/RenderLayerCompositor.cpp:2406: // Create a clipping layer if this is an iframe Is this comment true? https://codereview.chromium.org/16136004/diff/8001/Source/core/rendering/Rend... Source/core/rendering/RenderLayerCompositor.cpp:2421: // When using the pinch virtual viewport, the layer hierarchy looks like this: Can you add overhang layers too?
I see you're trying to reuse the existing clip/scroll layers as the inner viewport layers. I think it is conceptually wrong. There are at least 3 reasons why the existing viewport shouldn't be the inner viewport : 1. By definition the inner viewport scrollbar is the special overlay scrollbar that takes the combined scroll extent from inner and outer viewport. Is it the scrollbar you want for subframes and iframes? Suppose we want regular non-overlay scrollbar for subframes, it would be the outer scrollbar, but subframes won't have an outer viewport, and the outer scrollbar is not supposed to look at inner viewport scroll offset. 2. The fixed-position container would be inconsistent for main frame and subframes. For main frame it will be the outer viewport layer, and for subframes it will be the inner viewport layer. 3. It is error-prone to have an optional page scale layer between inner clip and inner scroll layer. The layer structure should be as monotonic as possible. The cleaner way to do it to see the existing clip/scroll layers as the outer viewport, and we add inner viewport layers (including the page scale layer) on top of it, so we have 1. correct scrollbar, 2. fixed-position container is always the outer viewport, and 3. the inner viewport layers are either all there or all gone Also nobody yet has answered me why the inner viewport stuff must belong to RLC. I still think page scale is a WebWidget concept for the following reason: 1. Page scale is only done at top level. No reason why subframe RLCs should have a number of null layer pointers that will never be used. Also we just removed the frame scale factor from frames. Doing this is equivalent to re-introduce the frame scale factor that will be always 1 for subrframes. 2. Page scale is syned during commit through WebViewImpl::applyScrollAndScale(). 3. Input event coordinate conversion are done in WebViewImpl::handle______() when casting WebInputEvent to PlatformEvent.
[jamesr: please look for the question about your preferences further down the page] > I see you're trying to reuse the existing clip/scroll layers as the inner > viewport layers. I think it is conceptually wrong. There are at least 3 reasons > why the existing viewport shouldn't be the inner viewport: > 1. By definition the inner viewport scrollbar is the special overlay scrollbar > that takes the combined scroll extent from inner and outer viewport. Is it the > scrollbar you want for subframes and iframes? Suppose we want regular > non-overlay scrollbar for subframes, it would be the outer scrollbar, but > subframes won't have an outer viewport, and the outer scrollbar is not supposed > to look at inner viewport scroll offset. I think the ultimate intent of this CL is that, for a sub-frame, it continues to operate as it does at present. Only the mainframe will have the revised tree topology, and the cc-side mechanisms regarding total-scroll versus inner/outer viewport scroll. > 2. The fixed-position container would be inconsistent for main frame and > subframes. For main frame it will be the outer viewport layer, and for subframes > it will be the inner viewport layer. Interesting. Not something we really considered in the design doc, but a good point. I would have thought that fixed position elements would find their nearest-enclosing fixed-position container in either case, and not care about the labels of inner/outer, though I cannot say I've investigated that to know if I'm right. Anyone else have thoughts on this aspect? > 3. It is error-prone to have an optional page scale layer between inner clip and > inner scroll layer. The layer structure should be as monotonic as possible. But the desired movement of the overlay scrollbars to be children of the inner clip layer sort of necessitates this, since the page scale layer *does* need to be (at or) above the inner scroll layer to match current behaviour. What sort of errors do you envision? We can experiment with placing it just above the outer clip layer [aelias - do you forsee any scrolling related problems with that?] > The cleaner way to do it to see the existing clip/scroll layers as the outer > viewport, and we add inner viewport layers (including the page scale layer) on > top of it, so we have 1. correct scrollbar, 2. fixed-position container is > always the outer viewport, and 3. the inner viewport layers are either all there > or all gone I'm not opposed to this in principal. > Also nobody yet has answered me why the inner viewport stuff must belong to RLC. As I'll say below, it's not *absolutely* the case it has to be in either RLC or in WVI. I think there's an argument to be made for keeping the tree structuring logic all in one place, so to me RLC looks appealing. Certainly pre-Blink we had to put some structuring logic in WVI (NonCompositedContentHost creation, for example), but I'm not sure that's as much of a problem anymore. > I still think page scale is a WebWidget concept for the following reason: > 1. Page scale is only done at top level. No reason why subframe RLCs should have > a number of null layer pointers that will never be used. Also we just removed > the frame scale factor from frames. Doing this is equivalent to re-introduce the > frame scale factor that will be always 1 for subrframes. Since the number of RLCs in any given tree will be small, I'm not sure the small number of unused pointers is a truly compelling argument (e.g. look at LayerImpl for scrollbar layer pointers). As for the frame scale factor, I was unaware of this as an issue, and will think about this some more. > 2. Page scale is syned during commit through WebViewImpl::applyScrollAndScale(). OK. > 3. Input event coordinate conversion are done in WebViewImpl::handle______() > when casting WebInputEvent to PlatformEvent. Ultimately, I don't see either of the reasons for/against the implementation in RLC as being absolute, so it does come down to a balance. How about this? I promise to take a serious look at what this would look like if done at the WVI level, and if that seems better than the RLC implementation I'm happy to go that route. JamesR: did you have any preferences for which way this goes? https://codereview.chromium.org/16136004/diff/8001/Source/core/rendering/Rend... File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/16136004/diff/8001/Source/core/rendering/Rend... Source/core/rendering/RenderLayerCompositor.cpp:2214: // FIXME: When the pinch virtual viewport is enabled, do we really want On 2013/05/30 21:03:31, enne wrote: > This is wrong, but this patch doesn't make it any more wrong. Can you remove > the question marks and more definitively say that this is wrong were there to be > page scale? Yes. Even better, it can be immortalized with a DCHECK to warn if the condition is ever violated. > On the upside, the overhang layers are only for Mac, where we never have non-1 > page scale. SGTM! https://codereview.chromium.org/16136004/diff/8001/Source/core/rendering/Rend... Source/core/rendering/RenderLayerCompositor.cpp:2406: // Create a clipping layer if this is an iframe On 2013/05/30 21:03:31, enne wrote: > Is this comment true? Hmm, not sure. The line below got changed in a search-and-replace operation, so I never really looked at it. I'll give this some thought though ... https://codereview.chromium.org/16136004/diff/8001/Source/core/rendering/Rend... Source/core/rendering/RenderLayerCompositor.cpp:2421: // When using the pinch virtual viewport, the layer hierarchy looks like this: On 2013/05/30 21:03:31, enne wrote: > Can you add overhang layers too? Will do.
On 2013/05/31 18:21:10, wjmaclean wrote: > How about this? I promise to take a serious look at what this would look like if > done at the WVI level, and if that seems better than the RLC implementation I'm > happy to go that route. > > JamesR: did you have any preferences for which way this goes? I think keeping the inner/outer viewport distinction out of RLC is better. core/ shouldn't have to know or care about this concept at all.
On 2013/05/31 18:26:15, jamesr wrote: > On 2013/05/31 18:21:10, wjmaclean wrote: > > How about this? I promise to take a serious look at what this would look like > if > > done at the WVI level, and if that seems better than the RLC implementation > I'm > > happy to go that route. > > > > JamesR: did you have any preferences for which way this goes? > > I think keeping the inner/outer viewport distinction out of RLC is better. > core/ shouldn't have to know or care about this concept at all. OK, I'll re-write to keep this at the WVI level.
The new cl is up at https://codereview.chromium.org/16799005/ (and https://codereview.chromium.org/16679011). I will close this codereview. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
