Bug 86670 - [chromium] plumb the frame for which a drag was initiated to the WebViewClient
Summary: [chromium] plumb the frame for which a drag was initiated to the WebViewClient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-16 13:32 PDT by jochen
Modified: 2012-05-18 01:11 PDT (History)
7 users (show)

See Also:

Attachments
Patch (4.37 KB, patch)
2012-05-16 13:33 PDT, jochen 		no flags Details | Formatted Diff | Diff
Patch (6.96 KB, patch)
2012-05-16 13:47 PDT, jochen 		no flags Details | Formatted Diff | Diff
Patch (6.96 KB, patch)
2012-05-18 00:18 PDT, jochen 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2012-05-16 13:32:44 PDT 
[chromium] plumb the frame for which a drag was initiated to the WebViewClient
Comment 1 jochen 2012-05-16 13:33:02 PDT 
Created attachment 142334 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-16 13:35:15 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 jochen 2012-05-16 13:37:03 PDT 
I intend to use the Frame to get its document's referrer policy, so it can be used for when the drag results in a download
Comment 4 jochen 2012-05-16 13:47:26 PDT 
Created attachment 142337 [details]
Patch
Comment 5 James Robinson 2012-05-16 14:44:30 PDT 
Comment on attachment 142337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142337&action=review

Left some comments on how to stage this, but I don't have any insight about the change itself.

> Source/WebKit/chromium/public/WebViewClient.h:254
> +    // FIXME: Remove once the WebKit side has landed.

I'm confused by this comment - this patch is the WebKit side, isn't it?  Did you mean chromium side?

One way to do this without waiting for rolls is to guard both sides of the change in an #ifdef like WEBVIEWCLIENT_STARTDRAGGING_HAS_FRAME and #define that in this header, then remove the #ifdefs/#defines after everything lands and rolls.
Comment 6 jochen 2012-05-18 00:18:26 PDT 
Created attachment 142651 [details]
Patch
Comment 7 jochen 2012-05-18 00:21:05 PDT 
(In reply to comment #5)
> (From update of attachment 142337 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142337&action=review
> 
> Left some comments on how to stage this, but I don't have any insight about the change itself.
> 
> > Source/WebKit/chromium/public/WebViewClient.h:254
> > +    // FIXME: Remove once the WebKit side has landed.
> 
> I'm confused by this comment - this patch is the WebKit side, isn't it?  Did you mean chromium side?

Yes

> One way to do this without waiting for rolls is to guard both sides of the change in an #ifdef like WEBVIEWCLIENT_STARTDRAGGING_HAS_FRAME and #define that in this header, then remove the #ifdefs/#defines after everything lands and rolls.

Your approach requires 4 patches, mine only three (or 6 vs 5 if you include rolls)

The advantage of your approach is that you can do breaking changes, e.g. introduce a new enum value. But for just changing a signature or similar, my approach seems to be the more common one
Comment 8 Kent Tamura 2012-05-18 00:33:23 PDT 
Comment on attachment 142651 [details]
Patch

looks good.
Comment 9 WebKit Review Bot 2012-05-18 01:11:19 PDT 
Comment on attachment 142651 [details]
Patch

Clearing flags on attachment: 142651

Committed r117560: <http://trac.webkit.org/changeset/117560>
Comment 10 WebKit Review Bot 2012-05-18 01:11:24 PDT 
All reviewed patches have been landed.  Closing bug.