Bug 26350 – Make WebInspector's console evaluation/completion asynchronous.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

This way of evaluation allows serializing calls between InspectorController and its frontend.

[Pavel Feldman]

Created attachment 31206 [details]
patch

[Timothy Hatcher]

Comment on attachment 31206 [details]
patch

The code looks correct. I think this just gets ready for asynchronous evaluation, but the callback is still called synchronously. Is that correct?

Maybe use setTimeout with a zero/quick delay to simulate asynchronous behaviour.

One concern I have with the completion code being asynchronous is if the user keeps typing and the completions for the previous range have not been returned yet. When the results are returned to the callback, the user typed text might get messed up or mangled in some way.

So I think completion results need to be ignored if more user typing has happened since. Make sense?

[Pavel Feldman]

Created attachment 31232 [details]
patch

Thanks for the review!

(In reply to comment #2)
> (From update of attachment 31206 [details] [review])
> The code looks correct. I think this just gets ready for asynchronous
> evaluation, but the callback is still called synchronously. Is that correct?
> 

Calls receiving callbacks are always the last ones in the methods, so that it should not really matter. However you are right - putting these into SetTimeout would give better guarantees. This is now done.

> Maybe use setTimeout with a zero/quick delay to simulate asynchronous
> behaviour.
> 

Done.

> One concern I have with the completion code being asynchronous is if the user
> keeps typing and the completions for the previous range have not been returned
> yet. When the results are returned to the callback, the user typed text might
> get messed up or mangled in some way.
> 
> So I think completion results need to be ignored if more user typing has
> happened since. Make sense?
> 

I've added post-evaluation filtering into completionReady, so that only the ones that do match currently added query are suggested.

I've also extracted couple of methods in the code: 
- installation of the console API 
    (just for convenience)
- Console::doEvalInWindow and ScriptsPanel::doEvalInCallFrame 
    (these are supposed to do the actual eval work over the serialization layer).

[Timothy Hatcher]

Comment on attachment 31232 [details]
patch

> +        setTimeout(function() {
> +            completionsReadyCallback(results);
> +        }, 0);

This can just be written as:

setTimeout(completionsReadyCallback, 0, results);

> +    _ensureCommandLineAPIInstalled: function(inspectedWindow) {
> +    doEvalInWindow: function(expression, callback) {
> +        function printResult(result, exception) {
> +        function updatingCallbackWrapper(result) {

The open brace should be on the next line for these.

> +        setTimeout(function() {
> +            var inspectedWindow = InspectorController.inspectedWindow();
> +            self._ensureCommandLineAPIInstalled(inspectedWindow);
> +            try {
> +                callback(inspectedWindow.eval(expression));
> +            } catch (e) {
> +                callback(e, true);
> +            }
> +        }, 0);

I think it would be cleaner to define a nested function then call setTimeout on it, not using an anonymous function inline.

> +        setTimeout(function() {
> +            try {
> +                callback(callFrame.evaluate(code));
> +            } catch (e) {
> +                callback(e, true);
> +            }
> +        }, 0);

Same here.

> -
> + 

Extra space added here.

> +        var currentText = fullWordRange.toString();
> +
> +        var matchingCompletions = []
> +        for (var i = 0; i < completions.length; ++i) {
> +            if (completions[i].indexOf(currentText) == 0)
> +                matchingCompletions.push(completions[i]);
> +        }  
> +        completions = matchingCompletions;
> +
> +        if (!completions || !completions.length)
> +            return;

Does this work when case dosen't match? The completions are found case-insenitvly and indexOf is case-sensitive.

I think a better way to detect instead of looking at all the completion prefixes, would be to get the word range that was present when completions was made. If the word range matches the current fullWordRange, then it is the right one. Otherwise return early when there isn't a match and don't call this.clearAutoComplete in that case.

How does that sound to you?

[Pavel Feldman]

Created attachment 31254 [details]
patch

(In reply to comment #4)
> (From update of attachment 31232 [details] [review])
> 
> This can just be written as:
> 
> setTimeout(completionsReadyCallback, 0, results);
> 

Done.

> 
> The open brace should be on the next line for these.
>

Done.
 
> 
> I think it would be cleaner to define a nested function then call setTimeout on
> it, not using an anonymous function inline.
>

Done.
 
> 
> Same here.
> 

Done.

> > -
> > + 
> 
> Extra space added here.
>

Not sure which one you mean. How do I find exact line number?
 
> 
> Does this work when case dosen't match? The completions are found
> case-insenitvly and indexOf is case-sensitive.
>

In fact, I think that completions are also case-sensitive. At least that is what I've learned from playing with the tool.
 
> I think a better way to detect instead of looking at all the completion
> prefixes, would be to get the word range that was present when completions was
> made. If the word range matches the current fullWordRange, then it is the right
> one. Otherwise return early when there isn't a match and don't call
> this.clearAutoComplete in that case.
>

Done. Regardless of case sensitivity I could do it this way.

[Pavel Feldman]

Created attachment 31255 [details]
patch

Removed extra spaces in blank lines.

[Eric Seidel (no email)]

Sigh, my script almost landed this correctly.

Anyway, it's in as r44701.