Bug 73594 - [chromium] Add plumbing for supporting custom MIME types in DataTransfer.
Summary: [chromium] Add plumbing for supporting custom MIME types in DataTransfer.
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: Daniel Cheng
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-01 15:09 PST by Daniel Cheng
Modified: 2011-12-02 10:53 PST (History)
3 users (show)

See Also:


Attachments
Patch (21.15 KB, patch)
2011-12-01 15:14 PST, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (21.19 KB, patch)
2011-12-01 15:55 PST, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (21.24 KB, patch)
2011-12-01 16:02 PST, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (21.22 KB, patch)
2011-12-01 22:44 PST, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (21.23 KB, patch)
2011-12-01 23:56 PST, Daniel Cheng
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Cheng 2011-12-01 15:09:46 PST
[chromium] Add plumbing for supporting custom MIME types in DataTransfer.
Comment 1 Daniel Cheng 2011-12-01 15:14:14 PST
Created attachment 117500 [details]
Patch
Comment 2 WebKit Review Bot 2011-12-01 15:16:29 PST
Attachment 117500 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

ERROR: FAILURES FOR <lucid, x86_64, release, cpu> in LayoutTests/platform/chromium/test_expectations.txt
ERROR: Line:1953 Duplicate or ambiguous expectation. fast/url/file-http-base.html
LayoutTests/platform/chromium/test_expectations.txt:1953:  Duplicate or ambiguous expectation. fast/url/file-http-base.html  [test/expectations] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 WebKit Review Bot 2011-12-01 15:16:47 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 Tony Chang 2011-12-01 15:45:15 PST
Comment on attachment 117500 [details]
Patch

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

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:187
> +
>      return String();

I think we still need to set success = false; here.

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:237
> +    m_customData.set(type, data);
> +    return true;

I see, so setData will always succeed now.

> LayoutTests/platform/chromium/test_expectations.txt:3738
> +// Depends on Chromium follow up change before they will pass.
> +BUG_DCHENG : fast/events/drag-customData.html = FAIL

Maybe just reference BUGCR31037 here?
Comment 5 Tony Chang 2011-12-01 15:45:59 PST
Comment on attachment 117500 [details]
Patch

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

>> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:237
>> +    return true;
> 
> I see, so setData will always succeed now.

Actually, should you verify that type is not an empty string?
Comment 6 Daniel Cheng 2011-12-01 15:55:54 PST
Created attachment 117507 [details]
Patch
Comment 7 WebKit Review Bot 2011-12-01 15:59:33 PST
Attachment 117507 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

ERROR: FAILURES FOR <lucid, x86_64, release, cpu> in LayoutTests/platform/chromium/test_expectations.txt
ERROR: Line:1953 Duplicate or ambiguous expectation. fast/url/file-http-base.html
LayoutTests/platform/chromium/test_expectations.txt:1953:  Duplicate or ambiguous expectation. fast/url/file-http-base.html  [test/expectations] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Daniel Cheng 2011-12-01 16:02:48 PST
Created attachment 117509 [details]
Patch
Comment 9 Daniel Cheng 2011-12-01 16:03:29 PST
(In reply to comment #5)
> (From update of attachment 117500 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117500&action=review
> 
> >> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:237
> >> +    return true;
> > 
> > I see, so setData will always succeed now.
> 
> Actually, should you verify that type is not an empty string?

Done.

(In reply to comment #4)
> (From update of attachment 117500 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117500&action=review
> 
> > Source/WebCore/platform/chromium/ChromiumDataObject.cpp:187
> > +
> >      return String();
> 
> I think we still need to set success = false; here.
> 

Done.

> > Source/WebCore/platform/chromium/ChromiumDataObject.cpp:237
> > +    m_customData.set(type, data);
> > +    return true;
> 
> I see, so setData will always succeed now.
> 
> > LayoutTests/platform/chromium/test_expectations.txt:3738
> > +// Depends on Chromium follow up change before they will pass.
> > +BUG_DCHENG : fast/events/drag-customData.html = FAIL
> 
> Maybe just reference BUGCR31037 here?

Done.
Comment 10 WebKit Review Bot 2011-12-01 16:05:54 PST
Attachment 117509 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

ERROR: FAILURES FOR <lucid, x86_64, release, cpu> in LayoutTests/platform/chromium/test_expectations.txt
ERROR: Line:1953 Duplicate or ambiguous expectation. fast/url/file-http-base.html
LayoutTests/platform/chromium/test_expectations.txt:1953:  Duplicate or ambiguous expectation. fast/url/file-http-base.html  [test/expectations] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Tony Chang 2011-12-01 16:17:23 PST
Comment on attachment 117509 [details]
Patch

LGTM, but please give fishd a chance to approve the API addition.
Comment 12 WebKit Review Bot 2011-12-01 17:37:18 PST
Comment on attachment 117509 [details]
Patch

Attachment 117509 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10693830
Comment 13 Darin Fisher (:fishd, Google) 2011-12-01 22:33:27 PST
Comment on attachment 117509 [details]
Patch

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

> Source/WebKit/chromium/public/platform/WebClipboard.h:82
> +        Buffer buffer, const WebString& type) { return WebString(); }

nit: Buffer parameter should be unnamed

> Source/WebKit/chromium/public/platform/WebDragData.h:102
> +        WebString m_type;

nit: public members should not have the m_ prefix.
Comment 14 Daniel Cheng 2011-12-01 22:44:38 PST
Created attachment 117572 [details]
Patch
Comment 15 Daniel Cheng 2011-12-01 22:44:53 PST
(In reply to comment #13)
> (From update of attachment 117509 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117509&action=review
> 
> > Source/WebKit/chromium/public/platform/WebClipboard.h:82
> > +        Buffer buffer, const WebString& type) { return WebString(); }
> 
> nit: Buffer parameter should be unnamed
> 

Done.


> > Source/WebKit/chromium/public/platform/WebDragData.h:102
> > +        WebString m_type;
> 
> nit: public members should not have the m_ prefix.

Done.
Comment 16 WebKit Review Bot 2011-12-01 23:46:26 PST
Comment on attachment 117572 [details]
Patch

Attachment 117572 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10725258

New failing tests:
editing/pasteboard/clipboard-customData.html
Comment 17 Daniel Cheng 2011-12-01 23:56:38 PST
Created attachment 117580 [details]
Patch
Comment 18 David Levin 2011-12-02 10:46:29 PST
Comment on attachment 117580 [details]
Patch

I'll do the final honors. It looks like you have a looks good on the code and the public api.
Comment 19 Daniel Cheng 2011-12-02 10:53:03 PST
Committed r101828: <http://trac.webkit.org/changeset/101828>