[webkit-reviews] review denied: [Bug 28696] Middle click doesn't trigger onpaste event in Chromium : [Attachment 38938] Handle middle click in Chromium like QT (including updated layout test)

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied  review:
Bug 28696: Middle click doesn't trigger onpaste event in Chromium
https://bugs.webkit.org/show_bug.cgi?id=28696

Attachment 38938: Handle middle click in Chromium like QT (including updated
layout test)
https://bugs.webkit.org/attachment.cgi?id=38938&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> 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.