Bug 38136 - [Qt] Add smart paste support
Summary: [Qt] Add smart paste support
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-04-26 13:13 PDT by Robert Hogan
Modified: 2010-05-01 03:47 PDT (History)
2 users (show)

See Also:


Attachments
Patch (40.23 KB, patch)
2010-04-26 13:25 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (39.07 KB, patch)
2010-04-27 14:08 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (39.07 KB, patch)
2010-04-29 11:36 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2010-04-26 13:13:31 PDT
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
Comment 1 Robert Hogan 2010-04-26 13:25:06 PDT
Created attachment 54327 [details]
Patch
Comment 2 WebKit Review Bot 2010-04-26 13:31:32 PDT
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.
Comment 3 Robert Hogan 2010-04-26 14:41:06 PDT
OK, the comments from the stylebot are a given. I'll wait for others before posting an update.
Comment 4 Simon Hausmann 2010-04-27 02:52:28 PDT
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!
Comment 5 Robert Hogan 2010-04-27 14:08:49 PDT
Created attachment 54454 [details]
Patch
Comment 6 WebKit Review Bot 2010-04-27 14:15:09 PDT
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.
Comment 7 Simon Hausmann 2010-04-28 13:36:56 PDT
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.
Comment 8 Simon Hausmann 2010-04-28 13:37:35 PDT
(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? :)
Comment 9 Robert Hogan 2010-04-28 14:18:48 PDT
(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.
Comment 10 Robert Hogan 2010-04-29 11:36:09 PDT
Created attachment 54723 [details]
Patch
Comment 11 WebKit Review Bot 2010-04-29 11:39:50 PDT
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.
Comment 12 Robert Hogan 2010-05-01 03:46:33 PDT
Landed as r58631 and r58632