Bug 132323

Summary: [GTK][WK2] Avoid Vector copies in WebViewBaseInputMethodFilter::setPreedit()
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cgarcia, commit-queue, gns, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Zan Dobersek 2014-04-29 00:40:53 PDT
[GTK][WK2] Avoid Vector copies in WebViewBaseInputMethodFilter::setPreedit(), WebTouchEvent
Comment 1 Zan Dobersek 2014-04-29 00:48:43 PDT
Created attachment 230360 [details]
Patch
Comment 2 WebKit Commit Bot 2014-04-29 00:49:51 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 WebKit Commit Bot 2014-04-29 00:50:00 PDT
Attachment 230360 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.cpp:91:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Zan Dobersek 2014-04-29 01:26:59 PDT
Created attachment 230362 [details]
Patch
Comment 5 WebKit Commit Bot 2014-04-29 01:27:57 PDT
Attachment 230362 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.cpp:91:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Zan Dobersek 2014-04-29 01:44:40 PDT
Comment on attachment 230362 [details]
Patch

Clearing flags on attachment: 230362

Committed r167924: <http://trac.webkit.org/changeset/167924>
Comment 7 Zan Dobersek 2014-04-29 01:44:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Martin Robinson 2014-04-29 07:31:55 PDT
Comment on attachment 230360 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230360&action=review

> Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.cpp:92
> +    m_webPageProxy->setComposition(newPreedit, Vector<CompositionUnderline>{ CompositionUnderline(0, newPreedit.length(), Color(1, 1, 1), false) },
> +        m_cursorOffset, m_cursorOffset, 0 /* replacement start */, 0 /* replacement end */);

When the TODO is fixed the vector constructor has to be moved out-of-line anyway. Perhaps it's best just to leave this? It's not called frequently enough for performance to matter.
Comment 9 Martin Robinson 2014-04-29 07:34:53 PDT
(In reply to comment #8)
 
> When the TODO is fixed the vector constructor has to be moved out-of-line anyway. Perhaps it's best just to leave this? It's not called frequently enough for performance to matter.

Oops. I see now that the patch has landed already.
Comment 10 Zan Dobersek 2014-04-29 08:13:40 PDT
Comment on attachment 230360 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230360&action=review

>> Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.cpp:92
>> +        m_cursorOffset, m_cursorOffset, 0 /* replacement start */, 0 /* replacement end */);
> 
> When the TODO is fixed the vector constructor has to be moved out-of-line anyway. Perhaps it's best just to leave this? It's not called frequently enough for performance to matter.

I went with inlining to keep the code compact, but std::move() could be used instead (and should be used when someone addresses the TODO and has to un-inline the constructor).

I know this Vector copy (and many others) wasn't anywhere near being a performance bottleneck, but it's still more correct to have the Vector moved than to have it unnecessarily copied. This is actually the case with most of such changes I've been landing recently.