MD History: Drop-down menu allows searching for history-items by domain name.

MD History: Drop-down menu allows searching for history-items by domain name.

Search results are returned based on the domain name associated with the open
drop-down menu. The search bar is also altered to reflect the search term.

BUG=425625

[hsampson, 2016-01-29 04:16:23]

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

[calamity, 2016-02-03 05:09:13]

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

https://codereview.chromium.org/1648803003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_card_manager.js:30: value: []
Shouldn't this be an object? Also, use value: function() { return {}; }.

https://codereview.chromium.org/1648803003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_card_manager.js:73:
this.fire('refresh-results', {search: domain});
I think it's better just to put the query call into the more-from-site handler
instead of firing this second event.

https://codereview.chromium.org/1648803003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_card_manager.js:96:
this.menuIdentifier == e.detail.itemIdentifier) {
Hmmmmmmmm. I believe this equality is never true since menuIdentifier is always
created with a new item. You would need to implement your own
menuIdentifiersEqual() function to compare 2 menu identifiers.

I checked and if you click on the thin sliver of menu that is not obscured by
the menu, it doesn't close.

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

https://codereview.chromium.org/1648803003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_toolbar.js:47: Polymer.dom.flush();
Do we need this flush?

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

https://codereview.chromium.org/1648803003/diff/1/chrome/test/data/webui/md_h...
chrome/test/data/webui/md_history/history_overflow_menu_test.js:15:
itemIdentifier: 2
Hmmmmmmm. These don't accurately reflect the real world anymore. I think we
should implement that makeTestHistoryEntry function in test_util.js and then
this test can just make some fake results, click the button for some real
history items and we can check that the right things happen.

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

https://codereview.chromium.org/1648803003/diff/1/chrome/test/data/webui/md_h...
chrome/test/data/webui/md_history/history_toolbar_test.js:101:
element.menuIdentifier = [];
Use {}.

[hsampson, 2016-02-08 00:39:37]

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

https://codereview.chromium.org/1648803003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_card_manager.js:30: value: []
On 2016/02/03 05:09:13, calamity wrote:
> Shouldn't this be an object? Also, use value: function() { return {}; }.

Done.

https://codereview.chromium.org/1648803003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_card_manager.js:73:
this.fire('refresh-results', {search: domain});
On 2016/02/03 05:09:13, calamity wrote:
> I think it's better just to put the query call into the more-from-site handler
> instead of firing this second event.

Done.

https://codereview.chromium.org/1648803003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_card_manager.js:96:
this.menuIdentifier == e.detail.itemIdentifier) {
On 2016/02/03 05:09:13, calamity wrote:
> Hmmmmmmmm. I believe this equality is never true since menuIdentifier is
always
> created with a new item. You would need to implement your own
> menuIdentifiersEqual() function to compare 2 menu identifiers.
> 
> I checked and if you click on the thin sliver of menu that is not obscured by
> the menu, it doesn't close.

Done.

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

https://codereview.chromium.org/1648803003/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_history/history_toolbar.js:47: Polymer.dom.flush();
On 2016/02/03 05:09:13, calamity wrote:
> Do we need this flush?

Yes otherwise we can't access the search input section of the search bar.

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

https://codereview.chromium.org/1648803003/diff/1/chrome/test/data/webui/md_h...
chrome/test/data/webui/md_history/history_overflow_menu_test.js:15:
itemIdentifier: 2
On 2016/02/03 05:09:13, calamity wrote:
> Hmmmmmmm. These don't accurately reflect the real world anymore. I think we
> should implement that makeTestHistoryEntry function in test_util.js and then
> this test can just make some fake results, click the button for some real
> history items and we can check that the right things happen.

Done.

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

https://codereview.chromium.org/1648803003/diff/1/chrome/test/data/webui/md_h...
chrome/test/data/webui/md_history/history_toolbar_test.js:101:
element.menuIdentifier = [];
On 2016/02/03 05:09:13, calamity wrote:
> Use {}.

Done.

[tsergeant, 2016-02-12 00:48:56]

(Make sure this one is rebased against the parent patch as well)

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

https://codereview.chromium.org/1648803003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:103:
console.log((menu.url == item.url) && (menu.timestamps == item.timestamps));
Remove this log

https://codereview.chromium.org/1648803003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_card_manager.js:104: return
((menu.url == item.url) && (menu.timestamps == item.timestamps));
You can get rid of the outer parens

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

https://codereview.chromium.org/1648803003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.js:44: var searchBar =
this.$$('#search-input');
Nit: Can use this.$['search-input'] now

https://codereview.chromium.org/1648803003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.js:47: Polymer.dom.flush();
can you use this.async here instead of Polymer.dom.flush?

(I tried it locally and it looks like it will work)

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

https://codereview.chromium.org/1648803003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/test_util.js:20: function
createHistoryEntry(timestamp, url) {
\ (•◡•) /

This is much nicer, thanks for refactoring everything!