131454 2014-04-09 14:35:35 -0700 [GTK][WK2] Move Vector objects into WebEditorClient::executePendingEditorCommands() invocations 2014-04-11 01:25:55 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 zan zan cgarcia mrobinson oldest_to_newest 999299 0 zan 2014-04-09 14:35:35 -0700 [GTK][WK2] Move Vector objects into WebEditorClient::executePendingEditorCommands() invocations 999303 1 228984 zan 2014-04-09 14:43:58 -0700 Created attachment 228984 Patch 999351 2 228984 mrobinson 2014-04-09 16:33:13 -0700 Comment on attachment 228984 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? 999463 3 228984 zan 2014-04-10 00:56:04 -0700 Comment on attachment 228984 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. 999530 4 mrobinson 2014-04-10 07:41:56 -0700 (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. 999798 5 zan 2014-04-10 22:42:42 -0700 (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. 999799 6 229110 zan 2014-04-10 22:43:08 -0700 Created attachment 229110 Patch 999806 7 229110 cgarcia 2014-04-10 23:40:13 -0700 Comment on attachment 229110 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. 999830 8 229110 zan 2014-04-11 01:21:42 -0700 Comment on attachment 229110 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 :> 999831 9 zan 2014-04-11 01:25:55 -0700 Committed r167116: <http://trac.webkit.org/changeset/167116> 228984 2014-04-09 14:43:58 -0700 2014-04-10 22:43:02 -0700 Patch bug-131454-20140409144336.patch text/plain 3192 zan SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDE2NzAzNCkKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDE0LTA0LTA5ICBaYW4gRG9i ZXJzZWsgIDx6ZG9iZXJzZWtAaWdhbGlhLmNvbT4KKworICAgICAgICBbR1RLXVtXSzJdIE1vdmUg VmVjdG9yIG9iamVjdHMgaW50byBXZWJFZGl0b3JDbGllbnQ6OmV4ZWN1dGVQZW5kaW5nRWRpdG9y Q29tbWFuZHMoKSBpbnZvY2F0aW9ucworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MTMxNDU0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgV2ViRWRpdG9yQ2xpZW50OjpleGVjdXRlUGVuZGluZ0VkaXRvckNvbW1h bmRzKCkgdGFrZXMgdGhlIFZlY3RvciBvZiBTdHJpbmcgb2JqZWN0cyBieSB2YWx1ZS4KKyAgICAg ICAgVGhlIHR3byBpbnZvY2F0aW9ucyBpbiBXZWJFZGl0b3JDbGllbnQ6OmhhbmRsZUtleWJvYXJk RXZlbnQoKSBzaG91bGQgbW92ZSB0aGUgdGVtcG9yYXJ5CisgICAgICAgIFZlY3RvciBvYmplY3Rz IGludG8gdGhlIGludm9jYXRpb24gaW5zdGVhZCBvZiBwYXNzaW5nIHRoZW0gYnkgcmVmZXJlbmNl IGFuZCBjYXVzaW5nIGEgY29weS4KKworICAgICAgICAqIFdlYlByb2Nlc3MvV2ViQ29yZVN1cHBv cnQvZ3RrL1dlYkVkaXRvckNsaWVudEd0ay5jcHA6CisgICAgICAgIChXZWJLaXQ6OldlYkVkaXRv ckNsaWVudDo6ZXhlY3V0ZVBlbmRpbmdFZGl0b3JDb21tYW5kcyk6CisgICAgICAgIChXZWJLaXQ6 OldlYkVkaXRvckNsaWVudDo6aGFuZGxlS2V5Ym9hcmRFdmVudCk6CisKIDIwMTQtMDQtMDkgIEJy YWR5IEVpZHNvbiAgPGJlaWRzb25AYXBwbGUuY29tPgogCiAgICAgICAgIERvbid0IG9mZmVyICJj b250YWN0IiBhY3Rpb25zIGluIHRlbGVwaG9uZSBudW1iZXIgbWVudXMKSW5kZXg6IFNvdXJjZS9X ZWJLaXQyL1dlYlByb2Nlc3MvV2ViQ29yZVN1cHBvcnQvZ3RrL1dlYkVkaXRvckNsaWVudEd0ay5j cHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJDb3JlU3VwcG9y dC9ndGsvV2ViRWRpdG9yQ2xpZW50R3RrLmNwcAkocmV2aXNpb24gMTY2ODg3KQorKysgU291cmNl L1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJDb3JlU3VwcG9ydC9ndGsvV2ViRWRpdG9yQ2xpZW50R3Rr LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNTAsMTYgKzUwLDE2IEBAIHZvaWQgV2ViRWRpdG9yQ2xp ZW50OjpnZXRFZGl0b3JDb21tYW5kc0YKIGJvb2wgV2ViRWRpdG9yQ2xpZW50OjpleGVjdXRlUGVu ZGluZ0VkaXRvckNvbW1hbmRzKEZyYW1lKiBmcmFtZSwgVmVjdG9yPFdURjo6U3RyaW5nPiBwZW5k aW5nRWRpdG9yQ29tbWFuZHMsIGJvb2wgYWxsb3dUZXh0SW5zZXJ0aW9uKQogewogICAgIFZlY3Rv cjxFZGl0b3I6OkNvbW1hbmQ+IGNvbW1hbmRzOwotICAgIGZvciAoc2l6ZV90IGkgPSAwOyBpIDwg cGVuZGluZ0VkaXRvckNvbW1hbmRzLnNpemUoKTsgaSsrKSB7Ci0gICAgICAgIEVkaXRvcjo6Q29t bWFuZCBjb21tYW5kID0gZnJhbWUtPmVkaXRvcigpLmNvbW1hbmQocGVuZGluZ0VkaXRvckNvbW1h bmRzLmF0KGkpLnV0ZjgoKS5kYXRhKCkpOworICAgIGZvciAoYXV0byYgY29tbWFuZFN0cmluZyA6 IHBlbmRpbmdFZGl0b3JDb21tYW5kcykgeworICAgICAgICBFZGl0b3I6OkNvbW1hbmQgY29tbWFu ZCA9IGZyYW1lLT5lZGl0b3IoKS5jb21tYW5kKGNvbW1hbmRTdHJpbmcudXRmOCgpLmRhdGEoKSk7 CiAgICAgICAgIGlmIChjb21tYW5kLmlzVGV4dEluc2VydGlvbigpICYmICFhbGxvd1RleHRJbnNl cnRpb24pCiAgICAgICAgICAgICByZXR1cm4gZmFsc2U7CiAKLSAgICAgICAgY29tbWFuZHMuYXBw ZW5kKGNvbW1hbmQpOworICAgICAgICBjb21tYW5kcy5hcHBlbmQoc3RkOjptb3ZlKGNvbW1hbmQp KTsKICAgICB9CiAKLSAgICBmb3IgKHNpemVfdCBpID0gMDsgaSA8IGNvbW1hbmRzLnNpemUoKTsg aSsrKSB7Ci0gICAgICAgIGlmICghY29tbWFuZHMuYXQoaSkuZXhlY3V0ZSgpKQorICAgIGZvciAo YXV0byYgY29tbWFuZCA6IGNvbW1hbmRzKSB7CisgICAgICAgIGlmICghY29tbWFuZC5leGVjdXRl KCkpCiAgICAgICAgICAgICByZXR1cm4gZmFsc2U7CiAgICAgfQogCkBAIC04OSwxNCArODksMTQg QEAgdm9pZCBXZWJFZGl0b3JDbGllbnQ6OmhhbmRsZUtleWJvYXJkRXZlbgogICAgICAgICAvLyB0 aGUgaW5zZXJ0aW9uIHVudGlsIHRoZSBrZXlwcmVzcyBldmVudC4gV2Ugd2FudCBrZXlkb3duIHRv IGJ1YmJsZSB1cAogICAgICAgICAvLyB0aHJvdWdoIHRoZSBET00gZmlyc3QuCiAgICAgICAgIGlm IChwbGF0Zm9ybUV2ZW50LT50eXBlKCkgPT0gUGxhdGZvcm1FdmVudDo6UmF3S2V5RG93bikgewot ICAgICAgICAgICAgaWYgKGV4ZWN1dGVQZW5kaW5nRWRpdG9yQ29tbWFuZHMoZnJhbWUsIHBlbmRp bmdFZGl0b3JDb21tYW5kcywgZmFsc2UpKQorICAgICAgICAgICAgaWYgKGV4ZWN1dGVQZW5kaW5n RWRpdG9yQ29tbWFuZHMoZnJhbWUsIHN0ZDo6bW92ZShwZW5kaW5nRWRpdG9yQ29tbWFuZHMpLCBm YWxzZSkpCiAgICAgICAgICAgICAgICAgZXZlbnQtPnNldERlZmF1bHRIYW5kbGVkKCk7CiAKICAg ICAgICAgICAgIHJldHVybjsKICAgICAgICAgfQogCiAgICAgICAgIC8vIE9ubHkgYWxsb3cgdGV4 dCBpbnNlcnRpb24gY29tbWFuZHMgaWYgdGhlIGN1cnJlbnQgbm9kZSBpcyBlZGl0YWJsZS4KLSAg ICAgICAgaWYgKGV4ZWN1dGVQZW5kaW5nRWRpdG9yQ29tbWFuZHMoZnJhbWUsIHBlbmRpbmdFZGl0 b3JDb21tYW5kcywgZnJhbWUtPmVkaXRvcigpLmNhbkVkaXQoKSkpIHsKKyAgICAgICAgaWYgKGV4 ZWN1dGVQZW5kaW5nRWRpdG9yQ29tbWFuZHMoZnJhbWUsIHN0ZDo6bW92ZShwZW5kaW5nRWRpdG9y Q29tbWFuZHMpLCBmcmFtZS0+ZWRpdG9yKCkuY2FuRWRpdCgpKSkgewogICAgICAgICAgICAgZXZl bnQtPnNldERlZmF1bHRIYW5kbGVkKCk7CiAgICAgICAgICAgICByZXR1cm47CiAgICAgICAgIH0K 229110 2014-04-10 22:43:08 -0700 2014-04-11 01:21:42 -0700 Patch bug-131454-20140410224247.patch text/plain 2425 zan SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDE2NzExMykKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDE0LTA0LTEwICBaYW4gRG9i ZXJzZWsgIDx6ZG9iZXJzZWtAaWdhbGlhLmNvbT4KKworICAgICAgICBbR1RLXVtXSzJdIE1vdmUg VmVjdG9yIG9iamVjdHMgaW50byBXZWJFZGl0b3JDbGllbnQ6OmV4ZWN1dGVQZW5kaW5nRWRpdG9y Q29tbWFuZHMoKSBpbnZvY2F0aW9ucworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MTMxNDU0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgKiBXZWJQcm9jZXNzL1dlYkNvcmVTdXBwb3J0L2d0ay9XZWJFZGl0b3JD bGllbnRHdGsuY3BwOgorICAgICAgICAoV2ViS2l0OjpXZWJFZGl0b3JDbGllbnQ6OmV4ZWN1dGVQ ZW5kaW5nRWRpdG9yQ29tbWFuZHMpOiBUYWtlIGEgY29uc3QgcmVmZXJlbmNlIG9mIHRoZSBWZWN0 b3IKKyAgICAgICAgb2JqZWN0IHRvIGF2b2lkIHVubmVjZXNzYXJ5IGNvcGllcy4gQWxzbyBkZXBs b3kgdHdvIHJhbmdlLWJhc2VkIGZvciBsb29wcyBhbmQgZWZmaWNpZW50bHkgbW92ZQorICAgICAg ICB0aGUgRWRpdG9yOjpDb21tYW5kIG9iamVjdHMgaW50byB0aGUgb3RoZXIgVmVjdG9yLgorCiAy MDE0LTA0LTEwICBEYW4gQmVybnN0ZWluICA8bWl0ekBhcHBsZS5jb20+CiAKICAgICAgICAgQnVu ZGxlUGFnZURpYWdub3N0aWNMb2dnaW5nQ2xpZW50IGxlYWtzIGV2ZXJ5IHN0cmluZyBwYXNzaW5n IHRocm91Z2ggaXQuCkluZGV4OiBTb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1dlYkNvcmVTdXBw b3J0L2d0ay9XZWJFZGl0b3JDbGllbnRHdGsuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJL aXQyL1dlYlByb2Nlc3MvV2ViQ29yZVN1cHBvcnQvZ3RrL1dlYkVkaXRvckNsaWVudEd0ay5jcHAJ KHJldmlzaW9uIDE2NzExMykKKysrIFNvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViQ29yZVN1 cHBvcnQvZ3RrL1dlYkVkaXRvckNsaWVudEd0ay5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTQ3LDE5 ICs0NywxOSBAQCB2b2lkIFdlYkVkaXRvckNsaWVudDo6Z2V0RWRpdG9yQ29tbWFuZHNGCiAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBtX3BhZ2UtPnBhZ2VJ RCgpLCBzdGQ6OmNocm9ubzo6bWlsbGlzZWNvbmRzOjptYXgoKSk7CiB9CiAKLWJvb2wgV2ViRWRp dG9yQ2xpZW50OjpleGVjdXRlUGVuZGluZ0VkaXRvckNvbW1hbmRzKEZyYW1lKiBmcmFtZSwgVmVj dG9yPFdURjo6U3RyaW5nPiBwZW5kaW5nRWRpdG9yQ29tbWFuZHMsIGJvb2wgYWxsb3dUZXh0SW5z ZXJ0aW9uKQorYm9vbCBXZWJFZGl0b3JDbGllbnQ6OmV4ZWN1dGVQZW5kaW5nRWRpdG9yQ29tbWFu ZHMoRnJhbWUqIGZyYW1lLCBjb25zdCBWZWN0b3I8V1RGOjpTdHJpbmc+JiBwZW5kaW5nRWRpdG9y Q29tbWFuZHMsIGJvb2wgYWxsb3dUZXh0SW5zZXJ0aW9uKQogewogICAgIFZlY3RvcjxFZGl0b3I6 OkNvbW1hbmQ+IGNvbW1hbmRzOwotICAgIGZvciAoc2l6ZV90IGkgPSAwOyBpIDwgcGVuZGluZ0Vk aXRvckNvbW1hbmRzLnNpemUoKTsgaSsrKSB7Ci0gICAgICAgIEVkaXRvcjo6Q29tbWFuZCBjb21t YW5kID0gZnJhbWUtPmVkaXRvcigpLmNvbW1hbmQocGVuZGluZ0VkaXRvckNvbW1hbmRzLmF0KGkp LnV0ZjgoKS5kYXRhKCkpOworICAgIGZvciAoYXV0byYgY29tbWFuZFN0cmluZyA6IHBlbmRpbmdF ZGl0b3JDb21tYW5kcykgeworICAgICAgICBFZGl0b3I6OkNvbW1hbmQgY29tbWFuZCA9IGZyYW1l LT5lZGl0b3IoKS5jb21tYW5kKGNvbW1hbmRTdHJpbmcudXRmOCgpLmRhdGEoKSk7CiAgICAgICAg IGlmIChjb21tYW5kLmlzVGV4dEluc2VydGlvbigpICYmICFhbGxvd1RleHRJbnNlcnRpb24pCiAg ICAgICAgICAgICByZXR1cm4gZmFsc2U7CiAKLSAgICAgICAgY29tbWFuZHMuYXBwZW5kKGNvbW1h bmQpOworICAgICAgICBjb21tYW5kcy5hcHBlbmQoc3RkOjptb3ZlKGNvbW1hbmQpKTsKICAgICB9 CiAKLSAgICBmb3IgKHNpemVfdCBpID0gMDsgaSA8IGNvbW1hbmRzLnNpemUoKTsgaSsrKSB7Ci0g ICAgICAgIGlmICghY29tbWFuZHMuYXQoaSkuZXhlY3V0ZSgpKQorICAgIGZvciAoYXV0byYgY29t bWFuZCA6IGNvbW1hbmRzKSB7CisgICAgICAgIGlmICghY29tbWFuZC5leGVjdXRlKCkpCiAgICAg ICAgICAgICByZXR1cm4gZmFsc2U7CiAgICAgfQogCg==