MD History: Add shortcuts for 'Delete' and 'Backspace' to delete selected items

MD History: Add shortcuts for 'Delete' and 'Backspace' to delete selected items

Press 'Delete' or 'Backspace' key from keyboard will delete selected history items
on MD History.

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

Committed: https://crrev.com/24cca661cb0b2afe2ca1b1fa960924b375d63256
Cr-Commit-Position: refs/heads/master@{#406715}

[lshang, 2016-07-19 01:41:29]

Description was changed from

==========
MD History: Add shortcuts for 'Delete' and 'Backspace' to delete selected items


Merge branch 'MDH_slash_key' into MDH_add_shortcut_for_delete_history


version 1


rebase


some change

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

to

==========
MD History: Add shortcuts for 'Delete' and 'Backspace' to delete selected items


Merge branch 'MDH_slash_key' into MDH_add_shortcut_for_delete_history


version 1


rebase


some change

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

[lshang, 2016-07-19 04:45:24]

Description was changed from

==========
MD History: Add shortcuts for 'Delete' and 'Backspace' to delete selected items


Merge branch 'MDH_slash_key' into MDH_add_shortcut_for_delete_history


version 1


rebase


some change

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

to

==========
MD History: Add shortcuts for 'Delete' and 'Backspace' to delete selected items

Press 'Delete' or 'Backspace' key from keyboard will delete selected history
items
on MD History. 

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

[lshang, 2016-07-19 04:48:33]

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

[lshang, 2016-07-19 04:48:34]

PTAL thanks!

[tsergeant, 2016-07-19 05:09:41]

Can you please add a test for this as well?

I'm not sure if you're aware -- this is actually different behavior than the old
page, which will delete the currently focused item rather than all selected
items.

I think changing the behavior is fine for now. This is much easier to implement
and we can change it later if anyone complains.

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

https://codereview.chromium.org/2159773003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/app.js:132: e.canExecute =
this.$['history'].getHistoryList().numToBeDeleted() > 0;
You could probably simplify this using `this.$.toolbar.count`, which is the
number shown in the toolbar.

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

https://codereview.chromium.org/2159773003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_list.js:212: numToBeDeleted:
function() {
Nit: Rename this to something like numSelected or selectedCount, since that's
what you're actually counting here.

[lshang, 2016-07-20 01:16:59]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 01:17:37]

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

[lshang, 2016-07-20 01:19:09]

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

https://codereview.chromium.org/2159773003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/app.js:132: e.canExecute =
this.$['history'].getHistoryList().numToBeDeleted() > 0;
On 2016/07/19 05:09:41, tsergeant wrote:
> You could probably simplify this using `this.$.toolbar.count`, which is the
> number shown in the toolbar.

Done.

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

https://codereview.chromium.org/2159773003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_list.js:212: numToBeDeleted:
function() {
On 2016/07/19 05:09:41, tsergeant wrote:
> Nit: Rename this to something like numSelected or selectedCount, since that's
> what you're actually counting here.

I deleted this method and used 'toolbar.count' instead, but I'll pay attention
to method's names next time. Good suggestion.

[lshang, 2016-07-20 01:25:59]

On 2016/07/19 05:09:41, tsergeant wrote:
> Can you please add a test for this as well?
> 

Test added.

> I'm not sure if you're aware -- this is actually different behavior than the
old
> page, which will delete the currently focused item rather than all selected
> items.
> 
> I think changing the behavior is fine for now. This is much easier to
implement
> and we can change it later if anyone complains.
> 

I think deleting all selected items feels more natural, at least to me, if I
have checkboxed multiple items there, especially when we don't highlight focus
on the recent selected history item.
So let's see what people will say about this.

[tsergeant, 2016-07-20 03:04:14]

https://codereview.chromium.org/2159773003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_list_test.js (right):

https://codereview.chromium.org/2159773003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_list_test.js:237: return
flush().then(function() {
Nit: If there's a `done` parameter to the test, you don't need a return here.
Just `flush().then(function() {`

(`return flush()...` returns a promise which controls the lifetime of the test.
Using a `done` parameter does the same thing, and having the test lifetime
controlled by two things could have unexpected results)

https://codereview.chromium.org/2159773003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_list_test.js:251:
flush().then(function() {
I think it's not necessary to do all of this, since it end-to-end tests that
deletion works. That's already tested elsewhere, here you can just check that
the right things are being deleted.

registerMessageCallback('removeVisits', this, function(toRemove) {
  // assert that toRemove[0] and toRemove[1] have the correct url
});

see:
https://cs.chromium.org/chromium/src/chrome/test/data/webui/md_history/browse...

[commit-bot: I haz the power, 2016-07-20 03:50:33]

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

[commit-bot: I haz the power, 2016-07-20 03:50:34]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-07-20 06:59:32]

Patchset #3 (id:40001) has been deleted

[lshang, 2016-07-20 07:00:45]

https://codereview.chromium.org/2159773003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_list_test.js (right):

https://codereview.chromium.org/2159773003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_list_test.js:237: return
flush().then(function() {
On 2016/07/20 03:04:14, tsergeant wrote:
> Nit: If there's a `done` parameter to the test, you don't need a return here.
> Just `flush().then(function() {`
> 
> (`return flush()...` returns a promise which controls the lifetime of the
test.
> Using a `done` parameter does the same thing, and having the test lifetime
> controlled by two things could have unexpected results)

Done.
Thanks for the explanation(∩_∩)

https://codereview.chromium.org/2159773003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_list_test.js:251:
flush().then(function() {
On 2016/07/20 03:04:14, tsergeant wrote:
> I think it's not necessary to do all of this, since it end-to-end tests that
> deletion works. That's already tested elsewhere, here you can just check that
> the right things are being deleted.
> 
> registerMessageCallback('removeVisits', this, function(toRemove) {
>   // assert that toRemove[0] and toRemove[1] have the correct url
> });
> 
> see:
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/md_history/browse...

Done.

[tsergeant, 2016-07-20 07:44:56]

lgtm

[lshang, 2016-07-20 08:08:58]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 08:09:23]

Dry run: 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-20 08:56:22]

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

[commit-bot: I haz the power, 2016-07-20 08:56:23]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-07-20 23:44:10]

The CQ bit was checked by lshang@chromium.org

[commit-bot: I haz the power, 2016-07-20 23:44:41]

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-20 23:50:04]

Description was changed from

==========
MD History: Add shortcuts for 'Delete' and 'Backspace' to delete selected items

Press 'Delete' or 'Backspace' key from keyboard will delete selected history
items
on MD History. 

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

to

==========
MD History: Add shortcuts for 'Delete' and 'Backspace' to delete selected items

Press 'Delete' or 'Backspace' key from keyboard will delete selected history
items
on MD History. 

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

[commit-bot: I haz the power, 2016-07-20 23:50:07]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-07-20 23:51:46]

Description was changed from

==========
MD History: Add shortcuts for 'Delete' and 'Backspace' to delete selected items

Press 'Delete' or 'Backspace' key from keyboard will delete selected history
items
on MD History. 

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

to

==========
MD History: Add shortcuts for 'Delete' and 'Backspace' to delete selected items

Press 'Delete' or 'Backspace' key from keyboard will delete selected history
items
on MD History.

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

Committed: https://crrev.com/24cca661cb0b2afe2ca1b1fa960924b375d63256
Cr-Commit-Position: refs/heads/master@{#406715}
==========

[commit-bot: I haz the power, 2016-07-20 23:51:47]

Patchset 3 (id:??) landed as
https://crrev.com/24cca661cb0b2afe2ca1b1fa960924b375d63256
Cr-Commit-Position: refs/heads/master@{#406715}