DevTools: sort completions by prototype.

DevTools: sort completions by prototype.
BUG=672615
Committed: https://crrev.com/1116787888994e1fb123c3c0e800d2734b95d10d
Cr-Commit-Position: refs/heads/master@{#437458}

[pfeldman, 2016-12-07 00:05:23]

pfeldman@chromium.org changed reviewers:
+ einbinder@chromium.org

[pfeldman, 2016-12-07 01:12:26]

don't look at it, we are adding moar

[pfeldman, 2016-12-07 01:51:02]

ready to be looked at

[einbinder, 2016-12-07 20:10:51]

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js
(right):

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:138:
if (typeof o === 'object' && o.constructor && o.constructor.name)
//TODO (einbinder) namespaces?

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:182:
var result = [{items: ['this']}];
Should this be added to the local scope? Also, it will be added again in the
keyword group down below. Maybe it should just stay there?

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:202:
object.getAllProperties(false, propertiesCollected.bind(null,
scope.typeName()));
Nested deep in some code, you might see many Closure subtitles stacked on top of
each other.

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:282:
propertyGroups.push({items: keywords});
Should keywords get a title?

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:322:
.map(item => ({title: item}));
Add higher priority to the case sensitive and prefix suggestions, so they are
selected by default.

caseSensitivePrefix.map(item => ({title: item, priority: 4}))
caseInsensitivePrefix.map(item => ({title: item, priority: 3}))
etc..

Otherwise document.rep will select onbeforepaste ahead of replaceChild

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:327:
return result;
You will have duplicate suggestions if the prototype and object share a
property.

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:327:
return result;
You will have duplicate suggestions if the prototype and object share a
property.

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js (right):

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:1210: return
'';
Fails silently. Maybe a warning?

[pfeldman, 2016-12-08 00:29:06]

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js
(right):

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:182:
var result = [{items: ['this']}];
On 2016/12/07 20:10:50, einbinder wrote:
> Should this be added to the local scope? Also, it will be added again in the
> keyword group down below. Maybe it should just stay there?

I'd want 'this' to be high in the list in the debugging case, because it makes
more sense on a breakpoint.

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:202:
object.getAllProperties(false, propertiesCollected.bind(null,
scope.typeName()));
On 2016/12/07 20:10:51, einbinder wrote:
> Nested deep in some code, you might see many Closure subtitles stacked on top
of
> each other.

Don't do deep nesting?

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:282:
propertyGroups.push({items: keywords});
On 2016/12/07 20:10:51, einbinder wrote:
> Should keywords get a title?

Sure, done.

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:322:
.map(item => ({title: item}));
Wouldn't that break the grouping?

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:327:
return result;
On 2016/12/07 20:10:50, einbinder wrote:
> You will have duplicate suggestions if the prototype and object share a
> property.

Maybe that is ok?

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:327:
return result;
On 2016/12/07 20:10:51, einbinder wrote:
> You will have duplicate suggestions if the prototype and object share a
> property.

Maybe that is ok?

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js (right):

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:1210: return
'';
On 2016/12/07 20:10:51, einbinder wrote:
> Fails silently. Maybe a warning?

We don't really do asserts for code correctness in places like this.

[pfeldman, 2016-12-08 01:56:19]

pfeldman@chromium.org changed reviewers:
+ dgozman@chromium.org

[einbinder, 2016-12-08 18:51:22]

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js
(right):

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:202:
object.getAllProperties(false, propertiesCollected.bind(null,
scope.typeName()));
On 2016/12/08 at 00:29:06, pfeldman wrote:
> On 2016/12/07 20:10:51, einbinder wrote:
> > Nested deep in some code, you might see many Closure subtitles stacked on
top of
> > each other.
> 
> Don't do deep nesting?

Will the order of the stacks always go Local, Closure, ..., Closure, Global? It
might be different to v8, but to me there is only one closure, which has the
variables I capture in the current function.

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:322:
.map(item => ({title: item}));
On 2016/12/08 at 00:29:06, pfeldman wrote:
> Wouldn't that break the grouping?

No, it preserves the order of the suggestions. It just picks a different one to
be selected by default. Like we do with "width" when you type "w" in styles.

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:327:
return result;
On 2016/12/08 at 00:29:06, pfeldman wrote:
> On 2016/12/07 20:10:50, einbinder wrote:
> > You will have duplicate suggestions if the prototype and object share a
> > property.
> 
> Maybe that is ok?

While it does get used for introspection, the main use of autocomplete is
autocomplete. I don't want to get suggestions for properties that I can't access
right now.

[pfeldman, 2016-12-08 21:26:18]

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js
(right):

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:202:
object.getAllProperties(false, propertiesCollected.bind(null,
scope.typeName()));
You can have multiple closures, why not? The order will be Local, Closure, ...,
Closure, Global. I am not sure what you are suggesting here.

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:322:
.map(item => ({title: item}));
On 2016/12/08 18:51:22, einbinder wrote:
> On 2016/12/08 at 00:29:06, pfeldman wrote:
> > Wouldn't that break the grouping?
> 
> No, it preserves the order of the suggestions. It just picks a different one
to
> be selected by default. Like we do with "width" when you type "w" in styles.

Done.

https://codereview.chromium.org/2557763003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:327:
return result;
On 2016/12/08 18:51:22, einbinder wrote:
> On 2016/12/08 at 00:29:06, pfeldman wrote:
> > On 2016/12/07 20:10:50, einbinder wrote:
> > > You will have duplicate suggestions if the prototype and object share a
> > > property.
> > 
> > Maybe that is ok?
> 
> While it does get used for introspection, the main use of autocomplete is
> autocomplete. I don't want to get suggestions for properties that I can't
access
> right now.

My worry is that with this new structure, not showing these properties could
lead to more confusion. At least when you just type "." in this edge case, you
might want to see dupes.

[pfeldman, 2016-12-08 21:36:35]

Description was changed from

==========
DevTools: sort completions by prototype.
==========

to

==========
DevTools: sort completions by prototype.
BUG=672615
==========

[dgozman, 2016-12-08 23:49:09]

lgtm

[pfeldman, 2016-12-09 01:31:42]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-12-09 01:31:43]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2557763003/#ps120001
(title: "for landing")

[commit-bot: I haz the power, 2016-12-09 01:32:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 01:37:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 01:37:07]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pfeldman, 2016-12-09 01:44:22]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-12-09 01:44:22]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2557763003/#ps140001
(title: "rebaselined")

[commit-bot: I haz the power, 2016-12-09 01:45:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 03:55:26]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481247862541970,
"parent_rev": "939918f23aedbf399da78566279429be0c3fd0a2", "commit_rev":
"fdede02d198a3b30da32ee176540e809de34fdc1"}

[commit-bot: I haz the power, 2016-12-09 03:56:20]

Description was changed from

==========
DevTools: sort completions by prototype.
BUG=672615
==========

to

==========
DevTools: sort completions by prototype.
BUG=672615
==========

[commit-bot: I haz the power, 2016-12-09 03:56:22]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-12-09 03:59:16]

Description was changed from

==========
DevTools: sort completions by prototype.
BUG=672615
==========

to

==========
DevTools: sort completions by prototype.
BUG=672615

Committed: https://crrev.com/1116787888994e1fb123c3c0e800d2734b95d10d
Cr-Commit-Position: refs/heads/master@{#437458}
==========

[commit-bot: I haz the power, 2016-12-09 03:59:17]

Patchset 8 (id:??) landed as
https://crrev.com/1116787888994e1fb123c3c0e800d2734b95d10d
Cr-Commit-Position: refs/heads/master@{#437458}