Bug 38136 – [Qt] Add smart paste support

• Status: CLOSED
• Resolution: FIXED
• Component: WebKit Qt

[Robert Hogan]

Unskip tests:
 editing/pasteboard/smart-paste-001.html
 editing/pasteboard/smart-paste-002.html
 editing/pasteboard/smart-paste-003.html
 editing/pasteboard/smart-paste-004.html
 editing/pasteboard/smart-paste-005.html
 editing/pasteboard/smart-paste-006.html
 editing/pasteboard/smart-paste-007.html

[Robert Hogan]

Created attachment 54327 [details]
Patch

[WebKit Review Bot]

Attachment 54327 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/editing/qt/SmartReplaceQt.cpp:43:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/editing/qt/SmartReplaceQt.cpp:43:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:44:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/editing/qt/SmartReplaceQt.cpp:44:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:45:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/editing/qt/SmartReplaceQt.cpp:45:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:46:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/editing/qt/SmartReplaceQt.cpp:46:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:47:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/editing/qt/SmartReplaceQt.cpp:47:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:48:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/editing/qt/SmartReplaceQt.cpp:48:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:49:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/editing/qt/SmartReplaceQt.cpp:49:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:50:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/editing/qt/SmartReplaceQt.cpp:50:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:51:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/editing/qt/SmartReplaceQt.cpp:51:  One space before end of line comments  [whitespace/comments] [5]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
Total errors found: 18 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robert Hogan]

OK, the comments from the stylebot are a given. I'll wait for others before posting an update.

[Simon Hausmann]

Comment on attachment 54327 [details]
Patch



> +    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!

[Robert Hogan]

Created attachment 54454 [details]
Patch

[WebKit Review Bot]

Attachment 54454 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/editing/qt/SmartReplaceQt.cpp:46:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:47:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:48:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:49:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:50:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:51:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:52:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:53:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:54:  One space before end of line comments  [whitespace/comments] [5]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
Total errors found: 9 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 54454 [details]
Patch


WebCore/editing/qt/SmartReplaceQt.cpp:36
 +  static const QString next = ")].,;:?\'!\"%*-/}";
Hm, what I had in mind is actually a function-local array of char and using that directly :). The above declaration still ends up allocating a QString private and converting the read-only constant C string into unicode.

The rest looks good. Sorry, r- because of the heap alloc'ed QString :)

Let me try to rephrase what I wrote in code:

const char next[] = "....";
const char prev[] = "....";

inside the function.

[Simon Hausmann]

(In reply to comment #6)
> Attachment 54454 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']"
> exit_code: 1
> WebCore/editing/qt/SmartReplaceQt.cpp:46:  One space before end of line
> comments  [whitespace/comments] [5]

I can't tell from the diff, but is this warning real? :)

[Robert Hogan]

(In reply to comment #8)
> (In reply to comment #6)
> > Attachment 54454 [details] [details] did not pass style-queue:
> > 
> > Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']"
> > exit_code: 1
> > WebCore/editing/qt/SmartReplaceQt.cpp:46:  One space before end of line
> > comments  [whitespace/comments] [5]
> 
> I can't tell from the diff, but is this warning real? :)

It is but the comments will look ugly as hell if I obey the stylebot. The comments are also in the same style as SmareReplaceICU.cpp and *CF.cpp from which I lifted them, so think it's ok.

[Robert Hogan]

Created attachment 54723 [details]
Patch

[WebKit Review Bot]

Attachment 54723 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/editing/qt/SmartReplaceQt.cpp:43:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:44:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:45:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:46:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:47:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:48:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:49:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:50:  One space before end of line comments  [whitespace/comments] [5]
WebCore/editing/qt/SmartReplaceQt.cpp:51:  One space before end of line comments  [whitespace/comments] [5]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
Total errors found: 9 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robert Hogan]

Landed as r58631 and r58632