Bug 132323 – [GTK][WK2] Avoid Vector copies in WebViewBaseInputMethodFilter::setPreedit()

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[GTK][WK2] Avoid Vector copies in WebViewBaseInputMethodFilter::setPreedit(), WebTouchEvent

[Zan Dobersek]

Created attachment 230360 [details]
Patch

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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.

[Zan Dobersek]

Created attachment 230362 [details]
Patch

[WebKit Commit Bot]

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.

[Zan Dobersek]

Comment on attachment 230362 [details]
Patch

Clearing flags on attachment: 230362

Committed r167924: <http://trac.webkit.org/changeset/167924>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Martin Robinson]

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.

[Martin Robinson]

(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.

[Zan Dobersek]

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.