[MD History] Factor out a common HistoryListBehavior.

[MD History] Factor out a common HistoryListBehavior.

This CL extracts a HistoryListBehavior that is used by the history-list
and the history-grouped-list. The selection system has been refactored
to accommodate different list structures.

Using this behavior in the grouped history list means that it now
implements selection and deletion.

BUG=589357, 589363
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1a7fa0b24d413123832e2707adbc0266122a225f
Cr-Commit-Position: refs/heads/master@{#411253}

[calamity, 2016-08-04 03:58:33]

Description was changed from

==========
[MD History] Factor out a common HistoryListBehavior.

This CL extracts a HistoryListBehavior that is used by the history-list
and the history-grouped-list. The selection system has been refactored
to accommodate different list structures.

Using this behavior in the grouped history list means that it now
implements selection and deletion.

BUG=589357,589363
==========

to

==========
[MD History] Factor out a common HistoryListBehavior.

This CL extracts a HistoryListBehavior that is used by the history-list
and the history-grouped-list. The selection system has been refactored
to accommodate different list structures.

Using this behavior in the grouped history list means that it now
implements selection and deletion.

BUG=589357,589363
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[calamity, 2016-08-04 04:00:26]

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

[calamity, 2016-08-04 04:00:27]

Hoo-ee, here we go. PTAL (I'm Mr CL, look at me!)

[tsergeant, 2016-08-05 01:39:09]

Huh. Where'd my friday morning go?

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

https://codereview.chromium.org/2207323002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.js:20: // history-list and into
history-app.
This can be deleted now 🙌

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

https://codereview.chromium.org/2207323002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_item.js:46:
this.fire('history-checkbox-select', {
I think we should consider making this fire on selectedChanged_. I'm a little
concerned about ways that the selected list can get out of sync. No need to
change anything right now.

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

https://codereview.chromium.org/2207323002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_list_behavior.js:37: selectedPaths:
{type: Array, value: function() { return []; }},
Yay! Now we can databind this through to the toolbar and spend a lot less effort
doing manual maintenance of the selection count.

Can you put a TODO in for this?

https://codereview.chromium.org/2207323002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_list_behavior.js:79:
removeItemsByPath: function(paths) {
I think I've made my peace with this tree craziness. A few things I'd like to
see, though:

a) Expand the method comment here to explain a little bit of what is happening
and why this is done. I think it's pretty hard to determine at a glance.

b) A comparison of speed for deleting items with this vs plain `this.splice` on
each path (on the grouped page, I guess). It would be nice to know how much this
saves.

c) It would be really nice to have a direct unit test for this.
`removeItemsByPath` is nicely decoupled from the rest of the Polymer stuff, but
`removeItemsBeneatNode_` makes things more difficult. I won't block on this.

https://codereview.chromium.org/2207323002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_list_behavior.js:142: * @private
Let's make this method and the next public.

I think the style in Chromium so far is that any methods in a behavior which are
accessed by an implementer of that behavior should be public.

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

https://codereview.chromium.org/2207323002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/list_container.js:88:
this.$.content.selectedItem.unselectAllItems(count);
Nit: Maybe extract this.$.content.selectedItem into a getter which returns a
HistoryListBehavior.

The idea being that then we are forced by closure in this file to not know which
type of list we are dealing with.

https://codereview.chromium.org/2207323002/diff/1/chrome/test/data/webui/md_h...
File chrome/test/data/webui/md_history/history_grouped_list_test.js (right):

https://codereview.chromium.org/2207323002/diff/1/chrome/test/data/webui/md_h...
chrome/test/data/webui/md_history/history_grouped_list_test.js:200:
assertEquals('https://en.wikipedia.org/d', items[1].item.title);
You should assert that there are no empty domains lying around.

https://codereview.chromium.org/2207323002/diff/1/chrome/test/data/webui/md_h...
File chrome/test/data/webui/md_history/test_util.js (right):

https://codereview.chromium.org/2207323002/diff/1/chrome/test/data/webui/md_h...
chrome/test/data/webui/md_history/test_util.js:84: function
polymerQuerySelectorAll(element, selector) {
If you want, you could make your/our lives easier by naming this
polymerSelectAll or something a bit shorter.

https://codereview.chromium.org/2207323002/diff/1/ui/webui/resources/js/util.js
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/2207323002/diff/1/ui/webui/resources/js/util....
ui/webui/resources/js/util.js:381: * @param {function(Event): boolean} predicate
@return {Promise}

https://codereview.chromium.org/2207323002/diff/1/ui/webui/resources/js/util....
ui/webui/resources/js/util.js:383: function waitForEvent(element, eventName,
predicate) {
I can conceive of situations where this would be useful in real code... but
considering at the moment this is only used in a test, I don't think there's any
need to ship this out with every WebUI.

I think we should just put this in test_util for now.

[calamity, 2016-08-08 05:32:42]

Patchset #2 (id:20001) has been deleted

[calamity, 2016-08-08 06:35:41]

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

[commit-bot: I haz the power, 2016-08-08 06:36:01]

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-08-08 07:46:31]

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

[commit-bot: I haz the power, 2016-08-08 07:46:32]

Dry run: This issue passed the CQ dry run.

[calamity, 2016-08-09 02:56:01]

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

https://codereview.chromium.org/2207323002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.js:20: // history-list and into
history-app.
On 2016/08/05 01:39:09, tsergeant wrote:
> This can be deleted now 🙌

Done.

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

https://codereview.chromium.org/2207323002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_item.js:46:
this.fire('history-checkbox-select', {
On 2016/08/05 01:39:09, tsergeant wrote:
> I think we should consider making this fire on selectedChanged_. I'm a little
> concerned about ways that the selected list can get out of sync. No need to
> change anything right now.

Yeah, I agree. Added a TODO.

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

https://codereview.chromium.org/2207323002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_list_behavior.js:37: selectedPaths:
{type: Array, value: function() { return []; }},
On 2016/08/05 01:39:09, tsergeant wrote:
> Yay! Now we can databind this through to the toolbar and spend a lot less
effort
> doing manual maintenance of the selection count.
> 
> Can you put a TODO in for this?

Done.

https://codereview.chromium.org/2207323002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_list_behavior.js:79:
removeItemsByPath: function(paths) {
On 2016/08/05 01:39:09, tsergeant wrote:
> I think I've made my peace with this tree craziness. A few things I'd like to
> see, though:
> 
> a) Expand the method comment here to explain a little bit of what is happening
> and why this is done. I think it's pretty hard to determine at a glance.

Done.

> 
> b) A comparison of speed for deleting items with this vs plain `this.splice`
on
> each path (on the grouped page, I guess). It would be nice to know how much
this
> saves.
> 

For 2400 items on the normal history list, this.splice takes ~7000ms while
notifySplices takes a balmy ~40ms.

> c) It would be really nice to have a direct unit test for this.
> `removeItemsByPath` is nicely decoupled from the rest of the Polymer stuff,
but
> `removeItemsBeneatNode_` makes things more difficult. I won't block on this.

I'll factor out the tree creation into its own method for testing. The actual
removal is 'straightforward' and more or less unit tested by the integration
test.

https://codereview.chromium.org/2207323002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_list_behavior.js:142: * @private
On 2016/08/05 01:39:09, tsergeant wrote:
> Let's make this method and the next public.
> 
> I think the style in Chromium so far is that any methods in a behavior which
are
> accessed by an implementer of that behavior should be public.

Done.

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

https://codereview.chromium.org/2207323002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/list_container.js:88:
this.$.content.selectedItem.unselectAllItems(count);
On 2016/08/05 01:39:09, tsergeant wrote:
> Nit: Maybe extract this.$.content.selectedItem into a getter which returns a
> HistoryListBehavior.
> 
> The idea being that then we are forced by closure in this file to not know
which
> type of list we are dealing with.

So this is exactly why I was asking how behaviors worked with Closure yesterday.
I can't seem to get a Type for HistoryListBehavior =(

https://codereview.chromium.org/2207323002/diff/1/chrome/test/data/webui/md_h...
File chrome/test/data/webui/md_history/history_grouped_list_test.js (right):

https://codereview.chromium.org/2207323002/diff/1/chrome/test/data/webui/md_h...
chrome/test/data/webui/md_history/history_grouped_list_test.js:200:
assertEquals('https://en.wikipedia.org/d', items[1].item.title);
On 2016/08/05 01:39:09, tsergeant wrote:
> You should assert that there are no empty domains lying around.

Done.

https://codereview.chromium.org/2207323002/diff/1/chrome/test/data/webui/md_h...
File chrome/test/data/webui/md_history/test_util.js (right):

https://codereview.chromium.org/2207323002/diff/1/chrome/test/data/webui/md_h...
chrome/test/data/webui/md_history/test_util.js:84: function
polymerQuerySelectorAll(element, selector) {
On 2016/08/05 01:39:09, tsergeant wrote:
> If you want, you could make your/our lives easier by naming this
> polymerSelectAll or something a bit shorter.

Done.

https://codereview.chromium.org/2207323002/diff/1/ui/webui/resources/js/util.js
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/2207323002/diff/1/ui/webui/resources/js/util....
ui/webui/resources/js/util.js:381: * @param {function(Event): boolean} predicate
On 2016/08/05 01:39:09, tsergeant wrote:
> @return {Promise}

Done.

https://codereview.chromium.org/2207323002/diff/1/ui/webui/resources/js/util....
ui/webui/resources/js/util.js:383: function waitForEvent(element, eventName,
predicate) {
On 2016/08/05 01:39:09, tsergeant wrote:
> I can conceive of situations where this would be useful in real code... but
> considering at the moment this is only used in a test, I don't think there's
any
> need to ship this out with every WebUI.
> 
> I think we should just put this in test_util for now.

Done.

[tsergeant, 2016-08-09 04:37:54]

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

https://codereview.chromium.org/2207323002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/list_container.js:88:
this.$.content.selectedItem.unselectAllItems(count);
On 2016/08/09 02:56:00, calamity wrote:
> On 2016/08/05 01:39:09, tsergeant wrote:
> > Nit: Maybe extract this.$.content.selectedItem into a getter which returns a
> > HistoryListBehavior.
> > 
> > The idea being that then we are forced by closure in this file to not know
> which
> > type of list we are dealing with.
> 
> So this is exactly why I was asking how behaviors worked with Closure
yesterday.
> I can't seem to get a Type for HistoryListBehavior =(

That's unfortunate. I think it would be worthwhile to write a getter if you can
anyway, though.

https://codereview.chromium.org/2207323002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list_behavior.html (right):

https://codereview.chromium.org/2207323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list_behavior.html:1: <script
src="/history_list_behavior.js"></script>
Elsewhere we've been using the style chrome://history/history_list_behavior.js

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

https://codereview.chromium.org/2207323002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_grouped_list_test.js:251:
assertEquals(
Does assertDeepEquals work here to avoid converting to JSON?

[calamity, 2016-08-09 05:28:18]

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

[calamity, 2016-08-09 05:28:30]

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

https://codereview.chromium.org/2207323002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/list_container.js:88:
this.$.content.selectedItem.unselectAllItems(count);
On 2016/08/09 04:37:54, tsergeant wrote:
> On 2016/08/09 02:56:00, calamity wrote:
> > On 2016/08/05 01:39:09, tsergeant wrote:
> > > Nit: Maybe extract this.$.content.selectedItem into a getter which returns
a
> > > HistoryListBehavior.
> > > 
> > > The idea being that then we are forced by closure in this file to not know
> > which
> > > type of list we are dealing with.
> > 
> > So this is exactly why I was asking how behaviors worked with Closure
> yesterday.
> > I can't seem to get a Type for HistoryListBehavior =(
> 
> That's unfortunate. I think it would be worthwhile to write a getter if you
can
> anyway, though.

Done.

https://codereview.chromium.org/2207323002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list_behavior.html (right):

https://codereview.chromium.org/2207323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list_behavior.html:1: <script
src="/history_list_behavior.js"></script>
On 2016/08/09 04:37:54, tsergeant wrote:
> Elsewhere we've been using the style chrome://history/history_list_behavior.js

Done.

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

https://codereview.chromium.org/2207323002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_grouped_list_test.js:251:
assertEquals(
On 2016/08/09 04:37:54, tsergeant wrote:
> Does assertDeepEquals work here to avoid converting to JSON?

It doesn't for some reason.. And given the terribly useless failure output, I'm
inclined to stick with JSON which is at least debuggable >_>

expected { Object (currentPath, leaf, ...) } to deeply equal { Object
(currentPath, leaf, ...) }

Useless.

[commit-bot: I haz the power, 2016-08-09 05:28:40]

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-08-09 06:41:52]

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

[commit-bot: I haz the power, 2016-08-09 06:41:53]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2016-08-09 23:20:00]

lgtm % a few nits

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

https://codereview.chromium.org/2207323002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_grouped_list_test.js:251:
assertEquals(
On 2016/08/09 05:28:30, calamity wrote:
> On 2016/08/09 04:37:54, tsergeant wrote:
> > Does assertDeepEquals work here to avoid converting to JSON?
> 
> It doesn't for some reason.. And given the terribly useless failure output,
I'm
> inclined to stick with JSON which is at least debuggable >_>
> 
> expected { Object (currentPath, leaf, ...) } to deeply equal { Object
> (currentPath, leaf, ...) }
> 
> Useless.

Acknowledged.

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/compiled_resources2.gyp (right):

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/compiled_resources2.gyp:53:
'browser_service',
Nit: I think you can get rid of this

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.html (right):

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.html:4: <link rel="import"
href="chrome://history/browser_service.html">
Nit: I think you can remove this

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/list_container.js (right):

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:141:
this.$.content.selectedItem.deleteSelected();
Nit: Change this one as well.

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:181: getSelectedList_:
function() {
Ultra nit: Move this up or down so that it's not between the two menu item
handlers (since they're a pretty logical grouping).

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:191:
this.$.content.selectedItem.removeItemsByPath(items[0].path);
Nit: And this one

[calamity, 2016-08-10 04:25:38]

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/compiled_resources2.gyp (right):

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/compiled_resources2.gyp:53:
'browser_service',
On 2016/08/09 23:20:00, tsergeant wrote:
> Nit: I think you can get rid of this

Done.

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.html (right):

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.html:4: <link rel="import"
href="chrome://history/browser_service.html">
On 2016/08/09 23:20:00, tsergeant wrote:
> Nit: I think you can remove this

Done.

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/list_container.js (right):

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:141:
this.$.content.selectedItem.deleteSelected();
On 2016/08/09 23:20:00, tsergeant wrote:
> Nit: Change this one as well.

Done.

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:181: getSelectedList_:
function() {
On 2016/08/09 23:20:00, tsergeant wrote:
> Ultra nit: Move this up or down so that it's not between the two menu item
> handlers (since they're a pretty logical grouping).

Done.

https://codereview.chromium.org/2207323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:191:
this.$.content.selectedItem.removeItemsByPath(items[0].path);
On 2016/08/09 23:20:00, tsergeant wrote:
> Nit: And this one

Done.

[calamity, 2016-08-10 04:25:45]

The CQ bit was checked by calamity@chromium.org

[calamity, 2016-08-10 04:25:46]

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/2207323002/#ps80001
(title: "fix nits")

[commit-bot: I haz the power, 2016-08-10 04:26:24]

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-08-10 05:52:24]

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

[commit-bot: I haz the power, 2016-08-10 05:52:25]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tsergeant, 2016-08-11 02:53:03]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2016-08-11 02:53:39]

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-08-11 03:37:00]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-08-11 03:40:38]

Description was changed from

==========
[MD History] Factor out a common HistoryListBehavior.

This CL extracts a HistoryListBehavior that is used by the history-list
and the history-grouped-list. The selection system has been refactored
to accommodate different list structures.

Using this behavior in the grouped history list means that it now
implements selection and deletion.

BUG=589357,589363
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD History] Factor out a common HistoryListBehavior.

This CL extracts a HistoryListBehavior that is used by the history-list
and the history-grouped-list. The selection system has been refactored
to accommodate different list structures.

Using this behavior in the grouped history list means that it now
implements selection and deletion.

BUG=589357,589363
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/1a7fa0b24d413123832e2707adbc0266122a225f
Cr-Commit-Position: refs/heads/master@{#411253}
==========

[commit-bot: I haz the power, 2016-08-11 03:40:40]

Patchset 4 (id:??) landed as
https://crrev.com/1a7fa0b24d413123832e2707adbc0266122a225f
Cr-Commit-Position: refs/heads/master@{#411253}