Bug 131454 – [GTK][WK2] Move Vector objects into WebEditorClient::executePendingEditorCommands() invocations

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[GTK][WK2] Move Vector objects into WebEditorClient::executePendingEditorCommands() invocations

[Zan Dobersek]

Created attachment 228984 [details]
Patch

[Martin Robinson]

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?

[Zan Dobersek]

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.

[Martin Robinson]

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

[Zan Dobersek]

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

[Zan Dobersek]

Created attachment 229110 [details]
Patch

[Carlos Garcia Campos]

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.

[Zan Dobersek]

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 :>

[Zan Dobersek]

Committed r167116: <http://trac.webkit.org/changeset/167116>