Bug 55792 – Add plumbing for paste support to ChromiumDataObject::types()

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

[Daniel Cheng]

Don't return "Files" from ChromiumDataObject::types()

[Daniel Cheng]

Created attachment 84806 [details]
Patch

[WebKit Review Bot]

Attachment 84806 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8098013

[Tony Chang]

Comment on attachment 84806 [details]
Patch

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

mostly just style nits

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:-104
> -    // This is currently broken for pasteboard events, and always has been.

Can we write a layout test for this?

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:111
> +        results = PlatformBridge::clipboardReadAvailableTypes(PasteboardPrivate::StandardBuffer,
> +                                                              &ignoredContainsFilenames);
> +    } else {
> +        if (!m_plainText.isEmpty()) {

Nit: You could just early return in the CopyAndPaste case to avoid having to indent/reformat this.

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:232
> +    if (m_clipboardType == Clipboard::CopyAndPaste)

The if part needs {} because the body is more than 1 line.

> Source/WebCore/platform/chromium/ClipboardChromium.cpp:136
> +        results.add("Files");

Should we put this in ClipboardMimeTypes?  The other elements in results are mime types, right?

[Daniel Cheng]

Created attachment 84814 [details]
Patch

[Daniel Cheng]

Created attachment 84883 [details]
Patch

[WebKit Review Bot]

Attachment 84883 [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

Source/WebCore/platform/chromium/ChromiumDataObject.cpp:238:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Cheng]

(In reply to comment #3)
> (From update of attachment 84806 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84806&action=review
> 
> mostly just style nits
> 
> > Source/WebCore/platform/chromium/ChromiumDataObject.cpp:-104
> > -    // This is currently broken for pasteboard events, and always has been.
> 
> Can we write a layout test for this?
> 

Done, though this change is now blocked on http://codereview.chromium.org/6635003.

> > Source/WebCore/platform/chromium/ChromiumDataObject.cpp:111
> > +        results = PlatformBridge::clipboardReadAvailableTypes(PasteboardPrivate::StandardBuffer,
> > +                                                              &ignoredContainsFilenames);
> > +    } else {
> > +        if (!m_plainText.isEmpty()) {
> 
> Nit: You could just early return in the CopyAndPaste case to avoid having to indent/reformat this.
> 

Done.

> > Source/WebCore/platform/chromium/ChromiumDataObject.cpp:232
> > +    if (m_clipboardType == Clipboard::CopyAndPaste)
> 
> The if part needs {} because the body is more than 1 line.
> 

I did that but the style checker complained.

> > Source/WebCore/platform/chromium/ClipboardChromium.cpp:136
> > +        results.add("Files");
> 
> Should we put this in ClipboardMimeTypes?  The other elements in results are mime types, right?

Done.

[Tony Chang]

Comment on attachment 84883 [details]
Patch

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

Please fix the style nit before landing.

>> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:238
>> +    }
> 
> One line control clauses should not use braces.  [whitespace/braces] [4]

The if should have {}, but the else shouldn't.

[Daniel Cheng]

Committed r80484: <http://trac.webkit.org/changeset/80484>

[Daniel Cheng]

Created attachment 84968 [details]
Patch

[Daniel Cheng]

I reverted the change since I forgot to include the DEPS roll.

[Daniel Cheng]

Created attachment 84969 [details]
Patch

[WebKit Review Bot]

Attachment 84969 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8103593

[Daniel Cheng]

Committed r80493: <http://trac.webkit.org/changeset/80493>

[WebKit Review Bot]

http://trac.webkit.org/changeset/80493 might have broken Qt Linux Release
The following tests are not passing:
editing/pasteboard/onpaste-text-html-types.html

[Ryosuke Niwa]

It seems like the test added by this patch has been failing on Qt.

Could you look into it? http://build.webkit.org/builders/Qt%20Linux%20Release/builds/29419

[Tony Chang]

(In reply to comment #16)
> It seems like the test added by this patch has been failing on Qt.
> 
> Could you look into it? http://build.webkit.org/builders/Qt%20Linux%20Release/builds/29419

Looks like dcheng updated the qt and win Skipped list in http://trac.webkit.org/changeset/80531 .