[GTK][WK2] Move Vector objects into WebEditorClient::executePendingEditorCommands() invocations
Created attachment 228984 [details] Patch
Comment on attachment 228984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228984&action=review > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:50 > bool WebEditorClient::executePendingEditorCommands(Frame* frame, Vector<WTF::String> pendingEditorCommands, bool allowTextInsertion) It seems like this should either copy the contents or take a reference to pendingEditorCommands. If we don't mind stomping the contents of pendingEditorCommands a reference more straight-forward. If we do mind, we cannot use std::move, right?
Comment on attachment 228984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228984&action=review >> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:50 >> bool WebEditorClient::executePendingEditorCommands(Frame* frame, Vector<WTF::String> pendingEditorCommands, bool allowTextInsertion) > > It seems like this should either copy the contents or take a reference to pendingEditorCommands. If we don't mind stomping the contents of pendingEditorCommands a reference more straight-forward. If we do mind, we cannot use std::move, right? We don't mind stomping the contents of that Vector, even if we don't stomp them in the end. But in WebEditorClient::handleKeyboardEvent() the Vector isn't used after the WebEditorClient::executePendingEditorCommands() call (whichever of the two is performed), so moving the object along into that invocation isn't harmful. Passing in the Vector by reference would make sense if we'd use that Vector later in handleKeyboardEvent(), but we don't. There's nothing wrong with passing the object by reference, but it's not necessary. Hence the proposal to use std::move() and pass it by value.
(In reply to comment #3) > Passing in the Vector by reference would make sense if we'd use that Vector later in handleKeyboardEvent(), but we don't. There's nothing wrong with passing the object by reference, but it's not necessary. Hence the proposal to use std::move() and pass it by value. It's slightly more efficient because you don't have to construct a new object and matches what we usually do in that situation.
(In reply to comment #4) > (In reply to comment #3) > > > Passing in the Vector by reference would make sense if we'd use that Vector later in handleKeyboardEvent(), but we don't. There's nothing wrong with passing the object by reference, but it's not necessary. Hence the proposal to use std::move() and pass it by value. > > It's slightly more efficient because you don't have to construct a new object and matches what we usually do in that situation. I see, WebEditorClient::executePendingEditorCommands() only reads from the Vector but doesn't store it, so taking the const reference of the Vector is indeed more intuitive.
Created attachment 229110 [details] Patch
Comment on attachment 229110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229110&action=review Please update also the header to fix the build. > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:50 > +bool WebEditorClient::executePendingEditorCommands(Frame* frame, const Vector<WTF::String>& pendingEditorCommands, bool allowTextInsertion) You need to update the header too.
Comment on attachment 229110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229110&action=review >> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:50 >> +bool WebEditorClient::executePendingEditorCommands(Frame* frame, const Vector<WTF::String>& pendingEditorCommands, bool allowTextInsertion) > > You need to update the header too. Heh, of course :>
Committed r167116: <http://trac.webkit.org/changeset/167116>