Chromium doesn't treat middle click on Linux as a paste event. (http://code.google.com/p/chromium/issues/detail?id=18792) The attached patch is part of an attempt to fix this. The remaining parts are at (http://codereview.chromium.org/174367)
Created attachment 38515 [details] Handle middle click in Chromium like QT
Comment on attachment 38515 [details] Handle middle click in Chromium like QT This needs a layout test. Also, funny spacing in the ChangeLog: + Handle middle click in Chromium like QT + + https://bugs.webkit.org/show_bug.cgi?id=28696 Seems wrong: +#if PLATFORM(QT) || PLATFORM(CHROMIUM) bool isSelectionMode() const; void setSelectionMode(bool selectionMode); #endif You only want this on Chromium Linux, or? Enums are almost always clearer than bools: + static bool clipboardIsFormatAvailable(PasteboardPrivate::ClipboardFormat, bool);
Created attachment 38897 [details] Handle middle click in Chromium like QT (try 2)
Created attachment 38898 [details] Layout test to test that middle click triggers the onpaste event
Address Eric's comments. (missed the obsoletes box for the WebCore patch)
The patch seems fine. I generally try to write new layout test as "js tests" aka" DOM only tests" our documentation for such is rather poor though: http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree That would save you some typing, as you get things like "debug" and "testPassed" and "shouldBe" functions all for free. Why does this test use edting.js? Seems it doesn't actually want to use any of that framework, no? The test and change should ideally be in one patch so they can be landed together by the bot.
Created attachment 38938 [details] Handle middle click in Chromium like QT (including updated layout test) Change the test to a "DOM only test" and merge the patches to one file.
Comment on attachment 38938 [details] Handle middle click in Chromium like QT (including updated layout test) Looks fine. You coudl have written: 23 shouldBe("1", stringify(pasteCount)); as shouldBe("pasteCount", "1"); shouldBe evals each half.
Comment on attachment 38938 [details] Handle middle click in Chromium like QT (including updated layout test) Rejecting patch 38938 from commit-queue. This patch will require manual commit. Patch https://bugs.webkit.org/attachment.cgi?id=38938 from bug 28696 failed to download and apply.
Comment on attachment 38938 [details] Handle middle click in Chromium like QT (including updated layout test) Patch looks malformed: can't find file to patch at input line 5 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -------------------------- |Index: LayoutTests/platform/gtk/editing/pasteboard/resources/TEMPLATE.html |=================================================================== |--- LayoutTests/platform/gtk/editing/pasteboard/resources/TEMPLATE.html (revision 0) |+++ LayoutTests/platform/gtk/editing/pasteboard/resources/TEMPLATE.html (working copy) -------------------------- No file to patch. Skipping patch. 1 out of 1 hunk ignored patch -p0 "LayoutTests/platform/gtk/editing/pasteboard/resources/TEMPLATE.html" returned 1. Pass --force to ignore patch failures. Index: LayoutTests/platform/gtk/editing/pasteboard/resources/TEMPLATE.html =================================================================== --- LayoutTests/platform/gtk/editing/pasteboard/resources/TEMPLATE.html (revision 47870) (from ../../../../../../../http:/svn.webkit.org/repository/webkit/trunk/LayoutTests/traversal/resources/TEMPLATE.html:47870) +++ LayoutTests/platform/gtk/editing/pasteboard/resources/TEMPLATE.html (working copy)
Comment on attachment 38938 [details] Handle middle click in Chromium like QT (including updated layout test) > Index: WebCore/platform/chromium/ChromiumBridge.h ... > + static String clipboardReadPlainText(PasteboardPrivate::TargetClipboard); > static void clipboardReadHTML(String*, KURL*); So, I guess it is implied that the target is the system clipboard for all of the methods that do not have a TargetClipboard parameter? It might be good to write a comment saying that. > Index: WebCore/platform/chromium/ClipboardChromium.cpp ... > #include "HTMLNames.h" > #include "NamedAttrMap.h" > #include "MIMETypeRegistry.h" > +#include "Pasteboard.h" > #include "markup.h" > #include "NamedNodeMap.h" > #include "PlatformString.h" nit: includes should be in alphabetical order > Index: WebCore/platform/chromium/PasteboardChromium.cpp ... > +bool Pasteboard::isSelectionMode() const > +{ > + return m_selectionMode; nit: indentation should be 4 white spaces > +void Pasteboard::setSelectionMode(bool selectionMode) > +{ > + m_selectionMode = selectionMode; ditto > PassRefPtr<DocumentFragment> Pasteboard::documentFragment(Frame* frame, PassRefPtr<Range> context, bool allowPlainText, bool& chosePlainText) > { > chosePlainText = false; > + PasteboardPrivate::TargetClipboard target = m_selectionMode ? PasteboardPrivate::PrimarySelection : PasteboardPrivate::Clipboard; nit: only one space after "?" > Index: WebCore/platform/chromium/PasteboardPrivate.h ... > + enum TargetClipboard { > + Clipboard, > + PrimarySelection, > + }; Please change TargetClipboard to ClipboardTarget to match the naming convention already in use (like ClipboardFormat). I would also change the enum values to something like the following: SystemTarget SelectionTarget Where SystemTarget refers to the system clipboard and SelectionTarget refers to the selection clipboard.
Created attachment 39014 [details] Handle middle click in Chromium like QT Address Darin's comments.
Comment on attachment 39014 [details] Handle middle click in Chromium like QT While we're fixing things, I would prefer "PASS 1 is 1" be something nicer. :) But if Darin finds the rest of this OK, it's fine to land. :) 23 shouldBe("1", stringify(pasteCount)); shouldBe(pasteCount, '1');
Created attachment 39016 [details] Handle middle click in Chromium like QT Oops, fix the layout test too.
Comment on attachment 39016 [details] Handle middle click in Chromium like QT As far as I can tell you have addressed all of Darin and my concerns. LGTM.
Comment on attachment 39016 [details] Handle middle click in Chromium like QT Clearing flags on attachment: 39016 Committed r48035: <http://trac.webkit.org/changeset/48035>
All reviewed patches have been landed. Closing bug.
This broke Chromium build. Rolling out. Please do not commit two-sided patches like this.
Rolled out in http://trac.webkit.org/changeset/48059.
Reopening.
This is commit-queue- because it is waiting for the corresponding chromium change: http://codereview.chromium.org/174367
This is now ready to be committed
A non-committer (like steve) setting commit-queue+ will sadly just hang the queue. bug 28605. I need to fix that so that it will properly cq- bugs in that situation. :)
Oops. Still getting the hang of this system. I guess I meant to set commit-queue? ?
(In reply to comment #24) > Oops. Still getting the hang of this system. I guess I meant to set > commit-queue? ? Yeah. It's also totally my fault that cq+ by a non-committer hangs things. I intend to fix that (bug 28605) today.
Landed as r48168.