MD History: Delete button in the toolbar allows deletion of multiple history-items.

MD History: Delete button in the toolbar allows deletion of multiple history-items.

Any history-item that has been selected is then deleted and removed from
view. Time-gap-separators are updated, as are the history-card sizes
and positions. The scroll position stays where it is.

BUG=425625
Committed: https://crrev.com/a28af166f376ecce3128844fae18b14556a4dfc1
Cr-Commit-Position: refs/heads/master@{#373764}

[hsampson, 2016-01-14 23:50:38]

hsampson@google.com changed reviewers:
+ calamity@chromium.org, tsergeant@chromium.org

[hsampson, 2016-01-18 00:14:47]

Description was changed from

==========
Delete button in the toolbar allows deletion of multiple history-items.

Any history-item that has been selected is then deleted and removed from
view. Time-gap-separators are updated, as are the history-card sizes
and positions. The scroll position stays where it is.

BUG=425625
==========

to

==========
MD History: Delete button in the toolbar allows deletion of multiple
history-items.

Any history-item that has been selected is then deleted and removed from
view. Time-gap-separators are updated, as are the history-card sizes
and positions. The scroll position stays where it is.

BUG=425625
==========

[tsergeant, 2016-01-18 02:33:52]

Guess what I'm going to ask for?

(hint: it's more tests)

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history-card-manager.js (right):

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history-card-manager.js:143:
itemDeletedFromCard = true;
There's a subtle issue here:

itemDeletedFromCard will be set to true after a single item is deleted, and then
will remain true for every other card. You'll need to make sure you reset it
after every card.

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history-card-manager.js:149: if
(itemDeletedFromCard) {
Nit: Remove {}

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history-card-manager.js:152: if
(overallItemCount == 0)
Why is overallItemCount checked twice? Is it sufficient to just check it in the
inner for loop?

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history-card-manager.js:163:
removeExtraCards_: function() {
Nit: Rename to removeEmptyCards?

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history-card-manager.js:205:
listOfItemsToDelete: function(count) {
Nit: rename to getItemsToDelete

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history-card-manager.js:218: if (count == 0)
{
Nit: Remove {}

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history-card.html (right):

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history-card.html:61: <template is="dom-if"
if="{{needsTimeGap_(index, historyItems.length)}}">
You should change the definition of needsTimeGap_ to include the new param (even
if it is unused)

Also, wrap this line to 80 characters

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history.js (right):

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:24: function historyResult(info,
results) {
Move this function down under the // Chrome Callbacks line

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_history_ui.cc (right):

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_history_ui.cc:64:
source->AddBoolean("allowDeletingHistory", allow_deleting_history);
Hmmmmmmmmmm, we'd better make sure there's a test for this.

(This is the sort of feature that's really easy to forget about, so it's useful
to have machines remember it rather than relying on humans)

There's an example for how to test this in the old UI:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w...

Ping me when you want to work on this test and I can explain what needs to be
done.

[tsergeant, 2016-01-18 02:35:59]

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_history_ui.cc (right):

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_history_ui.cc:64:
source->AddBoolean("allowDeletingHistory", allow_deleting_history);
On 2016/01/18 02:33:52, tsergeant wrote:
> Hmmmmmmmmmm, we'd better make sure there's a test for this.
> 
> (This is the sort of feature that's really easy to forget about, so it's
useful
> to have machines remember it rather than relying on humans)
> 
> There's an example for how to test this in the old UI:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w...
> 
> Ping me when you want to work on this test and I can explain what needs to be
> done.

(to be extra-clear, I'd like to see a test for this, but I'd also generally like
tests for the normal deletion functions).

[hsampson, 2016-01-19 23:41:39]

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history-card-manager.js (right):

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history-card-manager.js:143:
itemDeletedFromCard = true;
On 2016/01/18 02:33:52, tsergeant wrote:
> There's a subtle issue here:
> 
> itemDeletedFromCard will be set to true after a single item is deleted, and
then
> will remain true for every other card. You'll need to make sure you reset it
> after every card.

Done.

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history-card-manager.js:149: if
(itemDeletedFromCard) {
On 2016/01/18 02:33:52, tsergeant wrote:
> Nit: Remove {}

Done.

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history-card-manager.js:152: if
(overallItemCount == 0)
On 2016/01/18 02:33:52, tsergeant wrote:
> Why is overallItemCount checked twice? Is it sufficient to just check it in
the
> inner for loop?

Done.

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history-card-manager.js:163:
removeExtraCards_: function() {
On 2016/01/18 02:33:52, tsergeant wrote:
> Nit: Rename to removeEmptyCards?

Done.

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history-card-manager.js:205:
listOfItemsToDelete: function(count) {
On 2016/01/18 02:33:52, tsergeant wrote:
> Nit: rename to getItemsToDelete

Done.

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history-card-manager.js:218: if (count == 0)
{
On 2016/01/18 02:33:52, tsergeant wrote:
> Nit: Remove {}

Done.

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history-card.html (right):

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history-card.html:61: <template is="dom-if"
if="{{needsTimeGap_(index, historyItems.length)}}">
On 2016/01/18 02:33:52, tsergeant wrote:
> You should change the definition of needsTimeGap_ to include the new param
(even
> if it is unused)
> 
> Also, wrap this line to 80 characters

Done.

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history.js (right):

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:24: function historyResult(info,
results) {
On 2016/01/18 02:33:52, tsergeant wrote:
> Move this function down under the // Chrome Callbacks line 

Done.

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_history_ui.cc (right):

https://codereview.chromium.org/1586373002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_history_ui.cc:64:
source->AddBoolean("allowDeletingHistory", allow_deleting_history);
On 2016/01/18 02:33:52, tsergeant wrote:
> Hmmmmmmmmmm, we'd better make sure there's a test for this.
> 
> (This is the sort of feature that's really easy to forget about, so it's
useful
> to have machines remember it rather than relying on humans)
> 
> There's an example for how to test this in the old UI:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w...
> 
> Ping me when you want to work on this test and I can explain what needs to be
> done.

Done.

[calamity, 2016-01-21 00:11:09]

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history.js (right):

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:38: * to determine which one are
selected and deletes these.
s/one/ones/

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:70: // Chrome
Callbacks-------------------------------------------------------------
Woo, I heartily endorse this sectioning.

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:93: window.console.log('Delete
failed');
Should this still be here?

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card.html (left):

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.html:58:
timestamp="{{historyItem.time}}">
Why did this disappear?

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:103: * @param
{number} overallItemCount the number of items selected.
nit: Capitalize 'The' since it's the start of the description. (repeat
elsewhere)

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:134: for (var j =
items.length - 1; j > -1; j--) {
j >= 0 tends to be the usual way to do this.

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:151:
itemDeletedFromCard = false;
Instead of doing this, just move the declaration of itemDeletedFromCard inside
the first for loop.

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:197:
getItemsToDelete: function(count) {
After a small chat with Tim, I think there's a possibility of improving the
processing.

So the code is often looping through the entirety of history data in order to
find selected things. My idea was to pass more information in the notification
so that we could have history-card-manager track all selected items and just
have enough information there to do what we need.

I think this change is moderately complex and you would need to account for
changing indexes when deleting items. It's a fun data flow/structure and code
design problem though.

tl;dr: it would be nice to have a history-card-manager.selectedItems array

It's up to you whether you want to do this now/later/ever. Feel free to grab me
for a chat if you are having the big confuse.

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:211: break;
Change this to return toBeRemoved;

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:95: return
!loadTimeData.getBoolean('allowDeletingHistory');
It's sad that this needs to be retrieved twice. Tim, do you think it's worth
making this a public property or something?

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_history_ui.cc (right):

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_history_ui.cc:9: #include
"chrome/browser/prefs/incognito_mode_prefs.h"
Unneeded?

https://codereview.chromium.org/1586373002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_card_manager_test.js (right):

https://codereview.chromium.org/1586373002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_card_manager_test.js:26:
"allTimestamps": "4"
Just as a note: history_browsertest.js has a createHistoryEntry function which
makes fake history entries. We should leverage that.

https://codereview.chromium.org/1586373002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_card_manager_test.js:86: 
Why are these necessary?

https://codereview.chromium.org/1586373002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_overflow_menu_test.js (left):

https://codereview.chromium.org/1586373002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_overflow_menu_test.js:69: });
"Hooh! Looks like we got ourselves a sneaky newline here Rick."
"Morty, quit acting like a detective."

[hsampson, 2016-01-21 05:34:47]

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history.js (right):

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:38: * to determine which one are
selected and deletes these.
On 2016/01/21 00:11:08, calamity wrote:
> s/one/ones/

Done.

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:70: // Chrome
Callbacks-------------------------------------------------------------
On 2016/01/21 00:11:08, calamity wrote:
> Woo, I heartily endorse this sectioning.

Yay!

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history.js:93: window.console.log('Delete
failed');
On 2016/01/21 00:11:08, calamity wrote:
> Should this still be here?

Done.

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card.html (left):

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card.html:58:
timestamp="{{historyItem.time}}">
On 2016/01/21 00:11:08, calamity wrote:
> Why did this disappear?

Because it wasn't isn't being used yet, and later on the data associated with
the drop-down menu will be the array of timestamps rather than just one
timestamp.

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:103: * @param
{number} overallItemCount the number of items selected.
On 2016/01/21 00:11:08, calamity wrote:
> nit: Capitalize 'The' since it's the start of the description. (repeat
> elsewhere)

Done.

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:134: for (var j =
items.length - 1; j > -1; j--) {
On 2016/01/21 00:11:08, calamity wrote:
> j >= 0 tends to be the usual way to do this.

Done.

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:151:
itemDeletedFromCard = false;
On 2016/01/21 00:11:08, calamity wrote:
> Instead of doing this, just move the declaration of itemDeletedFromCard inside
> the first for loop.

Done.

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:197:
getItemsToDelete: function(count) {
On 2016/01/21 00:11:08, calamity wrote:
> After a small chat with Tim, I think there's a possibility of improving the
> processing.
> 
> So the code is often looping through the entirety of history data in order to
> find selected things. My idea was to pass more information in the notification
> so that we could have history-card-manager track all selected items and just
> have enough information there to do what we need.
> 
> I think this change is moderately complex and you would need to account for
> changing indexes when deleting items. It's a fun data flow/structure and code
> design problem though.
> 
> tl;dr: it would be nice to have a history-card-manager.selectedItems array
> 
> It's up to you whether you want to do this now/later/ever. Feel free to grab
me
> for a chat if you are having the big confuse.

Once all of the big cls are uploaded I will create a separate cl to deal with
this.

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:211: break;
On 2016/01/21 00:11:08, calamity wrote:
> Change this to return toBeRemoved;

Done.

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:95: return
!loadTimeData.getBoolean('allowDeletingHistory');
On 2016/01/21 00:11:09, calamity wrote:
> It's sad that this needs to be retrieved twice. Tim, do you think it's worth
> making this a public property or something?

Acknowledged.

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_history_ui.cc (right):

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_history_ui.cc:9: #include
"chrome/browser/prefs/incognito_mode_prefs.h"
On 2016/01/21 00:11:09, calamity wrote:
> Unneeded?

Done.

https://codereview.chromium.org/1586373002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_card_manager_test.js (right):

https://codereview.chromium.org/1586373002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_card_manager_test.js:26:
"allTimestamps": "4"
On 2016/01/21 00:11:09, calamity wrote:
> Just as a note: history_browsertest.js has a createHistoryEntry function which
> makes fake history entries. We should leverage that.

Acknowledged.

https://codereview.chromium.org/1586373002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_card_manager_test.js:86: 
On 2016/01/21 00:11:09, calamity wrote:
> Why are these necessary?

These were an attempt to reset the state of all the history-items before
selecting checkboxes but I've changed it now so that the state is reset at the
end of each test if this didn't occur during testing already.

[tsergeant, 2016-01-25 01:43:15]

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1586373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:197:
getItemsToDelete: function(count) {
On 2016/01/21 05:34:46, hsampson wrote:
> On 2016/01/21 00:11:08, calamity wrote:
> > After a small chat with Tim, I think there's a possibility of improving the
> > processing.
> > 
> > So the code is often looping through the entirety of history data in order
to
> > find selected things. My idea was to pass more information in the
notification
> > so that we could have history-card-manager track all selected items and just
> > have enough information there to do what we need.
> > 
> > I think this change is moderately complex and you would need to account for
> > changing indexes when deleting items. It's a fun data flow/structure and
code
> > design problem though.
> > 
> > tl;dr: it would be nice to have a history-card-manager.selectedItems array
> > 
> > It's up to you whether you want to do this now/later/ever. Feel free to grab
> me
> > for a chat if you are having the big confuse.
> 
> Once all of the big cls are uploaded I will create a separate cl to deal with
> this.

Chris and I have ~even more~ ideas about how to do this. Let's chat about it
when CLs start clearing up.

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:133: for (var j =
items.length - 1; j >= 0; j--) {
There's a problem here that is kinda tough to explain.

When you delete an item, the historyDataByDay[i].historyItems loses an item and
gets smaller.

Specifically, the item which used to be at position `j` is removed. The item
which used to be at position `j+1` will then move back to position `j`.

You'll continue looping, and move on to the item in position `j+1`, but *that*
item, in turn, used to be in position `j+2`. You will have completely skipped
over the item that used to at `j+1` and is now at `j`.

You can avoid this by decrementing j after you delete anything.

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:141: if
(this.historyDataByDay_.length > i) {
Remove {}

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.html (right):

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.html:85: checked="{{selected}}"
disabled="{{selectionAllowed_()}}">
I'm not convinced about this pattern...but I don't have any better ideas for
now.

(Mainly since this will only ever be called once, so you're relying on
loadTimeData being ready by that point).

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:94: selectionAllowed_:
function() {
The truthiness of your variable/function names flip around a lot:

In the html:
disabled = selectionAllowed

In the js:
selectionAllowed = not allowDeletingHistory

If you think through it, you can figure out that it's correct. It's really
confusing to read, though, since something 'truthy' is being set to something
'falsey'.

Can you rename things to make this clearer?

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_toolbar.html (right):

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.html:70: <paper-button
id="delete-button" on-tap="deleteSelection">
deleteSelection -> onDeleteTap_

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_history_ui.cc (right):

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_history_ui.cc:64:
source->AddBoolean("allowDeletingHistory", allow_deleting_history);
Move this up to between AddLocalizedString and AddResourcePath

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

https://codereview.chromium.org/1586373002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_card_manager_test.js:152:
MockInteractions.tap(items[1].$['checkbox']);
You can make a teardown() function which resets the checkboxes more reliably:

items[0].selected = false;

That way, even if the callback doesn't get called, the checkboxes will be reset.

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

https://codereview.chromium.org/1586373002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_supervised_user_test.js:15:
suite('history-card-manager', function() {
Suite name should reflect that these are supervised-user specific tests

https://codereview.chromium.org/1586373002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_supervised_user_test.js:22:
test('display for supervised user', function() {
The actual *test* here is whether the checkboxes are selectable. The test name
should reflect that.

https://codereview.chromium.org/1586373002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_supervised_user_test.js:41:
assertEquals(toBeRemoved, []);
If you want to make sure something is never called, you can do

assertTrue(false)

That way, if the line of code is ever touched, it is guaranteed to error.

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

https://codereview.chromium.org/1586373002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/md_history_browsertest.js:63: //
MaterialHistoryBrowserTest.prototype.testGenPreamble.call(this);
Remove this.

[hsampson, 2016-01-25 04:15:42]

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:133: for (var j =
items.length - 1; j >= 0; j--) {
On 2016/01/25 01:43:14, tsergeant wrote:
> There's a problem here that is kinda tough to explain.
> 
> When you delete an item, the historyDataByDay[i].historyItems loses an item
and
> gets smaller.
> 
> Specifically, the item which used to be at position `j` is removed. The item
> which used to be at position `j+1` will then move back to position `j`.
> 
> You'll continue looping, and move on to the item in position `j+1`, but *that*
> item, in turn, used to be in position `j+2`. You will have completely skipped
> over the item that used to at `j+1` and is now at `j`.
> 
> You can avoid this by decrementing j after you delete anything.

As discussed the loop going through backwards is to account for this.

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:141: if
(this.historyDataByDay_.length > i) {
On 2016/01/25 01:43:14, tsergeant wrote:
> Remove {}

Done.

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.html (right):

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.html:85: checked="{{selected}}"
disabled="{{selectionAllowed_()}}">
On 2016/01/25 01:43:14, tsergeant wrote:
> I'm not convinced about this pattern...but I don't have any better ideas for
> now.
> 
> (Mainly since this will only ever be called once, so you're relying on
> loadTimeData being ready by that point).

Acknowledged.

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_item.js (right):

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/history_item.js:94: selectionAllowed_:
function() {
On 2016/01/25 01:43:15, tsergeant wrote:
> The truthiness of your variable/function names flip around a lot:
> 
> In the html:
> disabled = selectionAllowed
> 
> In the js:
> selectionAllowed = not allowDeletingHistory
> 
> If you think through it, you can figure out that it's correct. It's really
> confusing to read, though, since something 'truthy' is being set to something
> 'falsey'.
> 
> Can you rename things to make this clearer?

Done.

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_toolbar.html (right):

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.html:70: <paper-button
id="delete-button" on-tap="deleteSelection">
On 2016/01/25 01:43:15, tsergeant wrote:
> deleteSelection -> onDeleteTap_

Done.

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_history_ui.cc (right):

https://codereview.chromium.org/1586373002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_history_ui.cc:64:
source->AddBoolean("allowDeletingHistory", allow_deleting_history);
On 2016/01/25 01:43:15, tsergeant wrote:
> Move this up to between AddLocalizedString and AddResourcePath

Done.

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

https://codereview.chromium.org/1586373002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_card_manager_test.js:152:
MockInteractions.tap(items[1].$['checkbox']);
On 2016/01/25 01:43:15, tsergeant wrote:
> You can make a teardown() function which resets the checkboxes more reliably:
> 
> items[0].selected = false;
> 
> That way, even if the callback doesn't get called, the checkboxes will be
reset.

Done.

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

https://codereview.chromium.org/1586373002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_supervised_user_test.js:15:
suite('history-card-manager', function() {
On 2016/01/25 01:43:15, tsergeant wrote:
> Suite name should reflect that these are supervised-user specific tests

Done.

https://codereview.chromium.org/1586373002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_supervised_user_test.js:22:
test('display for supervised user', function() {
On 2016/01/25 01:43:15, tsergeant wrote:
> The actual *test* here is whether the checkboxes are selectable. The test name
> should reflect that.

Done.

https://codereview.chromium.org/1586373002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_supervised_user_test.js:41:
assertEquals(toBeRemoved, []);
On 2016/01/25 01:43:15, tsergeant wrote:
> If you want to make sure something is never called, you can do
> 
> assertTrue(false)
> 
> That way, if the line of code is ever touched, it is guaranteed to error.

Done.

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

https://codereview.chromium.org/1586373002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/md_history_browsertest.js:63: //
MaterialHistoryBrowserTest.prototype.testGenPreamble.call(this);
On 2016/01/25 01:43:15, tsergeant wrote:
> Remove this.

Done.

[tsergeant, 2016-02-01 22:47:53]

This one is looking pretty good

https://codereview.chromium.org/1586373002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1586373002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:215: }
You should have

return [];

here so that the method always returns array.

(At the moment it returns (array|undefined). Does closure complain about this?)

https://codereview.chromium.org/1586373002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.html (right):

https://codereview.chromium.org/1586373002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:86: checked="{{selected}}"
disabled="{{selectionNotAllowed_()}}">
Nit: probably use one-way binding here

https://codereview.chromium.org/1586373002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_toolbar.html (right):

https://codereview.chromium.org/1586373002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_toolbar.html:69: <template
is="dom-if" if="{{deletingAllowed_()}}">
When deleting is disabled, you disable selection, and prevent deletion calls
from going through to the backend in history.js.

Given that, I think this one is kind of unnecessary. You shouldn't ever be able
to see the delete button, and even if you can, clicking it will do nothing.

https://codereview.chromium.org/1586373002/diff/140001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_supervised_user_test.js (right):

https://codereview.chromium.org/1586373002/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_supervised_user_test.js:26:
flush(function() {
If you're using flush, then you need done()

[hsampson, 2016-02-02 00:45:11]

https://codereview.chromium.org/1586373002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1586373002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:215: }
On 2016/02/01 22:47:53, tsergeant wrote:
> You should have
> 
> return [];
> 
> here so that the method always returns array.
> 
> (At the moment it returns (array|undefined). Does closure complain about
this?)

Done. The closure didn't complain about this though.

https://codereview.chromium.org/1586373002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_item.html (right):

https://codereview.chromium.org/1586373002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_item.html:86: checked="{{selected}}"
disabled="{{selectionNotAllowed_()}}">
On 2016/02/01 22:47:53, tsergeant wrote:
> Nit: probably use one-way binding here

Done.

https://codereview.chromium.org/1586373002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_toolbar.html (right):

https://codereview.chromium.org/1586373002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_toolbar.html:69: <template
is="dom-if" if="{{deletingAllowed_()}}">
On 2016/02/01 22:47:53, tsergeant wrote:
> When deleting is disabled, you disable selection, and prevent deletion calls
> from going through to the backend in history.js.
> 
> Given that, I think this one is kind of unnecessary. You shouldn't ever be
able
> to see the delete button, and even if you can, clicking it will do nothing.

Done.

https://codereview.chromium.org/1586373002/diff/140001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_supervised_user_test.js (right):

https://codereview.chromium.org/1586373002/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_supervised_user_test.js:26:
flush(function() {
On 2016/02/01 22:47:53, tsergeant wrote:
> If you're using flush, then you need done()

Done.

[tsergeant, 2016-02-02 03:36:51]

lgtm, let's see what Chris thinks

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:157: * If a given
day has had all the history it contains removed, remove this
Nit: Reword this comment.

The word 'given' implies that there's a parameter for removing a specific day.

(Although maybe don't worry too much, since history-card is due to be killed
anyway)

[calamity, 2016-02-02 05:34:59]

Mostly lg.

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history.js (right):

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history.js:48: var toBeRemoved =
$('history-card-manager').getItemsToDelete(
nit: wrap after the = instead.

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card.html (right):

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card.html:61:
if="[[needsTimeGap_(index, historyItems.length)]]">
Does this function correctly if the actual item is changed then (i.e does
needsTimeGap_ run when the historyItems array length doesn't change but the
actual item being rendered does)? (This happens when cards are swapped in during
the infinite scroll, which is why we needed historyItem here in the first place)

#IfYouLikeItThenYouShouldHaveWroteATestForIt

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:138: if
(items[j].selected) {
nit: Invert and use continue.

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:145: if
(this.historyDataByDay_.length > i)
nit: Flip this around. Seeing a greater than comparison is making my head spin.

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:198:
getItemsToDelete: function(count) {
nit: getSelectedItems?

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_toolbar.html (right):

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_toolbar.html:67: CANCEL
Do these need to be i18ned? (follow up patch if yes)

https://codereview.chromium.org/1586373002/diff/160001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_card_manager_test.js (right):

https://codereview.chromium.org/1586373002/diff/160001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_card_manager_test.js:26:
"allTimestamps": "4"
We should make a function that makes test results rather than just enumerating
all the fields here. (To be done in a follow-up)

[hsampson, 2016-02-02 22:59:54]

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history.js (right):

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history.js:48: var toBeRemoved =
$('history-card-manager').getItemsToDelete(
On 2016/02/02 05:34:58, calamity wrote:
> nit: wrap after the = instead.

Done.

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card.html (right):

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card.html:61:
if="[[needsTimeGap_(index, historyItems.length)]]">
On 2016/02/02 05:34:58, calamity wrote:
> Does this function correctly if the actual item is changed then (i.e does
> needsTimeGap_ run when the historyItems array length doesn't change but the
> actual item being rendered does)? (This happens when cards are swapped in
during
> the infinite scroll, which is why we needed historyItem here in the first
place)
> 
> #IfYouLikeItThenYouShouldHaveWroteATestForIt

I haven't found any errors just scrolling but I think you are right here.

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card_manager.js (right):

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:138: if
(items[j].selected) {
On 2016/02/02 05:34:58, calamity wrote:
> nit: Invert and use continue.

Done.

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:145: if
(this.historyDataByDay_.length > i)
On 2016/02/02 05:34:58, calamity wrote:
> nit: Flip this around. Seeing a greater than comparison is making my head
spin.

Done.

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:157: * If a given
day has had all the history it contains removed, remove this
On 2016/02/02 03:36:51, tsergeant wrote:
> Nit: Reword this comment.
> 
> The word 'given' implies that there's a parameter for removing a specific day.
> 
> (Although maybe don't worry too much, since history-card is due to be killed
> anyway)

Done.

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card_manager.js:198:
getItemsToDelete: function(count) {
On 2016/02/02 05:34:58, calamity wrote:
> nit: getSelectedItems?

Done.

[calamity, 2016-02-03 02:10:50]

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/md_history/history_card.html (right):

https://codereview.chromium.org/1586373002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_card.html:61:
if="[[needsTimeGap_(index, historyItems.length)]]">
On 2016/02/02 22:59:53, hsampson wrote:
> On 2016/02/02 05:34:58, calamity wrote:
> > Does this function correctly if the actual item is changed then (i.e does
> > needsTimeGap_ run when the historyItems array length doesn't change but the
> > actual item being rendered does)? (This happens when cards are swapped in
> during
> > the infinite scroll, which is why we needed historyItem here in the first
> place)
> > 
> > #IfYouLikeItThenYouShouldHaveWroteATestForIt
> 
> I haven't found any errors just scrolling but I think you are right here.

Can you write a regression test that recreates the issue and then verify that
having historyItem here fixes it?

[calamity, 2016-02-04 23:59:52]

lgtm.

https://codereview.chromium.org/1586373002/diff/200001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_card_test.js (right):

https://codereview.chromium.org/1586373002/diff/200001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_card_test.js:10: var items;
nit: remove spacers and items from here and just have them as local vars in
theirs tests.

https://codereview.chromium.org/1586373002/diff/200001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_card_test.js:31: test('separator
insertion when items change but card stays the same',
'but item list length stays the same'

https://codereview.chromium.org/1586373002/diff/200001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_card_test.js:56: spacers = [];
You should be able to remove this.

[hsampson, 2016-02-05 05:21:12]

The CQ bit was checked by hsampson@google.com

[hsampson, 2016-02-05 05:21:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org, calamity@chromium.org
Link to the patchset: https://codereview.chromium.org/1586373002/#ps240001
(title: "Fix rebase.")

[commit-bot: I haz the power, 2016-02-05 05:21:55]

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

[commit-bot: I haz the power, 2016-02-05 06:03:15]

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

[commit-bot: I haz the power, 2016-02-05 06:03:16]

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

[hsampson, 2016-02-05 06:17:16]

The CQ bit was checked by hsampson@google.com

[hsampson, 2016-02-05 06:17:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org, calamity@chromium.org
Link to the patchset: https://codereview.chromium.org/1586373002/#ps260001
(title: "Fix supervised user test.")

[commit-bot: I haz the power, 2016-02-05 06:17:33]

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

[commit-bot: I haz the power, 2016-02-05 07:56:23]

Description was changed from

==========
MD History: Delete button in the toolbar allows deletion of multiple
history-items.

Any history-item that has been selected is then deleted and removed from
view. Time-gap-separators are updated, as are the history-card sizes
and positions. The scroll position stays where it is.

BUG=425625
==========

to

==========
MD History: Delete button in the toolbar allows deletion of multiple
history-items.

Any history-item that has been selected is then deleted and removed from
view. Time-gap-separators are updated, as are the history-card sizes
and positions. The scroll position stays where it is.

BUG=425625
==========

[commit-bot: I haz the power, 2016-02-05 07:56:24]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-02-05 07:57:05]

Description was changed from

==========
MD History: Delete button in the toolbar allows deletion of multiple
history-items.

Any history-item that has been selected is then deleted and removed from
view. Time-gap-separators are updated, as are the history-card sizes
and positions. The scroll position stays where it is.

BUG=425625
==========

to

==========
MD History: Delete button in the toolbar allows deletion of multiple
history-items.

Any history-item that has been selected is then deleted and removed from
view. Time-gap-separators are updated, as are the history-card sizes
and positions. The scroll position stays where it is.

BUG=425625

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

[commit-bot: I haz the power, 2016-02-05 07:57:07]

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