DevTools: Create extensible QuickOpen control

DevTools: Create extensible QuickOpen control

GoToLineDialog and QuickOpen are spun off from OpenResourceDialog.
QuickOpen switches between GoToLineDialog and OpenResourceDialog depending on
whether the query starts with ':'.
QuickOpen loads delegates through the runtime system.

BUG=662081
Review-Url: https://codereview.chromium.org/2679483002
Cr-Commit-Position: refs/heads/master@{#460536}
Committed: https://chromium.googlesource.com/chromium/src/+/64eeaf6fafdb64505f589a8f1391460362ea09af

[einbinder, 2017-02-04 03:38:09]

Description was changed from

==========
DevTools: Create extensible QuickOpen control

BUG=662081
==========

to

==========
DevTools: Create extensible QuickOpen control

GoToLineDialog and QuickOpen are spun off from OpenResourceDialog.
QuickOpen switches between GoToLineDialog and OpenResourceDialog depending on
whether the query starts with ':'.
QuickOpen loads delegates through the runtime system.

BUG=662081
==========

[einbinder, 2017-02-04 03:38:10]

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

[pfeldman, 2017-02-07 02:10:01]

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

[pfeldman, 2017-02-07 02:10:03]

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js
(right):

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:440:
QuickOpen.FilteredListWidget.Delegate = class {
This is too brave. Should we split this into itself and a new
QuickOpen.QuickOpenProvider? Worst case inherit one from another.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js
(right):

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:10:
this._currentDelegate = new QuickOpen.Loading();
nit: here and below, remove _current prefix - I don't see any other _delegates,
prefixes and queries. Ok, I see other prefix, but you did not want to call them
the same way. Your this._prefix = [] is actually this._providers

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:21:
this._prefix.sort((a, b) => b.prefix.length - a.prefix.length);
They are already sorter by "order" extension property.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:29: for
(var i = 0; i < this._prefix.length; i++) {
for (var provider of this._providers)

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:37:
this._prefix[i].delegate = instance;
Store extension in this._providers and simply chain and convert this whole
_delegate method into a promise?

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:45: return
this._currentDelegate || new QuickOpen.FilteredListWidget.Delegate();
Oh, so QuickOpen.Loading() is not the same!

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:90:
setRefreshCallback(refreshCallback) {
Is this to make sure that JavaScript works?

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:162: for
(var i = 0; i < this._prefix.length; i++) {
I now see why you need delegates, but I'd manage sessions more carefully. For
example, instead of specifying the "className" in the extension, you can say
"factoryName" and every time you instantiate provider, you'll be getting new
instances. They will be simply collected then.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:172: static
show(query) {
move up.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:174: new
QuickOpen.FilteredListWidget(new QuickOpen.QuickOpen(),
QuickOpen.QuickOpen._history);
QuickOpen.QuickOpen again - why do you need to specify it all the time?

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:202:
QuickOpen.Loading = class extends QuickOpen.FilteredListWidget.Delegate {
QuickOpen.QuickOpen.Loading :)

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/GoToLineDialog.js
(right):

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/GoToLineDialog.js:5:
Sources.GoToLineDialog = class extends QuickOpen.FilteredListWidget.Delegate {
Sources.GoToLineQuickOpen

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(right):

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1104:
UI.shortcutRegistry.shortcutTitleForAction('quickOpen.show')));
meh!

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/OpenResourceDialog.js
(right):

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/OpenResourceDialog.js:10:
Sources.OpenResourceDialog = class extends
Sources.FilteredUISourceCodeListDelegate {
This is no longer a dialog, lets call it Sources.OpenFileQuickOpen ?

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js
(right):

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js:608:
_showGoToLineDialog(event) {
Extract this into an action in its own class (Sources.SourcesPanel.GoToHandler?)
where ever provider for ":" is.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js:667: static
defaultUISourceCodeScores() {
Declare static members first

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js:670: var
sourcesView = UI.context.flavor(Sources.SourcesView);
Don't abuse context - all setters and getters of this flavor are in this class.
Just make it a singleton for now.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/module.json (right):

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/module.json:218: "type":
"quick-open",
@QuickOpen.QuickOpenProvider

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/module.json:224: "type":
"quick-open",
ditto

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/module.json:225: "title":
"Go to line in file",
Go to line

[pfeldman, 2017-02-23 01:14:21]

ping

[einbinder, 2017-02-28 23:59:08]

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js
(right):

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:440:
QuickOpen.FilteredListWidget.Delegate = class {
On 2017/02/07 at 02:10:02, pfeldman wrote:
> This is too brave. Should we split this into itself and a new
QuickOpen.QuickOpenProvider? Worst case inherit one from another.

Done.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js
(right):

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:10:
this._currentDelegate = new QuickOpen.Loading();
On 2017/02/07 at 02:10:03, pfeldman wrote:
> nit: here and below, remove _current prefix - I don't see any other
_delegates, prefixes and queries. Ok, I see other prefix, but you did not want
to call them the same way. Your this._prefix = [] is actually this._providers

Done.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:21:
this._prefix.sort((a, b) => b.prefix.length - a.prefix.length);
On 2017/02/07 at 02:10:02, pfeldman wrote:
> They are already sorter by "order" extension property.

This is important so that longer prefix go first. If someone forgot to set order
= prefix.length, hard to catch bugs might arise.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:29: for
(var i = 0; i < this._prefix.length; i++) {
On 2017/02/07 at 02:10:02, pfeldman wrote:
> for (var provider of this._providers)

Done.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:37:
this._prefix[i].delegate = instance;
On 2017/02/07 at 02:10:02, pfeldman wrote:
> Store extension in this._providers and simply chain and convert this whole
_delegate method into a promise?

It is much easier if this method is synchronous. Otherwise you get async
creation of elements. I refactored it a bunch, I am not sure if it is better
though.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:45: return
this._currentDelegate || new QuickOpen.FilteredListWidget.Delegate();
On 2017/02/07 at 02:10:02, pfeldman wrote:
> Oh, so QuickOpen.Loading() is not the same!

There was no path to get to the blank delegate, I am not sure why I had it
there.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:90:
setRefreshCallback(refreshCallback) {
On 2017/02/07 at 02:10:02, pfeldman wrote:
> Is this to make sure that JavaScript works?

I was suspicious.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:162: for
(var i = 0; i < this._prefix.length; i++) {
On 2017/02/07 at 02:10:02, pfeldman wrote:
> I now see why you need delegates, but I'd manage sessions more carefully. For
example, instead of specifying the "className" in the extension, you can say
"factoryName" and every time you instantiate provider, you'll be getting new
instances. They will be simply collected then.

Dispose is to remove the event listeners on FilteredUISourceCodeListDelegate. I
am using factoryName.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:172: static
show(query) {
On 2017/02/07 at 02:10:03, pfeldman wrote:
> move up.

Done.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:174: new
QuickOpen.FilteredListWidget(new QuickOpen.QuickOpen(),
QuickOpen.QuickOpen._history);
On 2017/02/07 at 02:10:03, pfeldman wrote:
> QuickOpen.QuickOpen again - why do you need to specify it all the time?

Fixed :)

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:202:
QuickOpen.Loading = class extends QuickOpen.FilteredListWidget.Delegate {
On 2017/02/07 at 02:10:02, pfeldman wrote:
> QuickOpen.QuickOpen.Loading :)

Done.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/GoToLineDialog.js
(right):

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/GoToLineDialog.js:5:
Sources.GoToLineDialog = class extends QuickOpen.FilteredListWidget.Delegate {
On 2017/02/07 at 02:10:03, pfeldman wrote:
> Sources.GoToLineQuickOpen

Done.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/OpenResourceDialog.js
(right):

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/OpenResourceDialog.js:10:
Sources.OpenResourceDialog = class extends
Sources.FilteredUISourceCodeListDelegate {
On 2017/02/07 at 02:10:03, pfeldman wrote:
> This is no longer a dialog, lets call it Sources.OpenFileQuickOpen ?

Done.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js
(right):

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js:608:
_showGoToLineDialog(event) {
On 2017/02/07 at 02:10:03, pfeldman wrote:
> Extract this into an action in its own class
(Sources.SourcesPanel.GoToHandler?) where ever provider for ":" is.

Would it use flavor(SourcesView) to check if the editor has a current file?

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js:667: static
defaultUISourceCodeScores() {
On 2017/02/07 at 02:10:03, pfeldman wrote:
> Declare static members first

Done.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js:670: var
sourcesView = UI.context.flavor(Sources.SourcesView);
On 2017/02/07 at 02:10:03, pfeldman wrote:
> Don't abuse context - all setters and getters of this flavor are in this
class. Just make it a singleton for now.

A singleton feels weird because it operates on a visible SourcesView. But would
you want every user of SourcesView flavor to switch to the singleton, or just
this one? GoToLine uses it to get the current UISourceCode.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/module.json (right):

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/module.json:218: "type":
"quick-open",
On 2017/02/07 at 02:10:03, pfeldman wrote:
> @QuickOpen.QuickOpenProvider

Done.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/module.json:224: "type":
"quick-open",
On 2017/02/07 at 02:10:03, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/2679483002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/module.json:225: "title":
"Go to line in file",
On 2017/02/07 at 02:10:03, pfeldman wrote:
> Go to line

Done.

[pfeldman, 2017-03-02 22:42:27]

Let's chat about it!

https://codereview.chromium.org/2679483002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/persistence/persistence-go-to-file-dialog.html
(right):

https://codereview.chromium.org/2679483002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/persistence/persistence-go-to-file-dialog.html:45:
InspectorTest.addSnifferPromise(QuickOpen.QuickOpen.prototype,
'_providerLoadedForTest').then(provider => {
It is always nice to not use sniffers. Just saying.

https://codereview.chromium.org/2679483002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js
(right):

https://codereview.chromium.org/2679483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:233:
this._delegate.queryChanged(query);
Seems like a client that has a dialog in hand and also implements the delegate
would set query and be notified itself?

https://codereview.chromium.org/2679483002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js
(right):

https://codereview.chromium.org/2679483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:184:
QuickOpen.QuickOpenProvider = class extends
QuickOpen.FilteredListWidget.Delegate {};
That's still unpretty.

[einbinder, 2017-03-03 23:07:14]

QuickOpen is no longer a FilteredListWidget.Delegate

https://codereview.chromium.org/2679483002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js
(right):

https://codereview.chromium.org/2679483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:233:
this._delegate.queryChanged(query);
On 2017/03/02 at 22:42:27, pfeldman wrote:
> Seems like a client that has a dialog in hand and also implements the delegate
would set query and be notified itself?

It wasn't the delegate, but now it is so this is gone.

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js
(right):

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:452:
_selectItem(itemIndex) {
This was always called with promptValue = this._value()

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/module.json (right):

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/module.json:218: "type":
"@QuickOpen.FilteredListWidget.Delegate",
Should this go back to being "quick-open" or a QuickOpen.Provider type to
distinguish it from other delegates that aren't used by QuickOpen?

[pfeldman, 2017-03-06 18:37:18]

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js
(right):

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:49:
this._delegate = null;
Could you extract swappable delegate-related change into a separate CL? I'll
review it there. Also, swappable delegates are typically called "models" or
"providers".

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js
(right):

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:19:
this._prefix = null;
It is funny how you have this in both places - is it the same thing? expose it
on the widget?

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:24:
this._filteredListWidget.on(QuickOpen.FilteredListWidget.DisposeEvent,
this._dispose, this);
Ah, so you kind-of have both, a lifetime-long delegate and a model? I would not
make dispose an event - it is rather a callback that you pass into constructor,
same for query change - could be treated as a callback. Worst case you do get a
delegate and a model.

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:43: prefix:
extension.descriptor().prefix,
Everything not defined in the jsdoc should be accessed using 'quotes'. There is
no such thing 'prefix' in our docs.

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:48:
this._descriptors.splice(index, 0, descriptor);
How is it better than array of extension.descriptor()s?

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:58: if
(descriptor.delegate) {
There is no reason to split promise and sync behavior there.

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:64:
instance.dispose();
return after?

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:71:
this._filteredListWidget.setDelegate(this._loadingDelegate);
This is going to be instantaneous in most of the cases, why bothering?

[einbinder, 2017-03-08 22:39:57]

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js
(right):

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:49:
this._delegate = null;
On 2017/03/06 at 18:37:18, pfeldman wrote:
> Could you extract swappable delegate-related change into a separate CL? I'll
review it there. Also, swappable delegates are typically called "models" or
"providers".

Done.

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js
(right):

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:24:
this._filteredListWidget.on(QuickOpen.FilteredListWidget.DisposeEvent,
this._dispose, this);
On 2017/03/06 at 18:37:18, pfeldman wrote:
> Ah, so you kind-of have both, a lifetime-long delegate and a model? I would
not make dispose an event - it is rather a callback that you pass into
constructor, same for query change - could be treated as a callback. Worst case
you do get a delegate and a model.

Removed dispose, queryChange is a callback.

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:43: prefix:
extension.descriptor().prefix,
On 2017/03/06 at 18:37:18, pfeldman wrote:
> Everything not defined in the jsdoc should be accessed using 'quotes'. There
is no such thing 'prefix' in our docs.

Done.

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:48:
this._descriptors.splice(index, 0, descriptor);
On 2017/03/06 at 18:37:18, pfeldman wrote:
> How is it better than array of extension.descriptor()s?

Replaced with a map.

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:58: if
(descriptor.delegate) {
On 2017/03/06 at 18:37:18, pfeldman wrote:
> There is no reason to split promise and sync behavior there.

Done.

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:64:
instance.dispose();
On 2017/03/06 at 18:37:18, pfeldman wrote:
> return after?

n/a

https://codereview.chromium.org/2679483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:71:
this._filteredListWidget.setDelegate(this._loadingDelegate);
On 2017/03/06 at 18:37:18, pfeldman wrote:
> This is going to be instantaneous in most of the cases, why bothering?

For the cases where it is not instantaneous.

https://codereview.chromium.org/2679483002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js
(right):

https://codereview.chromium.org/2679483002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:10:
this._prefix = null;
I could expose _prefix on the filteredListWidget, but I'd still have to have a
flag here indicating whether or not a provider has been loaded.

https://codereview.chromium.org/2679483002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sources/OpenFileQuickOpen.js
(right):

https://codereview.chromium.org/2679483002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/OpenFileQuickOpen.js:12:
this.setDefaultScores(Sources.SourcesView.defaultUISourceCodeScores());
I removed the caching of providers in QuickOpen.js. Instead the providers are
cached by the runtime system. In order to re-use the same provider, I pass in
the current scores on attach instead of in  the constructor.

[pfeldman, 2017-03-22 01:29:51]

lgtm % nit

https://codereview.chromium.org/2679483002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js
(right):

https://codereview.chromium.org/2679483002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:34:
this._prefixes.splice(this._prefixes.lowerBound(prefix, (a, b) => b.length -
a.length), 0, prefix);
Sorting it afterwards the fact is actually cheaper.

[einbinder, 2017-03-23 06:28:12]

https://codereview.chromium.org/2679483002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js
(right):

https://codereview.chromium.org/2679483002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/quick_open/QuickOpen.js:34:
this._prefixes.splice(this._prefixes.lowerBound(prefix, (a, b) => b.length -
a.length), 0, prefix);
On 2017/03/22 at 01:29:51, pfeldman wrote:
> Sorting it afterwards the fact is actually cheaper.

Done.

[einbinder, 2017-03-29 02:45:26]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2017-03-29 02:45:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2679483002/#ps170001
(title: "merge")

[commit-bot: I haz the power, 2017-03-29 02:46:16]

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, 2017-03-29 04:47:26]

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

[commit-bot: I haz the power, 2017-03-29 04:47:27]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[einbinder, 2017-03-29 18:11:08]

The CQ bit was checked by einbinder@chromium.org

[commit-bot: I haz the power, 2017-03-29 18:12:20]

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, 2017-03-29 18:24:28]

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

[commit-bot: I haz the power, 2017-03-29 18:24:30]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[einbinder, 2017-03-29 19:16:30]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2017-03-29 19:16:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2679483002/#ps190001
(title: "merge")

[commit-bot: I haz the power, 2017-03-29 19:17:28]

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, 2017-03-29 21:04:45]

CQ is committing da patch.

Bot data: {"patchset_id": 190001, "attempt_start_ts": 1490814990083130,
"parent_rev": "1c59eda4e5a4c7427f355ed752e94f43561604a3", "commit_rev":
"64eeaf6fafdb64505f589a8f1391460362ea09af"}

[commit-bot: I haz the power, 2017-03-29 21:05:37]

Description was changed from

==========
DevTools: Create extensible QuickOpen control

GoToLineDialog and QuickOpen are spun off from OpenResourceDialog.
QuickOpen switches between GoToLineDialog and OpenResourceDialog depending on
whether the query starts with ':'.
QuickOpen loads delegates through the runtime system.

BUG=662081
==========

to

==========
DevTools: Create extensible QuickOpen control

GoToLineDialog and QuickOpen are spun off from OpenResourceDialog.
QuickOpen switches between GoToLineDialog and OpenResourceDialog depending on
whether the query starts with ':'.
QuickOpen loads delegates through the runtime system.

BUG=662081

Review-Url: https://codereview.chromium.org/2679483002
Cr-Commit-Position: refs/heads/master@{#460536}
Committed:
https://chromium.googlesource.com/chromium/src/+/64eeaf6fafdb64505f589a8f1391...
==========

[commit-bot: I haz the power, 2017-03-29 21:05:38]

Committed patchset #11 (id:190001) as
https://chromium.googlesource.com/chromium/src/+/64eeaf6fafdb64505f589a8f1391...