Summary: | Support bottom-right anchored fixed-position elements during a pinch gesture | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tien-Ren Chen <trchen> | ||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, aelias, andersca, buildbot, cmarcelo, dglazkov, enne, eric, esprehn+autocc, fishd, jamesr, luiz, ojan.autocc, shawnsingh, simon.fraser, skyostil, tkent+wkapi, tonikitoo, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Tien-Ren Chen
2013-03-06 19:31:25 PST
Created attachment 191899 [details]
Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. Comment on attachment 191899 [details] Patch Attachment 191899 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17000343 Created attachment 191901 [details]
Patch
Comment on attachment 191901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191901&action=review > Source/Platform/chromium/public/WebLayer.h:199 > + virtual void setFixedToRightEdge(bool) { } I wonder if we should have something more like the bitfields in the ViewportConstraints instead of this? That'll also cover cases where a layer is fixed to both edges on the same axis, in which case we may want to do something special in the compositor. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1921 > +RenderObject* RenderLayerCompositor::requiresFixedToContainerByRenderObject(const RenderLayer* layer) const This function name is kinda hard to parse. Maybe fixedToContainerByRenderObject? The description doesn't seem to match the implementation since we can also return the layer itself (so the RenderObject won't be between the layer and its nearest compositor ancestor). (In reply to comment #5) > (From update of attachment 191901 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191901&action=review > > > Source/Platform/chromium/public/WebLayer.h:199 > > + virtual void setFixedToRightEdge(bool) { } > > I wonder if we should have something more like the bitfields in the ViewportConstraints instead of this? That'll also cover cases where a layer is fixed to both edges on the same axis, in which case we may want to do something special in the compositor. I think you're right. I was too lazy to write a WebViewportConstraint. Will do. > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1921 > > +RenderObject* RenderLayerCompositor::requiresFixedToContainerByRenderObject(const RenderLayer* layer) const > > This function name is kinda hard to parse. Maybe fixedToContainerByRenderObject? The description doesn't seem to match the implementation since we can also return the layer itself (so the RenderObject won't be between the layer and its nearest compositor ancestor). Yep... The range is actually [this, compositing ancestor) The semantic is to return false if the layer doesn't need fixed position compensation, and true if it does, and if true also returns which RenderObject caused it. I'll change the function name and refine the comments. Comment on attachment 191901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191901&action=review Looks like you have a few refinements you plan to do with this patch. >>> Source/Platform/chromium/public/WebLayer.h:199 >>> + virtual void setFixedToRightEdge(bool) { } >> >> I wonder if we should have something more like the bitfields in the ViewportConstraints instead of this? That'll also cover cases where a layer is fixed to both edges on the same axis, in which case we may want to do something special in the compositor. > > I think you're right. I was too lazy to write a WebViewportConstraint. Will do. Still waiting for this. Also, do we need right-anchored right now? I think the immediate need is just for bottom-anchored, isn't it? We can add right-anchored later. Right-anchored is also solving a problem in that it will avoid jumping around on pinch zoom for those elements. Given that the logic is identical, why not do both? Ah, ok. That's fine. Created attachment 193395 [details]
Patch
Changes from last patch: * Moved RenderLayerCompositor::fixedPositionedByAncestor to ScrollingCoordinatorChromium. It is a chromium-specific concept that only makes sense to our implementation. * Added WebLayerPositionConstraint, deprecate fixedToContainerLayer flag. The structure can be extended to support sticky elements at a later time. Why not reuse WebCore::ViewportConstraints? Because our approach to fixed position layer is significantly different so there are too many parameters that are useless to us. Chromium patch coming soon. Created attachment 193420 [details]
Patch
Comment on attachment 193420 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193420&action=review > Source/WebCore/ChangeLog:11 > + No new tests. Can't test until chromium patch landed. Presumably the basic fixed position layer functionality is covered by existing layout tests, right? We just don't have tests for bottom/right fixed things yet. > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:239 > +static void clearPositionConstraintExcept(GraphicsLayer* layer, GraphicsLayer* except) Bikeshed: clearPositionConstraintExceptForLayer()? > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:267 > + // Avoid unnecessary commits I'm not sure I get this. What does clearing the position constraint have to do with commits? (In reply to comment #13) > (From update of attachment 193420 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193420&action=review > > > Source/WebCore/ChangeLog:11 > > + No new tests. Can't test until chromium patch landed. > > Presumably the basic fixed position layer functionality is covered by existing layout tests, right? We just don't have tests for bottom/right fixed things yet. Yes exactly. And I have adjusted the stubs so we migrate to the new API without breaking existing tests. > > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:239 > > +static void clearPositionConstraintExcept(GraphicsLayer* layer, GraphicsLayer* except) > > Bikeshed: clearPositionConstraintExceptForLayer()? SGTM. Will do. > > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:267 > > + // Avoid unnecessary commits > > I'm not sure I get this. What does clearing the position constraint have to do with commits? Modification to position constraints will need a commit, and the call might be made on layers all the time, even if the layer didn't change its positioning. Consider the situation that a layer is fixed-positioned, and a call to update position constraint has been made. If we just blindly clear all position constraint and set it again on the main layer, a commit will be triggered. That is why we need the ExceptForLayer thing, so we apply the updated constraint on the main layer in one shot, and an early exit can be made. I don't understand the bug title. You're not implementing pinch-zoom, you're altering the behavior when zooming, right? Created attachment 193683 [details]
Patch
Changes: * Renamed bug title * Renamed the function skyostil@ mentioned Comment on attachment 193683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193683&action=review > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:241 > + if (layer && layer != except && scrollingWebLayerForGraphicsLayer(layer)) I think "scrollingWebLayerForGraphicsLayer" is a pretty misleading name (also true before your patch) because the returned layer may or may not have anything to do with scrolling -- it's just the platformLayer() we're interested in here and below. I don't think you should necessarily fix it in your patch, though. > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:267 > + // Avoid unnecessary commits Thanks for explaining this. I'm on the fence about whether a simple if (backing->ancestorClippingLayer() != mainLayer) { ... } would be clearer but either way is fine by me. (In reply to comment #18) > (From update of attachment 193683 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193683&action=review > > > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:241 > > + if (layer && layer != except && scrollingWebLayerForGraphicsLayer(layer)) > > I think "scrollingWebLayerForGraphicsLayer" is a pretty misleading name (also true before your patch) because the returned layer may or may not have anything to do with scrolling -- it's just the platformLayer() we're interested in here and below. I don't think you should necessarily fix it in your patch, though. Can't agree with you more! I'm always wondering why we need a function for such simple access. I suggest we just replace it with ->platformLayer(), if everybody agree. (In reply to comment #19) > Can't agree with you more! I'm always wondering why we need a function for such simple access. I suggest we just replace it with ->platformLayer(), if everybody agree. Sounds good to me. Hello reviewers, do you want me to revise anything before starting to review this patch? Thanks! Reminder: this patch needs to be landed before the chromium counterpart. Please review this patch first when you got the chance to review the chromium patch: https://codereview.chromium.org/12552004/ I will add the WebKit-side tests when the chromium patch gets landed. Thanks! I think this patch looks good to go. Comment on attachment 193683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193683&action=review D'oh, I had comments I forgot to publish :( > Source/Platform/chromium/public/WebLayer.h:34 > +// Remove after chromium patch landed > +#include "WebLayerPositionConstraint.h" Not sure I understand this constraint - what chromium patch needs to land to remove the need for this include? > Source/Platform/chromium/public/WebLayer.h:199 > + // Deprecated, remove after chromium patch landed > + virtual void setFixedToContainerLayer(bool) { } > + virtual bool fixedToContainerLayer() const { return positionConstraint().isFixedPosition; } Our typical convention for this is to move the deprecated things to the bottom of the file under a big comment saying DEPRECATED. Having it inline like this makes it harder to see > Source/Platform/chromium/public/WebLayer.h:200 > + // Use this instead, remove stub after chromium patch landed Please document what these functions actually do first, then add another comment on its own line saying something like // FIXME: Make pure virtual after implementation lands. > Source/Platform/chromium/public/WebLayer.h:202 > + virtual WebLayerPositionConstraint positionConstraint() const { return WebLayerPositionConstraint(); } Why do you need the getter? (In reply to comment #24) > (From update of attachment 193683 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193683&action=review > > D'oh, I had comments I forgot to publish :( > > > Source/Platform/chromium/public/WebLayer.h:34 > > +// Remove after chromium patch landed > > +#include "WebLayerPositionConstraint.h" > > Not sure I understand this constraint - what chromium patch needs to land to remove the need for this include? The temporary stub below needs WebLayerPositionConstraint prototype. Let me change the comment to "// Remove after making setPositionConstraint() pure virtual." > > Source/Platform/chromium/public/WebLayer.h:199 > > + // Deprecated, remove after chromium patch landed > > + virtual void setFixedToContainerLayer(bool) { } > > + virtual bool fixedToContainerLayer() const { return positionConstraint().isFixedPosition; } > > Our typical convention for this is to move the deprecated things to the bottom of the file under a big comment saying DEPRECATED. Having it inline like this makes it harder to see Got it. > > Source/Platform/chromium/public/WebLayer.h:200 > > + // Use this instead, remove stub after chromium patch landed > > Please document what these functions actually do first, then add another comment on its own line saying something like // FIXME: Make pure virtual after implementation lands. Got it. > > Source/Platform/chromium/public/WebLayer.h:202 > > + virtual WebLayerPositionConstraint positionConstraint() const { return WebLayerPositionConstraint(); } > > Why do you need the getter? For the unit tests. The migration plan as following: As we land the Chromium counterpart, WebLayerImpl::fixedToContainerLayer() will be removed, so it turns to fallback implementation WebLayerImpl::fixedToContainerLayer(), which will query positionConstraint() as now Chromium-side implements the new methods. At last, we'll land another patch in WebKit that completely removes fixedToContainerLayer() and update tests. Created attachment 195635 [details]
Patch
Comment on attachment 195635 [details]
Patch
R=me
Comment on attachment 195635 [details] Patch Clearing flags on attachment: 195635 Committed r147163: <http://trac.webkit.org/changeset/147163> All reviewed patches have been landed. Closing bug. |