130702 2014-03-24 16:24:31 -0700 Web Inspector: protocol command invocations should return a promise if no callback is supplied 2014-11-12 14:45:52 -0800 1 1 1 Unclassified WebKit Web Inspector 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 1 bburg burg burg graouts joepeck timothy webkit-bug-importer oldest_to_newest 994011 0 bburg 2014-03-24 16:24:31 -0700 . 994012 1 webkit-bug-importer 2014-03-24 16:25:00 -0700 <rdar://problem/16412909> 1024522 2 235433 burg 2014-07-24 08:47:31 -0700 Created attachment 235433 Patch 1024654 3 235433 joepeck 2014-07-24 16:14:33 -0700 Comment on attachment 235433 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235433&action=review > Source/WebInspectorUI/ChangeLog:9 > + This allows the trailing Agent.command.promise(args) to be dropped in favor of just > + Agent.command(args). It should make it a bit easier to convert code to use promises. This is very neat. But it also will create a lot of unnecessary Promise objects that immediately get ignored and garbage collected. I am a bit worried about that. There are many backend calls where we don't care about callbacks, creating a Promise in those cases could hurt us (for example at startup). *Agent.enable/disable/start/stop ConsoleAgent.* DOMAgent.highlightRect/hideHighlight/markUndoableState RuntimeAgent.releaseObjectGroup/releaseObject Do you think this might be a negligible perf impact, or will the convenience used often enough to be worth it? > Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:329 > + var commandArguments = Array.prototype.slice.call(arguments); This seems like it could be a lot of work to just get our Array.lastValue implementation on an Arguments object. This (potentially) creates and throws away an Array each call. I would suggest just a quick check ourselves. // If the last argument to the command is not a function, return a result promise. if (arguments.length === 0 || typeof arguments[arguments.length - 1] !== "function") return instance.promise.apply(instance, arguments); Maybe even just "typeof arguments[arguments.length - 1]". 1024655 4 235433 joepeck 2014-07-24 16:15:19 -0700 Comment on attachment 235433 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235433&action=review > LayoutTests/inspector/protocol-promise-result.html:14 > + var p2 = RuntimeAgent.evaluate("43"); Gosh, this does look cool though... > LayoutTests/inspector/protocol-promise-result.html:30 > + }) Nit: semicolon 1024659 5 235433 burg 2014-07-24 16:28:32 -0700 Comment on attachment 235433 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235433&action=review >> Source/WebInspectorUI/ChangeLog:9 >> + Agent.command(args). It should make it a bit easier to convert code to use promises. > > This is very neat. But it also will create a lot of unnecessary Promise objects that immediately get ignored and garbage collected. I am a bit worried about that. > > There are many backend calls where we don't care about callbacks, creating a Promise in those cases could hurt us (for example at startup). > > *Agent.enable/disable/start/stop > ConsoleAgent.* > DOMAgent.highlightRect/hideHighlight/markUndoableState > RuntimeAgent.releaseObjectGroup/releaseObject > > Do you think this might be a negligible perf impact, or will the convenience used often enough to be worth it? I would be surprised if the impact is zero, but I have no way of knowing. This is obviously just a nice-to-have, or I would have implemented it a few months ago :) We could try whitelisting or blacklisting, or just forget about the shortcut altogether. >> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:329 >> + var commandArguments = Array.prototype.slice.call(arguments); > > This seems like it could be a lot of work to just get our Array.lastValue implementation on an Arguments object. This (potentially) creates and throws away an Array each call. I would suggest just a quick check ourselves. > > // If the last argument to the command is not a function, return a result promise. > if (arguments.length === 0 || typeof arguments[arguments.length - 1] !== "function") > return instance.promise.apply(instance, arguments); > > Maybe even just "typeof arguments[arguments.length - 1]". oh, duh :) >> LayoutTests/inspector/protocol-promise-result.html:14 >> + var p2 = RuntimeAgent.evaluate("43"); > > Gosh, this does look cool though... The main benefit in my 1.5 days of using this is that you no longer mess up and call RuntimeAgent.evaluate("42").promise() or RuntimeAgent.evaluate().promise("42") by mistake. But both of those are pretty easy to catch, since they raise a TypeError (invoking undefined). 1025018 6 235433 timothy 2014-07-26 09:19:53 -0700 Comment on attachment 235433 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235433&action=review >>> Source/WebInspectorUI/ChangeLog:9 >>> + Agent.command(args). It should make it a bit easier to convert code to use promises. >> >> This is very neat. But it also will create a lot of unnecessary Promise objects that immediately get ignored and garbage collected. I am a bit worried about that. >> >> There are many backend calls where we don't care about callbacks, creating a Promise in those cases could hurt us (for example at startup). >> >> *Agent.enable/disable/start/stop >> ConsoleAgent.* >> DOMAgent.highlightRect/hideHighlight/markUndoableState >> RuntimeAgent.releaseObjectGroup/releaseObject >> >> Do you think this might be a negligible perf impact, or will the convenience used often enough to be worth it? > > I would be surprised if the impact is zero, but I have no way of knowing. This is obviously just a nice-to-have, or I would have implemented it a few months ago :) > We could try whitelisting or blacklisting, or just forget about the shortcut altogether. I think the cost of creating the Promise will be negligible compared to the JSON objects we build for the protocol just to be converted to a string and thrown away. 1026957 7 235433 timothy 2014-08-05 15:23:30 -0700 Comment on attachment 235433 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235433&action=review >>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:329 >>> + var commandArguments = Array.prototype.slice.call(arguments); >> >> This seems like it could be a lot of work to just get our Array.lastValue implementation on an Arguments object. This (potentially) creates and throws away an Array each call. I would suggest just a quick check ourselves. >> >> // If the last argument to the command is not a function, return a result promise. >> if (arguments.length === 0 || typeof arguments[arguments.length - 1] !== "function") >> return instance.promise.apply(instance, arguments); >> >> Maybe even just "typeof arguments[arguments.length - 1]". > > oh, duh :) Yeah this would be good to fix. 1027220 8 burg 2014-08-06 10:59:21 -0700 Committed r172158: <http://trac.webkit.org/changeset/172158> 235433 2014-07-24 08:47:31 -0700 2014-11-12 14:45:52 -0800 Patch bug-130702-20140724084717.patch text/plain 5589 burg U3VidmVyc2lvbiBSZXZpc2lvbjogMTcxMjQ3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVj dG9yVUkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwppbmRleCBi MDk1NjcwNzNmNGVhMDMyMmI3M2VkNzYwYmViODNhMWM4MjFiY2Q4Li5kMTY2ZDQ3N2U5NjI1MjU3 ZGVkMDA5M2ZlZjJjMjIxZjE2MGE0OTViIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOSBAQAorMjAxNC0wNy0yNCAgQnJpYW4gSi4gQnVyZyAgPGJ1cmdAY3Mud2FzaGluZ3Rv bi5lZHU+CisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogcHJvdG9jb2wgY29tbWFuZCBpbnZvY2F0 aW9ucyBzaG91bGQgcmV0dXJuIGEgcHJvbWlzZSBpZiBubyBjYWxsYmFjayBpcyBzdXBwbGllZAor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTMwNzAyCisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhpcyBhbGxv d3MgdGhlIHRyYWlsaW5nIEFnZW50LmNvbW1hbmQucHJvbWlzZShhcmdzKSB0byBiZSBkcm9wcGVk IGluIGZhdm9yIG9mIGp1c3QKKyAgICAgICAgQWdlbnQuY29tbWFuZChhcmdzKS4gSXQgc2hvdWxk IG1ha2UgaXQgYSBiaXQgZWFzaWVyIHRvIGNvbnZlcnQgY29kZSB0byB1c2UgcHJvbWlzZXMuCisK KyAgICAgICAgVGVzdDogTGF5b3V0VGVzdHMvaW5zcGVjdG9yL3Byb3RvY29sLXByb21pc2UtcmVz dWx0Lmh0bWwKKworICAgICAgICAqIFVzZXJJbnRlcmZhY2UvUHJvdG9jb2wvSW5zcGVjdG9yQmFj a2VuZC5qczoKKyAgICAgICAgKC5jYWxsYWJsZSk6IFJlZGlyZWN0IHRvIHRoZSBwcm9taXNlIGVu dHJ5IHBvaW50IGlmIHRoZSBsYXN0IGFyZ3VtZW50IGlzbid0IGEgZnVuY3Rpb24uCisgICAgICAg IChJbnNwZWN0b3JCYWNrZW5kLkNvbW1hbmQuY3JlYXRlKToKKwogMjAxNC0wNy0yMyAgQnJpYW4g Si4gQnVyZyAgPGJ1cmdAY3Mud2FzaGluZ3Rvbi5lZHU+CiAKICAgICAgICAgV2ViIEluc3BlY3Rv cjogYWRkIGVqZWN0IGN1cnJlbnQgcmVjb3JkaW5nIGJ1dHRvbiB0byB0aGUgcmVwbGF5IGRhc2hi b2FyZApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvUHJv dG9jb2wvSW5zcGVjdG9yQmFja2VuZC5qcyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50 ZXJmYWNlL1Byb3RvY29sL0luc3BlY3RvckJhY2tlbmQuanMKaW5kZXggZjU3YzE1ZmJhNmQ1Mzcz NjEwOTYyNDBlOGFjYTMzNDcwZDJiNzc2Zi4uMzRhZjMzOGU0MWFiZDJhOTNiYzhmNDI1ZGY0ZTg4 YWZhMzFhZTM5YiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZh Y2UvUHJvdG9jb2wvSW5zcGVjdG9yQmFja2VuZC5qcworKysgYi9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvVXNlckludGVyZmFjZS9Qcm90b2NvbC9JbnNwZWN0b3JCYWNrZW5kLmpzCkBAIC0zMjUsNyAr MzI1LDEyIEBAIEluc3BlY3RvckJhY2tlbmQuQ29tbWFuZC5jcmVhdGUgPSBmdW5jdGlvbihiYWNr ZW5kLCBjb21tYW5kTmFtZSwgY2FsbFNpZ25hdHVyZSwKICAgICB2YXIgaW5zdGFuY2UgPSBuZXcg SW5zcGVjdG9yQmFja2VuZC5Db21tYW5kKGJhY2tlbmQsIGNvbW1hbmROYW1lLCBjYWxsU2lnbmF0 dXJlLCByZXBseVNpZ25hdHVyZSk7CiAKICAgICBmdW5jdGlvbiBjYWxsYWJsZSgpIHsKLSAgICAg ICAgaW5zdGFuY2UuX2ludm9rZVdpdGhBcmd1bWVudHMuYXBwbHkoaW5zdGFuY2UsIGFyZ3VtZW50 cyk7CisgICAgICAgIC8vIElmIHRoZSBsYXN0IGFyZ3VtZW50IHRvIHRoZSBjb21tYW5kIGlzIG5v dCBhIGZ1bmN0aW9uLCByZXR1cm4gYSByZXN1bHQgcHJvbWlzZS4KKyAgICAgICAgdmFyIGNvbW1h bmRBcmd1bWVudHMgPSBBcnJheS5wcm90b3R5cGUuc2xpY2UuY2FsbChhcmd1bWVudHMpOworICAg ICAgICBpZiAodHlwZW9mIGNvbW1hbmRBcmd1bWVudHMubGFzdFZhbHVlICE9PSAiZnVuY3Rpb24i KQorICAgICAgICAgICAgcmV0dXJuIGluc3RhbmNlLnByb21pc2UuYXBwbHkoaW5zdGFuY2UsIGFy Z3VtZW50cyk7CisKKyAgICAgICAgcmV0dXJuIGluc3RhbmNlLl9pbnZva2VXaXRoQXJndW1lbnRz LmFwcGx5KGluc3RhbmNlLCBhcmd1bWVudHMpOwogICAgIH0KICAgICBjYWxsYWJsZS5faW5zdGFu Y2UgPSBpbnN0YW5jZTsKICAgICBjYWxsYWJsZS5fX3Byb3RvX18gPSBJbnNwZWN0b3JCYWNrZW5k LkNvbW1hbmQucHJvdG90eXBlOwpkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nIGIv TGF5b3V0VGVzdHMvQ2hhbmdlTG9nCmluZGV4IDk1NDU2YWQ1ODVlYTBlZGRiMTk2ZGViOWE3YTBh NTNkMzA4Y2QzZGEuLmE2NDI2MzY5YmE1ODFhNzBhYjY3YjQyNDdkNmM4ODVkYTM1OTFjYTAgMTAw NjQ0Ci0tLSBhL0xheW91dFRlc3RzL0NoYW5nZUxvZworKysgYi9MYXlvdXRUZXN0cy9DaGFuZ2VM b2cKQEAgLTEsMyArMSwxNiBAQAorMjAxNC0wNy0yNCAgQnJpYW4gSi4gQnVyZyAgPGJ1cmdAY3Mu d2FzaGluZ3Rvbi5lZHU+CisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogcHJvdG9jb2wgY29tbWFu ZCBpbnZvY2F0aW9ucyBzaG91bGQgcmV0dXJuIGEgcHJvbWlzZSBpZiBubyBjYWxsYmFjayBpcyBz dXBwbGllZAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTMwNzAyCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg QWRkZCBhIHRlc3QgZm9yIHJlY2lldmluZyBwcm90b2NvbCBjb21tYW5kIHJlc3VsdHMgdGhyb3Vn aCBhbiBleHBsaWNpdCBjYWxsYmFjaywKKyAgICAgICAgdmlhIHRoZSAucHJvbWlzZSgpIGVudHJ5 IHBvaW50LCBhbmQgdmlhIGFuIGltcGxpY2l0bHkgY3JlYXRlZCBwcm9taXNlLgorCisgICAgICAg ICogaW5zcGVjdG9yL3Byb3RvY29sLXByb21pc2UtcmVzdWx0LWV4cGVjdGVkLnR4dDogQWRkZWQu CisgICAgICAgICogaW5zcGVjdG9yL3Byb3RvY29sLXByb21pc2UtcmVzdWx0Lmh0bWw6IEFkZGVk LgorCiAyMDE0LTA3LTIzICBCcmlhbiBKLiBCdXJnICA8YnVyZ0Bjcy53YXNoaW5ndG9uLmVkdT4K IAogICAgICAgICBXZWIgSW5zcGVjdG9yOiBJbnNwZWN0b3JCYWNrZW5kJ3MgcHJvbWlzZS1iYXNl ZCBhZ2VudCBBUEkgZG9lcyBub3Qgc3VwcG9ydCBtdWx0aXBsZSByZXR1cm4gdmFsdWVzCmRpZmYg LS1naXQgYS9MYXlvdXRUZXN0cy9pbnNwZWN0b3IvcHJvdG9jb2wtcHJvbWlzZS1yZXN1bHQtZXhw ZWN0ZWQudHh0IGIvTGF5b3V0VGVzdHMvaW5zcGVjdG9yL3Byb3RvY29sLXByb21pc2UtcmVzdWx0 LWV4cGVjdGVkLnR4dApuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwLi4xZWI3YTUzOThiYTZkZWIzYWRkMDEyZGUxMzA1NmQx Y2NmMWQwM2I4Ci0tLSAvZGV2L251bGwKKysrIGIvTGF5b3V0VGVzdHMvaW5zcGVjdG9yL3Byb3Rv Y29sLXByb21pc2UtcmVzdWx0LWV4cGVjdGVkLnR4dApAQCAtMCwwICsxLDcgQEAKK1Rlc3Rpbmcg dGhhdCB0aGUgaW5zcGVjdG9yIGJhY2tlbmQgY2FuIHJldHVybiBjb21tYW5kIHJlc3VsdHMgdXNp bmcgcHJvbWlzZXMuCisKK1VzaW5nIGEgY2FsbGJhY2ssIGdvdCBSdW50aW1lQWdlbnQuZXZhbHVh dGUoKSByZXN1bHQ6IDQxCitTZXR0aW5nIHVwIHR3byBwcm9taXNlcyBmb3IgUnVudGltZUFnZW50 LmV2YWx1YXRlLgorVXNpbmcgYSBwcm9taXNlLCBnb3QgUnVudGltZUFnZW50LmV2YWx1YXRlKCkg cmVzdWx0OiA0MgorVXNpbmcgYSBwcm9taXNlLCBnb3QgUnVudGltZUFnZW50LmV2YWx1YXRlKCkg cmVzdWx0OiA0MworCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9pbnNwZWN0b3IvcHJvdG9jb2wt cHJvbWlzZS1yZXN1bHQuaHRtbCBiL0xheW91dFRlc3RzL2luc3BlY3Rvci9wcm90b2NvbC1wcm9t aXNlLXJlc3VsdC5odG1sCm5ldyBmaWxlIG1vZGUgMTAwNjQ0CmluZGV4IDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAuLjUyZGM3MDZmMWE0YmZiODg3N2U0YWQ4MDY5ZDg3 YWM1OWFmNzJmZGQKLS0tIC9kZXYvbnVsbAorKysgYi9MYXlvdXRUZXN0cy9pbnNwZWN0b3IvcHJv dG9jb2wtcHJvbWlzZS1yZXN1bHQuaHRtbApAQCAtMCwwICsxLDM4IEBACis8IWRvY3R5cGUgaHRt bD4KKzxodG1sPgorPGhlYWQ+Cis8c2NyaXB0IHR5cGU9InRleHQvamF2YXNjcmlwdCIgc3JjPSIu Li9odHRwL3Rlc3RzL2luc3BlY3Rvci9pbnNwZWN0b3ItdGVzdC5qcyI+PC9zY3JpcHQ+Cis8c2Ny aXB0PgorZnVuY3Rpb24gdGVzdCgpCit7CisgICAgdmFyIGMxID0gUnVudGltZUFnZW50LmV2YWx1 YXRlKCI0MSIsIGZ1bmN0aW9uKGVyciwgcmVzdWx0LCB3YXNUaHJvd24pIHsKKyAgICAgICAgSW5z cGVjdG9yVGVzdC5sb2coIlVzaW5nIGEgY2FsbGJhY2ssIGdvdCBSdW50aW1lQWdlbnQuZXZhbHVh dGUoKSByZXN1bHQ6ICIgKyByZXN1bHQudmFsdWUpOworCisgICAgICAgIEluc3BlY3RvclRlc3Qu bG9nKCJTZXR0aW5nIHVwIHR3byBwcm9taXNlcyBmb3IgUnVudGltZUFnZW50LmV2YWx1YXRlLiIp OworCisgICAgICAgIHZhciBwMSA9IFJ1bnRpbWVBZ2VudC5ldmFsdWF0ZS5wcm9taXNlKCI0MiIp OworICAgICAgICB2YXIgcDIgPSBSdW50aW1lQWdlbnQuZXZhbHVhdGUoIjQzIik7CisKKyAgICAg ICAgSW5zcGVjdG9yVGVzdC5hc3NlcnQoIShjMSBpbnN0YW5jZW9mIFByb21pc2UpLCAiQSBwcm9t aXNlIHdhcyByZXR1cm5lZCBldmVuIHRob3VnaCBhIGNhbGxiYWNrIHdhcyBzdXBwbGllZCB0byB0 aGUgY29tbWFuZCEiKTsKKworICAgICAgICBwMS50aGVuKGZ1bmN0aW9uKHBheWxvYWQpIHsKKyAg ICAgICAgICAgIEluc3BlY3RvclRlc3QubG9nKCJVc2luZyBhIHByb21pc2UsIGdvdCBSdW50aW1l QWdlbnQuZXZhbHVhdGUoKSByZXN1bHQ6ICIgKyBwYXlsb2FkLnJlc3VsdC52YWx1ZSk7CisgICAg ICAgIH0pOworICAgICAgICBwMi50aGVuKGZ1bmN0aW9uKHBheWxvYWQpIHsKKyAgICAgICAgICAg IEluc3BlY3RvclRlc3QubG9nKCJVc2luZyBhIHByb21pc2UsIGdvdCBSdW50aW1lQWdlbnQuZXZh bHVhdGUoKSByZXN1bHQ6ICIgKyBwYXlsb2FkLnJlc3VsdC52YWx1ZSk7CisgICAgICAgIH0pOwor CisgICAgICAgIFByb21pc2UuYWxsKFtwMSwgcDJdKS50aGVuKGZ1bmN0aW9uKCkgeworICAgICAg ICAgICAgSW5zcGVjdG9yVGVzdC5jb21wbGV0ZVRlc3QoKTsKKyAgICAgICAgfSkuY2F0Y2goZnVu Y3Rpb24oZXJyb3IpIHsKKyAgICAgICAgICAgIEluc3BlY3RvclRlc3QubG9nKCJFUlJPUjogIiAr IGVycm9yKTsKKyAgICAgICAgICAgIEluc3BlY3RvclRlc3QuY29tcGxldGVUZXN0KCk7CisgICAg ICAgIH0pCisgICAgfSk7Cit9Cis8L3NjcmlwdD4KKzwvaGVhZD4KKzxib2R5IG9ubG9hZD0icnVu VGVzdCgpIj4KKyAgICA8cD5UZXN0aW5nIHRoYXQgdGhlIGluc3BlY3RvciBiYWNrZW5kIGNhbiBy ZXR1cm4gY29tbWFuZCByZXN1bHRzIHVzaW5nIHByb21pc2VzLjwvcD4KKzwvYm9keT4KKzwvaHRt bD4K