Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10854127:
Rewrite DownloadsDOMHandler (Closed)
|
Created:
8 years, 4 months ago by benjhayden Modified:
8 years, 4 months ago CC:
chromium-reviews, asanka, Randy Smith (Not in Mondays) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRewrite DownloadsDOMHandler
Instead of keeping its own vector of items, let DownloadManager keep track of items. That's what it's for.
Instead of using items' position in the vector as an identifier, use DownloadItem::GetId(). That's what it's for.
Add a test for DownloadsDOMHandler.
BUG=142389
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153116
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #
Total comments: 10
Patch Set 7 : . #Patch Set 8 : . #Patch Set 9 : . #Patch Set 10 : . #
Total comments: 6
Patch Set 11 : . #
Total comments: 2
Patch Set 12 : . #Patch Set 13 : . #
Total comments: 42
Patch Set 14 : . #Patch Set 15 : . #
Total comments: 19
Patch Set 16 : . #Patch Set 17 : . #Patch Set 18 : . #
Messages
Total messages: 22 (0 generated)
PTAL Bug fix by sledgehammer :-)
PTAL with the beginnings of a test. Please feel free to suggest specifically what other behaviors should be tested.
Sending intermediate comments since my comment on the destructor may result in moderate sized changes. I'll keep doing initial high level review anyway. http://codereview.chromium.org/10854127/diff/12/chrome/browser/ui/webui/downl... File chrome/browser/ui/webui/downloads_dom_handler.cc (left): http://codereview.chromium.org/10854127/diff/12/chrome/browser/ui/webui/downl... chrome/browser/ui/webui/downloads_dom_handler.cc:425: // out from under us, so update our idea of the downloads as soon as possible. How does this end up working? Do we tear down the danger prompt if the download disappears out from under us? http://codereview.chromium.org/10854127/diff/12/chrome/browser/ui/webui/downl... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): http://codereview.chromium.org/10854127/diff/12/chrome/browser/ui/webui/downl... chrome/browser/ui/webui/downloads_dom_handler.cc:255: } This is quite scary to me--you're relying on the search (which is a reasonably high level function) always returning the same downloads, even if there's a change in their state. Yes, it *should* work, but if it doesn't, you've got a use-after-free error. The usual pattern is to track all the items observed, and remove those items when you remove observation, and I'd rather keep that pattern. http://codereview.chromium.org/10854127/diff/12/chrome/browser/ui/webui/downl... chrome/browser/ui/webui/downloads_dom_handler.cc:313: if (!update_scheduled_) { Comment as to motivation. http://codereview.chromium.org/10854127/diff/12/chrome/browser/ui/webui/downl... chrome/browser/ui/webui/downloads_dom_handler.cc:426: // TODO(benjhayden): Should this only clear items that match search_text_? I vote yes, but obviously not in this CL.
Thoughts on tests: You're really close to pure unit-test level isolation (which is awesome), and if you did that, you could unit test each function. But I don't think that's needed for this CL :-}. The ideas I came up with are: * Confirm that the extension downloads filter works properly. * Make sure you see the right set of downloads when you have both an incognito and a regular download manager. * Make sure that bad download ids incoming from the javascript are dropped on the floor. * Confirm RemoveDownloadObserver is called on all download items. I'm not saying I want all of those, just brainstorming--I'd suggest you implement what's easy to implement, and what this change is likely to touch.
One more question. http://codereview.chromium.org/10854127/diff/12/base/nix/mime_util_xdg.cc File base/nix/mime_util_xdg.cc (right): http://codereview.chromium.org/10854127/diff/12/base/nix/mime_util_xdg.cc#new... base/nix/mime_util_xdg.cc:591: if (filepath.empty()) return ""; Right, sorry. Why are the changes in this file part of this CL?
PTAL http://codereview.chromium.org/10854127/diff/12/base/nix/mime_util_xdg.cc File base/nix/mime_util_xdg.cc (right): http://codereview.chromium.org/10854127/diff/12/base/nix/mime_util_xdg.cc#new... base/nix/mime_util_xdg.cc:591: if (filepath.empty()) return ""; On 2012/08/15 19:30:31, rdsmith wrote: > Right, sorry. Why are the changes in this file part of this CL? Split to http://codereview.chromium.org/10831337/ http://codereview.chromium.org/10854127/diff/12/chrome/browser/ui/webui/downl... File chrome/browser/ui/webui/downloads_dom_handler.cc (left): http://codereview.chromium.org/10854127/diff/12/chrome/browser/ui/webui/downl... chrome/browser/ui/webui/downloads_dom_handler.cc:425: // out from under us, so update our idea of the downloads as soon as possible. On 2012/08/15 19:04:16, rdsmith wrote: > How does this end up working? Do we tear down the danger prompt if the download > disappears out from under us? The danger prompt owns itself, so we hand it a weak ptr in its callbacks. If the tab is closed before the danger prompt is accepted/canceled, then the weak ptr will say "nevermind", and the WebContents will make sure that the danger prompt cleans itself up. As for downloads disappearing from under us, we notice as soon as possible (OnDownloadDestroyed), and update the renderer in the next UI message loop iteration. GetDownloadByValue now uses DownloadManager::GetDownload(), so if the download still exists, then we'll get it, and if it doesn't, then we won't. http://codereview.chromium.org/10854127/diff/12/chrome/browser/ui/webui/downl... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): http://codereview.chromium.org/10854127/diff/12/chrome/browser/ui/webui/downl... chrome/browser/ui/webui/downloads_dom_handler.cc:255: } On 2012/08/15 19:04:16, rdsmith wrote: > This is quite scary to me--you're relying on the search (which is a reasonably > high level function) always returning the same downloads, even if there's a > change in their state. Yes, it *should* work, but if it doesn't, you've got a > use-after-free error. The usual pattern is to track all the items observed, and > remove those items when you remove observation, and I'd rather keep that > pattern. Done, but I made a note to write AddObserver(WeakPtr<Observer>). http://codereview.chromium.org/10854127/diff/12/chrome/browser/ui/webui/downl... chrome/browser/ui/webui/downloads_dom_handler.cc:313: if (!update_scheduled_) { On 2012/08/15 19:04:16, rdsmith wrote: > Comment as to motivation. Done. http://codereview.chromium.org/10854127/diff/12/chrome/browser/ui/webui/downl... chrome/browser/ui/webui/downloads_dom_handler.cc:426: // TODO(benjhayden): Should this only clear items that match search_text_? On 2012/08/15 19:04:16, rdsmith wrote: > I vote yes, but obviously not in this CL. Done.
LGTM. WRT the comments below: * Please do the doc request. * Please do the test here (either manual or not) or make sure that we have some test already in place that attempts to cancel the download item after file name determination but before persistence--I'm worried about that window, but I don't think that there's any code between the DDH and the DMI that you need to test. * The DCHECK() is up to you. http://codereview.chromium.org/10854127/diff/4007/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): http://codereview.chromium.org/10854127/diff/4007/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:220: !item.GetTargetFilePath().empty()); This plus the shift to OnDownloadCreated() means that you're going to have a window during which the user can cancel downloads that you didn't before. Are you reasonably comfortable that the right things will happen in that case? Any way to do a test? (Manual is fine.) http://codereview.chromium.org/10854127/diff/4007/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:342: content::DownloadItem* download_item) { Worth DCHECKing to make sure we have it? http://codereview.chromium.org/10854127/diff/4007/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.h (right): http://codereview.chromium.org/10854127/diff/4007/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.h:99: // Send the current list of downloads to the page. Could you document somewhere (here, observing_items_, .cc file; I'm not sure where the best place is) that we observer all downloads associated with the managers, but filter at SendCurrentDownloads and OnDownloadUpdated()?
Asanka? http://codereview.chromium.org/10854127/diff/4007/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): http://codereview.chromium.org/10854127/diff/4007/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:220: !item.GetTargetFilePath().empty()); On 2012/08/17 17:49:47, rdsmith wrote: > This plus the shift to OnDownloadCreated() means that you're going to have a > window during which the user can cancel downloads that you didn't before. Are > you reasonably comfortable that the right things will happen in that case? Any > way to do a test? (Manual is fine.) Tested manually by calling Cancel(true) as soon as an item became displayable. No crashes, the ui updated just fine, sqlite3 confirms that the items still made it into the history. http://codereview.chromium.org/10854127/diff/4007/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:342: content::DownloadItem* download_item) { On 2012/08/17 17:49:47, rdsmith wrote: > Worth DCHECKing to make sure we have it? Who would call OnDownloadDestroyed besides ~DownloadItem? http://codereview.chromium.org/10854127/diff/4007/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.h (right): http://codereview.chromium.org/10854127/diff/4007/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.h:99: // Send the current list of downloads to the page. On 2012/08/17 17:49:47, rdsmith wrote: > Could you document somewhere (here, observing_items_, .cc file; I'm not sure > where the best place is) that we observer all downloads associated with the > managers, but filter at SendCurrentDownloads and OnDownloadUpdated()? Documented in OnDownloadCreated by the AddObserver.
http://codereview.chromium.org/10854127/diff/3012/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): http://codereview.chromium.org/10854127/diff/3012/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:324: if (IsDownloadDisplayable(*download_item) && OnDownloadCreated() is also called for temporary downloads. IsDownloadDisplayable() doesn't check for those.
http://codereview.chromium.org/10854127/diff/3012/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): http://codereview.chromium.org/10854127/diff/3012/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:324: if (IsDownloadDisplayable(*download_item) && On 2012/08/17 21:19:49, asanka wrote: > OnDownloadCreated() is also called for temporary downloads. > IsDownloadDisplayable() doesn't check for those. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10854127/7014
Presubmit check for 10854127-7014 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
chrome/browser/ui/webui
James, PTAL for OWNERShip. Thanks!
http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:265: if (original_profile_download_manager_) { Remove added braces; the precedence in this code is to not use braces for single line blocks. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:320: // DownloadsDOMHandler observes all items, and only chooses which downloads to nit: Remove comma. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:328: // together instead. We may handle hundreds of OnDownloadCreated() calls in nit: Remove 'we'. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:332: base::Bind(&DownloadsDOMHandler::SendCurrentDownloads, nit: Start of parameter rows must align on the same column. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:354: // together instead. We may handle hundreds of OnDownloadDestroyed() calls in nit: Remove 'we'. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:356: if (!update_scheduled_) { It appears some of this code can be shared with OnDownloadCreated. Please consider a refactor. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:359: base::Bind(&DownloadsDOMHandler::SendCurrentDownloads, nit: Start of parameter rows must align on the same column. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:479: download_manager_, Optional nit: Consolidate parameters to save a row. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.h (right): http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.h:99: typedef std::set<content::DownloadItem*> DownloadSet; nit: Documentation. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.h:105: void SearchDownloads(content::DownloadManager::DownloadVector* downloads); nit: Document |downloads|. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.h:128: // Keep track of all items that we are observing so that we can be sure to nit: Don't use pronouns in comments (we); pronouns are ambiguous. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.h:140: bool update_scheduled_; nit: Documentation. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc (right): http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:20: bool ListMatches(base::ListValue* left_list, const std::string& right_json) { Document method and parameters. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:23: CHECK(right_value->GetAsList(&right_list)); s/CHECK/DCHECK/ http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:35: LOG(WARNING) << iter.key(); Remove console spam? http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:43: class MockDownloadsDOMHandler : public DownloadsDOMHandler { Document. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:74: } // anonymous namespace s/anonymous// http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:100: IN_PROC_BROWSER_TEST_F(DownloadsDOMHandlerTest, Document what the test is testing. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:109: GURL(""), Optional nit: Consolidate parameters to save rows. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:122: EXPECT_EQ(1, static_cast<int>(mddh.downloads_list()->GetSize())); ASSERT_EQ
PTAL http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:265: if (original_profile_download_manager_) { On 2012/08/19 06:17:29, James Hawkins wrote: > Remove added braces; the precedence in this code is to not use braces for single > line blocks. Done. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:320: // DownloadsDOMHandler observes all items, and only chooses which downloads to On 2012/08/19 06:17:29, James Hawkins wrote: > nit: Remove comma. Done. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:328: // together instead. We may handle hundreds of OnDownloadCreated() calls in On 2012/08/19 06:17:29, James Hawkins wrote: > nit: Remove 'we'. Done. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:332: base::Bind(&DownloadsDOMHandler::SendCurrentDownloads, On 2012/08/19 06:17:29, James Hawkins wrote: > nit: Start of parameter rows must align on the same column. Done. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:354: // together instead. We may handle hundreds of OnDownloadDestroyed() calls in On 2012/08/19 06:17:29, James Hawkins wrote: > nit: Remove 'we'. Done. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:356: if (!update_scheduled_) { On 2012/08/19 06:17:29, James Hawkins wrote: > It appears some of this code can be shared with OnDownloadCreated. Please > consider a refactor. Done. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:359: base::Bind(&DownloadsDOMHandler::SendCurrentDownloads, On 2012/08/19 06:17:29, James Hawkins wrote: > nit: Start of parameter rows must align on the same column. Done. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:479: download_manager_, On 2012/08/19 06:17:29, James Hawkins wrote: > Optional nit: Consolidate parameters to save a row. Done. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.h (right): http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.h:99: typedef std::set<content::DownloadItem*> DownloadSet; On 2012/08/19 06:17:29, James Hawkins wrote: > nit: Documentation. Done. Not sure what you wanted. It's a set of DownloadItem*s. There's only one DownloadSet, which is documented. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.h:105: void SearchDownloads(content::DownloadManager::DownloadVector* downloads); On 2012/08/19 06:17:29, James Hawkins wrote: > nit: Document |downloads|. Done. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.h:128: // Keep track of all items that we are observing so that we can be sure to On 2012/08/19 06:17:29, James Hawkins wrote: > nit: Don't use pronouns in comments (we); pronouns are ambiguous. Done. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.h:140: bool update_scheduled_; On 2012/08/19 06:17:29, James Hawkins wrote: > nit: Documentation. Done. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc (right): http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:20: bool ListMatches(base::ListValue* left_list, const std::string& right_json) { On 2012/08/19 06:17:29, James Hawkins wrote: > Document method and parameters. Done. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:23: CHECK(right_value->GetAsList(&right_list)); On 2012/08/19 06:17:29, James Hawkins wrote: > s/CHECK/DCHECK/ Aren't DCHECKs compiled out entirely in non-debug builds? ListMatches() would still need to GetAsList() from right_value to right_list. Same for other CHECKs in this file. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:35: LOG(WARNING) << iter.key(); On 2012/08/19 06:17:29, James Hawkins wrote: > Remove console spam? It will only print when the test is about to fail, and provide valuable debugging information. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:43: class MockDownloadsDOMHandler : public DownloadsDOMHandler { On 2012/08/19 06:17:29, James Hawkins wrote: > Document. Done. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:74: } // anonymous namespace On 2012/08/19 06:17:29, James Hawkins wrote: > s/anonymous// Done. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:100: IN_PROC_BROWSER_TEST_F(DownloadsDOMHandlerTest, On 2012/08/19 06:17:29, James Hawkins wrote: > Document what the test is testing. Done. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:109: GURL(""), On 2012/08/19 06:17:29, James Hawkins wrote: > Optional nit: Consolidate parameters to save rows. I think that it's more readable this way. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:122: EXPECT_EQ(1, static_cast<int>(mddh.downloads_list()->GetSize())); On 2012/08/19 06:17:29, James Hawkins wrote: > ASSERT_EQ Done.
http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc (right): http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:23: CHECK(right_value->GetAsList(&right_list)); On 2012/08/19 22:20:27, benjhayden_chromium wrote: > On 2012/08/19 06:17:29, James Hawkins wrote: > > s/CHECK/DCHECK/ > > Aren't DCHECKs compiled out entirely in non-debug builds? ListMatches() would > still need to GetAsList() from right_value to right_list. Same for other CHECKs > in this file. No side-effect inducing calls should be made in either CHECKs or DCHECKs for this exact reason. If you want to check that the call succeeded, put the return value into a var and DCHECK that. http://codereview.chromium.org/10854127/diff/7014/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:35: LOG(WARNING) << iter.key(); On 2012/08/19 22:20:27, benjhayden_chromium wrote: > On 2012/08/19 06:17:29, James Hawkins wrote: > > Remove console spam? > > It will only print when the test is about to fail, and provide valuable > debugging information. Attempting to guess what's 'valuable' as far as logging goes leads to console spam. I'm not going to ask you to take it out, but keep in mind that logging used to debug a failure (test or otherwise) should only be used during debugging, including committing to get information from the bots then reverting the logging once the failure is fixed. As is this logging is taking up space solely to debug a hypothetical failure that may or may not happen in the future. http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:218: // Filter out extension downloads and downloads that don't have a filename yet. nit: Filters http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:374: if (!file || !web_contents) Under what circumstances is |web_contents| NULL? http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:525: void DownloadsDOMHandler::CallDownloadsList(const base::ListValue& downloads) { Remove these wrapper method and make the calls directly. http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.h (right): http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.h:99: // A set of DownloadItem*s. Nix this. The documentation should describe the purpose, e.g. to have a shorthand notation, and how it's used, e.g. DownloadSets are passed from the model and displayed in the view (or what have you). http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.h:102: // Schedule a call to SendCurrentDownloads() in the next message loop nit: Method documentation should use third-person active, i.e., Schedules. http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc (right): http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:20: // Read |right_json| into a ListValue |left_list|; return true if all key-value nit: Reads http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:20: // Read |right_json| into a ListValue |left_list|; return true if all key-value nit: returns http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:106: // Test that DownloadsDOMHandler detects new downloads and relays them to the nit: Tests
PTAL http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:218: // Filter out extension downloads and downloads that don't have a filename yet. On 2012/08/22 17:39:01, James Hawkins wrote: > nit: Filters Done. http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:374: if (!file || !web_contents) On 2012/08/22 17:39:01, James Hawkins wrote: > Under what circumstances is |web_contents| NULL? Only the test. GetWebUIWebContents(), CallDownloadsList(), and CallDownloadUpdated() exist solely to facilitate tests. http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:525: void DownloadsDOMHandler::CallDownloadsList(const base::ListValue& downloads) { On 2012/08/22 17:39:01, James Hawkins wrote: > Remove these wrapper method and make the calls directly. These wrapper methods facilitate the test. I'm happy to discuss alternative ways to facilitate the test. For example, a MockWebUI would be great, but I didn't see one in codesearch. http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.h (right): http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.h:99: // A set of DownloadItem*s. On 2012/08/22 17:39:01, James Hawkins wrote: > Nix this. The documentation should describe the purpose, e.g. to have a > shorthand notation, and how it's used, e.g. DownloadSets are passed from the > model and displayed in the view (or what have you). Done. http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.h:102: // Schedule a call to SendCurrentDownloads() in the next message loop On 2012/08/22 17:39:01, James Hawkins wrote: > nit: Method documentation should use third-person active, i.e., Schedules. Done. http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc (right): http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:20: // Read |right_json| into a ListValue |left_list|; return true if all key-value On 2012/08/22 17:39:01, James Hawkins wrote: > nit: Reads Done. http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:20: // Read |right_json| into a ListValue |left_list|; return true if all key-value On 2012/08/22 17:39:01, James Hawkins wrote: > nit: returns Done. http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc:106: // Test that DownloadsDOMHandler detects new downloads and relays them to the On 2012/08/22 17:39:01, James Hawkins wrote: > nit: Tests Done.
LGTM with on doc request. http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:374: if (!file || !web_contents) On 2012/08/22 20:19:22, benjhayden_chromium wrote: > On 2012/08/22 17:39:01, James Hawkins wrote: > > Under what circumstances is |web_contents| NULL? > > Only the test. > GetWebUIWebContents(), CallDownloadsList(), and CallDownloadUpdated() exist > solely to facilitate tests. Please document this case (that |web_contents| is only NULL in tests). http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:525: void DownloadsDOMHandler::CallDownloadsList(const base::ListValue& downloads) { On 2012/08/22 20:19:22, benjhayden_chromium wrote: > On 2012/08/22 17:39:01, James Hawkins wrote: > > Remove these wrapper method and make the calls directly. > > These wrapper methods facilitate the test. > I'm happy to discuss alternative ways to facilitate the test. > For example, a MockWebUI would be great, but I didn't see one in codesearch. Ah, right. I forgot.
Thanks, everybody! I'll hit the CQ when some of the trybots turn green. http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): http://codereview.chromium.org/10854127/diff/5029/chrome/browser/ui/webui/dow... chrome/browser/ui/webui/downloads_dom_handler.cc:374: if (!file || !web_contents) On 2012/08/22 20:24:20, James Hawkins wrote: > On 2012/08/22 20:19:22, benjhayden_chromium wrote: > > On 2012/08/22 17:39:01, James Hawkins wrote: > > > Under what circumstances is |web_contents| NULL? > > > > Only the test. > > GetWebUIWebContents(), CallDownloadsList(), and CallDownloadUpdated() exist > > solely to facilitate tests. > > Please document this case (that |web_contents| is only NULL in tests). Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10854127/20002
Change committed as 153116 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
