| Summary: | Web Inspector: protocol command invocations should return a promise if no callback is supplied | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | BJ Burg <bburg> | ||||
| Component: | Web Inspector | Assignee: | Brian Burg <burg> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | burg, graouts, joepeck, timothy, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Attachments: |
|
||||||
|
Description
BJ Burg
2014-03-24 16:24:31 PDT
Created attachment 235433 [details]
Patch
Comment on attachment 235433 [details] 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]". Comment on attachment 235433 [details] 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 Comment on attachment 235433 [details] 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). Comment on attachment 235433 [details] 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. Comment on attachment 235433 [details] 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. Committed r172158: <http://trac.webkit.org/changeset/172158> |