[webkit-reviews] review denied: [Bug 38136] [Qt] Add smart paste support : [Attachment 54327] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Robert Hogan
<robert at webkit.org>'s request for review:
Bug 38136: [Qt] Add smart paste support
https://bugs.webkit.org/show_bug.cgi?id=38136

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

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>


> +    ushort e = d.unicode();
> +    QString str((isPreviousCharacter) ? "([\"\'#$/-`{" :
")].,;:?\'!\"%*-/}");

I think a comment is needed here to indicate that the logic/constants come from
SmartReplaceCF.

Also constructing the QString like this results in unnecessary allocation and
data copying.
I think it would be better to place these two constant strings in read-only
memory and
iterate over them directly (in the loop below this hunk).

> +    if (canSmartCopyOrDelete)
> +	   md->setData("text/smartpaste", QByteArray());

I suggest to make the mime-type more specific, as it might end up published in
the system clipboard.
Imaging copy & paste from Chromium or Safari on a desktop machine to and from a
QtWebKit base application/browser.

Therefore I suggest for example "application/vnd.qtwebkit.smartpaste" as an
alternate mime-type.

>  }
>  
>  bool Pasteboard::canSmartReplace()
>  {
> +    if
(QApplication::clipboard()->mimeData()->formats().contains(QLatin1String("text/
smartpaste")))
> +	   return true;

Please use hasFormats() instead of formats().contains() if possible.

Other than that the patch looks good!