Bug 97156 – Update the CSS property used to support draggable regions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jian Li]

We want to change the CSS property from "-webkit-widget-region" to "-webkit-app-region" with the following syntax:
  -webkit-app-region: drag|no-drag

The reason for the renaming is that widget seems to be too vague and we also want to indicate that it is used for web app.

In addition, we want to simplify the syntax as above. We will get the bounding information from the element.

[Jian Li]

Created attachment 164815 [details]
Proposed Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 164815 [details]
Proposed Patch

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.

[Jian Li]

(In reply to comment #3)
> (From update of attachment 164815 [details])
> 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.

Should we still keep calling these methods as ***dashboardRegions*** in Document for both dashboard region and draggable region? DraggableRegionValue is different from DashboardRegionValue since DraggableRegionValue doesn't need label and clip as in DashboardRegionValue while it adds its own draggable field. In the future, we might add other fields to DraggableRegionValue to support complex shape and z-index if needed.

[Adam Barth]

> Should we still keep calling these methods as ***dashboardRegions*** in Document for both dashboard region and draggable region?

I'd probably name them something that makes sense for both.  DraggableRegion seems like a reasonable choice.

> DraggableRegionValue is different from DashboardRegionValue since DraggableRegionValue doesn't need label and clip as in DashboardRegionValue while it adds its own draggable field. In the future, we might add other fields to DraggableRegionValue to support complex shape and z-index if needed.

That fine.  We don't need to be able to compile in support for both of them yet.  We can just have DraggableRegionValue have different fields depending on which compile flag you've set.

[Jian Li]

(In reply to comment #5)
> > Should we still keep calling these methods as ***dashboardRegions*** in Document for both dashboard region and draggable region?
> 
> I'd probably name them something that makes sense for both.  DraggableRegion seems like a reasonable choice.
> 
> > DraggableRegionValue is different from DashboardRegionValue since DraggableRegionValue doesn't need label and clip as in DashboardRegionValue while it adds its own draggable field. In the future, we might add other fields to DraggableRegionValue to support complex shape and z-index if needed.
> 
> That fine.  We don't need to be able to compile in support for both of them yet.  We can just have DraggableRegionValue have different fields depending on which compile flag you've set.

(In reply to comment #5)
> > Should we still keep calling these methods as ***dashboardRegions*** in Document for both dashboard region and draggable region?
> 
> I'd probably name them something that makes sense for both.  DraggableRegion seems like a reasonable choice.

If we want to share the codes for both features, how about something that is different from both DashboardRegion and DraggableRegion such that we will not confuse these two regions, like AnnonatedRegion or TaggedRegion?

> 
> > DraggableRegionValue is different from DashboardRegionValue since DraggableRegionValue doesn't need label and clip as in DashboardRegionValue while it adds its own draggable field. In the future, we might add other fields to DraggableRegionValue to support complex shape and z-index if needed.
> 
> That fine.  We don't need to be able to compile in support for both of them yet.  We can just have DraggableRegionValue have different fields depending on which compile flag you've set.

We do have both feature guards turned on for Mac WebKit such that we can run tests for both features, though Safari or Chromium would turn on just one.

[Simon Fraser (smfr)]

Do you intent to submit the "app-region" CSS property for standardization?

[Jian Li]

(In reply to comment #7)
> Do you intent to submit the "app-region" CSS property for standardization?

Yes, we have plan to do so. Thanks.

[Jian Li]

Created attachment 165875 [details]
Proposed Patch

Changed as suggested in order to shared rendering update code for both features.

[Adam Barth]

Comment on attachment 165875 [details]
Proposed Patch

Thanks, this looks much better.

[Jian Li]

Committed as http://trac.webkit.org/changeset/130829.