[webkit-reviews] review denied: [Bug 88238] [Qt] X11 Global Selection : [Attachment 145747] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Allan Sandfeld Jensen
<allan.jensen at nokia.com>'s request for review:
Bug 88238: [Qt] X11 Global Selection
https://bugs.webkit.org/show_bug.cgi?id=88238

Attachment 145747: Patch
https://bugs.webkit.org/attachment.cgi?id=145747&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=145747&action=review


> Source/WebCore/editing/EditorCommand.cpp:1588
> +	   { "PasteGlobalSelection", { executePasteGlobalSelection,
supportedPasteGlobalSelection, enabledPaste, stateNone, valueNull,
notTextInsertion, allowExecutionWhenDisabled } },

This comment should NOT be exposed to scripts. So use
supportedFromMenuOrKeyBinding instead of supportedPasteGlobalSelection.
r- because of this.

> Source/WebCore/editing/FrameSelection.cpp:74
> +#if PLATFORM(QT)
> +#include <QClipboard>
> +#include <qguiapplication.h>
> +#endif

I don't want to see these if-defs.

> Source/WebCore/editing/FrameSelection.cpp:319
> +#if PLATFORM(QT) || (PLATFORM(CHROMIUM) && OS(UNIX) && !OS(DARWIN))
> +    if (newSelection.isRange())
> +	   updateGlobalSelection();
> +#endif

Ditto.

> Source/WebCore/editing/FrameSelection.cpp:1819
> +#if PLATFORM(QT) || (PLATFORM(CHROMIUM) && OS(UNIX) && !OS(DARWIN))
> +void FrameSelection::updateGlobalSelection()
> +{
> +#if PLATFORM(QT)
> +    if (qApp->clipboard()->supportsSelection())
> +#endif
> +    {
> +	   bool oldSelectionMode =
Pasteboard::generalPasteboard()->isSelectionMode();
> +	   Pasteboard::generalPasteboard()->setSelectionMode(true);
> +	  
Pasteboard::generalPasteboard()->writeSelection(toNormalizedRange().get(),
m_frame->editor()->canSmartCopyOrDelete(), m_frame);
> +	   Pasteboard::generalPasteboard()->setSelectionMode(oldSelectionMode);

> +    }
> +}
> +#endif

A better way to do this is by adding new preference/setting or editor client
callback like supportsGlobalSelection on all ports.
Also, we need a test for this because this code didn't exist at least for
chromium port. This is another reason for r-.