Bug 26350 - Make WebInspector's console evaluation/completion asynchronous.
: Make WebInspector's console evaluation/completion asynchronous.
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-06-12 12:35 PST by
Modified: 2009-06-15 18:14 PST (History)


Attachments
patch (10.25 KB, patch)
2009-06-12 12:39 PST, Pavel Feldman
no flags Review Patch | Details | Formatted Diff | Diff
patch (13.37 KB, patch)
2009-06-13 04:17 PST, Pavel Feldman
timothy: review-
Review Patch | Details | Formatted Diff | Diff
patch (12.99 KB, patch)
2009-06-14 00:33 PST, Pavel Feldman
no flags Review Patch | Details | Formatted Diff | Diff
patch (12.57 KB, patch)
2009-06-14 00:37 PST, Pavel Feldman
timothy: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-06-12 12:35:46 PST
This way of evaluation allows serializing calls between InspectorController and its frontend.
------- Comment #1 From 2009-06-12 12:39:29 PST -------
Created an attachment (id=31206) [details]
patch
------- Comment #2 From 2009-06-12 14:17:40 PST -------
(From update of attachment 31206 [details])
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?
------- Comment #3 From 2009-06-13 04:17:30 PST -------
Created an attachment (id=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).
------- Comment #4 From 2009-06-13 14:01:39 PST -------
(From update of attachment 31232 [details])
> +        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?
------- Comment #5 From 2009-06-14 00:33:32 PST -------
Created an attachment (id=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.
------- Comment #6 From 2009-06-14 00:37:46 PST -------
Created an attachment (id=31255) [details]
patch

Removed extra spaces in blank lines.
------- Comment #7 From 2009-06-15 18:14:19 PST -------
Sigh, my script almost landed this correctly.

Anyway, it's in as r44701.