MD History: Hook all elements into the page and add tests.

MD History: Hook all elements into the page and add tests.

This is the last of two patches for the start of MD History.
This patch adds all history elements to the page and implements
the history-toolbar functionality. This patch also includes
tests for all elements.

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

[hsampson, 2016-01-13 01:51:11]

Description was changed from

==========
Second material history patch.

Adding files for history-toolbar which include formatting and button
functionality. Add file for history.js and implement functionality, i.e.
adding new results and cancelling history-item selection. This cl also
includes tests for history-card-manager, history-card, history-overflow-menu
and history-toolbar.

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

to

==========
MD History: Hook all elements into the page and add tests.

This is the last of two patches for the start of MD History.
This patch adds all history elements to the page and implements
the history-toolbar functionality. This patch also includes
all tests.

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

[hsampson, 2016-01-13 01:52:04]

Description was changed from

==========
MD History: Hook all elements into the page and add tests.

This is the last of two patches for the start of MD History.
This patch adds all history elements to the page and implements
the history-toolbar functionality. This patch also includes
all tests.

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

to

==========
MD History: Hook all elements into the page and add tests.

This is the last of two patches for the start of MD History.
This patch adds all history elements to the page and implements
the history-toolbar functionality. This patch also includes
tests for all elements.

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

[hsampson, 2016-01-13 01:54:56]

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

[hsampson, 2016-01-13 01:54:56]

Please take a look.

[tsergeant, 2016-01-19 03:43:47]

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

https://codereview.chromium.org/1572383006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_history_ui.cc:33:
source->AddLocalizedString("moreFromSite", IDS_HISTORY_MORE_FROM_SITE);
Can you sort these, please?

[hsampson, 2016-01-19 03:56:50]

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

https://codereview.chromium.org/1572383006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_history_ui.cc:33:
source->AddLocalizedString("moreFromSite", IDS_HISTORY_MORE_FROM_SITE);
On 2016/01/19 03:43:46, tsergeant wrote:
> Can you sort these, please?

Done, thank you.

[tsergeant, 2016-01-22 02:37:11]

A once over:

https://codereview.chromium.org/1572383006/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1572383006/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5963: +          Enable the experimental
implementation of the "Cache-Control: stale-while-revalidate" directive. This
permits servers to specify that some resources may be revalidated in the
background to improve latency.
I guess this is Sublime changing this automatically? Probably should undo it.

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

https://codereview.chromium.org/1572383006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history.html:43: <link rel="import"
href="chrome://resources/polymer/v1_0/polymer/polymer.html">
There shouldn't be any need to include Polymer here.

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

https://codereview.chromium.org/1572383006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.js:24: * This function
changes the toolbar background color depending on whether or
Remove "this function" (it's implied):

Change the toolbar background color depending on whether any history
items are currently selected.

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

https://codereview.chromium.org/1572383006/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_card_manager_test.js:54:
element.addNewResults(TEST_HISTORY_RESULTS);
element.addNewResults() appears at the start of all of these tests. Can we put
it into a setup() function instead?

https://codereview.chromium.org/1572383006/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_card_manager_test.js:74:
flush(function() {
Why does this test get flushed when others don't?

https://codereview.chromium.org/1572383006/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_overflow_menu_test.js (right):

https://codereview.chromium.org/1572383006/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_overflow_menu_test.js:55: // menu.
Can we reword the comment to include a todo? (I think Yingran was the author of
this one in the first place):

// Test that pressing escape on the document closes the menu. In the actual
// page, it will take two presses to close the menu due to focus.
// TODO(yingran): Fix this behavior to only require one key press

https://codereview.chromium.org/1572383006/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_toolbar_test.js (right):

https://codereview.chromium.org/1572383006/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:40: // Ensure that
when an item is deselected that the count held by the
Nit: probably remove the extra 'that'

// Ensure that when an item is deselected the count held by the

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

https://codereview.chromium.org/1572383006/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/md_history_browsertest.js:24: 'test_util.js',
Probs should alphabetize these filenames.

[hsampson, 2016-01-22 05:05:03]

https://codereview.chromium.org/1572383006/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1572383006/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5963: +          Enable the experimental
implementation of the "Cache-Control: stale-while-revalidate" directive. This
permits servers to specify that some resources may be revalidated in the
background to improve latency.
On 2016/01/22 02:37:11, tsergeant wrote:
> I guess this is Sublime changing this automatically? Probably should undo it.

Done.

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

https://codereview.chromium.org/1572383006/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/history.html:43: <link rel="import"
href="chrome://resources/polymer/v1_0/polymer/polymer.html">
On 2016/01/22 02:37:11, tsergeant wrote:
> There shouldn't be any need to include Polymer here.

Done.

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

https://codereview.chromium.org/1572383006/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_card_manager_test.js:54:
element.addNewResults(TEST_HISTORY_RESULTS);
On 2016/01/22 02:37:11, tsergeant wrote:
> element.addNewResults() appears at the start of all of these tests. Can we put
> it into a setup() function instead?

Done.

https://codereview.chromium.org/1572383006/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_card_manager_test.js:74:
flush(function() {
On 2016/01/22 02:37:11, tsergeant wrote:
> Why does this test get flushed when others don't?

Because this test relies on the history-items being created visually in order to
be able to click them. The other tests are just looking at historyDataByDay_.

https://codereview.chromium.org/1572383006/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_overflow_menu_test.js (right):

https://codereview.chromium.org/1572383006/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_overflow_menu_test.js:55: // menu.
On 2016/01/22 02:37:11, tsergeant wrote:
> Can we reword the comment to include a todo? (I think Yingran was the author
of
> this one in the first place):
> 
> // Test that pressing escape on the document closes the menu. In the actual
> // page, it will take two presses to close the menu due to focus.
> // TODO(yingran): Fix this behavior to only require one key press

Done.

https://codereview.chromium.org/1572383006/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_toolbar_test.js (right):

https://codereview.chromium.org/1572383006/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_toolbar_test.js:40: // Ensure that
when an item is deselected that the count held by the
On 2016/01/22 02:37:11, tsergeant wrote:
> Nit: probably remove the extra 'that'
> 
> // Ensure that when an item is deselected the count held by the

Done.

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

https://codereview.chromium.org/1572383006/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/md_history_browsertest.js:24: 'test_util.js',
On 2016/01/22 02:37:11, tsergeant wrote:
> Probs should alphabetize these filenames.

Done.

[tsergeant, 2016-01-25 00:40:20]

Some more little things

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

https://codereview.chromium.org/1572383006/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_toolbar.html:61:
on-tap="clearSelection">
clearSelection -> onClearSelectionTap_

(onThingAction_ is the event naming convention for webui_)

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

https://codereview.chromium.org/1572383006/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_card_manager_test.js:79:
MockInteractions.tap(items[0].$['checkbox']);
Nit: items[0].$.checkbox (here and below)

https://codereview.chromium.org/1572383006/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_toolbar_test.js (right):

https://codereview.chromium.org/1572383006/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_toolbar_test.js:29:
MockInteractions.tap(item.$['checkbox']);
item.$.checkbox here and below again

[hsampson, 2016-01-25 02:14:01]

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

https://codereview.chromium.org/1572383006/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/history_toolbar.html:61:
on-tap="clearSelection">
On 2016/01/25 00:40:20, tsergeant wrote:
> clearSelection -> onClearSelectionTap_
> 
> (onThingAction_ is the event naming convention for webui_)

Done.

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

https://codereview.chromium.org/1572383006/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_card_manager_test.js:79:
MockInteractions.tap(items[0].$['checkbox']);
On 2016/01/25 00:40:20, tsergeant wrote:
> Nit: items[0].$.checkbox (here and below)

Done.

https://codereview.chromium.org/1572383006/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/md_history/history_toolbar_test.js (right):

https://codereview.chromium.org/1572383006/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_toolbar_test.js:29:
MockInteractions.tap(item.$['checkbox']);
On 2016/01/25 00:40:20, tsergeant wrote:
> item.$.checkbox here and below again

Done.

[tsergeant, 2016-01-26 23:04:06]

lgtm with one more nit

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

https://codereview.chromium.org/1572383006/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/history.html:18: background: #f2f2f2;
Nit: Sort these


(hmmm, these sorts of things should be automatically picked up by the presubmit
test. I wonder why it didn't work here)

[Dan Beam, 2016-01-26 23:06:43]

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

https://codereview.chromium.org/1572383006/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/history.html:22: flex-direction:column;
also, space after flex-direction:

[hsampson, 2016-01-28 05:44:49]

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

https://codereview.chromium.org/1572383006/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/history.html:18: background: #f2f2f2;
On 2016/01/26 23:04:06, tsergeant wrote:
> Nit: Sort these
> 
> 
> (hmmm, these sorts of things should be automatically picked up by the
presubmit
> test. I wonder why it didn't work here)

Done.

https://codereview.chromium.org/1572383006/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_history/history.html:22: flex-direction:column;
On 2016/01/26 23:06:43, Dan Beam wrote:
> also, space after flex-direction:

Done.

[tsergeant, 2016-01-28 22:30:53]

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

https://codereview.chromium.org/1572383006/diff/160001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_card_manager_test.js:92:
toolbar.clearSelection();
Hasn't this method been renamed? 

(I'm confused. Do you know why this test still passes?)

[hsampson, 2016-01-28 22:38:59]

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

https://codereview.chromium.org/1572383006/diff/160001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_card_manager_test.js:92:
toolbar.clearSelection();
On 2016/01/28 22:30:52, tsergeant wrote:
> Hasn't this method been renamed? 
> 
> (I'm confused. Do you know why this test still passes?)

Yes it has. Any of the tests with a flush() function in them need to have a
'done' otherwise they will pass without actually testing anything. Thanks for
picking this up.

[tsergeant, 2016-01-28 22:45:23]

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

https://codereview.chromium.org/1572383006/diff/160001/chrome/test/data/webui...
chrome/test/data/webui/md_history/history_card_manager_test.js:92:
toolbar.clearSelection();
On 2016/01/28 22:38:59, hsampson wrote:
> On 2016/01/28 22:30:52, tsergeant wrote:
> > Hasn't this method been renamed? 
> > 
> > (I'm confused. Do you know why this test still passes?)
> 
> Yes it has. Any of the tests with a flush() function in them need to have a
> 'done' otherwise they will pass without actually testing anything. Thanks for
> picking this up.

=P I only saw this after you changed it yourself in the follow-up search patch.

[hsampson, 2016-01-29 00:56:02]

Hi Dan, can you please review chrome/browser/browser_resources.grd.

Thanks

[Dan Beam, 2016-01-29 01:50:27]

lgtm

[tsergeant, 2016-01-29 04:52:41]

still lgtm

[tsergeant, 2016-01-31 22:58:25]

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

[commit-bot: I haz the power, 2016-01-31 22:58:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1572383006/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1572383006/220001

[commit-bot: I haz the power, 2016-01-31 23:07:59]

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

[commit-bot: I haz the power, 2016-01-31 23:08:00]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hsampson, 2016-02-01 06:19:41]

The CQ bit was checked by hsampson@google.com

[hsampson, 2016-02-01 06:19:41]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
tsergeant@chromium.org
Link to the patchset: https://codereview.chromium.org/1572383006/#ps260001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-02-01 06:20:06]

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

[commit-bot: I haz the power, 2016-02-01 07:35:20]

Description was changed from

==========
MD History: Hook all elements into the page and add tests.

This is the last of two patches for the start of MD History.
This patch adds all history elements to the page and implements
the history-toolbar functionality. This patch also includes
tests for all elements.

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

to

==========
MD History: Hook all elements into the page and add tests.

This is the last of two patches for the start of MD History.
This patch adds all history elements to the page and implements
the history-toolbar functionality. This patch also includes
tests for all elements.

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

[commit-bot: I haz the power, 2016-02-01 07:35:20]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-02-01 07:36:27]

Description was changed from

==========
MD History: Hook all elements into the page and add tests.

This is the last of two patches for the start of MD History.
This patch adds all history elements to the page and implements
the history-toolbar functionality. This patch also includes
tests for all elements.

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

to

==========
MD History: Hook all elements into the page and add tests.

This is the last of two patches for the start of MD History.
This patch adds all history elements to the page and implements
the history-toolbar functionality. This patch also includes
tests for all elements.

BUG=425625

Committed: https://crrev.com/23b2e9598614f22d4e9e35fff78ecd3b85e3f9d2
Cr-Commit-Position: refs/heads/master@{#372618}
==========

[commit-bot: I haz the power, 2016-02-01 07:36:28]

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