[GTK][WK2] Avoid Vector copies in WebViewBaseInputMethodFilter::setPreedit(), WebTouchEvent
Created attachment 230360 [details] Patch
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
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.
Created attachment 230362 [details] Patch
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 on attachment 230362 [details] Patch Clearing flags on attachment: 230362 Committed r167924: <http://trac.webkit.org/changeset/167924>
All reviewed patches have been landed. Closing bug.
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.
(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 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.