Summary: | [chromium] Make WebKit API support draggable region change update | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jian Li <jianli> | ||||||
Component: | WebKit Misc. | Assignee: | Jian Li <jianli> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, dglazkov, dimich, fishd, jamesr, tkent+wkapi, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Jian Li
2012-07-31 16:41:33 PDT
Created attachment 155681 [details]
Proposed Patch
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. Attachment 155681 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebKit/chromium/src/WebDocument.cpp:248: draggable_regions is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 1 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 155685 [details]
Proposed Patch
Comment on attachment 155685 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155685&action=review > Source/Platform/chromium/public/WebDraggableRegion.h:40 > +struct WebDraggableRegion { This looks like a wrapper for DashboardRegionValue, which is a WebCore concept, not a WebCore/platform concept. Therefore, this header should be in WebKit/chromium/public rather than in Platform. > Source/Platform/chromium/public/WebDraggableRegion.h:44 > + int type; Is this really an enum? Should we create an API for the enum values so we can COMPILE_ASSERT that they match their WebCore counterparts? > Source/WebKit/chromium/public/WebViewClient.h:384 > + // Draggable regions ---------------------------------------------------- Two blank lines before section headings. (In reply to comment #5) > (From update of attachment 155685 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155685&action=review > > > Source/Platform/chromium/public/WebDraggableRegion.h:40 > > +struct WebDraggableRegion { > > This looks like a wrapper for DashboardRegionValue, which is a WebCore concept, not a WebCore/platform concept. Therefore, this header should be in WebKit/chromium/public rather than in Platform. Will fix it at landing time. > > > Source/Platform/chromium/public/WebDraggableRegion.h:44 > > + int type; > > Is this really an enum? Should we create an API for the enum values so we can COMPILE_ASSERT that they match their WebCore counterparts? WebCore counterparts also use int type. For now, I am going to keep int type for our version. I am thinking of ignoring the type information (rectangular or circular). We will extract the shape information from the element when CSS shape is fully supported and then update this struct. > > > Source/WebKit/chromium/public/WebViewClient.h:384 > > + // Draggable regions ---------------------------------------------------- > > Two blank lines before section headings. Will fix it at landing time. > WebCore counterparts also use int type. For now, I am going to keep int type for our version. I am thinking of ignoring the type information (rectangular or circular). We will extract the shape information from the element when CSS shape is fully supported and then update this struct.
Just a bare int doesn't make any sense in the API. Perhaps we should omit it for now and add it later when we need it?
(In reply to comment #7) > > WebCore counterparts also use int type. For now, I am going to keep int type for our version. I am thinking of ignoring the type information (rectangular or circular). We will extract the shape information from the element when CSS shape is fully supported and then update this struct. > > Just a bare int doesn't make any sense in the API. Perhaps we should omit it for now and add it later when we need it? Sounds good. Committed as http://trac.webkit.org/changeset/124394. |