Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(525)

Issue 1648803003: MD History: Drop-down menu allows searching for history-items by domain name. (Closed)

Created:
4 years, 10 months ago by hsampson
Modified:
4 years, 9 months ago
Reviewers:
tsergeant, calamity
CC:
chromium-reviews, arv+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@search_functionality
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 12

Patch Set 2 : Address reviewer comments: add function to create test results. #

Patch Set 3 : Address reviewer comments and small fixes. #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+142 lines, -91 lines) Patch
M chrome/browser/resources/md_history/history.js View 1 1 chunk +4 lines, -1 line 0 comments Download
M chrome/browser/resources/md_history/history_card_manager.js View 1 2 4 chunks +20 lines, -4 lines 2 comments Download
M chrome/browser/resources/md_history/history_item.js View 1 1 chunk +5 lines, -1 line 0 comments Download
M chrome/browser/resources/md_history/history_toolbar.js View 1 1 chunk +13 lines, -0 lines 2 comments Download
M chrome/test/data/webui/md_history/history_card_manager_test.js View 1 3 chunks +26 lines, -45 lines 0 comments Download
M chrome/test/data/webui/md_history/history_card_test.js View 1 1 chunk +8 lines, -5 lines 0 comments Download
M chrome/test/data/webui/md_history/history_overflow_menu_test.js View 1 1 chunk +25 lines, -25 lines 0 comments Download
M chrome/test/data/webui/md_history/history_toolbar_test.js View 1 2 3 chunks +18 lines, -10 lines 0 comments Download
M chrome/test/data/webui/md_history/test_util.js View 1 1 chunk +23 lines, -0 lines 1 comment Download

Depends on Patchset:

Messages

Total messages: 5 (1 generated)
hsampson
4 years, 10 months ago (2016-01-29 04:16:24 UTC) #2
calamity
https://codereview.chromium.org/1648803003/diff/1/chrome/browser/resources/md_history/history_card_manager.js File chrome/browser/resources/md_history/history_card_manager.js (right): https://codereview.chromium.org/1648803003/diff/1/chrome/browser/resources/md_history/history_card_manager.js#newcode30 chrome/browser/resources/md_history/history_card_manager.js:30: value: [] Shouldn't this be an object? Also, use ...
4 years, 10 months ago (2016-02-03 05:09:13 UTC) #3
hsampson
https://codereview.chromium.org/1648803003/diff/1/chrome/browser/resources/md_history/history_card_manager.js File chrome/browser/resources/md_history/history_card_manager.js (right): https://codereview.chromium.org/1648803003/diff/1/chrome/browser/resources/md_history/history_card_manager.js#newcode30 chrome/browser/resources/md_history/history_card_manager.js:30: value: [] On 2016/02/03 05:09:13, calamity wrote: > Shouldn't ...
4 years, 10 months ago (2016-02-08 00:39:37 UTC) #4
tsergeant
4 years, 10 months ago (2016-02-12 00:48:56 UTC) #5
(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!

Powered by Google App Engine
This is Rietveld 408576698