Last Comment Bug 704295 - Autocomplete with a variable name that is equal to a prefix of a global variable makes it impossible to use the variable
: Autocomplete with a variable name that is equal to a prefix of a global varia...
Status: RESOLVED FIXED
[good-first-bug][mentor=msucan][fixed...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 11
Assigned To: Brijesh Patel
:
Mentors:
: 705843 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-21 14:02 PST by (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
Modified: 2011-12-14 06:34 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
My patch for this particular bug (753 bytes, patch)
2011-12-02 10:51 PST, Brijesh Patel
no flags Details | Diff | Review
patch_2 (961 bytes, patch)
2011-12-02 23:05 PST, Brijesh Patel
mihai.sucan: feedback+
Details | Diff | Review
patch_3 (1.09 KB, patch)
2011-12-04 10:11 PST, Brijesh Patel
no flags Details | Diff | Review
patch_4 (1.86 KB, patch)
2011-12-06 08:43 PST, Brijesh Patel
mihai.sucan: feedback+
Details | Diff | Review
patch_5 (1.38 KB, patch)
2011-12-06 22:42 PST, Brijesh Patel
mihai.sucan: feedback+
Details | Diff | Review
patch_6 (5.25 KB, patch)
2011-12-09 08:29 PST, Brijesh Patel
mihai.sucan: feedback+
Details | Diff | Review
patch_7 (7.22 KB, patch)
2011-12-10 07:20 PST, Brijesh Patel
mihai.sucan: review+
bugzilla: approval‑mozilla‑aurora-
Details | Diff | Review

Description (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2011-11-21 14:02:44 PST
STR:
1. In the web console, type the following:
var d = 5;
2. Hit enter
3. Type the following (note how there is no semicolon):
var a = d
4. Hit enter

Expected results:
The line |var a = d| is executed.

Actual results:
The line |var a = defaultStatus| is executed.
Comment 1 Rob Campbell [:rc] (:robcee) 2011-11-22 07:02:47 PST
This is a frustrating bug when you encounter it.
Comment 2 Mihai Sucan [:msucan] 2011-11-22 09:29:43 PST
Should be easy to fix.
Comment 3 Brijesh Patel 2011-12-02 10:51:36 PST
Created attachment 578639 [details] [diff] [review]
My patch for this particular bug
Comment 4 Mihai Sucan [:msucan] 2011-12-02 11:21:11 PST
Comment on attachment 578639 [details] [diff] [review]
My patch for this particular bug

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

Thank you very much for your contribution!

This works fine but there's a regression.

STR:
1. open the web console.
2. type dispa<Tab> to autocomplete to dispatchEvent.
3. backspace until you have only "d".
4. type oc<Tab> to autocomplete to document.

Expected result: "document" shows up.

Actual result: "docpatchEvent" is shown.

Please look into this. The problem is related to acceptProposedCompletion() and to complete(). I expect some properties are not cleared when needed.

Looking forward for the updated patch! Please let me know if you need further help.
Comment 5 Brijesh Patel 2011-12-02 23:05:11 PST
Created attachment 578812 [details] [diff] [review]
patch_2

Please check if any regression's there now or not.
Comment 6 Mihai Sucan [:msucan] 2011-12-03 05:45:03 PST
Comment on attachment 578812 [details] [diff] [review]
patch_2

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

Brijesh, thanks for your update!

This is indeed an improvement, but I still see a regression.

STR:

1. open the web console.
2. type "dispatch" and you will see dispatchEvent autocompleted.
3. backspace until you have only "d".

Expected result: the input only shows "d".

Actual result: the input shows "d   atchEvent".

Please also fix this case. Thank you!
Comment 7 Brijesh Patel 2011-12-04 10:11:32 PST
Created attachment 578917 [details] [diff] [review]
patch_3
Comment 8 Panos Astithas [:past] 2011-12-05 00:46:08 PST
*** Bug 705843 has been marked as a duplicate of this bug. ***
Comment 9 Mihai Sucan [:msucan] 2011-12-05 09:44:26 PST
Comment on attachment 578917 [details] [diff] [review]
patch_3

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

Thanks for your patch. I am still having problems with this patch.

STR:

1. open the Web Console and type "dis".
2. press the Down arrow key until you get to "dispatchEvent", so you have "patchEvent" in gray in the completion node.
3. press backspace once.

Expected result: completion node is cleared.

Actual result: I see "di patchEvent".

I took some time to investigate the bug. It seems to also be a problem in AutocompletePopup.jsm - there's a bug in clearItems(). You need to add at the start of the method this.selectedIndex = -1 such that the currently selectedIndex/Item is cleared. This confuses the Web Console code... From there, with this fix, we can move on to fixing the remaining issues in the Web Console.


Thanks a lot for your contribution! Looking forward for the updated patch!

::: browser/devtools/webconsole/HUDService.jsm
@@ +5438,4 @@
>                               matchProp: properties.matchProp};
>  
>        if (items.length > 1 && !popup.isOpen) {
> +        this.updateCompleteNode("");

This doesn't seem needed. The completion node should be automatically cleared in other places. If we need to clear it here, it's a bug. (see the calls to clearCompletion()).

@@ +5447,1 @@
>          popup.hidePopup();

Can you please explain why you do this?
Comment 10 Brijesh Patel 2011-12-06 08:43:28 PST
Created attachment 579315 [details] [diff] [review]
patch_4
Comment 11 Mihai Sucan [:msucan] 2011-12-06 12:28:34 PST
Comment on attachment 579315 [details] [diff] [review]
patch_4

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

Patch works as expected. Thank you very much!

Next step: writing a test. Please look into browser/devtools/webconsole/test/browser to see a good number of tests. Copy one of them and make it test the STR in comment 0.

Please ping me on IRC if you need help with writing the test!

Awesome first-contribution! Thanks!

::: browser/devtools/webconsole/AutocompletePopup.jsm
@@ +229,3 @@
>      // Reset the panel and list dimensions. New dimensions are calculated when a
>      // new set of items is added to the autocomplete popup.
> +    this.selectedIndex = -1;

Please move this before the while loop - before the list is cleared.

::: browser/devtools/webconsole/HUDService.jsm
@@ +5452,3 @@
>          if (items.length == 1) {
>            // onSelect is not fired when the popup is not open.
>            this.onAutocompleteSelect();

I investigated the code a bit more now.

Your patch does exactly what we need, but I would suggest:

- make this.onAutocompleteSelect() to be always called - so move it out of the ifs.

- do if (items.length == 1) { popup.selectedIndex = 0; }. This is the same as what you do, with popup.selectNextItem() - hopefully the proposed change is clearer when we come back to read the code.

- you can remove updateCompleteNode("") because the call to onAutocompleteSelect() will do that, when needed.

What do you think? Do these changes make sense to you?
Comment 12 Brijesh Patel 2011-12-06 22:42:15 PST
Created attachment 579594 [details] [diff] [review]
patch_5

yes, this changes are sensible to me though the work done by this patch and previous one is same, but this patch looks better as far as code readability is concerned...
Comment 13 Mihai Sucan [:msucan] 2011-12-07 11:01:10 PST
Comment on attachment 579594 [details] [diff] [review]
patch_5

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

Thanks for your update, Brijesh!

Looking forward to see the test!

::: browser/devtools/webconsole/AutocompletePopup.jsm
@@ +222,5 @@
>    clearItems: function AP_clearItems()
>    {
> +    // Reset the selectedIndex to -1 before clearing the list
> +    this.selectedIndex = -1;
> +        

Nit: this line has trailing whitespace. Please remove it.

::: browser/devtools/webconsole/HUDService.jsm
@@ +5448,1 @@
>          if (items.length == 1) {

You no longer need the outer if items.length > 0.

@@ +5448,2 @@
>          if (items.length == 1) {
> +          popup.selectedIndex = 0;          

Nit: this line has trailing whitespace. Please remove it.
Comment 14 Brijesh Patel 2011-12-09 08:29:35 PST
Created attachment 580425 [details] [diff] [review]
patch_6
Comment 15 Mihai Sucan [:msucan] 2011-12-09 11:18:21 PST
Comment on attachment 580425 [details] [diff] [review]
patch_6

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

This is great Brijesh! Your first test passes without any errors and it works nicely! I only have a comment on how you can simulate the steps-to-reproduce even more closely.

Still, there's one problem with this patch. I get test failures when I run all the Web Console tests. See this copy/paste:
http://pastebin.mozilla.org/1398729

Out of about a thousand of tests, 6 of them fail. Please look into the tests that fail and fix them. This happens usually when existing tests expect behavior that has changed. Please let me know if you need help with anything.

Thank you very much for your work and patience. We are very close to be ready to land this fix!

::: browser/devtools/webconsole/test/browser/browser_webconsole_bug_704295.js
@@ +64,5 @@
> +  
> +  // Test typing 'var a = d' and press RETURN
> +  input.value = "var a = d";
> +  input.setSelectionRange(9, 9);
> +  jsterm.complete(jsterm.COMPLETE_HINT_ONLY);

I suggest you do jsterm.setInputValue("vad a = ");

Then instead of setSelectionRange() and instead of calling jsterm.complete() manually, generate synthetic keyboard events:

EventUtils.synthesizeKey("d", {});

Then:
EventUtils.synthesizeKey("VK_ENTER", {})

Then you check what is the jsterm.completeNode.value and the input.value. This should be all.

What do you think?
Comment 16 Brijesh Patel 2011-12-10 07:20:46 PST
Created attachment 580647 [details] [diff] [review]
patch_7
Comment 17 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2011-12-11 00:21:45 PST
I have assigned this to Brijesh as it appears that Brijesh is working on this.

I'm trying to keep the status of our mentored bugs up to date. Please unassign if this was done in error.
Comment 18 Mihai Sucan [:msucan] 2011-12-12 09:52:33 PST
Comment on attachment 580647 [details] [diff] [review]
patch_7

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

This is great Brijesh! Your patch works as expected and all the tests pass.

This is ready to land. Thank you!
Comment 19 Rob Campbell [:rc] (:robcee) 2011-12-13 05:56:52 PST
https://hg.mozilla.org/integration/fx-team/rev/9366658ba029
Comment 20 Rob Campbell [:rc] (:robcee) 2011-12-13 08:20:34 PST
Comment on attachment 580647 [details] [diff] [review]
patch_7

this is a really annoying bug when you hit it. I'd love to get this into aurora.
Comment 21 Johnathan Nightingale [:johnath] 2011-12-13 14:46:10 PST
Comment on attachment 580647 [details] [diff] [review]
patch_7

Awesome to see this fixed, but we discussed this in channel triage and don't think it needs to be rushed into aurora. The problem is not new to FF10, and has straightforward workarounds. I'm really happy to have a fix in the tree and working its way through, though.
Comment 22 Rob Campbell [:rc] (:robcee) 2011-12-14 06:31:23 PST
https://hg.mozilla.org/mozilla-central/rev/9366658ba029

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