[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 #24 from James Robinson <jamesr at chromium.org>  2013-03-28 12:58:57 PST ---
(From update of attachment 193683)
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?

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