Bug 97156 - Update the CSS property used to support draggable regions
Summary: Update the CSS property used to support draggable regions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jian Li
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-19 18:09 PDT by Jian Li
Modified: 2012-10-09 17:07 PDT (History)
15 users (show)

See Also:


Attachments
Proposed Patch (63.58 KB, patch)
2012-09-19 18:19 PDT, Jian Li
abarth: review-
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (77.47 KB, patch)
2012-09-26 14:15 PDT, Jian Li
abarth: review+
jianli: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2012-09-19 18:09:58 PDT
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.
Comment 1 Jian Li 2012-09-19 18:19:40 PDT
Created attachment 164815 [details]
Proposed Patch
Comment 2 WebKit Review Bot 2012-09-19 18:24:54 PDT
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 3 Adam Barth 2012-09-19 18:39:01 PDT
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.
Comment 4 Jian Li 2012-09-19 18:55:03 PDT
(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.
Comment 5 Adam Barth 2012-09-20 10:36:42 PDT
> 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.
Comment 6 Jian Li 2012-09-25 15:10:33 PDT
(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.
Comment 7 Simon Fraser (smfr) 2012-09-25 15:14:50 PDT
Do you intent to submit the "app-region" CSS property for standardization?
Comment 8 Jian Li 2012-09-25 15:31:53 PDT
(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.
Comment 9 Jian Li 2012-09-26 14:15:40 PDT
Created attachment 165875 [details]
Proposed Patch

Changed as suggested in order to shared rendering update code for both features.
Comment 10 Adam Barth 2012-09-27 09:33:27 PDT
Comment on attachment 165875 [details]
Proposed Patch

Thanks, this looks much better.
Comment 11 Jian Li 2012-10-09 17:07:33 PDT
Committed as http://trac.webkit.org/changeset/130829.