Bug 150493 – REGRESSION (r191419): Web Inspector: Autocomplete is broken

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

[Nikita Vasilyev]

Created attachment 263909 [details]
[Animated GIF] Bug

See the attached animated GIF. I attempted to type "color:blue". It's a recent regression.

[Radar WebKit Bug Importer]

<rdar://problem/23231704>

[Devin Rousso]

This is a regression from r191419  (https://bugs.webkit.org/show_bug.cgi?id=147720)

[Nikita Vasilyev]

I unrolled r191419 in bug 150537.

[Devin Rousso]

Created attachment 288002 [details]
Patch

[Blaze Burg]

Comment on attachment 288002 [details]
Patch

LGTM but I want Joe to take a look as well.

We really, really need test coverage in this area as it's so complicated.

[Devin Rousso]

(In reply to comment #5)
> Comment on attachment 288002 [details]
> Patch
> 
> LGTM but I want Joe to take a look as well.
> 
> We really, really need test coverage in this area as it's so complicated.

Agreed.  Does your view testing framework include simulating keyboard events?  Should we try using WebDriver (https://webkit.org/blog/6900/webdriver-support-in-safari-10/)?

[Blaze Burg]

(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 288002 [details]
> > Patch
> > 
> > LGTM but I want Joe to take a look as well.
> > 
> > We really, really need test coverage in this area as it's so complicated.
> 
> Agreed.  Does your view testing framework include simulating keyboard
> events?

Yes.

> Should we try using WebDriver
> (https://webkit.org/blog/6900/webdriver-support-in-safari-10/)?

No, because that depends on Safari and doesn't fit well with our test runner architecture. The API is very similar though and will eventually use the same UIProcess methods to simulate user actions.

More details soon!

[Blaze Burg]

<rdar://problem/28175392>

[Joseph Pecoraro]

Comment on attachment 288002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288002&action=review

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorCompletionController.js:80
> +        this._currentCompletion = null;
> +        this._ignoreChange = false;
> +        this._ignoreNextCursorActivity = false;
> +        this._ignoreNextUndo = false;
> +        this._inCompletionActivity = false;

Thanks for declaring these in the constructor. This is quite a list of side states that can easily get mishandled. I really hope we understand when all of these get set/cleared. It just sounds like it would be very easy to end up in a state where oops we have some _ignore set to true that never gets cleared.

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorCompletionController.js:518
>          if (!force && !backwardScanResult.string && (!baseScanResult || !baseScanResult.string)) {
>              this.hideCompletions();
> +            this._inCompletionActivity = false;
>              return;
>          }

There is a return below, for extendedCompletionsProvider, that doesn't clear _inCompletionActivity.

This boolean seems to have far too many places where it is cleared. This really concerns me. It would be easy to miss one if something is changed in the future. There should be a much clearer entry / exit for this state so we don't need to clear it in ~8 places. Right now it is too complex to follow.

r- for now because I can't be certain this doesn't introduce another subtle bug.

Is there any reason that _applyCompletionHint can't set the state and clear the state when it is done? Can we create a clearer state machine for CompletionsController?

[Devin Rousso]

Comment on attachment 288002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288002&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorCompletionController.js:80
>> +        this._inCompletionActivity = false;
> 
> Thanks for declaring these in the constructor. This is quite a list of side states that can easily get mishandled. I really hope we understand when all of these get set/cleared. It just sounds like it would be very easy to end up in a state where oops we have some _ignore set to true that never gets cleared.

Yeah that was mainly the motivation for moving these into the constructor.  AFAICT, each of them gets cleared, but there may be edgecases :(

>> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorCompletionController.js:518
>>          }
> 
> There is a return below, for extendedCompletionsProvider, that doesn't clear _inCompletionActivity.
> 
> This boolean seems to have far too many places where it is cleared. This really concerns me. It would be easy to miss one if something is changed in the future. There should be a much clearer entry / exit for this state so we don't need to clear it in ~8 places. Right now it is too complex to follow.
> 
> r- for now because I can't be certain this doesn't introduce another subtle bug.
> 
> Is there any reason that _applyCompletionHint can't set the state and clear the state when it is done? Can we create a clearer state machine for CompletionsController?

So the idea behind _inCompletionActivity is that due to what CodeMirror.changeGeneration(true) does.  Basically (assuming I am understanding it right), it tells the history stack to "freeze" itself and start making new history entries from this point forwards.  This is necessary because if you rapidly type "color: b" without using the autocomplete, when it next displays the completion popover, it will attempt to undo, meaning that the entire change since the last undo (which is "r: ") will also be removed, leaving "colob" (or something close to that).  We can't call CodeMirror.changeGeneration(true) too often or literally each change becomes its own history event.  So, _inCompletionActivity is a flag that will track when to call CodeMirror.changeGeneration(true), that being when a new completion is about to be shown and you haven't shown one in a while.

Regardless, I think you are right in the issue with extendedCompletionsProvider (which is only JavaScriptRuntimeCompletionProvider AFAICT)     ( ͡° ͜ʖ ͡°)

[Blaze Burg]

Comment on attachment 288002 [details]
Patch

Does this patch need any more work before review?

[Devin Rousso]

(In reply to comment #11)
> Comment on attachment 288002 [details]
> Patch
> 
> Does this patch need any more work before review?

I don't think so.  There may be some work regarding `extendedCompletionsProvider`, but I haven't had a chance to really test it (lots of midterms/projects).

[Matt Baker]

Devin, does this patch still fix something not resolved by rolling out r191419? I couldn't reproduce the bug.

[Devin Rousso]

(In reply to comment #13)
> Devin, does this patch still fix something not resolved by rolling out
> r191419? I couldn't reproduce the bug.

I probably should have said this, but this bug was reopened from a past issue (change r191419).  Nikita fixed the issue by rolling out r191419, but the major problem remained.  The issue was that I changed the way autocompletion suggestions work, going from actually adding the text with a different color to using a CodeMirror marker.  A better description is in my comment at <https://webkit.org/b/147975#c3>.  The changes I added in the patch on 09/06 was to correctly resolve the problem described there.

As far as more work, I don't think there is any work left to do (I will check for sure this weekend and get back), but I did respond to Joe's comments and he may want to comment on them.  I am not sure if my solution is the best, but it is all I could think of, so any other reviews are welcome :)

[Devin Rousso]

Created attachment 292564 [details]
Patch

[Build Bot]

Comment on attachment 292564 [details]
Patch

Attachment 292564 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2353708

New failing tests:
svg/wicd/test-rightsizing-a.xhtml

[Build Bot]

Created attachment 292568 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Devin Rousso]

Created attachment 292998 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 292998 [details]
Patch

Clearing flags on attachment: 292998

Committed r208248: <http://trac.webkit.org/changeset/208248>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.