MD Downloads: make Ctrl+f invoke in-page search

MD Downloads: make Ctrl+f invoke in-page search

R=michaelpg@chromium.org
BUG=578323
Committed: https://crrev.com/60ebcccd01f6faaa0c85fe583bb73d2032e4f450
Cr-Commit-Position: refs/heads/master@{#370881}

[michaelpg, 2016-01-21 22:04:14]

https://codereview.chromium.org/1613233003/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_downloads/toolbar.js (right):

https://codereview.chromium.org/1613233003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_downloads/toolbar.js:43: var searchInput = this.$ &&
this.$['search-input'];
this should already be true anyway (the element should always be ready, given
how this is called), but i suppose it's okay to enforce this here too?

https://codereview.chromium.org/1613233003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_downloads/toolbar.js:48: searchInput.focus();
doesn't this need to be async if showingSearch was false? this results in:

    this.$$('#search-input').focus();

calling "focus" on undefined.

[Dan Beam, 2016-01-21 22:14:47]

https://codereview.chromium.org/1613233003/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_downloads/toolbar.js (right):

https://codereview.chromium.org/1613233003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_downloads/toolbar.js:48: searchInput.focus();
On 2016/01/21 22:04:14, michaelpg wrote:
> doesn't this need to be async if showingSearch was false? this results in:
> 
>     this.$$('#search-input').focus();
> 
> calling "focus" on undefined.

yeah, I think it was working because bugs.  maybe I should just expose a
"showAndFocus()" because I don't like the toolbar knowing implementation details
of the search input (that it's async).

wdyt?

https://codereview.chromium.org/1613233003/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/cr_search_field/cr_search_field.js (right):

https://codereview.chromium.org/1613233003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_search_field/cr_search_field.js:75:
searchInput.focus();
^ this is why the code was still working, but I also want to re-focus the search
input if it's already showing and the user presses Ctrl+f

[michaelpg, 2016-01-21 22:19:01]

https://codereview.chromium.org/1613233003/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_downloads/toolbar.js (right):

https://codereview.chromium.org/1613233003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_downloads/toolbar.js:48: searchInput.focus();
On 2016/01/21 22:14:46, Dan Beam wrote:
> On 2016/01/21 22:04:14, michaelpg wrote:
> > doesn't this need to be async if showingSearch was false? this results in:
> > 
> >     this.$$('#search-input').focus();
> > 
> > calling "focus" on undefined.
> 
> yeah, I think it was working because bugs.  maybe I should just expose a
> "showAndFocus()" because I don't like the toolbar knowing implementation
details
> of the search input (that it's async).
> 
> wdyt?

i agree. or just have focus() do it... seems like a reasonable side effect to
me.

https://codereview.chromium.org/1613233003/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/cr_search_field/cr_search_field.js (right):

https://codereview.chromium.org/1613233003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_search_field/cr_search_field.js:75:
searchInput.focus();
On 2016/01/21 22:14:46, Dan Beam wrote:
> ^ this is why the code was still working, but I also want to re-focus the
search
> input if it's already showing and the user presses Ctrl+f

right. Maybe just do that in focus() though:

this.showingSearch = true;
this.async(fn(){...});

[Dan Beam, 2016-01-21 23:39:27]

https://codereview.chromium.org/1613233003/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_downloads/toolbar.js (right):

https://codereview.chromium.org/1613233003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_downloads/toolbar.js:43: var searchInput = this.$ &&
this.$['search-input'];
On 2016/01/21 22:04:14, michaelpg wrote:
> this should already be true anyway (the element should always be ready, given
> how this is called), but i suppose it's okay to enforce this here too?

I'm not sure calling .focus() should show; I'd rather it assert() if anything. 
But I just exposed focusAndShow() to be more clear and not have to add an
assert.

https://codereview.chromium.org/1613233003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_downloads/toolbar.js:48: searchInput.focus();
On 2016/01/21 22:19:01, michaelpg wrote:
> On 2016/01/21 22:14:46, Dan Beam wrote:
> > On 2016/01/21 22:04:14, michaelpg wrote:
> > > doesn't this need to be async if showingSearch was false? this results in:
> > > 
> > >     this.$$('#search-input').focus();
> > > 
> > > calling "focus" on undefined.
> > 
> > yeah, I think it was working because bugs.  maybe I should just expose a
> > "showAndFocus()" because I don't like the toolbar knowing implementation
> details
> > of the search input (that it's async).
> > 
> > wdyt?
> 
> i agree. or just have focus() do it... seems like a reasonable side effect to
> me.

eh, I think it's more reasonable to compose out show and focus and invoke both
when needed (so there's no possibility for unexpected side effects)

[michaelpg, 2016-01-21 23:43:35]

https://codereview.chromium.org/1613233003/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_downloads/toolbar.js (right):

https://codereview.chromium.org/1613233003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_downloads/toolbar.js:48: searchInput.focus();
On 2016/01/21 23:39:27, Dan Beam wrote:
> On 2016/01/21 22:19:01, michaelpg wrote:
> > On 2016/01/21 22:14:46, Dan Beam wrote:
> > > On 2016/01/21 22:04:14, michaelpg wrote:
> > > > doesn't this need to be async if showingSearch was false? this results
in:
> > > > 
> > > >     this.$$('#search-input').focus();
> > > > 
> > > > calling "focus" on undefined.
> > > 
> > > yeah, I think it was working because bugs.  maybe I should just expose a
> > > "showAndFocus()" because I don't like the toolbar knowing implementation
> > details
> > > of the search input (that it's async).
> > > 
> > > wdyt?
> > 
> > i agree. or just have focus() do it... seems like a reasonable side effect
to
> > me.
> 
> eh, I think it's more reasonable to compose out show and focus and invoke both
> when needed (so there's no possibility for unexpected side effects)

sgtm

https://codereview.chromium.org/1613233003/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_search_field/cr_search_field.js (right):

https://codereview.chromium.org/1613233003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_search_field/cr_search_field.js:103:
toggleShowingSearch_: function() {
unused now?

[Dan Beam, 2016-01-21 23:54:24]

https://codereview.chromium.org/1613233003/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_search_field/cr_search_field.js (right):

https://codereview.chromium.org/1613233003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_search_field/cr_search_field.js:103:
toggleShowingSearch_: function() {
On 2016/01/21 23:43:35, michaelpg wrote:
> unused now?

nope, in html

[michaelpg, 2016-01-21 23:56:57]

lgtm

[Dan Beam, 2016-01-21 23:58:24]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2016-01-21 23:59:27]

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

[commit-bot: I haz the power, 2016-01-21 23:59:28]

This CL has an open dependency (Issue 1619013002 Patch 20001). Please resolve
the dependency and try again.

[Dan Beam, 2016-01-22 00:52:19]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-01-22 00:52:22]

The CQ bit was unchecked by dbeam@chromium.org

[Dan Beam, 2016-01-22 00:52:40]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2016-01-22 00:54:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1613233003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1613233003/20001

[commit-bot: I haz the power, 2016-01-22 02:26:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-22 02:28:51]

Description was changed from

==========
MD Downloads: make Ctrl+f invoke in-page search

R=michaelpg@chromium.org
BUG=578323
==========

to

==========
MD Downloads: make Ctrl+f invoke in-page search

R=michaelpg@chromium.org
BUG=578323

Committed: https://crrev.com/60ebcccd01f6faaa0c85fe583bb73d2032e4f450
Cr-Commit-Position: refs/heads/master@{#370881}
==========

[commit-bot: I haz the power, 2016-01-22 02:28:52]

Patchset 2 (id:??) landed as
https://crrev.com/60ebcccd01f6faaa0c85fe583bb73d2032e4f450
Cr-Commit-Position: refs/heads/master@{#370881}