Bug 130702 – Web Inspector: protocol command invocations should return a promise if no callback is supplied

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

.

[Radar WebKit Bug Importer]

<rdar://problem/16412909>

[Brian Burg]

Created attachment 235433 [details]
Patch

[Joseph Pecoraro]

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]".

[Joseph Pecoraro]

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

[Brian Burg]

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

[Timothy Hatcher]

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.

[Timothy Hatcher]

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.

[Brian Burg]

Committed r172158: <http://trac.webkit.org/changeset/172158>