790016 - Web console remote protocol sends more data than needed for completion

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P2)

Description

[Jim Blandy :jimb]

Under the protocol implemented in bug 768096, as a user types an identifier, the web console repeatedly sends the current prefix to the server, and the server replies with a list of possible completions for that prefix.

However, as a user types more characters onto the end of an identifier, the new completions are always a subset of the completions for the shorter prefix: adding characters narrows the possibilities. So the server has already sent the client, in its prior "autocomplete" replies, all the information the client needs.

I don't know whether this is a real performance problem, but I thought it might be nice for the client to avoid re-requesting completions unnecessarily.

Here's a log of the communication that makes the problem clear; look for replies to "autocomplete" requests.
https://bugzilla.mozilla.org/attachment.cgi?id=655956

Comment 1

[Christos Stathis]

Hello,

I am interested in working on this bug (actually, I have already done some work on it). Any advice will be welcome.

Thanks

Comment 2

[Mihai Sucan [:msucan]]

Hello Christos! Thanks for your interest in working on this bug.

I am not sure how much experience you have with the Firefox codebase so I will start with an overview of the workflow:

1. Get the firefox source:

https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/Mercurial

2. Build firefox:

https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

3. Make a patch:

https://developer.mozilla.org/en-US/docs/Creating_a_patch

4. Submit the patch:

https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

If you read through the source code about something you do not know about, you may find documentation here:

1. Mozilla Developer Network - has a ton of info about XUL elements, HTML, JS, DOM, Gecko-specific APIs and more.

http://developer.mozilla.org/

2. MXR (Mozilla Cross-Reference) is a source code search engine - search for symbols you want to learn about, eg. nsIDocument.

http://mxr.mozilla.org/mozilla-central/

3. DXR is a smart source code search tool, similar to MXR but sometimes better.

http://dxr.mozilla.org


Here's an overview of the code you will work with:

1. The webconsole server actor is found at:

toolkit/devtools/server/actors/webconsole.js

Search for onAutocomplete().

2. The webconsole client UI is found at:

browser/devtools/webconsole/webconsole.js

Search for JSTF_complete().

3. The autocomplete logic is found in:

toolkit/devtools/webconsole/WebConsoleUtils.jsm

Search for JSPropertyProvider.

To fix this bug you need to cache the autocomplete results received from the server. Then when the input is updated and another request for completion happens (when complete() is called) you do not call the server-side completion code - instead you take the cached results from the server and you filter through them based on the updated input value. You show only the results that match the new input. If the user updates the input to hold a different value that is not a subset of the previous value, then you invalidate the cache and continue as usual - request completion results from the server.

This is only the general idea, but it needs more work: for example you need to update the completion whenever a dot is typed (eg. foo.bar...). We need some logic to determine when to invalidate the cache - still we want to keep it simple enough.

You will mostly work with JST__updateCompletionResult() and JST__receiveAutocompleteProperties() in the browser/devtools/webconsole/webconsole.js.

Please let me know if you have any other questions. Thanks!

Comment 3

[Christos Stathis]

Attachment: First attempt to use cached autocomplete results

Comment 4

[Christos Stathis]

Hello Mihai and thanks for the guidance. This is my second bug so I already know some of the basic stuff. I used to work by cloning the fx-team repo instead of the mozilla-central. Would that be a problem ?

I attached a first attempt on this bug where I filter the results that are already in the autocomplete popup when the current input string starts with the previous input string and it does not end with '.'. In all other cases the code should work as before.

I would also need some help with the tests. I am not quite sure how to test it.

Thanks

Comment 5

[Mihai Sucan [:msucan]]

I tested the patch. Thank you!

If I type "doc" it autocompletes "document" without pressing Tab. Is that expected? I also don't see any properties when I type "window.d".

To write a test please look at: https://developer.mozilla.org/en-US/docs/Browser_chrome_tests

Web console tests live in browser/devtools/webconsole/test. Just copy one of the existing tests, rename, make test changes as needed, add the new test in Makefile.in, then run mach to get an updated build, then run the test with mach as explained in the aforementioned page. I suggest you pick one of the existing autocomplete tests and make the changes needed for this test.

Maybe the way you can test this is by doing something as follows:

1. type "window." in the jsterm input.
2. check that the list does not contain the "docfoobarz" property (or something else).
3. add the "docfoobarz" property to window.
4. type "doc".
5. check that the list still doesn't include the new property.

Currently the list is always updated as you type. With this patch the list updates less often. Thanks!

Comment 6

[Christos Stathis]

Attachment: Updated patch that fixed problems reported in comment 5

Hello Mihai! I think the updated patch fixes the two issues that you reported. I 'll see what I can do about the tests and I 'll send a complete patch ASAP.
Thanks

Comment 7

[Christos Stathis]

Attachment: Updated patch with test

Comment 8

[Mihai Sucan [:msucan]]

Comment on attachment 756781 [details] [diff] [review]
Updated patch with test

Review of attachment 756781 [details] [diff] [review]:
-----------------------------------------------------------------

This is working well for me now. Thank you!

General thoughts:

1. I think that the code should also check if the autocompletePopup is open and that it has some items inside, before it tries to take a shortcut. You currently check for endsWith() and that makes sense, and for startsWith(). Please also add a check for popup.isOpe and for the items count.

Why I want this: the code assume that an autocomplete has happened, while that may not always be true.

2. currently any backspace/delete is considered as an entirely new input. Could you also look into avoiding to ask the server for completions when we press backspace/delete? For example if I type 'document.add' then backspace twice, then 'c' we should not ask the server for completion results - we could keep the suggestions a bit longer. 

Make sure you have devtools.debugger.log turned on to see the messages exchanged between the debugger client and server.

I have some minor comments below.

::: browser/devtools/webconsole/test/Makefile.in
@@ +131,5 @@
>  	browser_console_native_getters.js \
>  	browser_bug_871156_ctrlw_close_tab.js \
>  	browser_console_private_browsing.js \
>  	head.js \
> +	browser_webconsole_bug_790016_cached_autocomplete.js \

nit: please put new tests before head.js.

::: browser/devtools/webconsole/test/browser_webconsole_bug_790016_cached_autocomplete.js
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const TEST_URI = "data:text/html;charset=utf8,<p>test cached autocompletion results";

Please also add a comment before the const that shortly describes the purpose of the test.

@@ +38,5 @@
> +  let items = popup.getItems().reverse().map(function(e) {return e.label;});
> +  ok(items.every(function(prop, index) {
> +        return (prop != 'docfoobar');}), "autocomplete results do not contain docfoobar");
> +
> +  window.docfoobar = true;

This test does not fail when I disable the caching in JST__updateCompletionResult().

|window| here refers to the top level browser chrome window, not to the tab content window object. I think you want |content.wrappedJSObject.docfoobar|.

@@ +41,5 @@
> +
> +  window.docfoobar = true;
> +
> +  // Test typing 'window.foo'.
> +  input.value = "window.d";

In the comment you mean you mean 'window.d'? That's what the code does.

@@ +52,5 @@
> +        return (prop != 'docfoobar');}), "autocomplete results still do not contain docfoobar. list has not been updated");
> +  delete window.docfoobar;
> +
> +  // ok(newItems.every(function(prop, index) {
> +  //  	   return (prop != 'foobar');}), "autocomplete results still do not contain foobar. list has not been updated");

Is this comment needed?

::: browser/devtools/webconsole/webconsole.js
@@ +4091,5 @@
>      let requestId = gSequenceId();
>      let input = this.inputNode.value;
> +
> +    /** 
> +     * If the current input starts with the previous input, then we already have a list of suggestions and we

coding style nit: please wrap at 80 chars.

(this comment applies to all lines in this patch)

@@ +4094,5 @@
> +    /** 
> +     * If the current input starts with the previous input, then we already have a list of suggestions and we
> +     * just need to filter it. Exception is when the current input ends with a '.' when we ask the server for suggestions
> +     */
> +    if (!input.endsWith('.') && this.lastCompletion.value && input.startsWith(this.lastCompletion.value)) {

coding style nit: please always use double quotes.

@@ +4096,5 @@
> +     * just need to filter it. Exception is when the current input ends with a '.' when we ask the server for suggestions
> +     */
> +    if (!input.endsWith('.') && this.lastCompletion.value && input.startsWith(this.lastCompletion.value)) {
> +      let filterBy = input;
> +      if (input.indexOf('.') > -1)

coding style nit: please always use curly braces around single-line expressions.

if (foo) {
  bar();
}

@@ +4112,5 @@
> +        requestId: null,
> +        completionType: aType,
> +        value: null,
> +      };
> +      

coding style nit: please make sure the patch does not add any lines with trailing whitespace.

@@ +4121,3 @@
>      let cursor = this.inputNode.selectionStart;
>  
>      // TODO: Bug 787986 - throttle/disable updates, deal with slow/high latency

Please remove this comment. I believe we are now also fixing bug 787986.

Comment 9

[Christos Stathis]

Attachment: Addressed most of the remarks in comment 8

I need to test it a bit more before requesting a review

Comment 10

[Christos Stathis]

(In reply to Mihai Sucan [:msucan] from comment #8)
> Comment on attachment 756781 [details] [diff] [review]
> Updated patch with test
> 
> Review of attachment 756781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is working well for me now. Thank you!
> 
> General thoughts:
> 
> 1. I think that the code should also check if the autocompletePopup is open
> and that it has some items inside, before it tries to take a shortcut. You
> currently check for endsWith() and that makes sense, and for startsWith().
> Please also add a check for popup.isOpe and for the items count.

I am not sure about that. Consider the following example: I type "w", the popup opens with "window" and "watch" as suggestions. The I type "i". The popup closes, so if I now type "n" the popup will be closed so a new unnecessary request to the server will be done. I handled the case a bit differently in combination with the solution to 2.
> 
> Why I want this: the code assume that an autocomplete has happened, while
> that may not always be true.
> 
> 2. currently any backspace/delete is considered as an entirely new input.
> Could you also look into avoiding to ask the server for completions when we
> press backspace/delete? For example if I type 'document.add' then backspace
> twice, then 'c' we should not ask the server for completion results - we
> could keep the suggestions a bit longer. 

This cannot be done if I use the popup itself as a cache. If I type 'document.add' and then backspace the previous results (for 'document.ad') are gone. So whenever I have results from the server I cache them in a separate list and filter this list according to the input. The check for the existence of this list and its length helps in the previous remark as well.

> 
> Make sure you have devtools.debugger.log turned on to see the messages
> exchanged between the debugger client and server.
> 
> I have some minor comments below.
> 
> ::: browser/devtools/webconsole/test/Makefile.in
> @@ +131,5 @@
> >  	browser_console_native_getters.js \
> >  	browser_bug_871156_ctrlw_close_tab.js \
> >  	browser_console_private_browsing.js \
> >  	head.js \
> > +	browser_webconsole_bug_790016_cached_autocomplete.js \
> 
> nit: please put new tests before head.js.
> 
> :::
> browser/devtools/webconsole/test/
> browser_webconsole_bug_790016_cached_autocomplete.js
> @@ +2,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +const TEST_URI = "data:text/html;charset=utf8,<p>test cached autocompletion results";
> 
> Please also add a comment before the const that shortly describes the
> purpose of the test.
> 
> @@ +38,5 @@
> > +  let items = popup.getItems().reverse().map(function(e) {return e.label;});
> > +  ok(items.every(function(prop, index) {
> > +        return (prop != 'docfoobar');}), "autocomplete results do not contain docfoobar");
> > +
> > +  window.docfoobar = true;
> 
> This test does not fail when I disable the caching in
> JST__updateCompletionResult().
> 
> |window| here refers to the top level browser chrome window, not to the tab
> content window object. I think you want |content.wrappedJSObject.docfoobar|.
> 
> @@ +41,5 @@
> > +
> > +  window.docfoobar = true;
> > +
> > +  // Test typing 'window.foo'.
> > +  input.value = "window.d";
> 
> In the comment you mean you mean 'window.d'? That's what the code does.
> 
> @@ +52,5 @@
> > +        return (prop != 'docfoobar');}), "autocomplete results still do not contain docfoobar. list has not been updated");
> > +  delete window.docfoobar;
> > +
> > +  // ok(newItems.every(function(prop, index) {
> > +  //  	   return (prop != 'foobar');}), "autocomplete results still do not contain foobar. list has not been updated");
> 
> Is this comment needed?
> 
> ::: browser/devtools/webconsole/webconsole.js
> @@ +4091,5 @@
> >      let requestId = gSequenceId();
> >      let input = this.inputNode.value;
> > +
> > +    /** 
> > +     * If the current input starts with the previous input, then we already have a list of suggestions and we
> 
> coding style nit: please wrap at 80 chars.
> 
> (this comment applies to all lines in this patch)
> 
> @@ +4094,5 @@
> > +    /** 
> > +     * If the current input starts with the previous input, then we already have a list of suggestions and we
> > +     * just need to filter it. Exception is when the current input ends with a '.' when we ask the server for suggestions
> > +     */
> > +    if (!input.endsWith('.') && this.lastCompletion.value && input.startsWith(this.lastCompletion.value)) {
> 
> coding style nit: please always use double quotes.
> 
> @@ +4096,5 @@
> > +     * just need to filter it. Exception is when the current input ends with a '.' when we ask the server for suggestions
> > +     */
> > +    if (!input.endsWith('.') && this.lastCompletion.value && input.startsWith(this.lastCompletion.value)) {
> > +      let filterBy = input;
> > +      if (input.indexOf('.') > -1)
> 
> coding style nit: please always use curly braces around single-line
> expressions.
> 
> if (foo) {
>   bar();
> }
> 
> @@ +4112,5 @@
> > +        requestId: null,
> > +        completionType: aType,
> > +        value: null,
> > +      };
> > +      
> 
> coding style nit: please make sure the patch does not add any lines with
> trailing whitespace.
> 
> @@ +4121,3 @@
> >      let cursor = this.inputNode.selectionStart;
> >  
> >      // TODO: Bug 787986 - throttle/disable updates, deal with slow/high latency
> 
> Please remove this comment. I believe we are now also fixing bug 787986.

Comment 11

[Mihai Sucan [:msucan]]

Thanks for the updated patch. Is this ready for review or you need to do further testing/work?

Comment 12

[Christos Stathis]

I discovered a couple of cases that the patch does not address. I am almost ready to upload another version for review.

Thanks

Comment 13

[Christos Stathis]

Attachment: Different approach that covers the backspace/delete cases

Comment 14

[Mihai Sucan [:msucan]]

Comment on attachment 765007 [details] [diff] [review]
Different approach that covers the backspace/delete cases

Thanks for the updated patch. There's still one remaining issue: if you type document.actifoo.activ you will see the activeElement suggestion. It seems that the "no suggestions" result from the server is not cached. Please fix this and I can r+ the patch.

Comment 15

[Christos Stathis]

The "no suggestions" result is not cached. I did this fix but now the "no suggestions" result means that the cache is empty, so I had to remove the check for a non empty cache. The updated patch checks only if there is an autocompletionQuery and if there is one, this means that we can use the cache even if it is empty.

Comment 16

[Christos Stathis]

Attachment: Fixed issue mentioned by Mihai in comment #14

Comment 17

[Mihai Sucan [:msucan]]

Comment on attachment 768608 [details] [diff] [review]
Fixed issue mentioned by Mihai in comment #14

Tested the patch and saw the following:

1. type |window.getC| then Tab. You should see 'getComputedStyle' being autocompleted, but I don't see this completion in the list of suggestions.

2. type |dump(d|. You should get more suggestions, but that doesn't work. Press Tab and nothing happens either.

Please do more testing before submitting the patch for review.

Comment 18

[Mihai Sucan [:msucan]]

You might want to always let the server request to happen if the input string ends with a non-alphanumeric character. Also, please let me know how I can help. Thank you!

Comment 19

[Christos Stathis]

Thanks. You are right about 2 (I didn't consider autocompletion after symbols like "("). Number 1 though is interesting because it should already work. Anyway, could you provide with "extreme" cases to test ? That would be very helpful.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

One thing I often do is add unit tests for all these corner cases that come up during review. Not all reviewers are as good as Mihai at finding bugs :-)

Comment 21

[Mihai Sucan [:msucan]]

(In reply to Christos Stathis from comment #19)
> Thanks. You are right about 2 (I didn't consider autocompletion after
> symbols like "("). Number 1 though is interesting because it should already
> work. Anyway, could you provide with "extreme" cases to test ? That would be
> very helpful.

I think Panos's suggestion to update the test to include all the corner cases that we discussed in this bug is the ideal approach. Can you please do this?

I didn't have any extreme/special tests to find the problems I mentioned in comment 17. I just spent a bit of time testing autocompletion, as a user. I tried different ways to write the same things, different objects and pages.

Thank you!

Comment 22

[Christos Stathis]

Trying to debug the "window.getC" issue, I noticed something weird. Without the patch, the request for "window." returns a list that does not contain "getComputedStyle". Then, when I type "g" the request for "window.g" returns a list that does contain "getComputedStyle" along with various entries starting with "g" that do not exist in the list for "window.". The patch 's logic is based on the assumption that the "window.g" list should always be a subset of the "window." list. Otherwise the caching will be problematic. Is there a chance that there is a bug on the server 's side ?

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

The initial list only contains own properties, but when we add "g", we start looking at the object prototype as well. Typing a plain "g" in the console correctly offers getComputedStyle as an option though. Omitting properties in the prototype chain does seem like a bug to me.

Comment 24

[Mihai Sucan [:msucan]]

Indeed, this is a bug. Hopefully it's easy to fix.

Christos, can you please file a separate bug that blocks this one? We need to get that issue fixed as well.

Comment 25

[Christos Stathis]

Attachment: All issues addressed. More tests added. Patch for 891374 included

Comment 26

[Mihai Sucan [:msucan]]

Comment on attachment 776667 [details] [diff] [review]
All issues addressed. More tests added. Patch for 891374 included

Review of attachment 776667 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the updated patch. Patch works well now, thank you. I only have requests for cleanup and style improvements.

In bug 812618 we changed how autocomplete works. The input is always considered up to the cursor location for completion suggestions. We are no longer bound by the input value end. This patch makes it such that we cache results at the end of the input, but if I type |dump()| then I move into the parentheses it stops doing any caching for suggestions. Please always consider the input up to the cursor location only.

Sorry for all the trouble this patch is giving you.

::: browser/devtools/webconsole/webconsole.js
@@ +2814,5 @@
>     */
>    lastCompletion: null,
>  
>    /**
> +   * Array that caches the autocompletion results that come from the server

Please add @type.

@@ +2816,5 @@
>  
>    /**
> +   * Array that caches the autocompletion results that come from the server
> +   */
> +  autocompletionCache: [],

This should be null. You should initialize this property to an empty array in the constructor, otherwise the same autocompletionCache will be used by all of the web console instances (putting this here on the prototype makes it shared by all).

@@ +2820,5 @@
> +  autocompletionCache: [],
> +
> +  /**
> +   * The input that caused the last request to the server, whose response is
> +   * cached in autocompletionCache array

Please add @type, mark both properties as private and prefix with _, and rename from autocompletion* to autocomplete*.

@@ +4047,5 @@
>  
>      let requestId = gSequenceId();
>      let input = this.inputNode.value;
> +    let popup = this.autocompletePopup;
> +    let items = popup.getItems();

Is |items| used anymore? And |popup|?

@@ +4048,5 @@
>      let requestId = gSequenceId();
>      let input = this.inputNode.value;
> +    let popup = this.autocompletePopup;
> +    let items = popup.getItems();
> +    let cache = this.autocompletionCache

Please end lines with ;

@@ +4056,5 @@
> +     * have a list of suggestions and we just need to filter it. Exception is
> +     * when the current input ends with a non-alphanumeric when we ask the server again for
> +     * suggestions
> +     */
> +    let nonAlphaNumericRE = new RegExp(/[^a-zA-Z0-9]/g);

Please change this into a constant.

Do you need the /g modifier? Why not write the regex as a literal regex: /foo/

@@ +4061,5 @@
> +
> +    /*
> +     * Check if last character is non-alphanumeric
> +     */
> +    let m = nonAlphaNumericRE.exec(input.charAt(input.length - 1));

You can simplify with:
const re = /[a-zA-Z0-9]$/;
if (!re.test(input)) {
...
}

Please use test instead of match. The former should be faster.

Also, we avoid one-letter/cryptic variable names. Please pick a more meaningful name.

@@ +4069,5 @@
> +
> +    if (this.autocompletionQuery && input.startsWith(this.autocompletionQuery)) {
> +      let filterBy = input;
> +      //Check if input contains non-alphanumerics
> +      let y = input.match(nonAlphaNumericRE);

Please pick a more meaningful variable name.

@@ +4074,5 @@
> +      /* If input contains non-alphanumerics, use the part after the last one t
> +       * to filter the cache
> +       */
> +      if (y) {
> +        filterBy = input.substring(input.lastIndexOf(y[y.length-1]) + 1);

It is a long time since I worked with this particular part of the code. Can you please explain why is this needed?

@@ +4097,1 @@
>      let cursor = this.inputNode.selectionStart;

When we don't use the autocompleteCache you should always invalidate it (clear it). You currently check if the input doesn't end with alphanumeric chars and you clear the cache explicitly. What happens if input doesn't start with the autocompleteQuery? You ask the server for results, but at this point you can be sure you are invalidating the local cache of suggestions.

@@ +4133,5 @@
> +    /*
> +     * Cache whatever came from the server if the last char is alphanumeric
> +     * or '.'
> +     */
> +    let m = inputValue.charAt(inputValue.length - 1).match(/[a-zA-Z0-9.]/g);

Do you need /g here? Why not use re.test() here?

@@ +4137,5 @@
> +    let m = inputValue.charAt(inputValue.length - 1).match(/[a-zA-Z0-9.]/g);
> +    if (aRequestId != null && m != null) {
> +      this.autocompletionCache = aMessage.matches;
> +      this.autocompletionQuery = inputValue;
> +    }

Please add an |else| here and clear the cache, since at this point we should be certain anything else in the cache is not useful.

Comment 27

[Christos Stathis]

(In reply to Mihai Sucan [:msucan] from comment #26)
> Comment on attachment 776667 [details] [diff] [review]
> All issues addressed. More tests added. Patch for 891374 included
> 
> Review of attachment 776667 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the updated patch. Patch works well now, thank you. I only have
> requests for cleanup and style improvements.
> 
> In bug 812618 we changed how autocomplete works. The input is always
> considered up to the cursor location for completion suggestions. We are no
> longer bound by the input value end. This patch makes it such that we cache
> results at the end of the input, but if I type |dump()| then I move into the
> parentheses it stops doing any caching for suggestions. Please always
> consider the input up to the cursor location only.
> 
> Sorry for all the trouble this patch is giving you.

No trouble at all! I wish I could spend more time doing this, cause it is definitely worth it.
 
> Please use test instead of match. The former should be faster.
> 
> Also, we avoid one-letter/cryptic variable names. Please pick a more
> meaningful name.
> 
> @@ +4069,5 @@
> > +
> > +    if (this.autocompletionQuery && input.startsWith(this.autocompletionQuery)) {
> > +      let filterBy = input;
> > +      //Check if input contains non-alphanumerics
> > +      let y = input.match(nonAlphaNumericRE);
>

I need to use 'match' in this case. I want the specific non-alphanumeric character, not just a true/false result.
 
> @@ +4074,5 @@
> > +      /* If input contains non-alphanumerics, use the part after the last one t
> > +       * to filter the cache
> > +       */
> > +      if (y) {
> > +        filterBy = input.substring(input.lastIndexOf(y[y.length-1]) + 1);
> 
> It is a long time since I worked with this particular part of the code. Can
> you please explain why is this needed?

If you type 'window.' and then 'd', you need to filter the cache using 'd' and not the whole input which is 'window.d'. You need to filter by the part of input after the last non-alphanumeric.

> 
> @@ +4097,1 @@
> >      let cursor = this.inputNode.selectionStart;
> 
> When we don't use the autocompleteCache you should always invalidate it
> (clear it). You currently check if the input doesn't end with alphanumeric
> chars and you clear the cache explicitly. What happens if input doesn't
> start with the autocompleteQuery? You ask the server for results, but at
> this point you can be sure you are invalidating the local cache of
> suggestions.
> 
> @@ +4133,5 @@
> > +    /*
> > +     * Cache whatever came from the server if the last char is alphanumeric
> > +     * or '.'
> > +     */
> > +    let m = inputValue.charAt(inputValue.length - 1).match(/[a-zA-Z0-9.]/g);
> 
> Do you need /g here? Why not use re.test() here?
> 
> @@ +4137,5 @@
> > +    let m = inputValue.charAt(inputValue.length - 1).match(/[a-zA-Z0-9.]/g);
> > +    if (aRequestId != null && m != null) {
> > +      this.autocompletionCache = aMessage.matches;
> > +      this.autocompletionQuery = inputValue;
> > +    }
> 
> Please add an |else| here and clear the cache, since at this point we should
> be certain anything else in the cache is not useful.

If 'aRequestId' is null (this is the case when no request has been done and the cache is used) I don't want to empty the cache.

Comment 28

[Christos Stathis]

Attachment: 790016.patch

Addressed issues in comment #26

Comment 29

[Mihai Sucan [:msucan]]

Comment on attachment 778936 [details] [diff] [review]
790016.patch

Thanks for the update. Giving this patch r+, however please wait before landing.

Here is a try push:
https://tbpl.mozilla.org/?tree=Try&rev=1257229d8fb1

... the patch I pushed is a bit cleaned-up by me (only coding style). I will land it when the results are green.

Comment 30

[Mihai Sucan [:msucan]]

Attachment: final patch

This is the cleaned-up patch that I just landed in fx-team:

https://hg.mozilla.org/integration/fx-team/rev/32d180146c1d

Updated green try push: https://tbpl.mozilla.org/?tree=Try&rev=643ed1efc4b8

Thank you for your patch!

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/32d180146c1d