Bug 56330 – [chromium] Implement glue between DataTransferItems and the pasteboard.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Daniel Cheng]

[chromium] Implement glue between DataTransferItems and the pasteboard.

[Daniel Cheng]

Created attachment 85715 [details]
Patch

[WebKit Review Bot]

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.

[Adam Barth]

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.

[Daniel Cheng]

Created attachment 86254 [details]
Patch

[Tony Chang]

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?

[Daniel Cheng]

Created attachment 86347 [details]
Patch

[Daniel Cheng]

(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.

[Tony Chang]

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?

[Daniel Cheng]

Created attachment 86385 [details]
Patch

[Daniel Cheng]

(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.

[Daniel Cheng]

Committed r81620: <http://trac.webkit.org/changeset/81620>

[James Simonsen]

The new test is failing on the Chrome bots:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20-%20chromium.org&tests=editing%2Fpasteboard%2Fdata-transfer-items.html