[Webkit-unassigned] [Bug 111670] Support bottom-right anchored fixed-position elements during a pinch gesture

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=111670





--- Comment #25 from Tien-Ren Chen <trchen at chromium.org>  2013-03-28 13:31:48 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.