MD History: add shortcuts for search

MD History: add shortcuts for search

Add keyboard shortcuts to 'Ctrl-f' and '/' to focus the search field.

BUG=619795
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/df095b24ea86eb8109ae6004fdbdc3532d62a968
Cr-Commit-Position: refs/heads/master@{#404329}

[lshang, 2016-07-05 05:38:26]

Description was changed from

==========
MD History: add shortcuts for search

some more change


add

BUG=
==========

to

==========
MD History: add shortcuts for search

some more change


add

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[lshang, 2016-07-05 06:02:50]

Description was changed from

==========
MD History: add shortcuts for search

some more change


add

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD History: add shortcuts for search

Add keyboard shortcuts to 'Ctrl-f' and '/' to focus the search field.

BUG=619795
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[lshang, 2016-07-05 06:14:09]

lshang@chromium.org changed reviewers:
+ tsergeant@chromium.org

[lshang, 2016-07-05 06:14:09]

Tim, PTAL thanks! ↖(^ω^)↗

[tsergeant, 2016-07-05 07:47:22]

https://codereview.chromium.org/2118383003/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/2118383003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/app.js:115: * @param {Event} e
Does the closure compile work successfully if you make this
{cr.ui.CanExecuteEvent} and remove the type cast below? I know this is what
Downloads does, but it seems like it's not the easiest way.

https://codereview.chromium.org/2118383003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/app.js:120: switch (e.command.id) {
I think there's no need to have the switch here, you can just do

e.canExecute = true;

for every event.

https://codereview.chromium.org/2118383003/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_history/history_toolbar.js (right):

https://codereview.chromium.org/2118383003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_toolbar.js:99: onFindCommand:
function() {
Since there's nothing about this function that ties it directly to the find
command, you could name it something more generic like "showSearchField".

https://codereview.chromium.org/2118383003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_toolbar.js:100:
this.$['main-toolbar'].$['search'].showAndFocus();
Use

this.$['main-toolbar'].getSearchField().showAndFocus();

as above in setSearchTerm();

Outside of tests, try to avoid nesting use of $[], since you end up depending on
the internal structure of other elements.

[lshang, 2016-07-06 05:03:08]

https://codereview.chromium.org/2118383003/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/2118383003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/app.js:115: * @param {Event} e
On 2016/07/05 07:47:22, tsergeant wrote:
> Does the closure compile work successfully if you make this
> {cr.ui.CanExecuteEvent} and remove the type cast below? I know this is what
> Downloads does, but it seems like it's not the easiest way.

Seems closure compiler can't recognize cr.ui.CanExecuteEvent as Event, as we
see.

https://codereview.chromium.org/2118383003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/app.js:120: switch (e.command.id) {
On 2016/07/05 07:47:22, tsergeant wrote:
> I think there's no need to have the switch here, you can just do
> 
> e.canExecute = true;
> 
> for every event.

Done.

https://codereview.chromium.org/2118383003/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_history/history_toolbar.js (right):

https://codereview.chromium.org/2118383003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_toolbar.js:99: onFindCommand:
function() {
On 2016/07/05 07:47:22, tsergeant wrote:
> Since there's nothing about this function that ties it directly to the find
> command, you could name it something more generic like "showSearchField".

Done.

https://codereview.chromium.org/2118383003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_toolbar.js:100:
this.$['main-toolbar'].$['search'].showAndFocus();
On 2016/07/05 07:47:22, tsergeant wrote:
> Use
> 
> this.$['main-toolbar'].getSearchField().showAndFocus();
> 
> as above in setSearchTerm();
> 
> Outside of tests, try to avoid nesting use of $[], since you end up depending
on
> the internal structure of other elements.

Done. Good point!

[tsergeant, 2016-07-06 06:26:12]

Implementation looks good. Can you please also add a test for this?

[lshang, 2016-07-07 07:31:28]

On 2016/07/06 06:26:12, tsergeant wrote:
> Implementation looks good. Can you please also add a test for this?

I‘ve added a test for this. PTAL thanks!

[tsergeant, 2016-07-08 00:26:05]

lgtm with nits

https://codereview.chromium.org/2118383003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_toolbar_test.js (right):

https://codereview.chromium.org/2118383003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:60:
field.$.searchInput, 191, '', '/');
In MockInteractions.pressAndReleaseKeyOn, it's probably better to not target
`field.$.searchInput`, since that's kind of like saying that the events will
work if the search input is already focused.

It's better to check that the events can be fired from anywhere on the document
and still work:

MockInteractions.pressAndReleaseKeyOn(document, 191, ...)

https://codereview.chromium.org/2118383003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:69:
MockInteractions.pressAndReleaseKeyOn(
As above, use document here.

[lshang, 2016-07-08 05:37:25]

https://codereview.chromium.org/2118383003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_toolbar_test.js (right):

https://codereview.chromium.org/2118383003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:60:
field.$.searchInput, 191, '', '/');
On 2016/07/08 00:26:05, tsergeant wrote:
> In MockInteractions.pressAndReleaseKeyOn, it's probably better to not target
> `field.$.searchInput`, since that's kind of like saying that the events will
> work if the search input is already focused.
> 
> It's better to check that the events can be fired from anywhere on the
document
> and still work:
> 
> MockInteractions.pressAndReleaseKeyOn(document, 191, ...)

Done.

https://codereview.chromium.org/2118383003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:69:
MockInteractions.pressAndReleaseKeyOn(
On 2016/07/08 00:26:05, tsergeant wrote:
> As above, use document here.

Done.

[lshang, 2016-07-08 05:37:37]

The CQ bit was checked by lshang@chromium.org

[lshang, 2016-07-08 05:37:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org
Link to the patchset: https://codereview.chromium.org/2118383003/#ps100001
(title: "test change")

[commit-bot: I haz the power, 2016-07-08 05:38:00]

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-07-08 09:38:22]

Description was changed from

==========
MD History: add shortcuts for search

Add keyboard shortcuts to 'Ctrl-f' and '/' to focus the search field.

BUG=619795
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD History: add shortcuts for search

Add keyboard shortcuts to 'Ctrl-f' and '/' to focus the search field.

BUG=619795
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-07-08 09:38:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-08 09:40:53]

Description was changed from

==========
MD History: add shortcuts for search

Add keyboard shortcuts to 'Ctrl-f' and '/' to focus the search field.

BUG=619795
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD History: add shortcuts for search

Add keyboard shortcuts to 'Ctrl-f' and '/' to focus the search field.

BUG=619795
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/df095b24ea86eb8109ae6004fdbdc3532d62a968
Cr-Commit-Position: refs/heads/master@{#404329}
==========

[commit-bot: I haz the power, 2016-07-08 09:40:54]

Patchset 6 (id:??) landed as
https://crrev.com/df095b24ea86eb8109ae6004fdbdc3532d62a968
Cr-Commit-Position: refs/heads/master@{#404329}