Bug 28696 – Middle click doesn't trigger onpaste event in Chromium

• Component: Platform
• Status: RESOLVED FIXED

[Steve VanDeBogart]

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)

[Steve VanDeBogart]

Created attachment 38515 [details]
Handle middle click in Chromium like QT

[Eric Seidel (no email)]

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);

[Steve VanDeBogart]

Created attachment 38897 [details]
Handle middle click in Chromium like QT (try 2)

[Steve VanDeBogart]

Created attachment 38898 [details]
Layout test to test that middle click triggers the onpaste event

[Steve VanDeBogart]

Address Eric's comments.  (missed the obsoletes box for the WebCore patch)

[Eric Seidel (no email)]

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.

[Steve VanDeBogart]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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)

[Darin Fisher (:fishd, Google)]

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.

[Steve VanDeBogart]

Created attachment 39014 [details]
Handle middle click in Chromium like QT

Address Darin's comments.

[Eric Seidel (no email)]

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');

[Steve VanDeBogart]

Created attachment 39016 [details]
Handle middle click in Chromium like QT

Oops, fix the layout test too.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

This broke Chromium build. Rolling out. Please do not commit two-sided patches like this.

[Dimitri Glazkov (Google)]

Rolled out in http://trac.webkit.org/changeset/48059.

[Dimitri Glazkov (Google)]

Reopening.

[Darin Fisher (:fishd, Google)]

This is commit-queue- because it is waiting for the corresponding chromium change:  http://codereview.chromium.org/174367

[Steve VanDeBogart]

This is now ready to be committed

[Eric Seidel (no email)]

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. :)

[Steve VanDeBogart]

Oops.  Still getting the hang of this system. I guess I meant to set commit-queue? ?

[Eric Seidel (no email)]

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

[Nate Chapin]

Landed as r48168.