Kill DownloadManager::SearchDownloads, DownloadItem::MatchesQuery

Kill DownloadManager::SearchDownloads, DownloadItem::MatchesQuery

jhawkins: chrome/browser/ui/webui
jcivelli: content/public/test

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157137

[benjhayden, 2012-09-11 18:44:04]

PTAL

[Randy Smith (Not in Mondays), 2012-09-12 18:20:57]

LGTM.  

("I love the smell of code disappearing in the afternoon.  It smells like
victory.")

http://codereview.chromium.org/10905215/diff/6001/chrome/browser/download/dow...
File chrome/browser/download/download_query_unittest.cc (right):

http://codereview.chromium.org/10905215/diff/6001/chrome/browser/download/dow...
chrome/browser/download/download_query_unittest.cc:136:
TEST_F(DownloadQueryTest, DownloadQuery_QueryFilter) {
Maybe switch Pawel's TODO over here?  This is where the code will go when we
finally execute it.  (Yes, I'm an optimist; why do you ask? :-})

http://codereview.chromium.org/10905215/diff/6001/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/10905215/diff/6001/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.cc:319: if
(IsDownloadDisplayable(*download_item)) {
If there's a download updated that isn't in the search text, won't this code
mean that it's displayed on the downloads.html page?

[asanka, 2012-09-12 18:43:49]

http://codereview.chromium.org/10905215/diff/6001/chrome/browser/download/dow...
File chrome/browser/download/download_query.cc (right):

http://codereview.chromium.org/10905215/diff/6001/chrome/browser/download/dow...
chrome/browser/download/download_query.cc:71: if (item.GetBrowserContext())
Nit: Braces

http://codereview.chromium.org/10905215/diff/6001/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/10905215/diff/6001/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.cc:461:
DCHECK(query.AddFilter(DownloadQuery::FILTER_QUERY, *query_text.get()));
Side-effects in a DCHECK

[benjhayden, 2012-09-12 19:31:57]

PTAL

http://codereview.chromium.org/10905215/diff/6001/chrome/browser/download/dow...
File chrome/browser/download/download_query.cc (right):

http://codereview.chromium.org/10905215/diff/6001/chrome/browser/download/dow...
chrome/browser/download/download_query.cc:71: if (item.GetBrowserContext())
On 2012/09/12 18:43:49, asanka wrote:
> Nit: Braces

Done.

http://codereview.chromium.org/10905215/diff/6001/chrome/browser/download/dow...
File chrome/browser/download/download_query_unittest.cc (right):

http://codereview.chromium.org/10905215/diff/6001/chrome/browser/download/dow...
chrome/browser/download/download_query_unittest.cc:136:
TEST_F(DownloadQueryTest, DownloadQuery_QueryFilter) {
On 2012/09/12 18:20:57, rdsmith wrote:
> Maybe switch Pawel's TODO over here?  This is where the code will go when we
> finally execute it.  (Yes, I'm an optimist; why do you ask? :-})

Done.

http://codereview.chromium.org/10905215/diff/6001/chrome/browser/ui/webui/dow...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/10905215/diff/6001/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.cc:319: if
(IsDownloadDisplayable(*download_item)) {
On 2012/09/12 18:20:57, rdsmith wrote:
> If there's a download updated that isn't in the search text, won't this code
> mean that it's displayed on the downloads.html page?

Done.

http://codereview.chromium.org/10905215/diff/6001/chrome/browser/ui/webui/dow...
chrome/browser/ui/webui/downloads_dom_handler.cc:461:
DCHECK(query.AddFilter(DownloadQuery::FILTER_QUERY, *query_text.get()));
On 2012/09/12 18:43:49, asanka wrote:
> Side-effects in a DCHECK

Done.

[asanka, 2012-09-12 19:34:58]

LGTM

[commit-bot: I haz the power, 2012-09-13 13:21:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10905215/32

[commit-bot: I haz the power, 2012-09-13 13:22:09]

Presubmit check for 10905215-32 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui
    content/public/test

Presubmit checks took 1.4s to calculate.

[benjhayden, 2012-09-13 13:36:32]

James, Jay, PTAL for OWNERShip. Thanks!

[James Hawkins, 2012-09-13 16:08:29]

webui lgtm with nit.

https://chromiumcodereview.appspot.com/10905215/diff/32/chrome/browser/ui/web...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

https://chromiumcodereview.appspot.com/10905215/diff/32/chrome/browser/ui/web...
chrome/browser/ui/webui/downloads_dom_handler.cc:322: // search_text_.
TODO(benjhayden): Consider splitting MatchesQuery() out
nit: Put the TODO on its own line.

[benjhayden, 2012-09-13 16:24:21]

Thanks!
Jay?

http://codereview.chromium.org/10905215/diff/32/chrome/browser/ui/webui/downl...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/10905215/diff/32/chrome/browser/ui/webui/downl...
chrome/browser/ui/webui/downloads_dom_handler.cc:322: // search_text_.
TODO(benjhayden): Consider splitting MatchesQuery() out
On 2012/09/13 16:08:29, James Hawkins wrote:
> nit: Put the TODO on its own line.

Done.

[Jay Civelli, 2012-09-13 16:49:11]

lgtm

[commit-bot: I haz the power, 2012-09-17 14:44:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10905215/5008

[commit-bot: I haz the power, 2012-09-17 17:19:14]

Change committed as 157137