Bug 56330 - [chromium] Implement glue between DataTransferItems and the pasteboard.
Summary: [chromium] Implement glue between DataTransferItems and the pasteboard.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Daniel Cheng
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-14 13:49 PDT by Daniel Cheng
Modified: 2011-03-21 17:32 PDT (History)
4 users (show)

See Also:


Attachments
Patch (9.40 KB, patch)
2011-03-14 13:49 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (19.16 KB, patch)
2011-03-18 19:42 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (18.32 KB, patch)
2011-03-21 11:59 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (18.32 KB, patch)
2011-03-21 16:40 PDT, Daniel Cheng
tony: 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-03-14 13:49:01 PDT
[chromium] Implement glue between DataTransferItems and the pasteboard.
Comment 1 Daniel Cheng 2011-03-14 13:49:27 PDT
Created attachment 85715 [details]
Patch
Comment 2 WebKit Review Bot 2011-03-14 13:51:31 PDT
Attachment 85715 [details] did not pass style-queue:

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

Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:48:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:116:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Adam Barth 2011-03-15 02:20:21 PDT
Comment on attachment 85715 [details]
Patch

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

> Source/WebCore/ChangeLog:5
> +        Need a short description and bug URL (OOPS!)

This needs to be filled out.

> Source/WebCore/ChangeLog:7
> +        No new tests. (OOPS!)

We need tests for this change!

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:47
>  PassRefPtr<DataTransferItemChromium> DataTransferItemChromium::create(PassRefPtr<Clipboard> owner,
>                                                                        ScriptExecutionContext* context,
> +                                                                      FromPasteboardType,
> +                                                                      const String& type) {

This should all be on one line.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:49
> +    if (type == mimeTypeTextPlain ||
> +        type == mimeTypeTextHTML) {

We'd usually just put these on one line.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:51
> +    }

We don't need these braces.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:108
> +    if (m_type == mimeTypeTextPlain) {
> +        callback->scheduleCallback(m_context, PlatformBridge::clipboardReadPlainText(PasteboardPrivate::StandardBuffer));
> +    } else if (m_type == mimeTypeTextHTML) {

No braces for one-line if bodies.  Also, prefer early return.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:120
> +    if ((m_owner->policy() != ClipboardReadable && m_owner->policy() != ClipboardWritable)
> +        || m_kind != kindFile)
> +        return 0;
> +    return 0;

Why are we returning zero in both cases?  That seems wrong...

> Source/WebKit/chromium/ChangeLog:5
> +        Need a short description and bug URL (OOPS!)

This needs to be filled out.
Comment 4 Daniel Cheng 2011-03-18 19:42:49 PDT
Created attachment 86254 [details]
Patch
Comment 5 Tony Chang 2011-03-21 10:34:26 PDT
Comment on attachment 86254 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Support retrieving clipboard data in a paste through DataTransferItems.
> +

There should be a "Test: editing/pasteboard/data-transfer-items.html" line there (normally prepare-ChangeLog does that for you, but you're probably just editing the previous message).

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:112
>  }

Should there be a NOTREACHED here?

> Source/WebCore/platform/chromium/DataTransferItemChromium.h:49
> +    enum FromPasteboardType {
> +        FromPasteboard,
> +    };

I'm confused as to what this is for.

> Source/WebCore/platform/chromium/PlatformBridge.h:102
> +    static PassRefPtr<SharedBuffer> clipboardReadImage(PasteboardPrivate::ClipboardBuffer);

Where is this implemented?
Comment 6 Daniel Cheng 2011-03-21 11:59:29 PDT
Created attachment 86347 [details]
Patch
Comment 7 Daniel Cheng 2011-03-21 12:00:49 PDT
(In reply to comment #5)
> (From update of attachment 86254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86254&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Support retrieving clipboard data in a paste through DataTransferItems.
> > +
> 
> There should be a "Test: editing/pasteboard/data-transfer-items.html" line there (normally prepare-ChangeLog does that for you, but you're probably just editing the previous message).

Done. For some reason, it's not emitting that line. I added it manually.

> 
> > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:112
> >  }
> 
> Should there be a NOTREACHED here?
> 

Done.

> > Source/WebCore/platform/chromium/DataTransferItemChromium.h:49
> > +    enum FromPasteboardType {
> > +        FromPasteboard,
> > +    };
> 
> I'm confused as to what this is for.
> 

It's just a marker to distinguish the overloads of DataTransferItem::create().

> > Source/WebCore/platform/chromium/PlatformBridge.h:102
> > +    static PassRefPtr<SharedBuffer> clipboardReadImage(PasteboardPrivate::ClipboardBuffer);
> 
> Where is this implemented?

I missed reverting this change. Fixed.
Comment 8 Tony Chang 2011-03-21 15:01:16 PDT
Comment on attachment 86347 [details]
Patch

(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 86254 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=86254&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        Support retrieving clipboard data in a paste through DataTransferItems.
> > > +
> > 
> > There should be a "Test: editing/pasteboard/data-transfer-items.html" line there (normally prepare-ChangeLog does that for you, but you're probably just editing the previous message).
> 
> Done. For some reason, it's not emitting that line. I added it manually.

Hmm, maybe it only adds it for new tests.  Anyway, it's nice to have either way.

> > > Source/WebCore/platform/chromium/DataTransferItemChromium.h:49
> > > +    enum FromPasteboardType {
> > > +        FromPasteboard,
> > > +    };
> > 
> > I'm confused as to what this is for.
> > 
> 
> It's just a marker to distinguish the overloads of DataTransferItem::create().

It seems like it would be a bit clearer to name the method createFromPasteboard.  I would probably also just have a single constructor that takes DataSource as a parameter (so you don't have to keep 2 sets of constructor initializer lists in sync).


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

> Source/WebCore/platform/chromium/DataTransferItemsChromium.h:60
> +    friend class ClipboardChromium;

What is this for?
Comment 9 Daniel Cheng 2011-03-21 16:40:13 PDT
Created attachment 86385 [details]
Patch
Comment 10 Daniel Cheng 2011-03-21 16:42:47 PDT
(In reply to comment #8)
> (From update of attachment 86347 [details])
> (In reply to comment #7)
> > (In reply to comment #5)
> > > (From update of attachment 86254 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=86254&action=review
> > > 
> > > > Source/WebCore/ChangeLog:9
> > > > +        Support retrieving clipboard data in a paste through DataTransferItems.
> > > > +
> > > 
> > > There should be a "Test: editing/pasteboard/data-transfer-items.html" line there (normally prepare-ChangeLog does that for you, but you're probably just editing the previous message).
> > 
> > Done. For some reason, it's not emitting that line. I added it manually.
> 
> Hmm, maybe it only adds it for new tests.  Anyway, it's nice to have either way.
> 
> > > > Source/WebCore/platform/chromium/DataTransferItemChromium.h:49
> > > > +    enum FromPasteboardType {
> > > > +        FromPasteboard,
> > > > +    };
> > > 
> > > I'm confused as to what this is for.
> > > 
> > 
> > It's just a marker to distinguish the overloads of DataTransferItem::create().
> 
> It seems like it would be a bit clearer to name the method createFromPasteboard.  I would probably also just have a single constructor that takes DataSource as a parameter (so you don't have to keep 2 sets of constructor initializer lists in sync).
> 
> 

Done.

> View in context: https://bugs.webkit.org/attachment.cgi?id=86347&action=review
> 
> > Source/WebCore/platform/chromium/DataTransferItemsChromium.h:60
> > +    friend class ClipboardChromium;
> 
> What is this for?

addPasteboardItem is only exposed to ClipboardChromium.
Comment 11 Daniel Cheng 2011-03-21 17:03:26 PDT
Committed r81620: <http://trac.webkit.org/changeset/81620>