[MD History] Implement grouped history UI.

[MD History] Implement grouped history UI.

This CL implements a history view that groups history by domain which
can be enabled by enabling the 'Group history by domain' flag.

A history-grouped-list element has been added which switches out with
the history-list if grouped mode is active.

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

Committed: https://crrev.com/613e58421d526c12935096fe282ce4b278857d85
Cr-Commit-Position: refs/heads/master@{#396069}

[calamity, 2016-05-18 07:12:51]

Description was changed from

==========
[MD History] Implement grouped history UI.

This CL implements a history view that groups history by domain which
can be enabled by enabling the 'Group history by domain' flag.

A history-grouped-list element has been added which switches out with
the history-list if grouped mode is active.

BUG=589357
==========

to

==========
[MD History] Implement grouped history UI.

This CL implements a history view that groups history by domain which
can be enabled by enabling the 'Group history by domain' flag.

A history-grouped-list element has been added which switches out with
the history-list if grouped mode is active.

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

[calamity, 2016-05-18 07:36:37]

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

[tsergeant, 2016-05-19 05:38:18]

Two high-level comments:

1) Have you considered pulling out a behavior from history-list? Seems like
there's potential to share a small amount of code this way, with more to come
once selection, deletion, the overflow menu,... are implemented.

2) It looks like at the moment search doesn't change the grouped layout at all.
In the old history page it searches that particular period of time using the
normal layout. Just something to keep in mind, not necessary in this CL.

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

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/app.html:59: <template is="dom-if"
if="[[grouped_]]">
🙌 doing it this way takes advantage of lazy registration 🙌

(maybe we should do this for synced-device-manager too?)

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

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/app.js:179: lastVisitTime, maxResults
It looks like the old history page sets maxResults to 0 when doing a grouped
history query. Any idea whether this is necessary to get the full week or month
worth of results?

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

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/constants.js:14: 
Nit: double blank line here.

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

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.html:1: <link rel="import"
href="chrome://resources/html/polymer.html">
Include-what-you-use iron-collapse and iron-icon.

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.html:69:
hidden$="{{hasResults(groupedHistoryData_.length)}}">
Nit: [[]] instead of {{}} where appropriate

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.html:78: <div>
Is this inner div necessary?

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.html:97: <div id="item-list">
Is -this- div necessary? It's not mentioned in the CSS.

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

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.js:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
Nit: It's 2016.

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.js:8: *            expanded:
boolean}}
Nit: Add rendered to this typedef, since it's added to every HistoryDomain
below?

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.js:160: return expanded ?
'expand-less' : 'expand-more';
Should be using `cr:expand-less` and `cr:expand-more`

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

https://codereview.chromium.org/1993613002/diff/1/chrome/test/data/webui/md_h...
chrome/test/data/webui/md_history/md_history_browsertest.js:32:
'history_grouped_list_test.js',
Looks like you forgot to git add this file.

[calamity, 2016-05-20 05:04:42]

Patchset #2 (id:20001) has been deleted

[calamity, 2016-05-20 06:51:58]

1.) I'm not sure what are good candidates for pulling out. I think selection,
deletion and the menu will all end up being handled by history-app at some
point. We _could_ duplicate the global menu between the 2 views, but we could
just as easily not do that. history-items will end up sending events up to
history-app which then does the things it should do. How does that sound?

2.) Hmm. Try it again. It does a weird grouped search now. Maybe this will do
for now?

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

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/app.html:59: <template is="dom-if"
if="[[grouped_]]">
On 2016/05/19 05:38:17, tsergeant wrote:
> 🙌 doing it this way takes advantage of lazy registration 🙌
> 
> (maybe we should do this for synced-device-manager too?)

Hmm. Based on tabSyncEnabled? I guess that could work? I'll make a follow-up.

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

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/app.js:179: lastVisitTime, maxResults
On 2016/05/19 05:38:17, tsergeant wrote:
> It looks like the old history page sets maxResults to 0 when doing a grouped
> history query. Any idea whether this is necessary to get the full week or
month
> worth of results?

That's done here for cargo-culty reasons. It does indeed cause the
HistoryService to give back all the results for the month. However... This makes
the web service do a default query which is like, max of 30. I think we should
make this a larger number when in grouped mode actually.

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

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/constants.js:14: 
On 2016/05/19 05:38:18, tsergeant wrote:
> Nit: double blank line here.

Done.

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

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.html:1: <link rel="import"
href="chrome://resources/html/polymer.html">
On 2016/05/19 05:38:18, tsergeant wrote:
> Include-what-you-use iron-collapse and iron-icon.

Done.

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.html:69:
hidden$="{{hasResults(groupedHistoryData_.length)}}">
On 2016/05/19 05:38:18, tsergeant wrote:
> Nit: [[]] instead of {{}} where appropriate 

Done.

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.html:78: <div>
On 2016/05/19 05:38:18, tsergeant wrote:
> Is this inner div necessary?

Removed.

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.html:97: <div id="item-list">
On 2016/05/19 05:38:18, tsergeant wrote:
> Is -this- div necessary? It's not mentioned in the CSS.

Oops. I was using this to play with collapse + dom repeat.

Removed.

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

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.js:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2016/05/19 05:38:18, tsergeant wrote:
> Nit: It's 2016.

Done.

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.js:8: *            expanded:
boolean}}
On 2016/05/19 05:38:18, tsergeant wrote:
> Nit: Add rendered to this typedef, since it's added to every HistoryDomain
> below?

Done.

https://codereview.chromium.org/1993613002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/grouped_list.js:160: return expanded ?
'expand-less' : 'expand-more';
On 2016/05/19 05:38:18, tsergeant wrote:
> Should be using `cr:expand-less` and `cr:expand-more`

Done.

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

https://codereview.chromium.org/1993613002/diff/1/chrome/test/data/webui/md_h...
chrome/test/data/webui/md_history/md_history_browsertest.js:32:
'history_grouped_list_test.js',
On 2016/05/19 05:38:18, tsergeant wrote:
> Looks like you forgot to git add this file.

Classic.

[tsergeant, 2016-05-23 01:21:11]

1) For the moment, noResultsMessage_ and hasResults are the only things that are
natural to share, so let's defer it for now.

More widely, though, I'm not convinced that moving selection and deletion up
into history-app makes sense. history-app is there to make coordination between
multiple elements easier. Having it implement some of the major features of two
of its n child elements is pretty far from its main role, and tightly couples
history-list and history-grouped-list to only be usable within the context of an
entire app.

Having a behavior for these seems more natural to me. It shows that the elements
are collections of items, and we have a standard set of operations in the
behavior you can perform on such a collection.

Anyway, let's chat about this offline.

2) This will do for now, but let's either confirm that this behavior is
acceptable, or make it match the old behavior.

Another point,

3) When I switch between Week and Month, I see a flash of the old data presented
in the new form before the new data loads in. This is really janky -- we should
delete (or hide) the old data when we switch tabs. (Happy for you to TODO this
if there's no simple solution for this patch).

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

https://codereview.chromium.org/1993613002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_grouped_list_test.js:53:
console.log(app.grouped_ + " " + groupedList);
🌲 remove a console.log, save a tree 🌲

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

https://codereview.chromium.org/1993613002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/grouped_list.html:23: margin:
var(--first-card-padding-top) var(--card-padding-side);
Why did this change to margin instead of padding? It causes the cards to shrink
incorrectly.

[calamity, 2016-05-24 07:03:19]

Patchset #4 (id:80001) has been deleted

[calamity, 2016-05-24 07:08:16]

1) Agreed. Said behavior is on my radar.

2) Ack.

3) I have a good idea about how to fix the jank, I need to split the query stuff
into a 'data to send' and a 'data received'. Then all UI should update based on
bindings to 'data received' which will eliminate the jank.

Added a TODO.

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

https://codereview.chromium.org/1993613002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_grouped_list_test.js:53:
console.log(app.grouped_ + " " + groupedList);
On 2016/05/23 01:21:11, tsergeant wrote:
> 🌲 remove a console.log, save a tree 🌲

Acknowledged.

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

https://codereview.chromium.org/1993613002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/grouped_list.html:23: margin:
var(--first-card-padding-top) var(--card-padding-side);
On 2016/05/23 01:21:11, tsergeant wrote:
> Why did this change to margin instead of padding? It causes the cards to
shrink
> incorrectly.

Oops, I think I was playing with something. The sizes were a bit broken last
patch (The max width didn't match with the other views for reasons). I've fixed
it all now.

[tsergeant, 2016-05-25 05:06:56]

https://codereview.chromium.org/1993613002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_history/grouped_list.js (right):

https://codereview.chromium.org/1993613002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_history/grouped_list.js:159: hasResults:
function(historyDataLength) {
Nit: The next 3 methods can be private

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

https://codereview.chromium.org/1993613002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_grouped_list_test.js:16:
app.queryingDisabled_ = true;
Is it worth making this a test-wide thing in md_history_browsertest.js? Seems
like most tests want this disabled, and those that don't can re-enable it
selectively.

https://codereview.chromium.org/1993613002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_grouped_list_test.js:51:
assertTrue(!!toolbar.$$('#grouped-buttons-container'));
Nit: Deindent

https://codereview.chromium.org/1993613002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/test_util.js (right):

https://codereview.chromium.org/1993613002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/md_history/test_util.js:67: finished: true,
Nit: normal formatting is probably

return {
  finished: true
  ...
};

[calamity, 2016-05-25 05:51:41]

https://codereview.chromium.org/1993613002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_history/grouped_list.js (right):

https://codereview.chromium.org/1993613002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_history/grouped_list.js:159: hasResults:
function(historyDataLength) {
On 2016/05/25 05:06:55, tsergeant wrote:
> Nit: The next 3 methods can be private

Done.

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

https://codereview.chromium.org/1993613002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_grouped_list_test.js:16:
app.queryingDisabled_ = true;
On 2016/05/25 05:06:56, tsergeant wrote:
> Is it worth making this a test-wide thing in md_history_browsertest.js? Seems
> like most tests want this disabled, and those that don't can re-enable it
> selectively.

Done.

https://codereview.chromium.org/1993613002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_grouped_list_test.js:51:
assertTrue(!!toolbar.$$('#grouped-buttons-container'));
On 2016/05/25 05:06:56, tsergeant wrote:
> Nit: Deindent

Done.

https://codereview.chromium.org/1993613002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/test_util.js (right):

https://codereview.chromium.org/1993613002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/md_history/test_util.js:67: finished: true,
On 2016/05/25 05:06:56, tsergeant wrote:
> Nit: normal formatting is probably
> 
> return {
>   finished: true
>   ...
> };

Done.

[tsergeant, 2016-05-25 06:56:17]

lgtm

[calamity, 2016-05-26 00:54:34]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2016-05-26 00:55:08]

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

[commit-bot: I haz the power, 2016-05-26 01:10:35]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-05-26 01:12:50]

Description was changed from

==========
[MD History] Implement grouped history UI.

This CL implements a history view that groups history by domain which
can be enabled by enabling the 'Group history by domain' flag.

A history-grouped-list element has been added which switches out with
the history-list if grouped mode is active.

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

to

==========
[MD History] Implement grouped history UI.

This CL implements a history view that groups history by domain which
can be enabled by enabling the 'Group history by domain' flag.

A history-grouped-list element has been added which switches out with
the history-list if grouped mode is active.

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

Committed: https://crrev.com/613e58421d526c12935096fe282ce4b278857d85
Cr-Commit-Position: refs/heads/master@{#396069}
==========

[commit-bot: I haz the power, 2016-05-26 01:12:52]

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