Implement Autocomplete in Scratchpad

RESOLVED FIXED

Status

P2
normal
RESOLVED FIXED
6 years ago
2 months ago

People

(Reporter: rcampbell, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
Need to figure out if we should use Orion's implementation of Autocomplete (Andrew Eisenberg, Thaddée to paste link), or use our own built around the Esprima parser.
Andew Eisenberg's work lies here: https://github.com/aeisenberg/esprimaContentAssist/blob/master/esprimaJsContentAssistPlugin.html
It is a plugin to Orion's autocomplete system.

Comment 2

6 years ago
Didn't Victor show off Scratchpad autocompletion recently?

Also, I'm hoping that autocompletion for Scratchpad can be extra awesome by completing on objects that are referenced in the script but live in the evaluation context (similar to the completion the Web Console does). Perhaps that's a followup, but I just wanted to note it.
(In reply to Kevin Dangoor from comment #2)
> Didn't Victor show off Scratchpad autocompletion recently?

I'll put a link for future reference: <https://gist.github.com/53c121d33dfd1e3e18fa>
It uses the SpiderMonkey Parser API and a floating <div> to show possible completions.

> Also, I'm hoping that autocompletion for Scratchpad can be extra awesome by
> completing on objects that are referenced in the script but live in the
> evaluation context (similar to the completion the Web Console does). Perhaps
> that's a followup, but I just wanted to note it.

100% with you on this.
Does anyone know why "CTRL+Space" shows the contextual menu on MacOS X? (and how to deactivate that behaviour?)

That is the keybinding I am going for.

Comment 5

6 years ago
(In reply to Thaddee Tyl [:espadrine] from comment #4)
> Does anyone know why "CTRL+Space" shows the contextual menu on MacOS X? (and
> how to deactivate that behaviour?)

Ctrl-Space is the standard way to bring the context menu on Linux. And apparently, on Mac too.

And I guess this is something people would like to configure. So maybe you want to store the binding definition in a pref.

Personally, I'd like to use Ctrl-n/Ctrl-p (like in Vim).
(In reply to Paul Rouget [:paul] from comment #5)
> (In reply to Thaddee Tyl [:espadrine] from comment #4)
> > Does anyone know why "CTRL+Space" shows the contextual menu on MacOS X? (and
> > how to deactivate that behaviour?)
> 
> Ctrl-Space is the standard way to bring the context menu on Linux. And
> apparently, on Mac too.
> 
> And I guess this is something people would like to configure. So maybe you
> want to store the binding definition in a pref.
> 
> Personally, I'd like to use Ctrl-n/Ctrl-p (like in Vim).

Do you mean "the context menu" as "word complete popup menu"?
Paul: having a pref would probably be handy. This being said, I don't get the context menu on Linux, it might be a gnome thing.

Ohzeki: "context menu" as in "right click menu".

Comment 8

6 years ago
(In reply to Thaddee Tyl [:espadrine] from comment #7)
> Paul: having a pref would probably be handy. This being said, I don't get
> the context menu on Linux, it might be a gnome thing.

It brings the window menu.

Comment 9

6 years ago
(and yes, probably a Gnome/XFCE thing)
Some Input method user who uses us keyboard assign Ctrl+Space with switching input method mode. If we use Ctrl+Space, it's going to conflict.

I agree to  use Ctrl-n/Ctrl-p (like in Vim).
Can't we have the autocompletion popup appear by default when the caret is at the end of a word and after a small idle timeout?
Panos: sure, I suppose I can. This being said, it will probably be
at the end of a word *and after a dot*, etc. This is all language-specific,
which is annoying, since I plan on adding autocompletion for the
Style Editor too.

Ohzeki: on Mac, Ctrl-n and Ctrl-p behave like Emacs, ironically.
And Ctrl-Space sometimes defaults to switching input method.
I'm not sure which version does that.

It would be great to find a solution that works out of the box
with the default settings of each OS.

Looking at XCode, it does autocompletion when pressing the Esc key.
Does that seem strange to anyone?
I think that we need not use same shortcut key on all platforms.
We should use a suited keyset for each platform.
(Reporter)

Comment 14

6 years ago
I can chime in here with a few opinions:

Ctrl-Space would be nice, but is apparently used on some systems for context menus.
Alt-Space I don't know about, but probably not as comfortable or convenient as Ctrl.
Cmd-Space (Mac Only) is Spotlight.
ESC is what Xcode uses. It's a little unusual, and I've never gotten used to using that as the Dictionary lookup key in cocoa text widgets (like, Mail.app).
Visual Studio does autocompletion automatically.

I like the automatic suggestion idea. We could even begin autocompleting after the first character if we have some notion of current scope. We could complete variables and globals visible at the current level.

Comment 15

6 years ago
What about just using Tab?

If the character just before the cursor is a letter/number/underscore (\w) or a dot (\.), then you stop the propagation of the key event and start the completion.

Comment 16

6 years ago
Here is the completion code I use in my jsterm extension: https://github.com/paulrouget/firefox-jsterm/blob/master/chrome/jsterm.js#L500
(In reply to Paul Rouget [:paul] from comment #16)
> Here is the completion code I use in my jsterm extension:
> https://github.com/paulrouget/firefox-jsterm/blob/master/chrome/jsterm.
> js#L500

I seem Paul's jsterm completion is good as one of the solution.
Full word completion with popup (like IDE) is more smart and typing will be easy.

jsterm's completion is very simple. This may be more useful for some case than introducing the completion popup and invoke key for showing it.
Ohzeki: Those completions are not exclusive.

I really like Paul's system (which is close to zsh autocompletion)
and I will implement something very similar.
The great pro that zsh autocompletion features is modeless completion.
This means that each key behaves exactly the same whatever the context.

As a result, what I will implement is:

- When we press tab in the right context, the popup appears.
- Resuming typing makes it go away. This ensures that there is an easy way to dismiss the popup. Typing "Esc" should do that, too.
- When we press tab twice, it starts cycling through all possible completions,
and inserts the first possibility.
- Typing shift+tab cycles back.
- If there is a huge number of possible completions, the user will probably want
to scroll through them, and click on the correct one when he finds it.
This inserts the corresponding completion.

All this was about UX. As for the system which provides autocompletion,
I am going for a stack of "levels" of autocompletion.
Each time we summon completion, it goes up the stack to gather all possible
completions.

1. The first level (of lowest priority, which appears below all others)
uses the same completion as the webconsole.
Pros: it is dynamic and comprehensive (well, it will be comprehensive after
the patch for bug 775812 [1] lands, and the patch for bug 774753 [2] gets created
and lands ;)
Cons: it doesn't see what isn't executed. In the scratchpad, we tend to write
a bunch of code without executing it at first, so this is limited.

2. The second level (of highest priority) statically reads local and global
variables in the script, using the Esprima parser (with some error tolerance that
I have yet to write), and suggests completions based on that.
Pros: it doesn't need to run the code.
Cons: the Esprima parser doesn't run if there are too many errors.


  [1] https://bugzilla.mozilla.org/show_bug.cgi?id=775812
  [2] https://bugzilla.mozilla.org/show_bug.cgi?id=774753
(Reporter)

Comment 19

6 years ago
WIP.

Filter on BLACKEAGLE?!
Assignee: nobody → thaddee.tyl
Status: NEW → ASSIGNED
Created attachment 648378 [details] [diff] [review]
WIP autocompletion for UX testing

This is a work-in-progress, but I'd be interested to know what you think about
the UX of this implementation.

If you hit a bug and the scratchpad goes havoc, close it and open it again.
I like it. There are still bugs for sure, like the popup hiding the completion text, but it seems natural to me.

One thing I'd like to see, is when I type window.b| and the available selections are window.bar and window.baz, then "ar" and "az" would be displayed with a more subtle color as I go through the list, and the selection would only be completed when I type TAB or select the completion with the mouse.
Panos: do you mean that you want properties that are closer to the object
in the prototype tree to appear first, or with a marker?

This is not a bad idea. Right now, properties are sorted with their keyChar number
(that is, it isn't even an alphabetic sort),
which is actually counter-intuitive for most users. For instance, "window.Date"
appears between "window.Boolean" and "window.btoa".

Sorting along a score would make more sense. Then, if the user knows
the first letter, (s)he will type it, and it automatically shows only those
results. I'll look into it.
I believe this is getting pretty stable, so I'd love to get bug reports against
the following patch: https://github.com/espadrine/moz-patches/blob/master/completion.diff

That patch is live, meaning that it may change at any time.

Please note that it doesn't include static variable lookups just yet.
I am working on that independently.
Panos: I think I understand what you mean by "the popup hiding the completion text".
You are on Mac, right?

The Mac symbol for "Command" ("⌘"), which was introduced
by the patch to bug 756423, makes the lines
of the source editor higher than Orion believes they are.

This bug has been reported upstream: https://bugs.eclipse.org/bugs/show_bug.cgi?id=386517.

Since I rely on Orion's line height information,
I can hardly correct this information. I believe I should simply wait for
the bug to get resolved on Orion's side, or for the "⌘" symbol to be changed
to "Cmd" on our side.
Depends on: 779642
Duplicate of this bug: 717366
(In reply to Thaddee Tyl [:espadrine] from comment #22)
> Panos: do you mean that you want properties that are closer to the object
> in the prototype tree to appear first, or with a marker?
> 
> This is not a bad idea. Right now, properties are sorted with their keyChar
> number
> (that is, it isn't even an alphabetic sort),
> which is actually counter-intuitive for most users. For instance,
> "window.Date"
> appears between "window.Boolean" and "window.btoa".
> 
> Sorting along a score would make more sense. Then, if the user knows
> the first letter, (s)he will type it, and it automatically shows only those
> results. I'll look into it.

Actually I wasn't talking about the order of properties in the list, but how the currently selected completion appears in the editor. Assuming that window.b| is what is typed in the editor (and the cursor is right after "b"), then if the list has both "bar" and "baz" and the user navigating with the arrow keys has paused over "baz", I'd like "b" to be displayed with the same color as the rest of the editor text, but "az" to be displayed with a more subtle color (light gray?) to indicate that this is not a finalized selection.

Exactly how the Web Console does it.

(In reply to Thaddee Tyl [:espadrine] from comment #24)
> Panos: I think I understand what you mean by "the popup hiding the
> completion text".
> You are on Mac, right?
> 
> The Mac symbol for "Command" ("⌘"), which was introduced
> by the patch to bug 756423, makes the lines
> of the source editor higher than Orion believes they are.
> 
> This bug has been reported upstream:
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=386517.
> 
> Since I rely on Orion's line height information,
> I can hardly correct this information. I believe I should simply wait for
> the bug to get resolved on Orion's side, or for the "⌘" symbol to be changed
> to "Cmd" on our side.

Yes, I'm on a Mac and this makes a lot of sense.
I couldn't get this working for me.

I have the patch from bug 759351 applied then this patch. No popup is displayed. Am I doing something wrong?
Mihai: I wanted to have a patch that anyone could easily apply, so this patch
isn't meant to have the Orion update.

On the other hand, I will need to update a couple components for the Orion update,
and an added benefit this patch will get from the update will be that
the popover will be able to get CSS from the orion.css stylesheets we already have.

Panos: I am trying to make it clear to the user that (s)he doesn't have
to type <Enter> to get the autocompletion. It's already typed in, so that
(s)he can just resume writing code.

The way this system works is twofold:
1. In the normal state, the autocompletion popover may appear as we type,
   but it is unobtrusive and obviously does not modify the source code.
2. Getting to the cycling state happens when the user types <TAB>.
   As the user types <TAB> several times, all candidates get selected,
   one after the other. Every time, it inserts the candidate where the user
   expects it to appear. Using the mouse and clicking on a candidate
   also cycles to it.
   Getting back to the normal state happens when the user resumes typing code,
   moves the cursor around, or press the <ESC> key.
For some reason, I got failures when I tried to apply only this patch. Does this patch depend on any other work?
(In reply to Thaddee Tyl [:espadrine] from comment #28)
> Panos: I am trying to make it clear to the user that (s)he doesn't have
> to type <Enter> to get the autocompletion. It's already typed in, so that
> (s)he can just resume writing code.
> 
> The way this system works is twofold:
> 1. In the normal state, the autocompletion popover may appear as we type,
>    but it is unobtrusive and obviously does not modify the source code.
> 2. Getting to the cycling state happens when the user types <TAB>.
>    As the user types <TAB> several times, all candidates get selected,
>    one after the other. Every time, it inserts the candidate where the user
>    expects it to appear. Using the mouse and clicking on a candidate
>    also cycles to it.
>    Getting back to the normal state happens when the user resumes typing
> code,
>    moves the cursor around, or press the <ESC> key.

What happens when the user doesn't hit TAB, but instead peruses the available options using the arrow keys, or the mouse? The reason I'm asking is that the current web console behavior facilitates an important (in my experience) use case: making a typo that filters out what you are trying to type.

Say for instance that the user intends to type window.HTMLTitleElement, but types window.HTMLTa| before noticing the popup and then mechanically starts to scroll down to find the right completion. When he realizes that what he is looking for is not there, he may observe the typo that he made and hit backspace to erase the one mistaken character.

If the behavior is to always complete the property name, without some decisive action on behalf of the user (like TAB/ENTER/click), then the user will have to erase a lot of characters to fix the single typo that he made. Essentially what I'm suggesting is that we do not treat browsing the available selections the same as picking one of them. If our autocomplete code maintains this separation, the most common way that I know to make this evident to the user, is the use of a subtle color hint.
Panos: Scrolling down the popup doesn't start the cycling state.
Clicking on a candidate (which is an asserted request for the corresponding
entry) does. As a result, in the use case you describe, when the user
realizes that what (s)he is looking for is not there, (s)he hits backspace
and erases the mistaken character.

In a more generic use-case, I made sure to map all bindings that
I would expect, as a user, to give me the ability to "back out" from the
cycling state, removing the completion that appeared.
Both the <ESC> key and the <CTRL-Z> / <CMD-Z> bindings do that.

I sincerely believe that modeless autocompletion will give
a more streamlined and intuitive experience, and let the act of completing
be one key press away instead of two.

I talked to testers, comparing the modeless system to Google Chrome's system
in use in their Console (which features a two-key press autocompletion
with a shadow candidate, much like you suggest, if not exactly what you
suggest), and they found it actually easier to use.
(In reply to Mihai Sucan [:msucan] from comment #29)
> For some reason, I got failures when I tried to apply only this patch. Does
> this patch depend on any other work?

No. It applies cleanly directly on fx-team on my machine.

Are you using https://github.com/espadrine/moz-patches/blob/master/completion.diff?
How up-to-date is your tree? I pulled from fx-team last saturday.

Is anyone else experiencing the same issues?
Thanks Thaddee.

The problem was I used the patch from this bug.

Now with the patch from Github, it works. Really cool stuff. I like what you did!

When the code is ready, please also ask me for feedback/review. I'd like to see how you did the source editor integration. Thanks!
No longer depends on: 779642
Created attachment 653587 [details] [diff] [review]
Autocompletion in the scratchpad.

This patch adds autocompletion to the scratchpad.

It contains a workaround to fix bug 779642 which should be removed when Orion gets the fix, since it adds a local O(n*m) performance impact which is heavy for very long lines (n is the number of visible lines in the DOM, always low, and, m the number of characters in a single line, which can be high).

The UI is very simple (read "not pretty"), but upgrading it requires the next version of Orion, bug 759351, to avoid adding a lot of workarounds that would get erased after the update.
Attachment #648378 - Attachment is obsolete: true
Attachment #653587 - Flags: review?(rcampbell)
Attachment #653587 - Flags: review?(mihai.sucan)
Comment on attachment 653587 [details] [diff] [review]
Autocompletion in the scratchpad.

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

Robcee asked me to give you some feedback. So here I am. I have a quick glance and I may have missed some stuff.
Feel free to ignore everything I say :)

This is cool. Overall the functionality is pretty fluid indeed. The fact that there's a popup displayed on each keypress is something I really like.

UX-wise, sometimes I was a bit confused on what key I should press in order to hide the popup. Here's some examples:

* Autocompletion is case sensitive. I think it'd be a good idea to have the matching as fuzzy as possible, especially starting with making it case insensitive (I may be biased because I'm a terrible typer, trading precision for speed). Ideally, when I do a foo.BCAN i'd like it to show me "BEER_CAN_SOUNDS_LIKE_BACON" and "BREAKPOINT_CHANGE", but I can settle just for "window.com" showing "Components" instead of nothing.

* Typing "window.get" and pressing TAB would autocomplete to "getComputedStyle" but still display the popup. I'd then need to type ESC or another key to hide it.

I also think there's some duplicated behavior here. Generally you'd go to your preferred completion using the UP and DOWN keys, press TAB to complete and hide the popup. Currently TAB functions exactly like pressing UP and DOWN, which seems redundant.

The fact that the initial entry in the popupbox is selected automatically is very good imo, but I feel like TAB should just autocomplete on what's currently selected and close the popup.

* This was said previously in this bug, but I'll mention it too: the fact that UP/DOWN automatically changes the entered string with what's in the popup is not something I'd like to happen. UP/DOWN should just jump to the next element in the popup, and TAB would do one thing only: finish autocompleting and hide the popup. 

There's a certain degree of "instinct" built in when dealing with how autocompletion popups work, and while your proposed interaction is actually pretty damn fine, it'll require people detaching from what they learned to trust so far. For example my sentiment was something like "Ok, DOWN, DOWN... here it is. TAB. Oh? Um.. No, not that. DOWN, DOWN, DOWN. ESC. Hmm, ok then, I guess this is ok..".

Here's a concrete example:
- "typed windows.co"
- pressed DOWN till I reached "controllers"
- pressed TAB (by reflex)
- the selection jumped to the first entry.
That's definitely not what I was expecting. My intention was "write that and hide the popup", the effect was "select a different thing and don't hide the popup" which is the exact opposite :)

* When only one completion option is available, the popup looks like a collapsed dropdown. Will the Orion update help us with this?

***

There are a few places in which things aren't working as expected like trying to get properties of a global defined in a lazy getter (ex: without ever opening the Debugger, writing "DebuggerUI." gives me a "Parsing error: missing name after . operator").

I'm not sure if there's a followup for it, but it seems that the statical analysis is not finished yet. If I had a source containing "var foo = { bar: 42 }" doing "foo." wouldn't display anything in the popup. Same goes for arrays. You'd have to eval the source first. While this is generally ok, I think the Parser API is super powerful and we should make full use of it especially in these cases.

One last thing: when relying on eval for the global scope or current context (or even when using the Parser API before any initial eval), having a bit more information than just the autocompletion string in the popup would be really awesome to have.
For example, when displaying the function name in the popup, you could also show the params in on the right, when dealing with primitives (numbers, strings etc.) showing their value, or const if it's the case. This would be trivial to add, just regexp your way around the evaled values or dive deep into the abstract syntax tree from the Parser API. Take a look into the gist from comment #3, here's a screen: http://tinyurl.com/9gehfbn
Can totally follow up for all of this.

::: browser/devtools/scratchpad/scratchpad.js
@@ +409,5 @@
> +  getSandbox: function SP_getSandbox()
> +  {
> +    // Evaluate the sandbox one to get all the autocompletion we can.
> +    this.evalForContext("");
> +    return this.executionContext == SCRATCHPAD_CONTEXT_CONTENT ?

The name of this function is a bit misleading. Either getSandboxForAutocompletion here (implying an automatic evalForContext beforehand), or just leave it as getSandbox and do a this.evalForContext("") manually where necessary. I'd go with the second option.

::: browser/devtools/scratchpad/test/browser_scratchpad_bug_660560_tab.js
@@ +40,4 @@
>  
>    EventUtils.synthesizeKey("VK_TAB", {}, gScratchpadWindow);
>  
> +  is(sp.getText(), "     window.foo;    ",

Pressing TAB inside of a string tries to autocomplete? Ok, I think that's ok.

::: browser/devtools/scratchpad/test/browser_scratchpad_bug_762164_autocompletion.js
@@ +41,5 @@
> +  EventUtils.synthesizeKey("VK_TAB", {}, gScratchpadWindow);
> +
> +  let popover = sp.editor._autocomplete.popover;
> +  is(popover.style.display, "block",
> +     "Autocompletion of window.F gives a popover");

Aren't you displaying the popup after a timeout(,0)? You're probably going to get into race conditions if that's the case. Fire an event to handle the autocompletion popover showing/hiding.

::: browser/devtools/sourceeditor/autocompletion.jsm
@@ +16,5 @@
> + * It starts invisible (display: none).
> + *
> + * @param object document
> + *        The DOM document in which to add a popover.
> + *

Nit: don't add newlines between @params in comments.

@@ +23,5 @@
> + *
> + * @return object popover
> + *         The DOM element representing the popover.
> + */
> +function createPopover(document, cssClass) {

function createPopover(document, cssClass = "autocomplete") {
Up to you.

@@ +45,5 @@
> + *        The Orion editor to target.
> + *        ed._mode must be "js".
> + *        ed._view must be a TextView.
> + *        ed._model must be a TextModel.
> + *        ed._undoStack must be an UndoStack.

:)

@@ +46,5 @@
> + *        ed._mode must be "js".
> + *        ed._view must be a TextView.
> + *        ed._model must be a TextModel.
> + *        ed._undoStack must be an UndoStack.
> + *        ed.editorElement must be the XUL iframe containing the editor.

Those are a lot of assertions. Maybe test for those in the constructor and throw an Error if something's not right.

@@ +50,5 @@
> + *        ed.editorElement must be the XUL iframe containing the editor.
> + *
> + * @param object options
> + *        Properties for tuning certain defaults:
> + *        - nbVisibleCompletions (defaults to 10): number of visible

Nit: how about num instead of nb? It took me 4 seconds to figure out what nb means.

@@ +58,5 @@
> + *        - getSandbox (defaults to null): function to get a sandbox environment
> + *          to evaluate expressions for dynamic autocompletion purposes.
> + */
> +function Autocompletion(ed, options = {}) {
> +  let cssClass = options.cssClass || "autocomplete";

..and you could remove the || "autocomplete" here.

@@ +59,5 @@
> + *          to evaluate expressions for dynamic autocompletion purposes.
> + */
> +function Autocompletion(ed, options = {}) {
> +  let cssClass = options.cssClass || "autocomplete";
> +  this.nbVisibleCompletions = options.nbVisibleCompletions || 10;

My comment about the magic 10 number applies here too. If you'll do something about it, make sure to update the comment for the constructor param.

@@ +66,5 @@
> +  this.document = ed.editorElement.contentDocument;
> +  this.popover = createPopover(this.document, cssClass);
> +
> +  // The following will fire the autocompletion system on each character!
> +  this.editor.editorElement.addEventListener("keypress", function(e) {

I'd avoid creating functions in the constructor. I think it's also a good practice to have the number of addEventListener equal to removeEventListener. Why not move this function in the prototype, and bind the |this| before adding it as a listener?

@@ +68,5 @@
> +
> +  // The following will fire the autocompletion system on each character!
> +  this.editor.editorElement.addEventListener("keypress", function(e) {
> +    if (!this._insertingText) {
> +      if (e.keyCode == 13) {

e.DOM_VK_RETURN instead of 13.

@@ +71,5 @@
> +    if (!this._insertingText) {
> +      if (e.keyCode == 13) {
> +        this._fireStaticAnalysis = true;
> +      }
> +      if (e.charCode > 0) {

Nit: if (e.charCode)

@@ +72,5 @@
> +      if (e.keyCode == 13) {
> +        this._fireStaticAnalysis = true;
> +      }
> +      if (e.charCode > 0) {
> +        this.document.defaultView.setTimeout(this.displayCompletion.bind(this),0);

setTimeout is the devil. But sometimes the devil is the only option.

However, I think I understand the reasoning of waiting for a short delay before showing and populating the popup. In that case, why not use a longer delay? I don't think anyone would want such an incredibly snappy popup flashing around each time I type. Try out a timeout of 50ms (and put it in a const if it feels right).

If that's not necessary, how about
Services.tm.currentThread.dispatch({ run: yourFuncHere }, 1);
instead of the timeout(,0)?

@@ +76,5 @@
> +        this.document.defaultView.setTimeout(this.displayCompletion.bind(this),0);
> +      }
> +    }
> +  }.bind(this));
> +  this.editor.addEventListener("Selection", function(e) {

Just like with keypress, move it in the prototype. And removeEventListener when unneeded.

@@ +78,5 @@
> +    }
> +  }.bind(this));
> +  this.editor.addEventListener("Selection", function(e) {
> +    // If the distance between the old position and the new is bigger than 1,
> +    // the static analysis is worth updating.

Heh, this is hacky. Why are you doing this on selection events?
Not saying this is bad, just wanting to understand what you're trying to do.

@@ +94,5 @@
> +Autocompletion.prototype = {
> +
> +  // The following set the way in which the autocompletion system works.
> +  getSandbox: null,
> +  nbVisibleCompletions: 10,

:)

@@ +112,5 @@
> +  _end: 0,
> +  _insertingText: false,
> +  _completion: null,
> +  // Static analysis would better be cached.
> +  _staticCandidates: null,

Are the completions uncached when not present in the source code anymore? This concern is somewhat related to the selection events that trigger static analysis.

@@ +145,5 @@
> +    // The first child is a <select> element.
> +    let html = "<select>";
> +    for (let i = 0; i < completions.length; i++) {
> +      // The first option gets selected by default.
> +      html += "<option " + (i == this._index? "selected='true'": "") + ">" +

Nit: whitespace before ? and :

@@ +149,5 @@
> +      html += "<option " + (i == this._index? "selected='true'": "") + ">" +
> +        completions[i] + "</option>";
> +    }
> +    html += "</select>";
> +    this.popover.innerHTML = html;

I'd avoid using innerHTML, I'm sure document.createElement would work better. Just make sure you're using the correct namespace.

@@ +151,5 @@
> +    }
> +    html += "</select>";
> +    this.popover.innerHTML = html;
> +    // Track clicking on options.
> +    this.popover.firstChild.addEventListener("click", function() {

You're creating a function each time when displaying the popover. And if you're going to do this on each key press, the gc may be stressed out a bit (just a bit). Again, can you move this definition in the prototype?

@@ +160,5 @@
> +      }
> +      this.editor._view.focus();
> +    }.bind(this), false);
> +    this.popover.firstChild.size =
> +      Math.min(this.nbVisibleCompletions, completions.length);

max-height and a scrollbar maybe?

@@ +165,5 @@
> +
> +    // Positioning the popover.
> +    let coord = ed._view.getLocationAtOffset(ed._view.getCaretOffset());
> +    coord.y += ed._view.getLineHeight();
> +    ed._view.convert(coord, "document", "page");

Instead of all this getting of locations, caret offsets and convertions, would using a tooltip tied to the editor caret (by calling openPopup with an anchor) work better? You can make a custom tooltip item and add whatever you want to it: https://developer.mozilla.org/en-US/docs/XUL/tooltip

On the other hand, maybe this is just trying to make things too elegant.

@@ +180,5 @@
> +    for (; line + " " !== linesOM[lineIndex].textContent; lineIndex++) {}
> +    coord.y = insideDoc.defaultView.scrollY +
> +      linesOM[lineIndex].getBoundingClientRect().top +
> +      linesOM[lineIndex].scrollHeight;
> +    // End of Bug 779642 fix.

Custom tooltips may render this unneeded. If you have the time, it could be a good idea to try it out.

@@ +189,5 @@
> +    let dwidth = this.document.documentElement.clientWidth;
> +    let dheight = this.document.documentElement.clientHeight;
> +    if (coord.y + this.popover.offsetHeight > dheight) {
> +      this.popover.style.top = (coord.y - this.popover.offsetHeight +
> +                                - ed._view.getLineHeight()) + "px";

Nit: + -
How about just -

@@ +211,5 @@
> +  },
> +
> +  // Specific autocompletion-only keys.
> +  _keyBindings: function AC_keyBindings(e) {
> +    if (e.keyCode == 27) {          // ESC key.

e.DOM_VK_ESCAPE

@@ +217,5 @@
> +      if (this._cycling) {
> +        this.editor.setText("", this._start, this._end);
> +      }
> +      e.stopPropagation();
> +    } else if (e.keyCode == 38) {   // ↑ key.

e.DOM_VK_UP

@@ +223,5 @@
> +        this.cycle(-1);
> +        e.stopPropagation();
> +        e.preventDefault();
> +      }
> +    } else if (e.keyCode == 40) {   // ↓ key.

e.DOM_VK_DOWN

@@ +349,5 @@
> +   * Autocompletion happens based on the following factors
> +   * (with increasing relevance):
> +   *
> +   * Level 0 = JS keywords.
> +   * Level 1 = dynamic lookup of available properties.

Describe Level 2 here as well.

@@ +385,5 @@
> +    }
> +    let allStaticCandidates = this._staticCandidates;
> +    // Right now, we can only complete variables.
> +    if (identifier.indexOf(".") == -1 && identifier.indexOf("[") == -1 &&
> +        // Thou shalt only complete what there is to complete.

Nit: funky comment line in the middle of a conditional expression. Add it after { or before the if.

@@ +398,5 @@
> +      }
> +      staticCandidates.sort(function(a, b) {
> +        // Sort them according to nearest scope.
> +        return allStaticCandidates.get(b) - allStaticCandidates.get(a);
> +      });

This is cool.
What would be even more awesome is to colorize or somehow delimit the candidates based on the scope.
Just a suggestion, follow up for this if you think it's a good idea.

@@ +400,5 @@
> +        // Sort them according to nearest scope.
> +        return allStaticCandidates.get(b) - allStaticCandidates.get(a);
> +      });
> +      candidates = candidates.concat(staticCandidates);
> +      completions = completions.concat(staticCandidates.map(function(candidate){

Nit: space before {

@@ +414,5 @@
> +        candidates = candidates.concat(sandboxCompletion.matches
> +            .filter(function(candidate) {
> +          // We are removing candidates from level 2.
> +          if (allStaticCandidates == null)  return true;
> +          if (allStaticCandidates.has(candidate)) {

Useless if. Are you planning on adding things here later?

@@ +434,5 @@
> +      "default", "delete", "do", "else", "export", "false", "finally", "for",
> +      "function", "get", "if", "import", "in", "instanceof", "let", "new",
> +      "null", "of", "return", "set", "super", "switch", "this", "true", "throw",
> +      "try", "typeof", "undefined", "var", "void", "while", "with",
> +    ];

Did you get those from https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Reserved_Words ?
Seems that "each" is missing (maybe some other stuff too). I wouldn't worry too much about it though.

@@ +435,5 @@
> +      "function", "get", "if", "import", "in", "instanceof", "let", "new",
> +      "null", "of", "return", "set", "super", "switch", "this", "true", "throw",
> +      "try", "typeof", "undefined", "var", "void", "while", "with",
> +    ];
> +    // This autocompletion is not meaningful when we type a property…

Hmm, why?
I'd like to have autocompletion when writing foo["bar

@@ +437,5 @@
> +      "try", "typeof", "undefined", "var", "void", "while", "with",
> +    ];
> +    // This autocompletion is not meaningful when we type a property…
> +    if (identifier.indexOf(".") == -1 && identifier.indexOf("[") == -1 &&
> +        // Thou shalt only complete what there is to complete.

Again funky comment line in the middle of a conditional expression.

@@ +537,5 @@
> +          // Parameters are one level deeper than the function's name itself.
> +          argumentNames(subnode.params, store, stack.length + 1);
> +        }
> +      }
> +      deeper = nestedNodes(subnode, line, column);

Should probably add tests for the supported subnode types too.

@@ +551,5 @@
> +    if (!deeper) {
> +      node = stack.pop();
> +      index = indices.pop();
> +    }
> +  } while (stack.length > 0 || (node && index < node.length) || !!deeper);

This while loop is pretty dense. Probably a recursive approach would simplify things a bit. Not sure about the performance implications of that though.

@@ +640,5 @@
> + */
> +function argumentNames(node, store, weight) {
> +  for (let i = 0; i < node.length; i++) {
> +    store.set(node[i].name, weight);
> +  }

I didn't manage to make these show up in the completion popup.
If they're working, should probably add a test for them too.

@@ +654,5 @@
> +  COMMENT  : 4, // comment
> +
> +  // Relevant character codes.
> +  // May be used as states.
> +  ETHER   : 0,  // ∅

A bit hacky. Probably no real-world implications though.

@@ +779,5 @@
> +function isWhiteSpace(c) {
> +  return (c === 32) || (c === 0x0009) || (c === 0x000B) ||
> +    (c === 0x000C) || (c === 0x00A0) ||
> +    (c >= 0x1680 &&
> +     [0x1680,0x180E,0x2000,0x2001,0x2002,0x2003,0x2004,0x2005,0x2006,0x2007,0x2008,0x2009,0x200A,0x202F,0x205F,0x3000,0xFEFF].indexOf(c) >= 0);

Wouldn't just /\s/.match(c) or /[ \t\r\n]/ work? How many kinds of (JS) whitespace are there?

@@ +795,5 @@
> +          (c > 128 && c != 0xffff && c != 0x2028 && c != 0x2029) ||
> +          // Letters (uppercase, lowercase) are identifier chars.
> +          (c > 0x40 && c < 0x5b) || (c > 0x60 && c < 0x7b) ||
> +          // $ and _ are identifier chars.
> +          c == 0x5f || c == 0x24);

Maybe /[_$a-zA-Z\xA0-\uFFFF]/ is more readable than all those ||s?

PS: And if you'd need to test for a full identifier: ^[_$a-zA-Z\xA0-\uFFFF][_$a-zA-Z0-9\xA0-\uFFFF]*$

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +371,5 @@
>      this.setMode(config.mode);
>  
> +    // The following needs the editor to have its mode set.
> +    this._autocomplete = new Autocompletion(this, {
> +      nbVisibleCompletions: 10,

10 is a beautiful number. Also magic. Put it in a const so everybody can admire it's grandeur.

Or why not show all available completions, have a max-height styled popup and scroll when necessary. Performance?

@@ +372,5 @@
>  
> +    // The following needs the editor to have its mode set.
> +    this._autocomplete = new Autocompletion(this, {
> +      nbVisibleCompletions: 10,
> +      cssClass: "autocomplete",

Nit: maybe use a default value for cssClass in the createPopover function in autocompletion.jsm? Up to you.

@@ +524,5 @@
>        return true;
>      }
>  
> +    // Do autocompletion.
> +    let previousChar = this.getText()[this.getCaretOffset() - 1];

I'm a bit concerned on the performance on getting the whole text in the editor on large files. Could just passing start and end params to getText based on the caret offset be better?

@@ +566,5 @@
> +        && /^[a-zA-Z0-9.$_]$/.test(previousChar)) {
> +      // We have the right to autocomplete now.
> +      this._autocomplete.cycle(-1);
> +      return true;
> +    }

Might want to create a method handling this here and not duplicate the conditional. Maybe something like
if (this._isAutocompletionAllowed()) {
  this._autocomplete.cycle()
  ...
}

Why autocomplete on shift+tab though? Maybe I actually want to unindent instead :)
(In reply to Victor Porof [:vp] from comment #35)
> * Autocompletion is case sensitive. I think it'd be a good idea to have the
> matching as fuzzy as possible, especially starting with making it case
> insensitive (I may be biased because I'm a terrible typer, trading precision
> for speed). Ideally, when I do a foo.BCAN i'd like it to show me
> "BEER_CAN_SOUNDS_LIKE_BACON" and "BREAKPOINT_CHANGE", but I can settle just
> for "window.com" showing "Components" instead of nothing.

Not a bad idea, but I feel it should really be a follow-up!

> * Typing "window.get" and pressing TAB would autocomplete to
> "getComputedStyle" but still display the popup. I'd then need to type ESC or
> another key to hide it.

As documented in a previous comment, this is desired behaviour. When autocompleting, just like in zsh, typing TAB selects the first candidate, and each successive TAB selects the next candidate. Selecting candidates in the dark, without the popup, would result in a suboptimal user experience.

On the other hand, if there is only one candidate, the popup disappears after autocompletion.

> * This was said previously in this bug, but I'll mention it too: the fact
> that UP/DOWN automatically changes the entered string with what's in the
> popup is not something I'd like to happen. UP/DOWN should just jump to the
> next element in the popup, and TAB would do one thing only: finish
> autocompleting and hide the popup. 
> 
> There's a certain degree of "instinct" built in when dealing with how
> autocompletion popups work, and while your proposed interaction is actually
> pretty damn fine, it'll require people detaching from what they learned to
> trust so far. For example my sentiment was something like "Ok, DOWN, DOWN...
> here it is. TAB. Oh? Um.. No, not that. DOWN, DOWN, DOWN. ESC. Hmm, ok then,
> I guess this is ok..".
> 
> Here's a concrete example:
> - "typed windows.co"
> - pressed DOWN till I reached "controllers"
> - pressed TAB (by reflex)
> - the selection jumped to the first entry.
> That's definitely not what I was expecting. My intention was "write that and
> hide the popup", the effect was "select a different thing and don't hide the
> popup" which is the exact opposite :)

This isn't really about jumping to the first entry. When cycling through candidates, TAB selects the next candidate. When on the last candidate, it goes back to the first candidate.

There is absolutely no need to press TAB when you have selected "controllers", since it is already written down.

> * When only one completion option is available, the popup looks like a
> collapsed dropdown. Will the Orion update help us with this?
> 
> ***
> 
> There are a few places in which things aren't working as expected like
> trying to get properties of a global defined in a lazy getter (ex: without
> ever opening the Debugger, writing "DebuggerUI." gives me a "Parsing error:
> missing name after . 

Parsing errors are not to be worried about. If I can't parse, I keep the data I already have.
I didn't notice I left a dump() here, I'll remove it.

> I'm not sure if there's a followup for it, but it seems that the statical
> analysis is not finished yet. If I had a source containing "var foo = { bar:
> 42 }" doing "foo." wouldn't display anything in the popup. Same goes for
> arrays. You'd have to eval the source first. While this is generally ok, I
> think the Parser API is super powerful and we should make full use of it
> especially in these cases.

That is entirely true, there is a lot to add still. Fetching scope variables is the most useful thing for now, and I really want to have a CSS autocompletion working too, so I'll start with that as a follow-up.

> One last thing: when relying on eval for the global scope or current context
> (or even when using the Parser API before any initial eval), having a bit
> more information than just the autocompletion string in the popup would be
> really awesome to have.
> For example, when displaying the function name in the popup, you could also
> show the params in on the right, when dealing with primitives (numbers,
> strings etc.) showing their value, or const if it's the case. This would be
> trivial to add, just regexp your way around the evaled values or dive deep
> into the abstract syntax tree from the Parser API. Take a look into the gist
> from comment #3, here's a screen: http://tinyurl.com/9gehfbn
> Can totally follow up for all of this.

I started with something that had this, but Paul convinced me against it. His driving argument was that flooding the editing workflow with too much information is detrimental to readability and usability.

We can obviously discuss this as a follow-up, but it isn't part of this patch.

> ::: browser/devtools/scratchpad/scratchpad.js
> @@ +409,5 @@
> > +  getSandbox: function SP_getSandbox()
> > +  {
> > +    // Evaluate the sandbox one to get all the autocompletion we can.
> > +    this.evalForContext("");
> > +    return this.executionContext == SCRATCHPAD_CONTEXT_CONTENT ?
> 
> The name of this function is a bit misleading. Either
> getSandboxForAutocompletion here (implying an automatic evalForContext
> beforehand), or just leave it as getSandbox and do a this.evalForContext("")
> manually where necessary. I'd go with the second option.

Ok, can do that.

> :::
> browser/devtools/scratchpad/test/
> browser_scratchpad_bug_762164_autocompletion.js
> @@ +41,5 @@
> > +  EventUtils.synthesizeKey("VK_TAB", {}, gScratchpadWindow);
> > +
> > +  let popover = sp.editor._autocomplete.popover;
> > +  is(popover.style.display, "block",
> > +     "Autocompletion of window.F gives a popover");
> 
> Aren't you displaying the popup after a timeout(,0)? You're probably going
> to get into race conditions if that's the case. Fire an event to handle the
> autocompletion popover showing/hiding.

There's no race condition, it's all running sequentially, for simplicity's sake.

> @@ +46,5 @@
> > + *        ed._mode must be "js".
> > + *        ed._view must be a TextView.
> > + *        ed._model must be a TextModel.
> > + *        ed._undoStack must be an UndoStack.
> > + *        ed.editorElement must be the XUL iframe containing the editor.
> 
> Those are a lot of assertions. Maybe test for those in the constructor and
> throw an Error if something's not right.

I'd rather not, because accessing TextView, TextModel, etc. would require a major refactoring of the source editor component to circumvent a long chain of circular dependencies which had me banging my head on the wall for a week.

> @@ +50,5 @@
> > + *        ed.editorElement must be the XUL iframe containing the editor.
> > + *
> > + * @param object options
> > + *        Properties for tuning certain defaults:
> > + *        - nbVisibleCompletions (defaults to 10): number of visible
> 
> Nit: how about num instead of nb? It took me 4 seconds to figure out what nb
> means.

Sure! 

> @@ +72,5 @@
> > +      if (e.keyCode == 13) {
> > +        this._fireStaticAnalysis = true;
> > +      }
> > +      if (e.charCode > 0) {
> > +        this.document.defaultView.setTimeout(this.displayCompletion.bind(this),0);
> 
> setTimeout is the devil. But sometimes the devil is the only option.
> 
> However, I think I understand the reasoning of waiting for a short delay
> before showing and populating the popup. In that case, why not use a longer
> delay? I don't think anyone would want such an incredibly snappy popup
> flashing around each time I type. Try out a timeout of 50ms (and put it in a
> const if it feels right).
> 
> If that's not necessary, how about
> Services.tm.currentThread.dispatch({ run: yourFuncHere }, 1);
> instead of the timeout(,0)?

The idea was more about dispatching the function in the next event loop cycle. So, importing services.jsm…

> @@ +78,5 @@
> > +    }
> > +  }.bind(this));
> > +  this.editor.addEventListener("Selection", function(e) {
> > +    // If the distance between the old position and the new is bigger than 1,
> > +    // the static analysis is worth updating.
> 
> Heh, this is hacky. Why are you doing this on selection events?
> Not saying this is bad, just wanting to understand what you're trying to do.

I don't want to run the static analysis too often, especially in places where it is likely to fail. When I am typing a newline, it is likely to work and to be useful, since we may change of scope. When I am putting the caret somewhere else, I want it to run, because the scope has very probably changed.

> @@ +112,5 @@
> > +  _end: 0,
> > +  _insertingText: false,
> > +  _completion: null,
> > +  // Static analysis would better be cached.
> > +  _staticCandidates: null,
> 
> Are the completions uncached when not present in the source code anymore?
> This concern is somewhat related to the selection events that trigger static
> analysis.

Static analysis is cached every time it succeeds. This avoids losing the valuable information that it provides every time parsing the code fails.

> @@ +149,5 @@
> > +      html += "<option " + (i == this._index? "selected='true'": "") + ">" +
> > +        completions[i] + "</option>";
> > +    }
> > +    html += "</select>";
> > +    this.popover.innerHTML = html;
> 
> I'd avoid using innerHTML, I'm sure document.createElement would work
> better. Just make sure you're using the correct namespace.

Using innerHTML allows to have only one interaction with the DOM, and allows a much more concise manipulation of code.

Please note that this part is bound to change heavily with the Orion update.

> @@ +160,5 @@
> > +      }
> > +      this.editor._view.focus();
> > +    }.bind(this), false);
> > +    this.popover.firstChild.size =
> > +      Math.min(this.nbVisibleCompletions, completions.length);
> 
> max-height and a scrollbar maybe?

Not sure what you mean here?

> @@ +165,5 @@
> > +
> > +    // Positioning the popover.
> > +    let coord = ed._view.getLocationAtOffset(ed._view.getCaretOffset());
> > +    coord.y += ed._view.getLineHeight();
> > +    ed._view.convert(coord, "document", "page");
> 
> Instead of all this getting of locations, caret offsets and convertions,
> would using a tooltip tied to the editor caret (by calling openPopup with an
> anchor) work better? You can make a custom tooltip item and add whatever you
> want to it: https://developer.mozilla.org/en-US/docs/XUL/tooltip
> 
> On the other hand, maybe this is just trying to make things too elegant.

I don't know much about XUL tooltips, so they scare me. The simple calculations that I have right now work just fine. Maybe a follow-up?

> @@ +180,5 @@
> > +    for (; line + " " !== linesOM[lineIndex].textContent; lineIndex++) {}
> > +    coord.y = insideDoc.defaultView.scrollY +
> > +      linesOM[lineIndex].getBoundingClientRect().top +
> > +      linesOM[lineIndex].scrollHeight;
> > +    // End of Bug 779642 fix.
> 
> Custom tooltips may render this unneeded. If you have the time, it could be
> a good idea to try it out.

Follow-up?

> @@ +385,5 @@
> > +    }
> > +    let allStaticCandidates = this._staticCandidates;
> > +    // Right now, we can only complete variables.
> > +    if (identifier.indexOf(".") == -1 && identifier.indexOf("[") == -1 &&
> > +        // Thou shalt only complete what there is to complete.
> 
> Nit: funky comment line in the middle of a conditional expression. Add it
> after { or before the if.

The comment refers to half of the condition. Having it after the { would make it apply to the first instruction inside the if, having it before would make it apply to the full condition, which it doesn't.

I can remove the comment entirely if you want.

> @@ +398,5 @@
> > +      }
> > +      staticCandidates.sort(function(a, b) {
> > +        // Sort them according to nearest scope.
> > +        return allStaticCandidates.get(b) - allStaticCandidates.get(a);
> > +      });
> 
> This is cool.
> What would be even more awesome is to colorize or somehow delimit the
> candidates based on the scope.
> Just a suggestion, follow up for this if you think it's a good idea.

Yep! Lots of possible follow-up on autocompletion!

> @@ +414,5 @@
> > +        candidates = candidates.concat(sandboxCompletion.matches
> > +            .filter(function(candidate) {
> > +          // We are removing candidates from level 2.
> > +          if (allStaticCandidates == null)  return true;
> > +          if (allStaticCandidates.has(candidate)) {
> 
> Useless if. Are you planning on adding things here later?

Huh. Not sure what that was supposed to be. Maybe a dump().

> @@ +434,5 @@
> > +      "default", "delete", "do", "else", "export", "false", "finally", "for",
> > +      "function", "get", "if", "import", "in", "instanceof", "let", "new",
> > +      "null", "of", "return", "set", "super", "switch", "this", "true", "throw",
> > +      "try", "typeof", "undefined", "var", "void", "while", "with",
> > +    ];
> 
> Did you get those from
> https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Reserved_Words
> ?
> Seems that "each" is missing (maybe some other stuff too). I wouldn't worry
> too much about it though.

No, it's from ES.next :)

> @@ +435,5 @@
> > +      "function", "get", "if", "import", "in", "instanceof", "let", "new",
> > +      "null", "of", "return", "set", "super", "switch", "this", "true", "throw",
> > +      "try", "typeof", "undefined", "var", "void", "while", "with",
> > +    ];
> > +    // This autocompletion is not meaningful when we type a property…
> 
> Hmm, why?
> I'd like to have autocompletion when writing foo["bar

Autocompleting the contents of strings with JS keywords seems like a stretch of the feature.

> @@ +537,5 @@
> > +          // Parameters are one level deeper than the function's name itself.
> > +          argumentNames(subnode.params, store, stack.length + 1);
> > +        }
> > +      }
> > +      deeper = nestedNodes(subnode, line, column);
> 
> Should probably add tests for the supported subnode types too.

I have a test which can only work if the static analysis goes through a lot of different subnode types.

I can make it even more complex, to go through all subnode types, if you want.

> @@ +551,5 @@
> > +    if (!deeper) {
> > +      node = stack.pop();
> > +      index = indices.pop();
> > +    }
> > +  } while (stack.length > 0 || (node && index < node.length) || !!deeper);
> 
> This while loop is pretty dense. Probably a recursive approach would
> simplify things a bit. Not sure about the performance implications of that
> though.

You have no idea how much simpler the recursive algorithm was :)
But yeah, the performance was abysmal.

> @@ +640,5 @@
> > + */
> > +function argumentNames(node, store, weight) {
> > +  for (let i = 0; i < node.length; i++) {
> > +    store.set(node[i].name, weight);
> > +  }
> 
> I didn't manage to make these show up in the completion popup.
> If they're working, should probably add a test for them too.

Can you give me the code you were writing and the position of your caret? I may have missed a possibility.

> @@ +779,5 @@
> > +function isWhiteSpace(c) {
> > +  return (c === 32) || (c === 0x0009) || (c === 0x000B) ||
> > +    (c === 0x000C) || (c === 0x00A0) ||
> > +    (c >= 0x1680 &&
> > +     [0x1680,0x180E,0x2000,0x2001,0x2002,0x2003,0x2004,0x2005,0x2006,0x2007,0x2008,0x2009,0x200A,0x202F,0x205F,0x3000,0xFEFF].indexOf(c) >= 0);
> 
> Wouldn't just /\s/.match(c) or /[ \t\r\n]/ work? How many kinds of (JS)
> whitespace are there?

Unfortunately, in JS, regexes are not designed to work the way the JS parser works.

All allowed ES whitespace characters are taken into account here.

> @@ +795,5 @@
> > +          (c > 128 && c != 0xffff && c != 0x2028 && c != 0x2029) ||
> > +          // Letters (uppercase, lowercase) are identifier chars.
> > +          (c > 0x40 && c < 0x5b) || (c > 0x60 && c < 0x7b) ||
> > +          // $ and _ are identifier chars.
> > +          c == 0x5f || c == 0x24);
> 
> Maybe /[_$a-zA-Z\xA0-\uFFFF]/ is more readable than all those ||s?

If I was to convert this to a regexp, while matching JS strictly, it would weight a lot more than that (see for example https://github.com/ariya/esprima/blob/master/esprima.js#L180). More information at http://qfox.nl/weblog/263.

This being said, the line parser has no pretense of implementing ES completely, even though this is something that I'd like to do. It does take time to write a complete and fast tokenizer.

> @@ +524,5 @@
> >        return true;
> >      }
> >  
> > +    // Do autocompletion.
> > +    let previousChar = this.getText()[this.getCaretOffset() - 1];
> 
> I'm a bit concerned on the performance on getting the whole text in the
> editor on large files. Could just passing start and end params to getText
> based on the caret offset be better?

That is one of the weakest perf optimization that I have done in a while, but sure.

Testing shows that with this change, on the complete source code of jQuery pasted five times (36916 loc), this line of code used to take 3ms and now is no longer measurable :)

> @@ +566,5 @@
> > +        && /^[a-zA-Z0-9.$_]$/.test(previousChar)) {
> > +      // We have the right to autocomplete now.
> > +      this._autocomplete.cycle(-1);
> > +      return true;
> > +    }
> 
> Might want to create a method handling this here and not duplicate the
> conditional. Maybe something like
> if (this._isAutocompletionAllowed()) {
>   this._autocomplete.cycle()
>   ...
> }

Sure!

Obviously it should be more intelligent than a simple regex; making the line parser into a full-fledged tokenizer and putting it into shared/ would help here.

> Why autocomplete on shift+tab though? Maybe I actually want to unindent
> instead :)

Yeah, sure. Except that neither tab nor shift-tab have any effect on indenting when the selection is empty.
Created attachment 655520 [details] [diff] [review]
Autocompletion in the scratchpad.

Taken Victor's comments into account.
Attachment #653587 - Attachment is obsolete: true
Attachment #653587 - Flags: review?(rcampbell)
Attachment #653587 - Flags: review?(mihai.sucan)
Attachment #655520 - Flags: review?(rcampbell)
Attachment #655520 - Flags: review?(mihai.sucan)
(In reply to Thaddee Tyl [:espadrine] from comment #36)
> (In reply to Victor Porof [:vp] from comment #35)
> > * Autocompletion is case sensitive. I think it'd be a good idea to have the
> > matching as fuzzy as possible, especially starting with making it case
> > insensitive (I may be biased because I'm a terrible typer, trading precision
> > for speed). Ideally, when I do a foo.BCAN i'd like it to show me
> > "BEER_CAN_SOUNDS_LIKE_BACON" and "BREAKPOINT_CHANGE", but I can settle just
> > for "window.com" showing "Components" instead of nothing.
> 
> Not a bad idea, but I feel it should really be a follow-up!
> 

Your choice.

> > * Typing "window.get" and pressing TAB would autocomplete to
> > "getComputedStyle" but still display the popup. I'd then need to type ESC or
> > another key to hide it.
> 
> As documented in a previous comment, this is desired behaviour. When
> autocompleting, just like in zsh, typing TAB selects the first candidate,
> and each successive TAB selects the next candidate. Selecting candidates in
> the dark, without the popup, would result in a suboptimal user experience.
> 

Yup, I understand that. Like I said, the behavior is really good, but confusing.
I'd argue about the suboptimal user experience :) The autocompletion in Sublime Text (and any other editor I tried, for that matter), works by using TAB to complete and UP/DOWN to select.

Think about the "certain degree of instinct" that I mentioned. If someone starts writing some code in Scratchpad and the autocompletion doesn't feel familiar at all, then switching back and forth between interaction paradigms is going to be awkward.

> On the other hand, if there is only one candidate, the popup disappears
> after autocompletion.
> 
> > * This was said previously in this bug, but I'll mention it too: the fact
> > that UP/DOWN automatically changes the entered string with what's in the
> > popup is not something I'd like to happen. UP/DOWN should just jump to the
> > next element in the popup, and TAB would do one thing only: finish
> > autocompleting and hide the popup. 
> > 
> > There's a certain degree of "instinct" built in when dealing with how
> > autocompletion popups work, and while your proposed interaction is actually
> > pretty damn fine, it'll require people detaching from what they learned to
> > trust so far. For example my sentiment was something like "Ok, DOWN, DOWN...
> > here it is. TAB. Oh? Um.. No, not that. DOWN, DOWN, DOWN. ESC. Hmm, ok then,
> > I guess this is ok..".
> > 
> > Here's a concrete example:
> > - "typed windows.co"
> > - pressed DOWN till I reached "controllers"
> > - pressed TAB (by reflex)
> > - the selection jumped to the first entry.
> > That's definitely not what I was expecting. My intention was "write that and
> > hide the popup", the effect was "select a different thing and don't hide the
> > popup" which is the exact opposite :)
> 
> This isn't really about jumping to the first entry. When cycling through
> candidates, TAB selects the next candidate. When on the last candidate, it
> goes back to the first candidate.
> 
> There is absolutely no need to press TAB when you have selected
> "controllers", since it is already written down.

Duplicated behavior.
Let's see what others think about this.

> 
> > * When only one completion option is available, the popup looks like a
> > collapsed dropdown. Will the Orion update help us with this?
> > 
> > ***
> > 
> > There are a few places in which things aren't working as expected like
> > trying to get properties of a global defined in a lazy getter (ex: without
> > ever opening the Debugger, writing "DebuggerUI." gives me a "Parsing error:
> > missing name after . 
> 
> Parsing errors are not to be worried about. If I can't parse, I keep the
> data I already have.
> I didn't notice I left a dump() here, I'll remove it.

My comment was referring to the fact that it should have parsed it.
"DebuggerUI."
is a totally viable thing to write, and it exists in the global context.

> 
> > :::
> > browser/devtools/scratchpad/test/
> > browser_scratchpad_bug_762164_autocompletion.js
> > @@ +41,5 @@
> > > +  EventUtils.synthesizeKey("VK_TAB", {}, gScratchpadWindow);
> > > +
> > > +  let popover = sp.editor._autocomplete.popover;
> > > +  is(popover.style.display, "block",
> > > +     "Autocompletion of window.F gives a popover");
> > 
> > Aren't you displaying the popup after a timeout(,0)? You're probably going
> > to get into race conditions if that's the case. Fire an event to handle the
> > autocompletion popover showing/hiding.
> 
> There's no race condition, it's all running sequentially, for simplicity's
> sake.
> 

I saw a setTimeout(this.displayCompletion.bind(this),0); in the patch. Is it not related? (Just making sure).

> > @@ +46,5 @@
> > > + *        ed._mode must be "js".
> > > + *        ed._view must be a TextView.
> > > + *        ed._model must be a TextModel.
> > > + *        ed._undoStack must be an UndoStack.
> > > + *        ed.editorElement must be the XUL iframe containing the editor.
> > 
> > Those are a lot of assertions. Maybe test for those in the constructor and
> > throw an Error if something's not right.
> 
> I'd rather not, because accessing TextView, TextModel, etc. would require a
> major refactoring of the source editor component to circumvent a long chain
> of circular dependencies which had me banging my head on the wall for a week.
> 

This scares me, but ok...

> 
> The idea was more about dispatching the function in the next event loop
> cycle. So, importing services.jsm…

I think n this case you'd definitely need events for tests.

> 
> > @@ +78,5 @@
> > > +    }
> > > +  }.bind(this));
> > > +  this.editor.addEventListener("Selection", function(e) {
> > > +    // If the distance between the old position and the new is bigger than 1,
> > > +    // the static analysis is worth updating.
> > 
> > Heh, this is hacky. Why are you doing this on selection events?
> > Not saying this is bad, just wanting to understand what you're trying to do.
> 
> I don't want to run the static analysis too often, especially in places
> where it is likely to fail. When I am typing a newline, it is likely to work
> and to be useful, since we may change of scope. When I am putting the caret
> somewhere else, I want it to run, because the scope has very probably
> changed.

Is "Selection" a good event to handle this though? Mihai is best to decide this.

> 
> > @@ +112,5 @@
> > > +  _end: 0,
> > > +  _insertingText: false,
> > > +  _completion: null,
> > > +  // Static analysis would better be cached.
> > > +  _staticCandidates: null,
> > 
> > Are the completions uncached when not present in the source code anymore?
> > This concern is somewhat related to the selection events that trigger static
> > analysis.
> 
> Static analysis is cached every time it succeeds. This avoids losing the
> valuable information that it provides every time parsing the code fails.
> 

My question was: what happens after I delete the code?

> > @@ +149,5 @@
> > > +      html += "<option " + (i == this._index? "selected='true'": "") + ">" +
> > > +        completions[i] + "</option>";
> > > +    }
> > > +    html += "</select>";
> > > +    this.popover.innerHTML = html;
> > 
> > I'd avoid using innerHTML, I'm sure document.createElement would work
> > better. Just make sure you're using the correct namespace.
> 
> Using innerHTML allows to have only one interaction with the DOM, and allows
> a much more concise manipulation of code.

It's also frowned upon. If performance is so critical here (a thing for which innerHTML I'm sure is definitely not a good idea), then use DocumentFragments.

> > @@ +160,5 @@
> > > +      }
> > > +      this.editor._view.focus();
> > > +    }.bind(this), false);
> > > +    this.popover.firstChild.size =
> > > +      Math.min(this.nbVisibleCompletions, completions.length);
> > 
> > max-height and a scrollbar maybe?
> 
> Not sure what you mean here?

Why display 10 autocompletions? Why not show them all, use a max-height for the popup and display a scrollbar.

> 
> > @@ +165,5 @@
> > > +
> > > +    // Positioning the popover.
> > > +    let coord = ed._view.getLocationAtOffset(ed._view.getCaretOffset());
> > > +    coord.y += ed._view.getLineHeight();
> > > +    ed._view.convert(coord, "document", "page");
> > 
> > Instead of all this getting of locations, caret offsets and convertions,
> > would using a tooltip tied to the editor caret (by calling openPopup with an
> > anchor) work better? You can make a custom tooltip item and add whatever you
> > want to it: https://developer.mozilla.org/en-US/docs/XUL/tooltip
> > 
> > On the other hand, maybe this is just trying to make things too elegant.
> 
> I don't know much about XUL tooltips, so they scare me. The simple
> calculations that I have right now work just fine. Maybe a follow-up?
> 

Pretty hacky, but ok...

> > 
> > Hmm, why?
> > I'd like to have autocompletion when writing foo["bar
> 
> Autocompleting the contents of strings with JS keywords seems like a stretch
> of the feature.

What would an argument for this being a stretch be?
I make full use of it in my editor :)

> 
> > Why autocomplete on shift+tab though? Maybe I actually want to unindent
> > instead :)
> 
> Yeah, sure. Except that neither tab nor shift-tab have any effect on
> indenting when the selection is empty.

I believe it does.
I wrote "    test" in a scratchpad, pressed SHIFT+TAB and it unindented it for me.
(In reply to Paul Rouget [:paul] from comment #5)
> (In reply to Thaddee Tyl [:espadrine] from comment #4)
> > Does anyone know why "CTRL+Space" shows the contextual menu on MacOS X? (and
> > how to deactivate that behaviour?)
> 
> Ctrl-Space is the standard way to bring the context menu on Linux. And
> apparently, on Mac too.

Ctrl-Space does nothing on my Ubuntu, but I do have a Context Menu key on my keyboard.

I can bring up the window menu with Alt-Space. Same as on Windows.


From Victor's feedback and Thaddee's responses:

- I agree with the getText() optimization. Getting the entire text has the potential to be slower than retrieving the relevant chunk.

- Agree you should use a document fragment instead of innerHTML. You can avoid touching the live DOM.

- Agree with the suggested behavior (Up/Down for selection and Tab for completion + hide popup). zsh is not Scratchpad and it doesn't have an autocompletion popup. You are right that there it makes sense to insert the completion at the cursor, in the editor, while you cycle through the possible completion items. In Scratchpad we have a popup, so we don't need to update the editor. (maybe I am wrong! buut it seems victor agrees)


(In reply to Thaddee Tyl [:espadrine] from comment #36)
> (In reply to Victor Porof [:vp] from comment #35)
> > @@ +46,5 @@
> > > + *        ed._mode must be "js".
> > > + *        ed._view must be a TextView.
> > > + *        ed._model must be a TextModel.
> > > + *        ed._undoStack must be an UndoStack.
> > > + *        ed.editorElement must be the XUL iframe containing the editor.
> > 
> > Those are a lot of assertions. Maybe test for those in the constructor and
> > throw an Error if something's not right.
> 
> I'd rather not, because accessing TextView, TextModel, etc. would require a
> major refactoring of the source editor component to circumvent a long chain
> of circular dependencies which had me banging my head on the wall for a week.

Can you please elaborate which issues you had? Also, what kind of refactoring would you have in mind? Maybe in an email - please CC Rob as well. I'm all ears for improving the source editor.


> > @@ +149,5 @@
> > > +      html += "<option " + (i == this._index? "selected='true'": "") + ">" +
> > > +        completions[i] + "</option>";
> > > +    }
> > > +    html += "</select>";
> > > +    this.popover.innerHTML = html;
> > 
> > I'd avoid using innerHTML, I'm sure document.createElement would work
> > better. Just make sure you're using the correct namespace.
> 
> Using innerHTML allows to have only one interaction with the DOM, and allows
> a much more concise manipulation of code.
> 
> Please note that this part is bound to change heavily with the Orion update.

I'd like to know why this part of code needs to change a lot with the Orion update.
More notes taken during testing:

- It's cool autocomplete popup shows as you type, but this is also a reason to find it "too much". If I start typing a lot of stuff, the popup stands in my way. Everything I type can be autocompleted. I'd like to have it shown when I need it.
- Case sensitive. This was an issue I quickly noticed.
- Behavior is somewhat unexpected:
  - Type |window.get|. I see getComputedStyle listed first and selected and I press Down. I expect to have getInterface selected (the second item in the popup). Instead, selection stays at the first element in the popup and it does autocomplete at the cursor, in the editor - I see getComputedStyle. To have getInterface I need to press Down twice, which feels awkward.
  - continuing from the example above: once I start cycling through the autocomplete items I cannot cancel the autocompletion: I want to have "window.get". I can't. Escape doesn't do that and I'm required to backspace whatever was autocompleted. Am I missing something?
  - It is not obvious how to get rid of the autocompletion popup, once I selected what I want. Tab just cycles through results/items.

Behavior suggestions:

- Do not autocomplete by default - trigger autocompletion after 3 letters, or require something like Ctrl-Space. I know, this is a tough choice, but should be a better UX overall.
- When you cycle through the autocomplete list do not change the editor text, just select the item in the list. This allows expected behavior for Up/Down in the list and it allows a key like Tab to finalize autocompletion - it puts the selected item in the editor and hides the popup. (I wrote this note before I read Victor's feedback, now I see Victor asked for the same behavior)
- Allow Esc to cancel autocomplete, so it puts the initial text back - without any autocompletion changes. If the previous point is implemented, then this is not needed - since no changes occur in the editor anyway - Esc just needs to close the popup (as it does now).

(more comments will follow)
(In reply to Mihai Sucan [:msucan] from comment #40)
> - Do not autocomplete by default - trigger autocompletion after 3 letters,
> or require something like Ctrl-Space. I know, this is a tough choice, but
> should be a better UX overall.

It is unclear to me what "after 3 letters" should behave like (or, if I understand it correctly, it feels very random to show the popup every 3 letters typed, until we autocomplete, and back again).

Ctrl-Space is not discoverable. I have seen users utterly fail to find autocompletion back when that was what triggered it. They tried to find it for minutes before giving up. The very last thing I want is that users struggle to find the autocompletion feature. Some try Esc, some try Ctrl-N, some try Ctrl or Alt-Enter, some even Ctrl-J (I'm still not sure what that one was about), nearly all end up trying Tab. I am fairly confident that there are more autocompletion keybinding expectations than there are stars in the night sky. However, Tab is the common ground.

I guess that what I'm trying to say is that, unlike what even I believed, people don't know about the Ctrl-Space binding. Again, I started with that key binding. I had to change it. Now, everyone finds the feature.

Ctrl-Space has other issues that I talked about, related to how Macs work.

Victor suggested having a delay before displaying the popup. As an experiment, my next patch will have a delay of one second. I thought it might feel nice, but after using it for five minutes, it just feels annoyingly slow. 

>   - Type |window.get|. I see getComputedStyle listed first and selected
> and I press Down. I expect to have getInterface selected
> (the second item in the popup). Instead, selection stays
> at the first element in the popup and it does autocomplete
> at the cursor, in the editor - I see getComputedStyle.
> To have getInterface I need to press Down twice, which feels awkward.

Having the downward arrow key go to the second item instead of the first does make sense, so I  followed your advice and added that to the latest patch.

> - Allow Esc to cancel autocomplete, so it puts the initial text back -
> without any autocompletion changes. If the previous point is implemented,
> then this is not needed - since no changes occur in the editor anyway - Esc
> just needs to close the popup (as it does now).

It feels like users will end up finding that Ctrl-Z does that, so this is the behaviour I implemented.

>   - It is not obvious how to get rid of the autocompletion popup, once I
> selected what I want. Tab just cycles through results/items.

You mention that you've noticed that Esc does just that, which is exactly the intended discoverability.
Victor has asked some questions in one of the previous comments, and I am also looking forward to your replies.


(In reply to Thaddee Tyl [:espadrine] from comment #41)
> (In reply to Mihai Sucan [:msucan] from comment #40)
> > - Do not autocomplete by default - trigger autocompletion after 3 letters,
> > or require something like Ctrl-Space. I know, this is a tough choice, but
> > should be a better UX overall.
> 
> It is unclear to me what "after 3 letters" should behave like (or, if I
> understand it correctly, it feels very random to show the popup every 3
> letters typed, until we autocomplete, and back again).

I've seen other editors do this: autocomplete only after you type N letters in a word/token (where N is 2 or 3). You don't need to have autocomplete be too eager. You need balance.


> Ctrl-Space is not discoverable. I have seen users utterly fail to find
> [...]

While I agree, we do rely on some not-too-obvious but very well known behaviors in the developer tools world. Ctrl-Space (or any common combination) would not surprise devs.

I do not hold strongly to this suggestion - if we agree to have the autocompletion popup show as-you-type, then we need to make it less eager - less "in your face".


> Victor suggested having a delay before displaying the popup. As an
> experiment, my next patch will have a delay of one second. I thought it
> might feel nice, but after using it for five minutes, it just feels
> annoyingly slow. 

I can imagine. One second is several hundreds of milliseconds too late. Try lowering the delay to 250ms, or less.

Maybe this delay thing combined with the above suggestion of having the popup show after 2 letters in a token, will make the popup more conservative in when it shows.


> > - Allow Esc to cancel autocomplete, so it puts the initial text back -
> > without any autocompletion changes. If the previous point is implemented,
> > then this is not needed - since no changes occur in the editor anyway - Esc
> > just needs to close the popup (as it does now).
> 
> It feels like users will end up finding that Ctrl-Z does that, so this is
> the behaviour I implemented.

This is not what I expect. You add an extra step: undo the completion. It also involves finding you need to use undo (ctrl-z).


> >   - It is not obvious how to get rid of the autocompletion popup, once I
> > selected what I want. Tab just cycles through results/items.
> 
> You mention that you've noticed that Esc does just that, which is exactly
> the intended discoverability.

This breaks my expectations as a user. You add a key I need to press, to close the popup.
> > >   - It is not obvious how to get rid of the autocompletion popup, once I
> > > selected what I want. Tab just cycles through results/items.
> > 
> > You mention that you've noticed that Esc does just that, which is exactly
> > the intended discoverability.
> 
> This breaks my expectations as a user. You add a key I need to press, to
> close the popup.

That's exactly the point I'm also trying to make.
People enjoy tools that map their intuition/instinct more. Even if the interaction may be more or less of the same fluidity, users don't like rewiring their fingers to noticeably different interaction schemes.
Comment on attachment 655520 [details] [diff] [review]
Autocompletion in the scratchpad.

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

General comments/concerns:

- We did an autocompletion popup implementation JSM for the Web Console. If I recall correctly, that is not tied directly to the way the Web Console works. Would it be possible to reuse the autocomplete popup implementation from the Web Console? I know it's ugly, but with some CSS I believe it can look good.

Why I suggest this: it will become harder/less nice to maintain two different code paths, both with the same purpose.

- function arguments need to be prefixed with "a", foo becomes aFoo.
- methods have their curly braces on a new line: |method: function method()\n{|.
- not all methods defined have function names.

More comments below.

(review is incomplete. I still need to look more into autocompletion.jsm.)

::: browser/devtools/sourceeditor/autocompletion.jsm
@@ +44,5 @@
> + * Autocompletion object.
> + * This constructor creates the necessary DOM popup.
> + *
> + * @param object ed
> + *        The Orion editor to target.

s/Orion editor/Source Editor instance/

@@ +48,5 @@
> + *        The Orion editor to target.
> + *        ed._mode must be "js".
> + *        ed._view must be a TextView.
> + *        ed._model must be a TextModel.
> + *        ed._undoStack must be an UndoStack.

I'm worried that this approach is problematic. You require an object, a Source Editor instance, which needs to give you access to private properties and methods.

What I could suggest now:

(1) look into adding public properties/methods that are missing in the source editor, so you can avoid accessing the private ones.
(2) provide a custom object from the source editor, like this:
let providers = {
  mode: this.mode,
  view: this._view,
  model: this._model,
  undoStack: this._undoStack,
};
let foo = new Autocompletion(providers, options);

This gives you a well known list of private properties and we control it from the source editor - the Autocompletion object is no longer directly tied to the private properties.

I would prefer approach (1). After looking into most of the code it seems we need most of the methods you use to be available as public methods on the source editor object.

@@ +118,5 @@
> +   */
> +  displayCompletion: function AC_displayCompletion() {
> +    if (this._completion == null) {
> +      // The following line may be computationally intensive.
> +      this._completion = COMPLETERS[this.editor._mode].bind(this)();

This easily breaks if we add a new mode to the Source Editor that is not known here. Please make sure it does not break with unknown editor modes.

@@ +154,5 @@
> +    ed._view.convert(coord, "document", "page");
> +    if (!this._cycling) {
> +      this.popover.style.left = coord.x + "px";
> +    }
> +    // Bug 779642 forces me to add this bit of fixing…

We need to remove this fix from here, once bug 779642 lands. I suggest adding that patch in the queue.

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +11,4 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource:///modules/source-editor-ui.jsm");
> +Cu.import("resource:///modules/autocompletion.jsm");

Please put this new jsm in modules/devtools/.

@@ +252,4 @@
>                            SourceEditor.DEFAULTS[key];
>      }
>  
> +    this._getSandbox = aConfig.getSandbox || null;

|| null is needed?

@@ +472,5 @@
>  
>    /**
> +   * @return boolean
> +   *         True if we should be allowed to launch the autocompletion system.
> +   * @private

Missing method description and list of arguments.

Also, english nit: "True if we should allow autocompletion, otherwise false".

nit: @private comes before arguments and @return.

@@ +570,5 @@
>        indent = (new Array(this._tabSize + 1)).join(" ");
>      }
>  
> +    // Do autocompletion.
> +    let previousChar = this.getText()[this.getCaretOffset() - 1];

Like in the other case, please give getText() a start and an end.

::: browser/devtools/sourceeditor/test/browser_sourceeditor_initialization.js
@@ -170,5 @@
>  
> -  EventUtils.synthesizeKey(".", {}, testWin);
> -  EventUtils.synthesizeKey("VK_TAB", {}, testWin);
> -
> -  is(editor.getText(), "code-ed..     aitor", "Tab works");

Shouldn't in this case Tab actually insert a tab, since there's nothing to autocomplete? What happens in other editors?
Attachment #655520 - Flags: review?(mihai.sucan)
Created attachment 655910 [details] [diff] [review]
Autocompletion in the scratchpad.

I have taken Mihai's code review into account. I'd like a review from Rob…
Attachment #655520 - Attachment is obsolete: true
Attachment #655520 - Flags: review?(rcampbell)
Attachment #655910 - Flags: review?(rcampbell)
(Reporter)

Comment 46

6 years ago
Comment on attachment 655910 [details] [diff] [review]
Autocompletion in the scratchpad.

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

need to spend more time digging through autocompletion.jsm. That's a big one. Impressive work so far!

::: browser/devtools/sourceeditor/autocompletion.jsm
@@ +28,5 @@
> +  let document = aDocument;
> +  let cssClass = aCssClass;
> +  let popover = document.createElement("div");
> +  popover.classList.add("popover");
> +  if (cssClass) {

this is going to always be at least "autocomplete" is it not?

@@ +31,5 @@
> +  popover.classList.add("popover");
> +  if (cssClass) {
> +    popover.classList.add(cssClass);
> +  }
> +  popover.style.position = "absolute";

are these necessary? Can't you just add the styles to the stylesheet?

@@ +45,5 @@
> +/**
> + * Autocompletion object.
> + * This constructor creates the necessary DOM popup.
> + *
> + * @param object aEd

could we call this aEditor? Use your words! :)

@@ +62,5 @@
> +function Autocompletion(aEd, aOptions = {}) {
> +  this.numVisibleCompletions = aOptions.numVisibleCompletions ||
> +      NUM_VISIBLE_COMPLETIONS;
> +  this.getSandbox = aOptions.getSandbox || null;
> +  if (this.getSandbox) {

can this work without this.getSandbox being initialized? I wonder if this should just return early if (!this.getSandbox).

@@ +81,5 @@
> +}
> +
> +Autocompletion.prototype = {
> +
> +  // The following set the way in which the autocompletion system works.

for once, I think we can remove a comment. Not really needed here. If you really want to have a comment, maybe describe what "getSandbox" is and what it's for.

::: browser/devtools/webconsole/WebConsoleUtils.jsm
@@ +996,3 @@
>  {
>    // Argument defaults.
> +  aOptions = aOptions || {matchProp: ""};

this is a bit of a puzzler. Why this change? Why did we need the aOptions.matchProp = aOptions.matchProp || "" check at all in the previous version? Can we not just set a default and not worry about it?
(In reply to Rob Campbell [:rc] (:robcee) from comment #46)
> need to spend more time digging through autocompletion.jsm. That's a big
> one. Impressive work so far!

Thanks!

> ::: browser/devtools/sourceeditor/autocompletion.jsm
> @@ +28,5 @@
> > +  let document = aDocument;
> > +  let cssClass = aCssClass;
> > +  let popover = document.createElement("div");
> > +  popover.classList.add("popover");
> > +  if (cssClass) {
> 
> this is going to always be at least "autocomplete" is it not?



> @@ +31,5 @@
> > +  popover.classList.add("popover");
> > +  if (cssClass) {
> > +    popover.classList.add(cssClass);
> > +  }
> > +  popover.style.position = "absolute";
> 
> are these necessary? Can't you just add the styles to the stylesheet?

Since I'm not and cannot be on the same iframe as the editor, we'd need to add stylesheets specifically for this.
This however would be fixable with the Orion update.

> @@ +62,5 @@
> > +function Autocompletion(aEd, aOptions = {}) {
> > +  this.numVisibleCompletions = aOptions.numVisibleCompletions ||
> > +      NUM_VISIBLE_COMPLETIONS;
> > +  this.getSandbox = aOptions.getSandbox || null;
> > +  if (this.getSandbox) {
> 
> can this work without this.getSandbox being initialized? I wonder if this
> should just return early if (!this.getSandbox).

It actually can work without a sandbox. It doesn't do dynamic autocompletion by looking up properties, but it can still do the other levels.

> ::: browser/devtools/webconsole/WebConsoleUtils.jsm
> @@ +996,3 @@
> >  {
> >    // Argument defaults.
> > +  aOptions = aOptions || {matchProp: ""};
> 
> this is a bit of a puzzler. Why this change? Why did we need the
> aOptions.matchProp = aOptions.matchProp || "" check at all in the previous
> version? Can we not just set a default and not worry about it?

There is a bug… really faint to see… before this change I used to hit it…

https://bugzilla.mozilla.org/show_bug.cgi?id=784197

I cannot even imagine what happens in the JS engine there.
Comment on attachment 655910 [details] [diff] [review]
Autocompletion in the scratchpad.

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

Thank you for the updated patch. I like the source editor changes.

General comments:

- While playing with Scratchpad I sometimes get the following in the error console:

Timestamp: 29/08/2012 18:14:48
Error: TypeError: source is undefined
Source File: resource:///modules/devtools/autocompletion.jsm
Line: 729

- In the Style Editor after typing a bit, I get:

Timestamp: 29/08/2012 18:24:35
Error: TypeError: this._completion is null
Source File: resource:///modules/devtools/autocompletion.jsm
Line: 131

- In one of my previous comments I asked about the AutocompletePopup.jsm if it could be reused. Any thoughts? After completely reviewing Autocompletion.jsm it feels like there's quite some code duplication without any obvious benefits.

- The approach you used for some methods where you changed argument names then you do |let foo = aFoo| is not ideal.

- Please add source editor tests for all of the new API.

- The source editor autocompletion test needs to do more testing. Please play with arrow keys, Escape, Shift-Tab and with the |getSandbox| config. Also poke with other editor modes.

- I suggest that all of the global functions from autocompletion.jsm that are related to the JS completer should be grouped into one object, a JSCompleter object. This will help us with adding new completers - less noise in the global scope.

- All tests pass. This is cool!


Autocompletion behavior:

- lower the delay, try 100ms.
- in one of the comments above we talked about undoing to what I had before starting autocompletion. I mentioned that Escape only closes the popup and there's no way for me to get back to what I had in the editor before completion changes. You suggested I use Ctrl-Z and I tried. After I press Down several times to see the possible completions I decide to press Escape and Ctrl-Z. The result is I have to Ctrl-Z for each of the items I went through. One Ctrl-Z is not sufficient...

More comments below...

::: browser/devtools/sourceeditor/autocompletion.jsm
@@ +20,5 @@
> + * @param string aCssClass
> + *        The CSS class that you want it to have (optional).
> + *        Defaults to "autocomplete".
> + *
> + * @return object

Looks like that's a stray empty comment line.

@@ +76,5 @@
> +  this.editor.addEventListener("Selection", this._selection.bind(this));
> +
> +  // Those will become event listeners.
> +  this._stopBound = this.stop.bind(this);
> +  this._keyBindingsBound = this._keyBindings.bind(this);

For private methods you could just do:
this._keyBindings = this._keyBindings.bind(this);

You don't need to keep the unbound function.

@@ +93,5 @@
> +
> +  // When the autocompletion is triggered, it is "on",
> +  // and the index gives us the selected candidate.
> +  _on: false,
> +  _cycling: false,

_on, _cycling, _insertingText and _fireStaticAnalysis could be confusing flags. Can you please add comments for each? Why are they needed?

@@ +191,5 @@
> +    this._on = true;
> +  },
> +
> +  // Specific autocompletion-only keys.
> +  _keyBindings: function AC__keyBindings(aEvent)

Maybe rename this to _onEditorKeyDown()? Currently the method name is confusing. Also, a description of the method would be useful.

@@ +219,5 @@
> +      }
> +    }
> +  },
> +
> +  _keyPress: function AC__keyPress(aEvent)

Similar request here: rename to something like _onEditorKeyPress.

@@ +234,5 @@
> +      }
> +    }
> +  },
> +
> +  _selection: function AC__selection(aEvent)

_onEditorSelection

@@ +305,5 @@
> +      this.runCompleters();
> +      if (count == 0) { count = 1; }
> +      if (count > 0) {
> +        // We can start from the beginning.
> +        this._index = count - 1;

This is confusing. Why if (count == 0) { count = 1; } ?

@@ +565,5 @@
> +          subnode.type == "FunctionExpression" ||
> +          // Expressions, eg, (function(){…}());
> +          (subnode.callee && subnode.callee.type == "FunctionExpression")) {
> +        if (subnode.callee)  { subnode = subnode.callee; }
> +        if (subnode.id)  { store.set(subnode.id.name, stack.length); }

Can you please put these on new lines?

if (foo) {
  bar();
}

... to make the code easier to follow.

@@ +752,5 @@
> +      if (c == STATES.QUOTE || c == STATES.DQUOTE || c == STATES.SLASH)  {
> +        setState(c);
> +      }
> +      // Unicode identifiers, eg. |var \u0042 = 42;|.
> +      else if (c == STATES.BSLASH)  { setState(STATES.IDEN); i += 5; }

Please always keep code on new lines, after conditionals, to allow for easier reading.

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +252,4 @@
>                            SourceEditor.DEFAULTS[key];
>      }
>  
> +    this._getSandbox = aConfig.getSandbox;

Please explain the new configuration option in SourceEditor.DEFAULTS. Maybe rename this to getScriptSandbox.

@@ +370,5 @@
>  
>      this.setMode(config.mode);
>  
> +    // The following needs the editor to have its mode set.
> +    this._autocomplete = new Autocompletion(this, {

Do we need to create the autocompletion object instance in all cases? This should be avoided for unsupported modes.

Maybe we also need an option to enable/disable autocompletion in the Source Editor - since don't want to always enable completion. Default should be off.

@@ +542,5 @@
>      }
>  
> +    // Do autocompletion.
> +    let previousChar = this.getText(this.getCaretOffset() - 1,
> +        this.getCaretOffset());

nit: alignment is odd. We usually align the arguments.

let previousChar = this.getText(this.getCaretOffset() - 1,
                             this.getCaretOffset());

@@ +1852,5 @@
> +   * Returns the {x, y} pixel location of the top-left corner of the character
> +   * bounding box at the specified offset in the document.  The pixel location
> +   * is relative to the document.
> +   *
> +   * @param number offset

aOffset

@@ +1855,5 @@
> +   *
> +   * @param number offset
> +   *        The character offset
> +   * @returns object
> +   *          The {x, y} pixel location of the given offset.

s/returns/return/ (for consistency, in all places)

Also, please mention the location is relative to the document in the @return as well.

@@ +1864,5 @@
> +  },
> +
> +  /**
> +   * Returns the line height for a given line index.  Returns the default line
> +   * height if the line index is not specified.

Return information should be in @return

@@ +1866,5 @@
> +  /**
> +   * Returns the line height for a given line index.  Returns the default line
> +   * height if the line index is not specified.
> +   *
> +   * @param number lineIndex

aLineIndex

@@ +1878,5 @@
> +  },
> +
> +  /**
> +   * Converts the given rectangle from coordinates relative to the document (the
> +   * origin being hte top-left corner of the first line) to coordinates

s/hte/the/

@@ +1879,5 @@
> +
> +  /**
> +   * Converts the given rectangle from coordinates relative to the document (the
> +   * origin being hte top-left corner of the first line) to coordinates
> +   * relative to the html page.

relative to the editor view.

(in our case the HTML page is the editor view)

@@ +1882,5 @@
> +   * origin being hte top-left corner of the first line) to coordinates
> +   * relative to the html page.
> +   *
> +   * @param object aCoord
> +   *        An object with properties x and y.

Please mention the object is updated to hold the converted coordinates.

@@ +1884,5 @@
> +   *
> +   * @param object aCoord
> +   *        An object with properties x and y.
> +   */
> +  convertCoordFromDocumentToPage: function SE_convertCoordFromDocumentToPage(aCoord)

convertDocumentToView() ? not sure...

::: browser/devtools/sourceeditor/test/browser_sourceeditor_autocompletion.js
@@ +17,5 @@
> +  waitForExplicitFinish();
> +
> +  const windowUrl = "data:text/xml,<?xml version='1.0'?>" +
> +    "<window xmlns='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul'" +
> +    " title='test for bug 660784' width='600' height='500'><hbox flex='1'/></window>";

please update the bug number or say 'test for autocompletion'

@@ +38,5 @@
> +
> +function editorLoaded()
> +{
> +  // JS autocompletion.
> +  editor.setMode(SourceEditor.MODES.JAVASCRIPT);

You can set the mode from editor.init() above.

@@ +65,5 @@
> +  editor.setText(before + after);
> +  editor.setCaretOffset(before.length);
> +  EventUtils.synthesizeKey("VK_TAB", {}, testWin);
> +  is(editor.getText(), before + "bar" + after,
> +     "Deep static JS autocompletion.");

Is this the result of static JS analysis?
(Reporter)

Comment 49

6 years ago
(In reply to Mihai Sucan [:msucan] from comment #48)
> Comment on attachment 655910 [details] [diff] [review]
> Autocompletion in the scratchpad.
> 
> Review of attachment 655910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for the updated patch. I like the source editor changes.
> 
> General comments:
> 
> - While playing with Scratchpad I sometimes get the following in the error
> console:
> 
> Timestamp: 29/08/2012 18:14:48
> Error: TypeError: source is undefined
> Source File: resource:///modules/devtools/autocompletion.jsm
> Line: 729
> 
> - In the Style Editor after typing a bit, I get:
> 
> Timestamp: 29/08/2012 18:24:35
> Error: TypeError: this._completion is null
> Source File: resource:///modules/devtools/autocompletion.jsm
> Line: 131

We'll need these fixed up.

> - In one of my previous comments I asked about the AutocompletePopup.jsm if
> it could be reused. Any thoughts? After completely reviewing
> Autocompletion.jsm it feels like there's quite some code duplication without
> any obvious benefits.

I mentioned this to Thaddée yesterday after talking to you. I think I'd like to see that get reused as well. It'll make for a smaller patch and we can restyle the AutocompletePopup.jsm panel after it goes in.

> - The approach you used for some methods where you changed argument names
> then you do |let foo = aFoo| is not ideal.

agreed. Didn't like that either.


> - Please add source editor tests for all of the new API.
> 
> - The source editor autocompletion test needs to do more testing. Please
> play with arrow keys, Escape, Shift-Tab and with the |getSandbox| config.
> Also poke with other editor modes.
> 
> - I suggest that all of the global functions from autocompletion.jsm that
> are related to the JS completer should be grouped into one object, a
> JSCompleter object. This will help us with adding new completers - less
> noise in the global scope.
> 
> - All tests pass. This is cool!
> 
> 
> Autocompletion behavior:
> 
> - lower the delay, try 100ms.

or 200ms. 100 might be just on the too-fast side.

Going to cancel the review request until these (and Mihai's other) comments are addressed.
(Reporter)

Updated

6 years ago
Attachment #655910 - Flags: review?(rcampbell)
(Reporter)

Comment 50

6 years ago
also mentioned that we will need a "killswitch" pref. Suggest devtools.scratchpad.autocomplete.enabled = true/false to enable or disable the feature.
(In reply to Mihai Sucan [:msucan] from comment #48)
> Comment on attachment 655910 [details] [diff] [review]
> Autocompletion in the scratchpad.
> 
> Review of attachment 655910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for the updated patch. I like the source editor changes.
> 
> General comments:
> 
> - While playing with Scratchpad I sometimes get the following in the error
> console:
> 
> Timestamp: 29/08/2012 18:14:48
> Error: TypeError: source is undefined
> Source File: resource:///modules/devtools/autocompletion.jsm
> Line: 729
> 
> - In the Style Editor after typing a bit, I get:
> 
> Timestamp: 29/08/2012 18:24:35
> Error: TypeError: this._completion is null
> Source File: resource:///modules/devtools/autocompletion.jsm
> Line: 131

I'd love to have more information on these errors. I'm trying to reproduce them.

> - In one of my previous comments I asked about the AutocompletePopup.jsm if
> it could be reused. Any thoughts? After completely reviewing
> Autocompletion.jsm it feels like there's quite some code duplication without
> any obvious benefits.

The popup could be reused, but I'd love this to be a follow-up. The look and feel of the AutocompletePopup is aesthetically incompatible with the source editor right now (although it does fit the web console).

> - The approach you used for some methods where you changed argument names
> then you do |let foo = aFoo| is not ideal.

Yes, I wrote a fast macro to automate the change, but it wasn't ideal :)
Consider it fixed.

> - Please add source editor tests for all of the new API.

Will do.

> - lower the delay, try 100ms.

I've put 100ms in the patch to come, and it does feel better than 200ms, while not being fast enough to show up while typing.

> - in one of the comments above we talked about undoing to what I had before
> starting autocompletion. I mentioned that Escape only closes the popup and
> there's no way for me to get back to what I had in the editor before
> completion changes. You suggested I use Ctrl-Z and I tried. After I press
> Down several times to see the possible completions I decide to press Escape
> and Ctrl-Z. The result is I have to Ctrl-Z for each of the items I went
> through. One Ctrl-Z is not sufficient...

This is a very bad regression! (and it is fixed, along with a couple other regressions I noticed, in the next patch.)

> @@ +305,5 @@
> > +      this.runCompleters();
> > +      if (count == 0) { count = 1; }
> > +      if (count > 0) {
> > +        // We can start from the beginning.
> > +        this._index = count - 1;
> 
> This is confusing. Why if (count == 0) { count = 1; } ?

I changed that part to make it clearer that all we strive to avoid here is having a negative index.

> @@ +565,5 @@
> > +          subnode.type == "FunctionExpression" ||
> > +          // Expressions, eg, (function(){…}());
> > +          (subnode.callee && subnode.callee.type == "FunctionExpression")) {
> > +        if (subnode.callee)  { subnode = subnode.callee; }
> > +        if (subnode.id)  { store.set(subnode.id.name, stack.length); }
> 
> Can you please put these on new lines?
> 
> if (foo) {
>   bar();
> }
> 
> ... to make the code easier to follow.

Obviously I thought it made the code easier to read for me, but I changed it for you ;)

> Maybe we also need an option to enable/disable autocompletion in the Source
> Editor - since don't want to always enable completion. Default should be off.

I agree with having the possibility to turn it off, but I believe it should be on by default.

I strongly believe that features that are off by default are never seen by non-mozillians (and then, only a small percentage of mozillians). You'd be surprised how many people amongst mozilla employees I've taught how to enable chrome mode in the scratchpad.

I'd be really sad to see this feature never get used (or get used only two years from now). I'm quite proud of how it turned out.

> @@ +1884,5 @@
> > +   *
> > +   * @param object aCoord
> > +   *        An object with properties x and y.
> > +   */
> > +  convertCoordFromDocumentToPage: function SE_convertCoordFromDocumentToPage(aCoord)
> 
> convertDocumentToView() ? not sure...

It doesn't change the document at all, it just changes 'aCoord'.

> @@ +38,5 @@
> > +
> > +function editorLoaded()
> > +{
> > +  // JS autocompletion.
> > +  editor.setMode(SourceEditor.MODES.JAVASCRIPT);
> 
> You can set the mode from editor.init() above.

This is somewhat part of the test. The autocompletion mechanism should be flexible to changes in the editor's mode (to allow, in the future, for autocompleting JS in HTML).

Added a comment.

> @@ +65,5 @@
> > +  editor.setText(before + after);
> > +  editor.setCaretOffset(before.length);
> > +  EventUtils.synthesizeKey("VK_TAB", {}, testWin);
> > +  is(editor.getText(), before + "bar" + after,
> > +     "Deep static JS autocompletion.");
> 
> Is this the result of static JS analysis?

Yes.
(In reply to Thaddee Tyl [:espadrine] from comment #51)
> (In reply to Mihai Sucan [:msucan] from comment #48)
> > Comment on attachment 655910 [details] [diff] [review]
> > Autocompletion in the scratchpad.
> > 
> > Review of attachment 655910 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thank you for the updated patch. I like the source editor changes.
> > 
> > General comments:
> > 
> > - While playing with Scratchpad I sometimes get the following in the error
> > console:
> > 
> > Timestamp: 29/08/2012 18:14:48
> > Error: TypeError: source is undefined
> > Source File: resource:///modules/devtools/autocompletion.jsm
> > Line: 729
> > 
> > - In the Style Editor after typing a bit, I get:
> > 
> > Timestamp: 29/08/2012 18:24:35
> > Error: TypeError: this._completion is null
> > Source File: resource:///modules/devtools/autocompletion.jsm
> > Line: 131
> 
> I'd love to have more information on these errors. I'm trying to reproduce
> them.

I tested the patch on Ubuntu 12.04. Please let me know if you need any specific information.


> > - In one of my previous comments I asked about the AutocompletePopup.jsm if
> > it could be reused. Any thoughts? After completely reviewing
> > Autocompletion.jsm it feels like there's quite some code duplication without
> > any obvious benefits.
> 
> The popup could be reused, but I'd love this to be a follow-up. The look and
> feel of the AutocompletePopup is aesthetically incompatible with the source
> editor right now (although it does fit the web console).

I leave the final decision to Rob. I incline to agree - a follow-up would be fine with me. Please make sure you file all of the suggested follow up bugs.


> > - The approach you used for some methods where you changed argument names
> > then you do |let foo = aFoo| is not ideal.
> 
> Yes, I wrote a fast macro to automate the change, but it wasn't ideal :)
> Consider it fixed.
> 
> > - Please add source editor tests for all of the new API.
> 
> Will do.

Thank you!


> > - lower the delay, try 100ms.
> 
> I've put 100ms in the patch to come, and it does feel better than 200ms,
> while not being fast enough to show up while typing.

Uh oh. Rob, maybe you can come up with a detailed explanation of the UX we want for autocomplete? Down to how keyboard shortcuts should work in various cases.

> > - in one of the comments above we talked about undoing to what I had before
> > starting autocompletion. I mentioned that Escape only closes the popup and
> > there's no way for me to get back to what I had in the editor before
> > completion changes. You suggested I use Ctrl-Z and I tried. After I press
> > Down several times to see the possible completions I decide to press Escape
> > and Ctrl-Z. The result is I have to Ctrl-Z for each of the items I went
> > through. One Ctrl-Z is not sufficient...
> 
> This is a very bad regression! (and it is fixed, along with a couple other
> regressions I noticed, in the next patch.)

Good then!

> > Maybe we also need an option to enable/disable autocompletion in the Source
> > Editor - since don't want to always enable completion. Default should be off.
> 
> I agree with having the possibility to turn it off, but I believe it should
> be on by default.
> 
> I strongly believe that features that are off by default are never seen by
> non-mozillians (and then, only a small percentage of mozillians). You'd be
> surprised how many people amongst mozilla employees I've taught how to
> enable chrome mode in the scratchpad.
> 
> I'd be really sad to see this feature never get used (or get used only two
> years from now). I'm quite proud of how it turned out.

Oh, sorry - I was misunderstood in this case.

For Scratchpad users we definitely want this enabled by default - enable it from scratchpad.js.

But, the default for the Source Editor component should be off (like syntax highlighting is off by default).

Later, when we have CSS autocomplete support, we can enable it from the Style Editor as well (by default!).

> > @@ +1884,5 @@
> > > +   *
> > > +   * @param object aCoord
> > > +   *        An object with properties x and y.
> > > +   */
> > > +  convertCoordFromDocumentToPage: function SE_convertCoordFromDocumentToPage(aCoord)
> > 
> > convertDocumentToView() ? not sure...
> 
> It doesn't change the document at all, it just changes 'aCoord'.

convertDocumentCoordsToView() then?

> > @@ +38,5 @@
> > > +
> > > +function editorLoaded()
> > > +{
> > > +  // JS autocompletion.
> > > +  editor.setMode(SourceEditor.MODES.JAVASCRIPT);
> > 
> > You can set the mode from editor.init() above.
> 
> This is somewhat part of the test. The autocompletion mechanism should be
> flexible to changes in the editor's mode (to allow, in the future, for
> autocompleting JS in HTML).
> 
> Added a comment.

Oh, then as previously suggested please change modes and test for behavior differences between modes.


> > @@ +65,5 @@
> > > +  editor.setText(before + after);
> > > +  editor.setCaretOffset(before.length);
> > > +  EventUtils.synthesizeKey("VK_TAB", {}, testWin);
> > > +  is(editor.getText(), before + "bar" + after,
> > > +     "Deep static JS autocompletion.");
> > 
> > Is this the result of static JS analysis?
> 
> Yes.

That wasn't obvious for me. Can you test something that makes it more obvious? How do I know the result doesn't come from, maybe, a list of known tokens in the document? Or maybe you can compare to how it works in different scopes when the same |foo| is typed.

Thanks!
(In reply to Mihai Sucan [:msucan] from comment #52)
> (In reply to Thaddee Tyl [:espadrine] from comment #51)
> > (In reply to Mihai Sucan [:msucan] from comment #48)
> > > Comment on attachment 655910 [details] [diff] [review]
> > > Autocompletion in the scratchpad.
> > > 
> > > Review of attachment 655910 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Thank you for the updated patch. I like the source editor changes.
> > > 
> > > General comments:
> > > 
> > > - While playing with Scratchpad I sometimes get the following in the error
> > > console:
> > > 
> > > Timestamp: 29/08/2012 18:14:48
> > > Error: TypeError: source is undefined
> > > Source File: resource:///modules/devtools/autocompletion.jsm
> > > Line: 729
> > > 
> > > - In the Style Editor after typing a bit, I get:
> > > 
> > > Timestamp: 29/08/2012 18:24:35
> > > Error: TypeError: this._completion is null
> > > Source File: resource:///modules/devtools/autocompletion.jsm
> > > Line: 131
> > 
> > I'd love to have more information on these errors. I'm trying to reproduce
> > them.
> 
> I tested the patch on Ubuntu 12.04. Please let me know if you need any
> specific information.

Can you confirm that you still get this? I can't seem to reproduce, but I might have fixed it.

> > > Maybe we also need an option to enable/disable autocompletion in the Source
> > > Editor - since don't want to always enable completion. Default should be off.
> > 
> > I agree with having the possibility to turn it off, but I believe it should
> > be on by default.
> > 
> > I strongly believe that features that are off by default are never seen by
> > non-mozillians (and then, only a small percentage of mozillians). You'd be
> > surprised how many people amongst mozilla employees I've taught how to
> > enable chrome mode in the scratchpad.
> > 
> > I'd be really sad to see this feature never get used (or get used only two
> > years from now). I'm quite proud of how it turned out.
> 
> Oh, sorry - I was misunderstood in this case.
> 
> For Scratchpad users we definitely want this enabled by default - enable it
> from scratchpad.js.
> 
> But, the default for the Source Editor component should be off (like syntax
> highlighting is off by default).
> 
> Later, when we have CSS autocomplete support, we can enable it from the
> Style Editor as well (by default!).

Ok. Done that, *and* the devtools.editor.autocomplete.enabled preference. Two birds with one stone.

> > > @@ +1884,5 @@
> > > > +   *
> > > > +   * @param object aCoord
> > > > +   *        An object with properties x and y.
> > > > +   */
> > > > +  convertCoordFromDocumentToPage: function SE_convertCoordFromDocumentToPage(aCoord)
> > > 
> > > convertDocumentToView() ? not sure...
> > 
> > It doesn't change the document at all, it just changes 'aCoord'.
> 
> convertDocumentCoordsToView() then?

Deal.
Created attachment 657098 [details] [diff] [review]
WIP Autocompletion in the scratchpad.

I have yet to add a couple more tests as discussed.

This patch uses 150ms as the delay before the autocompletion popup shows up.

It also uses a preference, devtools.editor.autocomplete.enabled.
Attachment #655910 - Attachment is obsolete: true
Since I am to reuse the AutocompletePopup from the webconsole, should I move it to shared/?

Also, some of it is webconsole-specific (three occurrences of /[cC]onsole/), due to the use of webconsole.properties and an ID named "webConsole_autocompletePopup". Should I change those to make it webconsole-agnostic?

Finally, I get the following error:
JavaScript error: resource:///modules/AutocompletePopup.jsm, line 228: TypeError: this._list.ensureIndexIsVisible is not a function
Does that mean anything to you, Mihai? Do you know what causes that, and how to overcome it?
(In reply to Thaddee Tyl [:espadrine] from comment #55)
> Since I am to reuse the AutocompletePopup from the webconsole, should I move
> it to shared/?

You could do that, but you don't really have to bother here. We can do it in a follow up bug.

> Also, some of it is webconsole-specific (three occurrences of /[cC]onsole/),
> due to the use of webconsole.properties and an ID named
> "webConsole_autocompletePopup". Should I change those to make it
> webconsole-agnostic?

We could change those to "AutocompletePopup."...foobar. I'm not sure if we have a shared l10n properties file.

> Finally, I get the following error:
> JavaScript error: resource:///modules/AutocompletePopup.jsm, line 228:
> TypeError: this._list.ensureIndexIsVisible is not a function
> Does that mean anything to you, Mihai? Do you know what causes that, and how
> to overcome it?

This happens when the popup is not visible. Open it, then call the methods that select different items.
Priority: -- → P2
An update on what I have been doing.

I have spent quite some time trying to reuse the AutocompletePopup from the webconsole instead of using a simple HTML popup (which is what the current WIP patch has).
It turned out to be harder than planned, and not very rewarding.

As a result, and for the interest of modularity, I have decided to fork the autocompletion algorithms out[0].
The way I see it, they could be integrated much like Orion itself is integrated into the project.
This new project is UI-agnostic, editor-agnostic, and you'll be able to use it in many parts of the DevTools
(the WebConsole springs to mind, for reasons discussed below, but also, ultimately, the Style Editor; CSS autocompletion is on the "to do" list).

In my attempt to make it both generic and precise, I use Esprima as an explicit dependency.
I have spoken about its flaws in chrome code in the past;
its speed problem can however be alleviated by using a Web Worker.
I may give the ability to feed this autocompletion engine another parser, however.

One pro in using Esprima is that it contains an actual tokenizer. I am working to add mainstream an API to access it[1].
The current WebConsole has a weaker tokenizer that doesn't always realize where it is (say, in a comment), and may miss part of the identifier[2].

Keep in mind that this project is a work in progress. Yet, it is quite promising.

[0]: https://github.com/espadrine/aulx
[1]: http://code.google.com/p/esprima/issues/detail?id=398
[2]: /toolkit/devtools/webconsole/WebConsoleUtils.jsm:986
That's awesome Thaddee, thank you for still working on this!

However, I'd definitely vote no for using Esprima. Apart from all the obvious speed issues you mentioned yourself, it'll take a lot of work to make Esprima support ES 6 syntax (and Mozilla specific JS features) which we get for free from our Parser API. This is a must, especially because working with Scratchpads in chrome code, and I just don't think it's worth it.

For finding autocompletion information itself, bug 762160 also gives us a fast AST walker on top of our Parser API. Apart from other methods in there, one could get all the symbols in a source text just by doing
> new Parser(source).getAllSymbols()
all with location information etc., or filtering just function definitions with
> new Parser(source).getFunctionIdentifiers()
It's very convenient, and a good plan is to use that for this bug.

Plans are to unify Scratchpad and Debugger pretty soon, hence the AST walker implemented in bug 762160 fits like a glove. Using our Parser API is essential for chrome code.

Regarding the AutocompletePopup, I don't have any particularly strong opinions, but I'd much prefer seeing improvements to the implementation itself than a new fork (which also has Esprima as an explicit dependency), potentially incompatible with all the changes Mihai's been doing to the WebConsole.
(In reply to Victor Porof [:vp] from comment #58)
> That's awesome Thaddee, thank you for still working on this!
> 
> However, I'd definitely vote no for using Esprima. Apart from all the
> obvious speed issues you mentioned yourself, it'll take a lot of work to
> make Esprima support ES 6 syntax (and Mozilla specific JS features) which we
> get for free from our Parser API. This is a must, especially because working
> with Scratchpads in chrome code, and I just don't think it's worth it.

Fair enough. I'll allow to input a different parser than Esprima.
I'll also allow to input a tokenizer, provided that it has the same output as Esprima's.
Unless you provide both, however, Esprima is still a necessary dependency.

Note that a tokenizer for JS is both harder to write (because of our friend the slash character)
and somewhat more resilient, so that, even if the tokenizer is ES5-only, it will still work
in ES6 and chrome code.

> For finding autocompletion information itself, bug 762160 also gives us a
> fast AST walker on top of our Parser API. Apart from other methods in there,
> one could get all the symbols in a source text just by doing
> > new Parser(source).getAllSymbols()
> all with location information etc., or filtering just function definitions
> with
> > new Parser(source).getFunctionIdentifiers()
> It's very convenient, and a good plan is to use that for this bug.

Thanks, but I actually have a pretty good working algorithm to extract all visible symbols *in scope*
from the source and a caret position, and have been since last July.
That's the one in use in Aulx right now.
It relies on Esprima's parse tree structure, aka SpiderMonkey's parse tree structure,
so that I literally had to do no change to the code when moving from Reflect.parse to esprima.
It is sufficiently fast so that Reflect.parse was the bottleneck, not it.

> Regarding the AutocompletePopup, I don't have any particularly strong
> opinions, but I'd much prefer seeing improvements to the implementation
> itself than a new fork (which also has Esprima as an explicit dependency),
> potentially incompatible with all the changes Mihai's been doing to the
> WebConsole.

The AutocompletePopup requirement is from Rob.
You probably saw that, last summer, I had my own UI, which I very much liked.
For the sake of factorizing similar code,
Rob decided that I should use the same UI as the WebConsole instead.

Aulx doesn't have a UI at all.
I forked it because I wished to focus my efforts on making the system better,
rather than rage to make the WebConsole's XUL UI work with the editor's HTML system.
As a result, incompatibility with the WebConsole is impossible.
It's just a matter of wiring things up correctly.
On the other hand, Aulx is already much improved, and it is easier to improve it further.

To be clear, Aulx could be used in WebKit's DevTools, or in Brackets, in exactly the same way.
Nothing is Firefox-specific in it.
Regarding parsers. I was talking to Waldo today about an unrelated but similar project, and his opinion is that it's not that easy and straightforward to expose our tokenizer to the JS side. In addition to that, they have many more important projects right now so there is virtually no manpower to do that work.

Which means that unless someone bites the bullet and implements it themselves, we're not getting a tokenizer in Reflect.jsm or elsewhere. Which blocks not only this bug but also the implementation of a JS prettifier in the debugger (lack of which is embarrassing, even IE8 has it!)[1].

I think it's worth to reconsider our stance on not allowing external parsers/lexers into the tree. Hand-rolling an ES6 lexer in JavaScript is not the toughest job ever, I can do it.

[1] — This is basically why I got interested in it today. I decided to implement JS prettifier and hit that tokenizer requirement.
(In reply to Anton Kovalyov (:anton) from comment #60)
> I think it's worth to reconsider our stance on not allowing external
> parsers/lexers into the tree. Hand-rolling an ES6 lexer in JavaScript is not
> the toughest job ever, I can do it.

Integrating Esprima was considered before (see bug 759837).
I even had a submittable patch for that integration
(which has probably bitrotten to the grave by now).
I ended up not needing it, mostly because I wasn't looking for a tokenizer,
which in turn is because the autocompletion system wasn't precise enough yet
(as in, it didn't seek to complete things like "/[0-9]+/.|").

Also, autocompleting CSS or HTML, for instance, both need a tokenizer too.
I have added CSS autocompletion to Aulx, and it uses Tab Atkins' small tokenizer
(which I have tweaked to add line and column information).
Requiring to make all the C++ tokenizers have a JS API
is great work on those that work on the engines.

Two things to note:
* I did add an option to Aulx to plug-in your own parser (Reflect.parse, for instance.)
  Speed is therefore a lot less of an issue.
* The complexity of an ES6 lexer is just the same as that of an ES5 lexer.
  The most difficult bit by far is / (divide vs. regex) in both.
  Resolving that requires the same algorithm afaik.
  Beyond that, you can take an ES5 tokenizer as is, it'll behave almost exactly
  the same as an ES6 tokenizer. The only difference is that the ES5 tokenizer
  doesn't recognize some of the new keywords in ES6. (For some reason,
  Esprima already recognizes "restricted future ES keywords"
  such as let, const, class, export, … so the difference is very, very limited.)
  I believe Esprima is looking forward to having both ES5 and ES6 lexers
  and parsers in the near future.

All in all, I believe that integrating Esprima is the path to transcendence.
I think Aulx is trying to solve a problem we don't currently have, and introduces a few complications along the way. It also adds a (rather complex) layer of code in our tree, that needs to be maintained.

We had the discussion about Esprima vs. our Parser in the past, and I don't think the situation has changed or we have good reasons to reconsider. Regarding JS autocompletion, having a lexer is not necessary in my opinion. Completing things like /[0-9]+/.| is nice to have, but not a deal breaker; I personally never had to use such features.

Regarding HTML and CSS parsing, a Lexer would definitely be cool. However, I don't think having a smart autocompletion system for such languages is realistically useful:
* For HTML, just having "<hea" complete to "<header>|</header>" is good enough. Adding attributes into the mix would be awesome, and I agree that a Lexer fits here well.
* For CSS, it's almost the same story as with HTML. Smart autocompletion systems are redundant IMO. You only need "back" (for example) to give you a list of background-related CSS properties. Of course, in the case of sugar toppings like SASS, a lexer would also be definitely nice, it's not something that we currently support.

> Fair enough. I'll allow to input a different parser than Esprima.
> I'll also allow to input a tokenizer, provided that it has the same output
> as Esprima's.
> Unless you provide both, however, Esprima is still a necessary dependency.
> 

Allowing a different parser to be plugged in is a good thing, but a mandatory requirement of a tokenizer is a bit aggressive. Simply having a compatible, easily extendable and fast AST walker is good enough.

> Thanks, but I actually have a pretty good working algorithm to extract all
> visible symbols *in scope*
> from the source and a caret position, and have been since last July.
> That's the one in use in Aulx right now.

I'm not suggesting you should use the Parser from bug 762160 in Aulx :) What I'm trying to point out is that the implementation there is flexible enough to provide accurate autocompletion and fits like a glove in the current state of things. 
Like I said, plans are to unify Scratchpad and Debugger, and we already have a decent AST walker implemented.

Finding/separating identifiers by scope is an easy problem. Heck, you could even sort the identifiers by line/column distance to the caret and you'd still have a reasonably accurate autocompletion system!

> It relies on Esprima's parse tree structure, aka SpiderMonkey's parse tree
> structure,
> so that I literally had to do no change to the code when moving from
> Reflect.parse to esprima.
> It is sufficiently fast so that Reflect.parse was the bottleneck, not it.
> 

Of course, traversing syntax tress can be done easily and fast. But if, as you said, Reflect.parse was the bottleneck, imagine how Esprima would feel like. You mentioned yourself a while ago that it's many times (>10?) slower than our native implementation. 

I experimented with moving the parsing in a ChromeWorker, but that lead to unsatisfactory results. The speed difference was either unnoticeable in some cases, or much much worse in others (due to the huge amount of data that had to be carried between the Worker and the parent thread; we can't use transferable objects here if that's what you're thinking.)

> The AutocompletePopup requirement is from Rob.
> You probably saw that, last summer, I had my own UI, which I very much liked.
> For the sake of factorizing similar code,
> Rob decided that I should use the same UI as the WebConsole instead.
> 

I also vouch for using the AutocompletePopup. It's solid code, and improving it would benefit both the WebConsole and an autocompletion system for Scratchpad or Debugger etc. We have and Orion update that's very close to landing (it's been almost a year since the last update), so most of the issues encountered last time may be very well fixed.

Using the AutocompletePopup along with the AST walker from bug 762160 would literally result in a fast and accurate implementation that lives in tens of lines of code.

(In reply to Anton Kovalyov (:anton) from comment #60)
> Regarding parsers. I was talking to Waldo today about an unrelated but
> similar project, and his opinion is that it's not that easy and
> straightforward to expose our tokenizer to the JS side. In addition to that,
> they have many more important projects right now so there is virtually no
> manpower to do that work.
> 
> Which means that unless someone bites the bullet and implements it
> themselves, we're not getting a tokenizer in Reflect.jsm or elsewhere. Which
> blocks not only this bug but also the implementation of a JS prettifier in
> the debugger (lack of which is embarrassing, even IE8 has it!)[1].
> 

Thanks for giving a thought to this. There are two ways of solving prettification for JS in the debugger. Without generating source maps (hence beautifying the code and feeding it to the engine before the debugger has a chance to see it), then we already have jsbeautify in the tree (under /shared) and we can use that (there's a gcli command available already for basic usage).

However, generating source maps is the smarter way of fixing the problem, especially with Nick's work coming along.

> I think it's worth to reconsider our stance on not allowing external
> parsers/lexers into the tree. Hand-rolling an ES6 lexer in JavaScript is not
> the toughest job ever, I can do it.
> 

Since exposing our tokenizer to JS is not trivial, I'm all for external lexers. It'd be great to have on. Regarding parsers, however, I'm having a hard time finding a solid argument for not using our reflection API. It's very fast, maintained, and we can be sure it property supports ES6 features (and our mozilla specific chrome code)

> [1] — This is basically why I got interested in it today. I decided to
> implement JS prettifier and hit that tokenizer requirement.

If it generates source maps, then you're my hero :)

(In reply to Thaddee Tyl [:espadrine] from comment #61)
> Integrating Esprima was considered before (see bug 759837).

We didn't use Esprima because it was slow, it didn't support ES6 features, nor chrome code. When it comes to Scratchpad, supporting such things is absolutely mandatory.
(In reply to Victor Porof [:vp] from comment #62)
> I think Aulx is trying to solve a problem we don't currently have, and
> introduces a few complications along the way. It also adds a (rather
> complex) layer of code in our tree, that needs to be maintained.
> 
> Regarding HTML and CSS parsing, a Lexer would definitely be cool. However, I
> don't think having a smart autocompletion system for such languages is
> realistically useful:
> * For HTML, just having "<hea" complete to "<header>|</header>" is good
> enough. Adding attributes into the mix would be awesome, and I agree that a
> Lexer fits here well.
> * For CSS, it's almost the same story as with HTML. Smart autocompletion
> systems are redundant IMO. You only need "back" (for example) to give you a
> list of background-related CSS properties. Of course, in the case of sugar
> toppings like SASS, a lexer would also be definitely nice, it's not
> something that we currently support.

Aulx factorizes common code in JS / CSS / HTML autocompletion.
The complexity of its code, compared to the autocompletion patch I had before,
is hugely lower because of it.
It certainly is a different design, but a much simpler one at that.
I learned a lot from my mistakes.

Quick mention: JS and CSS autocompletion in HTML. For free.
Just because of that factorization of common code in Aulx.
(Not in yet, but I'm on it, and it looks promising.)
 
> Allowing a different parser to be plugged in is a good thing, but a
> mandatory requirement of a tokenizer is a bit aggressive. Simply having a
> compatible, easily extendable and fast AST walker is good enough.

Did you know we already have a JS tokenizer? It is quite primitive.
I mentioned it before.
It's at /toolkit/devtools/webconsole/WebConsoleUtils.jsm:986.
Using it (which is what I did in the patch)
caused odd behaviours because of its obvious limitations.
For instance, it has no idea when we are in a comment, or a regex.
It only knows when we are in a string,
and then again, if it isn't multi-line, etc.

We can't do autocompletion without some kind of a tokenizer.
We need to know where the cursor stands.
Better use the fastest tokenizer available, and the fastest is Esprima.
(I'm backing this out with figures below.)

> Of course, traversing syntax tress can be done easily and fast. But if, as
> you said, Reflect.parse was the bottleneck, imagine how Esprima would feel
> like. You mentioned yourself a while ago that it's many times (>10?) slower
> than our native implementation. 

Now comes the figures.
You can use https://gist.github.com/4647725 to try it out yourself.
It uses the complete latest edition of jQuery.

Esprima parser:    330ms.
Reflect parser:    70ms.
Esprima tokenizer: 1ms.
Reflect tokenizer: I don't think we'll get it, ever. Anton, and Waldo, can back me up.
Other tokenizers:  they're starting to appear (I'm pushing for it), but are not there yet.

The way I interpret these results is: tokenizer speed is not an issue.
Reflect.parse is the one bottleneck, BY FAR.
It is the only thing that is slow in the whole system,
and any attempt at making the rest faster won't make the system faster.


> > The AutocompletePopup requirement is from Rob.
> > You probably saw that, last summer, I had my own UI, which I very much liked.
> > For the sake of factorizing similar code,
> > Rob decided that I should use the same UI as the WebConsole instead.
> > 
> 
> I also vouch for using the AutocompletePopup. It's solid code, and improving
> it would benefit both the WebConsole and an autocompletion system for
> Scratchpad or Debugger etc. We have and Orion update that's very close to
> landing (it's been almost a year since the last update), so most of the
> issues encountered last time may be very well fixed.

I don't disagree. I simply believe that using an HTML popup in an
HTML WebConsole will eventually be the only option if we want to compete.
In the meantime, I'm working on non-UI stuff.

I'm also hopeful that we can switch to CodeMirror.
I know for sure that updates would be a breeze with it.
I can only remember the pain of trying to update Orion myself.


> (In reply to Thaddee Tyl [:espadrine] from comment #61)
> > Integrating Esprima was considered before (see bug 759837).
> 
> We didn't use Esprima because it was slow, it didn't support ES6 features,
> nor chrome code. When it comes to Scratchpad, supporting such things is
> absolutely mandatory.

The state of the world is different.
I am not looking at Esprima as a parser anymore;
I'm looking at a tokenizer now.
A very fast, robust (~600 tests!), promising tokenizer that we can rely on.
It tokenizes ES6 and chrome code just fine.
It is very resilient, and very easy to use.

I know what I said before, and I agree with the issues I raised before.
But those are not the same issues.
To do any autocompletion at all, we need both a tokenizer and a parser.
Let's have the best of both worlds,
with Esprima's tokenizer and Reflect's parser.
While this is an interesting discussion, I suggest people continue it on the new mozilla developer tools mailing list:

https://lists.mozilla.org/listinfo/dev-developer-tools

Please remember this is a bug about adding and implementing the autocomplete feature in Scratchpad. Talks about parsers, tokenizers, performance comparisons, UIs, and more should take place outside of the bug. Once we reach relevant conclusions, they will be part of the action taken to fix this bug.

Thank you!
Now that we're using CodeMirror we should consider using TernJS (http://ternjs.net). It comes in CodeMirror plugin form and is created by the author of CodeMirror, so it integrates well.
Sorry, didn't thoroughly read the thread. Ignore my last comment.
(In reply to Brandon Benvie [:bbenvie] from comment #65)
> Now that we're using CodeMirror we should consider using TernJS
> (http://ternjs.net). It comes in CodeMirror plugin form and is created by
> the author of CodeMirror, so it integrates well.

FWIW, I have (while working on this bug) repeatedly been told against using
anything that uses an external JS parser, for performance reasons.
Tern relies on Acorn — and an error-tolerant version of Acorn at that.

Which is why I developed Aulx[1] together with Optimizer, exactly for that
purpose. It works particularly well with CodeMirror (but is editor-agnostic),
it can use an external parser (eg, using the Parser API[2]), and it
performs good type inference that you can try out[3].

[1]: https://github.com/espadrine/aulx
[2]: https://developer.mozilla.org/en-US/docs/SpiderMonkey/Parser_API
[3]: http://espadrine.github.io/aulx/
I've opened a thread on the mailing list so we can have a discussion about this: https://groups.google.com/forum/#!topic/mozilla.dev.developer-tools/cWANPm0FWVM.
Depends on: 924466
Thaddee, this lies around for several years now. Scratchpad obviously already got some autocompletion in the meantime. So, is there anything left to do?

Sebastian
Flags: needinfo?(thaddee.tyl)
Per comment 69 + no answer from logger, closing this.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Flags: needinfo?(thaddee.tyl)
Resolution: --- → FIXED

Updated

a year ago
Assignee: thaddee.tyl → nobody

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.