[MD History] Add history-list-container between app and history lists.

[MD History] Add history-list-container between app and history lists.

This CL adds a history-list-container element that contains the history
list and the grouped history list in order to pull logic out of the app
element and in preparation for implementing routing.

It also splits apart some of the data model where appropriate so that
concerns about the query state and results are separated appropriately.

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

Committed: https://crrev.com/54c31198705504d87135e00e6ff01cb13c853cdc
Cr-Commit-Position: refs/heads/master@{#403732}

[calamity, 2016-06-21 05:47:58]

Description was changed from

==========
[MD History] Add history-list-container between app and history lists.

This CL adds a history-list-container element that contains the history
list and the grouped history list in order to pull logic out of the app
element and in preparation for implementing routing.

BUG=619799
==========

to

==========
[MD History] Add history-list-container between app and history lists.

This CL adds a history-list-container element that contains the history
list and the grouped history list in order to pull logic out of the app
element and in preparation for implementing routing.

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

[calamity, 2016-06-22 06:57:36]

Patchset #1 (id:1) has been deleted

[calamity, 2016-06-22 07:06:18]

Patchset #1 (id:20001) has been deleted

[calamity, 2016-06-22 07:07:43]

Description was changed from

==========
[MD History] Add history-list-container between app and history lists.

This CL adds a history-list-container element that contains the history
list and the grouped history list in order to pull logic out of the app
element and in preparation for implementing routing.

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

to

==========
[MD History] Add history-list-container between app and history lists.

This CL adds a history-list-container element that contains the history
list and the grouped history list in order to pull logic out of the app
element and in preparation for implementing routing.

It also splits apart some of the data model where appropriate so that concerns
about the query state and results are separated appropriately.

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

[calamity, 2016-06-22 07:10:04]

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

[tsergeant, 2016-06-23 06:35:26]

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

https://codereview.chromium.org/2084843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.html:53: <history-list-container
id="history" query-state="[[queryState_]]"
grouped="[[grouped_]]"></history-list-container>
Nit: wrap line

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

https://codereview.chromium.org/2084843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:101: this.set('queryState_.querying',
false);
This line is not necessary, it is set in list-container as well.

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

https://codereview.chromium.org/2084843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.html:15: #main-container,
#main-container doesn't exist here.

https://codereview.chromium.org/2084843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.html:23: <history-list
id="history-list" querying="[[queryState.querying]]"
Here's a yak to shave while you're here:

How about changing the IDs to #infinite-list and #grouped-list? I've never been
a particular fan of element-name-as-ID.

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

https://codereview.chromium.org/2084843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:18: var QueryState;
I find it strange that the typedef for this type is here, but the initialisation
of the only instance is in another element. Maybe move that here?

Also, you should add queryingDisabled to the typedef.

https://codereview.chromium.org/2084843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:117: if
(queryState.queryingDisabled || this.queryResult_.results == null)
Very minor nit: Maybe reorder the two || clauses? I was confused at first after
reading the comment and then seeing "queryState.queryingDisabled" as the first
thing after it, which is entirely different.

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

https://codereview.chromium.org/2084843002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_list_test.js:209:
flush().then(function() {
Is this extra flush necessary? I wouldn't expect there to be DOM work in here.

deleteComplete();
flush().then(function() {
  assert...
});

https://codereview.chromium.org/2084843002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_list_test.js:221: })
Nit: ;

https://codereview.chromium.org/2084843002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_list_test.js:225: return flush();
Wouldn't be surprised if you could get rid of this one as well.

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

Description was changed from

==========
[MD History] Add history-list-container between app and history lists.

This CL adds a history-list-container element that contains the history
list and the grouped history list in order to pull logic out of the app
element and in preparation for implementing routing.

It also splits apart some of the data model where appropriate so that concerns
about the query state and results are separated appropriately.

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

to

==========
[MD History] Add history-list-container between app and history lists.

This CL adds a history-list-container element that contains the history
list and the grouped history list in order to pull logic out of the app
element and in preparation for implementing routing.

It also splits apart some of the data model where appropriate so that
concerns about the query state and results are separated appropriately.

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

[calamity, 2016-06-24 05:39:47]

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

https://codereview.chromium.org/2084843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.html:53: <history-list-container
id="history" query-state="[[queryState_]]"
grouped="[[grouped_]]"></history-list-container>
On 2016/06/23 06:35:25, tsergeant wrote:
> Nit: wrap line

Done.

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

https://codereview.chromium.org/2084843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:101: this.set('queryState_.querying',
false);
On 2016/06/23 06:35:25, tsergeant wrote:
> This line is not necessary, it is set in list-container as well.

Removed it from the other place since this is kind of the element that 'owns'
the query state.

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

https://codereview.chromium.org/2084843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.html:15: #main-container,
On 2016/06/23 06:35:25, tsergeant wrote:
> #main-container doesn't exist here.

Removed.

https://codereview.chromium.org/2084843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.html:23: <history-list
id="history-list" querying="[[queryState.querying]]"
On 2016/06/23 06:35:25, tsergeant wrote:
> Here's a yak to shave while you're here:
> 
> How about changing the IDs to #infinite-list and #grouped-list? I've never
been
> a particular fan of element-name-as-ID.

Shaved.

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

https://codereview.chromium.org/2084843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:18: var QueryState;
On 2016/06/23 06:35:25, tsergeant wrote:
> I find it strange that the typedef for this type is here, but the
initialisation
> of the only instance is in another element. Maybe move that here?
> 
> Also, you should add queryingDisabled to the typedef.

I think.... I want to move this to constants.js or something. It needs to live
somewhere a bit more common.

https://codereview.chromium.org/2084843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:117: if
(queryState.queryingDisabled || this.queryResult_.results == null)
On 2016/06/23 06:35:25, tsergeant wrote:
> Very minor nit: Maybe reorder the two || clauses? I was confused at first
after
> reading the comment and then seeing "queryState.queryingDisabled" as the first
> thing after it, which is entirely different.

Done.

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

https://codereview.chromium.org/2084843002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_list_test.js:209:
flush().then(function() {
On 2016/06/23 06:35:26, tsergeant wrote:
> Is this extra flush necessary? I wouldn't expect there to be DOM work in here.
> 
> deleteComplete();
> flush().then(function() {
>   assert...
> });

This is here to cause get a new event loop before calling deleteComplete().

https://codereview.chromium.org/2084843002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_list_test.js:221: })
On 2016/06/23 06:35:25, tsergeant wrote:
> Nit: ;

Done.

https://codereview.chromium.org/2084843002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_list_test.js:225: return flush();
On 2016/06/23 06:35:26, tsergeant wrote:
> Wouldn't be surprised if you could get rid of this one as well.

Done.

[calamity, 2016-06-24 05:41:30]

Patchset #2 (id:60001) has been deleted

[tsergeant, 2016-06-24 06:19:18]

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

https://codereview.chromium.org/2084843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:19: // TODO(calamity): Split out
readOnly data into a separate property which is
Can remove this todo

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

https://codereview.chromium.org/2084843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:9: var QueryResult;
Should this one move to constants.js as well?

https://codereview.chromium.org/2084843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:48: initializeResults_:
function(info, results) {
Nit: /** @private */

https://codereview.chromium.org/2084843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:82: historyResult:
function(info, results) {
Style guide is for public functions to appear before private
functions/observers/listeners/etc, so we should probably follow that in the new
file.

[calamity, 2016-06-27 03:35:46]

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

https://codereview.chromium.org/2084843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:19: // TODO(calamity): Split out
readOnly data into a separate property which is
On 2016/06/24 06:19:17, tsergeant wrote:
> Can remove this todo

Done.

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

https://codereview.chromium.org/2084843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:9: var QueryResult;
On 2016/06/24 06:19:17, tsergeant wrote:
> Should this one move to constants.js as well?

This will likely stay confined to this element.

https://codereview.chromium.org/2084843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:48: initializeResults_:
function(info, results) {
On 2016/06/24 06:19:17, tsergeant wrote:
> Nit: /** @private */

Done.

https://codereview.chromium.org/2084843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:82: historyResult:
function(info, results) {
On 2016/06/24 06:19:17, tsergeant wrote:
> Style guide is for public functions to appear before private
> functions/observers/listeners/etc, so we should probably follow that in the
new
> file.

Done.

[tsergeant, 2016-06-27 03:58:15]

lgtm % nit

https://codereview.chromium.org/2084843002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_history/list_container.js (right):

https://codereview.chromium.org/2084843002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/list_container.js:49: initializeResults_:
function(info, results) {
Nit: This private function got left behind when the rest of them moved down.

Also, add @params.

[calamity, 2016-06-29 07:52:49]

https://codereview.chromium.org/2084843002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_history/list_container.js (right):

https://codereview.chromium.org/2084843002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/list_container.js:49: initializeResults_:
function(info, results) {
On 2016/06/27 03:58:15, tsergeant wrote:
> Nit: This private function got left behind when the rest of them moved down.
> 
> Also, add @params.

Done.

[calamity, 2016-07-05 01:47:41]

The CQ bit was checked by calamity@chromium.org

[calamity, 2016-07-05 01:47:41]

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/2084843002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-05 01:47:55]

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-05 02:26:19]

Description was changed from

==========
[MD History] Add history-list-container between app and history lists.

This CL adds a history-list-container element that contains the history
list and the grouped history list in order to pull logic out of the app
element and in preparation for implementing routing.

It also splits apart some of the data model where appropriate so that
concerns about the query state and results are separated appropriately.

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

to

==========
[MD History] Add history-list-container between app and history lists.

This CL adds a history-list-container element that contains the history
list and the grouped history list in order to pull logic out of the app
element and in preparation for implementing routing.

It also splits apart some of the data model where appropriate so that
concerns about the query state and results are separated appropriately.

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

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

Committed patchset #5 (id:140001)

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

CQ bit was unchecked.

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

Description was changed from

==========
[MD History] Add history-list-container between app and history lists.

This CL adds a history-list-container element that contains the history
list and the grouped history list in order to pull logic out of the app
element and in preparation for implementing routing.

It also splits apart some of the data model where appropriate so that
concerns about the query state and results are separated appropriately.

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

to

==========
[MD History] Add history-list-container between app and history lists.

This CL adds a history-list-container element that contains the history
list and the grouped history list in order to pull logic out of the app
element and in preparation for implementing routing.

It also splits apart some of the data model where appropriate so that
concerns about the query state and results are separated appropriately.

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

Committed: https://crrev.com/54c31198705504d87135e00e6ff01cb13c853cdc
Cr-Commit-Position: refs/heads/master@{#403732}
==========

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

Patchset 5 (id:??) landed as
https://crrev.com/54c31198705504d87135e00e6ff01cb13c853cdc
Cr-Commit-Position: refs/heads/master@{#403732}