[webkit-reviews] review denied: [Bug 97156] Update the CSS property used to support draggable regions : [Attachment 164815] Proposed Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 97156: Update the CSS property used to support draggable regions
https://bugs.webkit.org/show_bug.cgi?id=97156

Attachment 164815: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=164815&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=164815&action=review


I think you went a bit too far in make the implementations separate.  It seems
fine to make the CSS part separate as that part is different between the two,
but the stuff in Document looks really silly duplicated.

> Source/WebCore/dom/Document.cpp:466
> -#if ENABLE(DASHBOARD_SUPPORT) || ENABLE(WIDGET_REGION)
> +#if ENABLE(DASHBOARD_SUPPORT)
>      , m_hasDashboardRegions(false)
>      , m_dashboardRegionsDirty(false)
>  #endif
> +#if ENABLE(WIDGET_REGION)
> +    , m_hasDraggableRegions(false)
> +    , m_draggableRegionsDirty(false)
> +#endif

It doesn't make senes to continue sharing this part?

> Source/WebCore/dom/Document.h:1060
> +#if ENABLE(WIDGET_REGION)
> +    void setDraggableRegionsDirty(bool f) { m_draggableRegionsDirty = f; }
> +    bool draggableRegionsDirty() const { return m_draggableRegionsDirty; }
> +    bool hasDraggableRegions () const { return m_hasDraggableRegions; }
> +    void setHasDraggableRegions(bool f) { m_hasDraggableRegions = f; }
> +    const Vector<DraggableRegionValue>& draggableRegions() const;
> +    void setDraggableRegions(const Vector<DraggableRegionValue>&);
> +#endif

It seems silly to duplicate all of this.