Don't return "Files" from ChromiumDataObject::types()
Created attachment 84806 [details] Patch
Attachment 84806 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8098013
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?
Created attachment 84814 [details] Patch
Created attachment 84883 [details] Patch
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.
(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.
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.
Committed r80484: <http://trac.webkit.org/changeset/80484>
Created attachment 84968 [details] Patch
I reverted the change since I forgot to include the DEPS roll.
Created attachment 84969 [details] Patch
Attachment 84969 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8103593
Committed r80493: <http://trac.webkit.org/changeset/80493>
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
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
(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 .