Bug 26350 - Make WebInspector's console evaluation/completion asynchronous.
: Make WebInspector's console evaluation/completion asynchronous.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-12 12:35 PDT by Pavel Feldman
Modified: 2009-06-15 18:14 PDT (History)
1 user (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2009-06-12 12:35:46 PDT
This way of evaluation allows serializing calls between InspectorController and its frontend.
Comment 1 Pavel Feldman 2009-06-12 12:39:29 PDT
Created attachment 31206 [details]
patch
Comment 2 Timothy Hatcher 2009-06-12 14:17:40 PDT
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?
Comment 3 Pavel Feldman 2009-06-13 04:17:30 PDT
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).
Comment 4 Timothy Hatcher 2009-06-13 14:01:39 PDT
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?
Comment 5 Pavel Feldman 2009-06-14 00:33:32 PDT
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.
Comment 6 Pavel Feldman 2009-06-14 00:37:46 PDT
Created attachment 31255 [details]
patch

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

Anyway, it's in as r44701.