Quick Open JS returns spurious results in codemirror.js

[peterflynn]

1. Open codemirror.js
2. Ctrl+T

Result: First five results in the list are "}", "}", "{toString", "{token", and "{set"

Expected: Braces should never appear in the list -- only valid function names.

[peterflynn]

I'm pretty sure this is a regression from #1085 so we should at least consider it for Sprint 10 (this bug did not occur earlier today).

[peterflynn]

The main problem seems to be that when we spot the pattern ": function(...)", we assume the function name is everything from the colon back to the previous whitespace char. If code doesn't have a lot of whitespace, then that could include an arbitrary amount of stuff that's not really part of the function name.

E.g. the code var foo={a:100,b:-1,c:function () {}} shows "foo={a:100,b:-1,c" as the function name.

We should stop at the first character that's not a valid identifier instead, and if that results in an empty/all-whitespace string we should filter out the result entirely. (That would eliminate the first two spurious results, which come from treating the middle clause of a ternary operator as if it were a function name. There'd still be some cases where we'd mistake a ternary operator for a function declaration, but they'd be far less ugly with this fix).

This bug presumably has existed in the JS Quick Edit code for a while, but not surfaced because it only allows search queries that are real identifiers. Now that Quick Open uses the same code, the spurious results are visible.

[pthiess]

Reviewed - we want to look at this and get this fixed in Sprint 10 if still possible.

[pthiess]

@njx - has a fix

[jasonsanjose]

For the record, Quick Open is still a bonus feature that hasn't been fully vetted or tested. I don't think this should have held up Sprint 10 sign off. As is, JavaScript Quick Open would not be able to exhibit this behavior since it requires a valid function identifier token (as provided by CodeMirror) as the search query. I'll admit, the argument against this is that JSUtils results (the list of functions in a file) are now accessible to extensions, so fixing it for extensions that don't exist could still be a valid reason to fix this bug.

[njx]

Actually, this bug does affect JS Quick Edit in a user-visible way because it meant that certain functions wouldn't be found when Quick Edit was requested. (In Peter's example above, the "c" function would not be found, for example.)

[jasonsanjose]

Ah, gotcha. Thanks for the correction.

[jasonsanjose]

FBNC @peterflynn

[tvoliter]

Verified fix. Closing