Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9568003:
Fixed issue with DownloadTestObserver. (Closed)
|
Created:
8 years, 9 months ago by ahendrickson Modified:
8 years, 9 months ago CC:
chromium-reviews, rdsmith+dwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFixed issue with DownloadTestObserver.
The possibility for a hang existed if the download ended up in a different state than the test expected. This fixes that.
BUG=117544
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126099
Patch Set 1 #Patch Set 2 : Merged with trunk #
Total comments: 6
Patch Set 3 : Added check that download finished in expected state. #
Total comments: 9
Patch Set 4 : Comment changes per Randy's suggestions. #Patch Set 5 : Split DownloadTestObserver in two #
Total comments: 15
Patch Set 6 : Merged with trunk. #Patch Set 7 : Changes per Randy's comments. #Patch Set 8 : Fixed CLANG issue. #
Total comments: 4
Patch Set 9 : Split DownloadTestObserver further. #
Total comments: 29
Patch Set 10 : More changes per Randy's comments. #Patch Set 11 : Fixed indentation. #Patch Set 12 : Merged with parent. #Patch Set 13 : Fixed GCC compiler issues. #Patch Set 14 : Changes per Randy's request. #Patch Set 15 : Fixed initialization order. #
Total comments: 2
Patch Set 16 : Changed DCHECKs to EXPECTs. #
Total comments: 5
Patch Set 17 : Merged with trunk. #Patch Set 18 : Fixed comment. #
Messages
Total messages: 35 (0 generated)
Ben, could you take a look at this as well? Specifically think about the interface to DownloadTestObserver and whether it's clean and cohesive with the new addition (I haven't looked at it--that's just my general worry about this kind of change). It's not as vital to have clean, cohesive interfaces in test code, but DTO's a little more important than most, as if it's used poorly that could lead to races and timeouts. I'll do my own review following.
http://codereview.chromium.org/9568003/diff/3001/chrome/browser/download/down... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/3001/chrome/browser/download/down... chrome/browser/download/download_test_observer.h:98: DownloadSet previously_finished_downloads_; So now we have 4 DownloadSets. It seems that it would be a simpler if we had a single map/set of DownloadItems, and filtered that single data-structure in different ways. Is there any way to compute these sets when they're needed?
http://codereview.chromium.org/9568003/diff/3001/chrome/browser/download/down... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/3001/chrome/browser/download/down... chrome/browser/download/download_test_observer.h:88: // Should never be called normally, only when there's a problem with a test. Remove 2nd line of comment. It's incorrect. http://codereview.chromium.org/9568003/diff/3001/chrome/browser/download/down... chrome/browser/download/download_test_observer.h:97: // we start. May overlap with |finished_downloads_|. May -> Will not
http://codereview.chromium.org/9568003/diff/3001/chrome/browser/download/down... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/3001/chrome/browser/download/down... chrome/browser/download/download_test_observer.h:88: // Should never be called normally, only when there's a problem with a test. On 2012/03/01 20:20:14, ahendrickson wrote: > Remove 2nd line of comment. It's incorrect. Done. http://codereview.chromium.org/9568003/diff/3001/chrome/browser/download/down... chrome/browser/download/download_test_observer.h:97: // we start. May overlap with |finished_downloads_|. On 2012/03/01 20:20:14, ahendrickson wrote: > May -> Will not Done. http://codereview.chromium.org/9568003/diff/3001/chrome/browser/download/down... chrome/browser/download/download_test_observer.h:98: DownloadSet previously_finished_downloads_; On 2012/03/01 19:58:32, benjhayden_chromium wrote: > So now we have 4 DownloadSets. > It seems that it would be a simpler if we had a single map/set of DownloadItems, > and filtered that single data-structure in different ways. > Is there any way to compute these sets when they're needed? I think the code would be more cluttered. Yes, we could put them all in one DownloadSet that contained a structure (including a DownloadItem, but we would need a flag that indicated whether or not it was marked after the test object's constructor finished). Then each test of inclusion would need to search the DownloadSet by looping through it. If you can think of a simpler way, I'd be interested in hearing about it.
So this feels uncomfortable to me; it's making an interface complicated and somewhat unclear. Before we could say "wait until we reach this state"; now we're saying "wait until we reach this state or any of a preset set of terminal states, and return a value indicating which of those set we've gotten to". I'm uncomfortable with that because it turns this class from a pure waiter to one that makes a value judgement between states, it puts IN_PROGRESS in a special state category, and because of the new EXPECT_EQ at the end of WaitForFinished that strikes me as restricting functionality in a way we don't need to. I'd rather keep the interface and functionality of this class conceptually simple. I'd suggest that we look for an interface that doesn't make specific states special. Instead, DownloadTestObserver will just wait, and when it's done waiting, the calling test will probe the manager to figure out if it's in a good state or a bad state. I think the simplest way to do this (which I did in a CL that I just went looking for and appears to be lost somewhere, foreshadowed by my TODO at line 44) is make DownloadTestObserver take a set of states rather than just one. Alternatively, we could have separate classes for terminal states and IN_PROGRESS (though if we ever transition straight to terminal from IN_PROGRESS, that's a problem; maybe that waiter should simply wait for a number of download items on the DownloadManager). But I do think the "make the waiter finish on the full set of things that could conceivably end the test, and outside the waiter probe for success or failure" is probably the right pattern. I've included a set of other comments, mostly to give Ben a sense of what kind of things I'd like him to look for in reviews like this. But this is the primary issue; if you're willing, I'd like to solve the problem you're trying to solve in a different way. (If we do stick with the current solution, I'd recommend updating the issue description to be clearer about exactly what problem we're trying to solve; i.e. "For example, if we are waiting for a completed download and that download completes in error (state INTERRUPTED) the test will time out.") http://codereview.chromium.org/9568003/diff/4003/chrome/browser/download/down... File chrome/browser/download/download_test_observer.cc (right): http://codereview.chromium.org/9568003/diff/4003/chrome/browser/download/down... chrome/browser/download/download_test_observer.cc:57: finished_downloads_at_construction_ = finished_downloads_.size(); This member variable is now redundant with keeping the full list; I'd get rid of it and just used finished_other_downloads_.size(). http://codereview.chromium.org/9568003/diff/4003/chrome/browser/download/down... chrome/browser/download/download_test_observer.cc:80: EXPECT_EQ(0u, finished_other_downloads_.size()); This is an extra constraint on behavior. We may not currently have any tests that rely on the old behavior, but it seems a shame to restrict behavior without reason. http://codereview.chromium.org/9568003/diff/4003/chrome/browser/download/down... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/4003/chrome/browser/download/down... chrome/browser/download/download_test_observer.h:21: // wait on it. You are changing the behavior of this class, so you need to update the comments that describe that behavior. That's either here, around WaitForFinished(), or both. http://codereview.chromium.org/9568003/diff/4003/chrome/browser/download/down... chrome/browser/download/download_test_observer.h:78: size_t NumOtherDownloadsSeen() const; In context, it isn't clear what "rogue downloads" are, so it's not clear how to use this new call.
http://codereview.chromium.org/9568003/diff/4003/chrome/browser/download/down... File chrome/browser/download/download_test_observer.cc (right): http://codereview.chromium.org/9568003/diff/4003/chrome/browser/download/down... chrome/browser/download/download_test_observer.cc:57: finished_downloads_at_construction_ = finished_downloads_.size(); On 2012/03/01 21:07:53, rdsmith wrote: > This member variable is now redundant with keeping the full list; I'd get rid of > it and just used finished_other_downloads_.size(). The list excludes items that match download_finished_state_, so it can't replace that variable. http://codereview.chromium.org/9568003/diff/4003/chrome/browser/download/down... chrome/browser/download/download_test_observer.cc:80: EXPECT_EQ(0u, finished_other_downloads_.size()); On 2012/03/01 21:07:53, rdsmith wrote: > This is an extra constraint on behavior. We may not currently have any tests > that rely on the old behavior, but it seems a shame to restrict behavior without > reason. Replaced with a logging statement. http://codereview.chromium.org/9568003/diff/4003/chrome/browser/download/down... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/4003/chrome/browser/download/down... chrome/browser/download/download_test_observer.h:21: // wait on it. On 2012/03/01 21:07:53, rdsmith wrote: > You are changing the behavior of this class, so you need to update the comments > that describe that behavior. That's either here, around WaitForFinished(), or > both. Done. http://codereview.chromium.org/9568003/diff/4003/chrome/browser/download/down... chrome/browser/download/download_test_observer.h:23: // TODO(rdsmith): Detect manager going down, remove pointer to Does my change fix this issue? I think so, because Shutdown() canceling the downloads should make IsFinished() return true. http://codereview.chromium.org/9568003/diff/4003/chrome/browser/download/down... chrome/browser/download/download_test_observer.h:78: size_t NumOtherDownloadsSeen() const; On 2012/03/01 21:07:53, rdsmith wrote: > In context, it isn't clear what "rogue downloads" are, so it's not clear how to > use this new call. Changed the comment.
PTAL.
lgtm
Is there a bug associated with this? There should be which describes the problem.
On 2012/03/02 20:27:19, cbentzel wrote: > Is there a bug associated with this? There should be which describes the > problem. There isn't, AFAIK. This only affects tests.
Split DownloadObserverTest into two classes. The original remains the same, except that it only tests terminal states (basically, anything but IN_PROGRESS). The other, DownloadObserverTestInProgress, just tests for IN_PROGRESS.
http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.cc (right): http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:77: if (finished_download_count >= wait_count_) Why the change? It looks functionally the same--am I confused? http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:19: // as either CANCELLED, or one which you can't switch out of). I'd just list them here; INTERRUPTED can be switched out of once you do the resumption, and CANCELLED can't be until then. http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:28: class DownloadTestObserver : public content::DownloadManager::Observer, I'd make the names more parallel. Maybe DownloadTestObserverTerminal vs. DownloadTestObserverInProgress? http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:46: // to treat as completion events. This TODO is now obsolete. http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:58: // Wait for a terminal state, for the number of downloads requested. nit, suggestion: Maybe better worded as "Wait for the number of downloads requested to enter a terminal state"? http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:64: // content::DownloadItem::Observer Not your problem; my mistake when I wrote the class, but I've since learned that you can actually make these types of interfaces private, and inherit from the observer classes privately, and everything will work. Specifically, the point at which you do the "cast" (pass the pointer in) from this class to a *::Observer is within methods of this class, so it's able to access private aspects of the class, and thus to cast to a base class that's inherited from privately. It's a neat trick. (Again, you don't have to do anything about this; I just thought I'd call out a neat bit of C++ arcana.) http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:76: protected: So this gives me a bit of an interface minimality allergic reaction; you're opening up the entire implementation of this class to anything that inherits from it. Can you figure out what the common functionality needed is and have that be an implementation class that you either inherit from (privately) or delegate to? (Another way to think of it: conceptually, DownloadTestObserver() is a class that blocks until some number of Downloads enter a terminal state. DownloadTestObserverInProgress block until some number of downloads enter a *non* terminal state. So there is *not* an ISA relationship between the two classes, which makes this type of change purely about implementation convenience at the cost of interface cleanliness. Which, as you might imagine, doesn't make me happy :-J.) http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:152: protected: FYI: This can be private and everything will work well.
http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.cc (right): http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:77: if (finished_download_count >= wait_count_) On 2012/03/06 21:49:58, rdsmith wrote: > Why the change? It looks functionally the same--am I confused? Sorry, that was left over from a previous patch. Reverted. http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:19: // as either CANCELLED, or one which you can't switch out of). On 2012/03/06 21:49:58, rdsmith wrote: > I'd just list them here; INTERRUPTED can be switched out of once you do the > resumption, and CANCELLED can't be until then. I've changed it to read "anything but IN_PROGRESS". [CANCELLED can be a state on the way to REMOVING.] http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:28: class DownloadTestObserver : public content::DownloadManager::Observer, On 2012/03/06 21:49:58, rdsmith wrote: > I'd make the names more parallel. Maybe DownloadTestObserverTerminal vs. > DownloadTestObserverInProgress? Done. http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:46: // to treat as completion events. On 2012/03/06 21:49:58, rdsmith wrote: > This TODO is now obsolete. Removed comment. http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:58: // Wait for a terminal state, for the number of downloads requested. On 2012/03/06 21:49:58, rdsmith wrote: > nit, suggestion: Maybe better worded as "Wait for the number of downloads > requested to enter a terminal state"? Done. http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:76: protected: On 2012/03/06 21:49:58, rdsmith wrote: > So this gives me a bit of an interface minimality allergic reaction; you're > opening up the entire implementation of this class to anything that inherits > from it. Can you figure out what the common functionality needed is and have > that be an implementation class that you either inherit from (privately) or > delegate to? > > (Another way to think of it: conceptually, DownloadTestObserver() is a class > that blocks until some number of Downloads enter a terminal state. > DownloadTestObserverInProgress block until some number of downloads enter a > *non* terminal state. So there is *not* an ISA relationship between the two > classes, which makes this type of change purely about implementation convenience > at the cost of interface cleanliness. Which, as you might imagine, doesn't make > me happy :-J.) Done. http://codereview.chromium.org/9568003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:152: protected: On 2012/03/06 21:49:58, rdsmith wrote: > FYI: This can be private and everything will work well. Done.
http://codereview.chromium.org/9568003/diff/25001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9568003/diff/25001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:695: 1, Shouldn't all the observers where the state has been removed have a following on test to make sure that the download item(s) being waited for completed in the right state? http://codereview.chromium.org/9568003/diff/25001/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/25001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:139: class DownloadTestObserverInProgress : public DownloadTestObserverTerminal { This is test code, so I won't insist on a change, but the right thing to do here is to have a separate class that has all the shared functionality for *InProgress and *Terminal and have them both inherit from it. If anyone were ever to take a *InProgress and cast it to a *Terminal they'd get a really bad surprise (which is what I meant by there not being an ISA relationship between the two classes). It's possible that you could get the proper behavior (ban the ISA relationship) by making the inheritance private (you'd probably need to write some wrapper functions). I don't know if that would work, but if it did, you'd make me noticeably more comfortable about this implementation (it wouldn't be right, but at least it wouldn't be broken anymore :-}). If that doesn't work, please put in a note saying "Inheritance purely for implementation convenience; there is no ISA relationship between these classes and they should not be casted to each other."
http://codereview.chromium.org/9568003/diff/25001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9568003/diff/25001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:695: 1, On 2012/03/07 21:39:16, rdsmith wrote: > Shouldn't all the observers where the state has been removed have a following on > test to make sure that the download item(s) being waited for completed in the > right state? Done for all the non-IN_PROGRESS observers. http://codereview.chromium.org/9568003/diff/25001/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/25001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:139: class DownloadTestObserverInProgress : public DownloadTestObserverTerminal { On 2012/03/07 21:39:16, rdsmith wrote: > This is test code, so I won't insist on a change, but the right thing to do here > is to have a separate class that has all the shared functionality for > *InProgress and *Terminal and have them both inherit from it. If anyone were > ever to take a *InProgress and cast it to a *Terminal they'd get a really bad > surprise (which is what I meant by there not being an ISA relationship between > the two classes). > > It's possible that you could get the proper behavior (ban the ISA relationship) > by making the inheritance private (you'd probably need to write some wrapper > functions). I don't know if that would work, but if it did, you'd make me > noticeably more comfortable about this implementation (it wouldn't be right, but > at least it wouldn't be broken anymore :-}). If that doesn't work, please put > in a note saying "Inheritance purely for implementation convenience; there is no > ISA relationship between these classes and they should not be casted to each > other." Done.
This looks basically good--thanks very much for going along with my class relationship preference. Several comments, but only a few even semi-substantive ones. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:466: DCHECK_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::COMPLETE)); I think of DCHECKs as not being good in test code, and from my perspective a lot of your DCHECK_EQ() really should be EXPECT_EQ or ASSERT_EQ. Could you change them or help me understand why they're DCHECKs? http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:580: CheckDownloadStatesForBrowser(browser, 1, DownloadItem::COMPLETE); These seem redundant. You're welcome to leave it like this if you want, but I don't think it's needed. (Applies throughout file; as suggested elsewhere, if you nuke one of these I'd nuke the NumDownloadsSeenInState.) http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.cc (right): http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:219: states_observed_.clear(); Completely up to you, but why duplicate this code in the derived classes rather than in the base class constructor? http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:237: ON_DANGEROUS_DOWNLOAD_FAIL) { Why FAIL rather than IGNORE? Dangerous downloads won't interfere with transitioning to IN_PROGRESS, so the class is more flexible without failing (I think?). http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:73: content::DownloadItem::DownloadState state) const; Up to you, but I'd be more inclined to leave this functionality out of the class and let the user probe for it separately (maybe by a separate routine on DownloadTest). http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:168: // - A specified number of downloads change to the IN_PROGRESS state. You haven't specified the behavior on a dangerous download (which I'm suggesting changing elsewhere) or on a select file prompt. The select file prompt behavior I'm feeling iffy about--if the select file dialog is popped up, that will block the download visibly transitioning to the IN_PROGRESS state, and if the select file dialog is nulled, a "finish_on_select_file" may result in an exit before the downloads really get to IN_PROGRESS. So you might want to actually give the user control over select file dialog behavior? (I think I said something different than this earlier (and my apologies for the mixed message). That means that one of the two times I'm confused--I obviously believe it's the previous one, but you might want to check the code. I think that we don't see IN_PROGRESS downloads until after entry into history, which is delayed on the select file dialog--that's the important set of events to figure out.) http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:170: // nit: Could you nuke the blank comment line? I think that's not the usual custom. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_close_browsertest.cc (right): http://codereview.chromium.org/9568003/diff/31001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:128: DCHECK_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::IN_PROGRESS)); Why is this 1 rather than the value num_downloads was at the beginning of the routine?
Still catching up on reading the thread of comments.
Here's an idea. If it's too painful to contemplate, don't worry about it.
What would the simplest possible interface that still did the job look like?
It looks to me like we want a function that blocks until one of a few kinds of
conditions are met, then tells us which condition was met first.
That could look something like this:
static int WaitForAnyCondition(const std::vector<base::Callback<bool()> >&
conditions);
For example, we could have one condition (callback) that returns true when N
downloads match a query (such as state==in_progress):
static bool NumDownloadsFromQuery(DownloadManager* manager, DownloadQuery*
query, int expected_num_downloads) {
vector all_downloads, results;
query->Search(..., &results);
return expected_num_downloads == results.size();
}
We could let the Callback own the DownloadQuery.
We could curry all the arguments in to the callbacks before passing them to
WaitForAnyCondition.
We could have another condition that returns true when the file selection dialog
is shown.
We could have a condition that composes other conditions: return cond0.Run() &&
(cond1.Run() || cond2.Run());
Under the covers, WFAC might observe a given DownloadManager or all available
managers and/or items. WFAC could just run all its conditions whenever any
observer event happens without thinking too hard about what's happening. Or it
could poll on an interval or something else.
Clients might look something like this:
conditions.push_back(base::Bind(&AllDownloadsComplete));
conditions.push_back(base::Bind(&AnyDownloadInterrupted));
conditions.push_back(base::Bind(&FileSelectionDialogShown));
EXPECT_EQ(0, WaitForAnyCondition(conditions));
If more than one test.cc would use a given condition or set of conditions, then
those conditions could go in the same file-pair as WFAC.
It looks to me like DTO could hold up for a while as it is now and maybe even
evolve a little more, but I don't know how much longer it can evolve before it
will need a rewrite.
http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):
http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:405: int num_downloads) {
pre-existing nit: indentation.
http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow...
File chrome/browser/download/download_test_observer.h (right):
http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow...
chrome/browser/download/download_test_observer.h:85:
scoped_refptr<content::DownloadManager> download_manager_;
Would it be too much trouble to keep member fields private and provide protected
accessors as needed? 'Yes' is an acceptable answer, although 'not this CL' would
be preferable. It's just a readability issue.
http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:466: DCHECK_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::COMPLETE)); On 2012/03/08 18:25:31, rdsmith wrote: > I think of DCHECKs as not being good in test code, and from my perspective a lot > of your DCHECK_EQ() really should be EXPECT_EQ or ASSERT_EQ. Could you change > them or help me understand why they're DCHECKs? Done. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:580: CheckDownloadStatesForBrowser(browser, 1, DownloadItem::COMPLETE); On 2012/03/08 18:25:31, rdsmith wrote: > These seem redundant. You're welcome to leave it like this if you want, but I > don't think it's needed. (Applies throughout file; as suggested elsewhere, if > you nuke one of these I'd nuke the NumDownloadsSeenInState.) For this test, they are somewhat redundant. In others, NumDownloadsSeenInState is more reliable. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.cc (right): http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:219: states_observed_.clear(); On 2012/03/08 18:25:31, rdsmith wrote: > Completely up to you, but why duplicate this code in the derived classes rather > than in the base class constructor? This one is a bit tricky: In the base class constructor, you can't override the functions using the virtual function table (it hasn't been set up yet), so it would be calling its own IsDownloadInFinalState() instead of the derived class'. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:237: ON_DANGEROUS_DOWNLOAD_FAIL) { On 2012/03/08 18:25:31, rdsmith wrote: > Why FAIL rather than IGNORE? Dangerous downloads won't interfere with > transitioning to IN_PROGRESS, so the class is more flexible without failing (I > think?). I don't think a download class can be declared dangerous before it goes into IN_PROGRESS, so I don't think it matters what I put here. There isn't an _IGNORE, but I could use _ACCEPT if you'd prefer. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:73: content::DownloadItem::DownloadState state) const; On 2012/03/08 18:25:31, rdsmith wrote: > Up to you, but I'd be more inclined to leave this functionality out of the class > and let the user probe for it separately (maybe by a separate routine on > DownloadTest). It's here specifically for IN_PROGRESS tests, which could transition out of that into another state before checking in the tests. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:168: // - A specified number of downloads change to the IN_PROGRESS state. On 2012/03/08 18:25:31, rdsmith wrote: > You haven't specified the behavior on a dangerous download (which I'm suggesting > changing elsewhere) or on a select file prompt. The select file prompt behavior > I'm feeling iffy about--if the select file dialog is popped up, that will block > the download visibly transitioning to the IN_PROGRESS state, and if the select > file dialog is nulled, a "finish_on_select_file" may result in an exit before > the downloads really get to IN_PROGRESS. So you might want to actually give the > user control over select file dialog behavior? (I think I said something > different than this earlier (and my apologies for the mixed message). That > means that one of the two times I'm confused--I obviously believe it's the > previous one, but you might want to check the code. I think that we don't see > IN_PROGRESS downloads until after entry into history, which is delayed on the > select file dialog--that's the important set of events to figure out.) I think we're in trouble if the select file dialog appears, no matter what. The tests can specify to automatically accept the default and avoid them. I changed it to ignoring the select file dialog for IN_PROGRESS tests. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:170: // On 2012/03/08 18:25:31, rdsmith wrote: > nit: Could you nuke the blank comment line? I think that's not the usual > custom. Done. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_close_browsertest.cc (right): http://codereview.chromium.org/9568003/diff/31001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:128: DCHECK_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::IN_PROGRESS)); On 2012/03/08 18:25:31, rdsmith wrote: > Why is this 1 rather than the value num_downloads was at the beginning of the > routine? Cut & paste error. Fixed.
On 2012/03/08 20:30:02, benjhayden_chromium wrote:
> Still catching up on reading the thread of comments.
>
> Here's an idea. If it's too painful to contemplate, don't worry about it.
>
> What would the simplest possible interface that still did the job look like?
> It looks to me like we want a function that blocks until one of a few kinds of
> conditions are met, then tells us which condition was met first.
> That could look something like this:
> static int WaitForAnyCondition(const std::vector<base::Callback<bool()> >&
> conditions);
>
> For example, we could have one condition (callback) that returns true when N
> downloads match a query (such as state==in_progress):
> static bool NumDownloadsFromQuery(DownloadManager* manager, DownloadQuery*
> query, int expected_num_downloads) {
> vector all_downloads, results;
> query->Search(..., &results);
> return expected_num_downloads == results.size();
> }
> We could let the Callback own the DownloadQuery.
> We could curry all the arguments in to the callbacks before passing them to
> WaitForAnyCondition.
> We could have another condition that returns true when the file selection
dialog
> is shown.
> We could have a condition that composes other conditions: return cond0.Run()
&&
> (cond1.Run() || cond2.Run());
>
> Under the covers, WFAC might observe a given DownloadManager or all available
> managers and/or items. WFAC could just run all its conditions whenever any
> observer event happens without thinking too hard about what's happening. Or it
> could poll on an interval or something else.
>
> Clients might look something like this:
> conditions.push_back(base::Bind(&AllDownloadsComplete));
> conditions.push_back(base::Bind(&AnyDownloadInterrupted));
> conditions.push_back(base::Bind(&FileSelectionDialogShown));
> EXPECT_EQ(0, WaitForAnyCondition(conditions));
>
> If more than one test.cc would use a given condition or set of conditions,
then
> those conditions could go in the same file-pair as WFAC.
>
> It looks to me like DTO could hold up for a while as it is now and maybe even
> evolve a little more, but I don't know how much longer it can evolve before it
> will need a rewrite.
I like the basic idea, but I'm not thrilled about having to create a bunch of
callbacks even if some of them are reusable.
Perhaps we could have a set of 'built-in' (that is, part of the WAFC class)
callbacks that can be used, and then the tester would only have to write custom
ones when necessary.
Not in this CL, though.
http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:405: int num_downloads) { On 2012/03/08 20:30:02, benjhayden_chromium wrote: > pre-existing nit: indentation. Done. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:85: scoped_refptr<content::DownloadManager> download_manager_; On 2012/03/08 20:30:02, benjhayden_chromium wrote: > Would it be too much trouble to keep member fields private and provide protected > accessors as needed? 'Yes' is an acceptable answer, although 'not this CL' would > be preferable. It's just a readability issue. My preference would be to punt this to another CL, but if people feel strongly enough about it I'll do it here.
On 2012/03/08 20:30:02, benjhayden_chromium wrote: > Still catching up on reading the thread of comments. > > Here's an idea. If it's too painful to contemplate, don't worry about it. > > What would the simplest possible interface that still did the job look like? > It looks to me like we want a function that blocks until one of a few kinds of > conditions are met, then tells us which condition was met first. > That could look something like this: > static int WaitForAnyCondition(const std::vector<base::Callback<bool()> >& > conditions); So there's a key requirement for DownloadTestObserver that (I don't think--please correct me if I'm wrong) this doesn't satisfy. The problem is handling races. If you initiate something and wait for a state transition, you are screwed if the state transition happens before your actual wait. The usual way I've seen to get around this is waiting for a particular end state rather than a transition, but that can be messed up if the end state was true before your initiation, or if the end state becomes true after the initiation then goes back to false before you wait. So what DownloadTestObserver allows is creation of the waiter before initiation (so it can take stock of the current state) and waiting for your event after the initiation; it can catch all state transitions in the window of time you care about. The downloads tests used to be lousy with races like this; I think one of Andy's and my biggest accomplishments was rooting them all out. As you might imagine, I want to be careful that any changes in waiting technology doesn't bring them back :-}.
On 2012/03/08 22:13:26, rdsmith wrote: > On 2012/03/08 20:30:02, benjhayden_chromium wrote: > > Still catching up on reading the thread of comments. > > > > Here's an idea. If it's too painful to contemplate, don't worry about it. > > > > What would the simplest possible interface that still did the job look like? > > It looks to me like we want a function that blocks until one of a few kinds of > > conditions are met, then tells us which condition was met first. > > That could look something like this: > > static int WaitForAnyCondition(const std::vector<base::Callback<bool()> >& > > conditions); > > So there's a key requirement for DownloadTestObserver that (I don't > think--please correct me if I'm wrong) this doesn't satisfy. The problem is > handling races. If you initiate something and wait for a state transition, you > are screwed if the state transition happens before your actual wait. The usual > way I've seen to get around this is waiting for a particular end state rather > than a transition, but that can be messed up if the end state was true before > your initiation, or if the end state becomes true after the initiation then goes > back to false before you wait. So what DownloadTestObserver allows is creation > of the waiter before initiation (so it can take stock of the current state) and > waiting for your event after the initiation; it can catch all state transitions > in the window of time you care about. Do we need to look for state transitions, or can we poll for the current states? What if WFAC ran its conditions right before starting to wait? So, if a condition is met before WFAC is called, it could return without actually waiting. > > The downloads tests used to be lousy with races like this; I think one of Andy's > and my biggest accomplishments was rooting them all out. As you might imagine, > I want to be careful that any changes in waiting technology doesn't bring them > back :-}.
On 2012/03/08 22:28:50, benjhayden_chromium wrote: > On 2012/03/08 22:13:26, rdsmith wrote: > > On 2012/03/08 20:30:02, benjhayden_chromium wrote: > > > Still catching up on reading the thread of comments. > > > > > > Here's an idea. If it's too painful to contemplate, don't worry about it. > > > > > > What would the simplest possible interface that still did the job look like? > > > It looks to me like we want a function that blocks until one of a few kinds > of > > > conditions are met, then tells us which condition was met first. > > > That could look something like this: > > > static int WaitForAnyCondition(const std::vector<base::Callback<bool()> >& > > > conditions); > > > > So there's a key requirement for DownloadTestObserver that (I don't > > think--please correct me if I'm wrong) this doesn't satisfy. The problem is > > handling races. If you initiate something and wait for a state transition, > you > > are screwed if the state transition happens before your actual wait. The > usual > > way I've seen to get around this is waiting for a particular end state rather > > than a transition, but that can be messed up if the end state was true before > > your initiation, or if the end state becomes true after the initiation then > goes > > back to false before you wait. So what DownloadTestObserver allows is > creation > > of the waiter before initiation (so it can take stock of the current state) > and > > waiting for your event after the initiation; it can catch all state > transitions > > in the window of time you care about. > > Do we need to look for state transitions, or can we poll for the current states? There may be a way to poll for current state, but we need to be fairly careful that we know all the possible state transitions that might be happening so we know what states to poll for. > What if WFAC ran its conditions right before starting to wait? So, if a > condition is met before WFAC is called, it could return without actually > waiting. WFAC running its conditions before starting to wait doesn't handle the case where the initiation has already changed the conditions; there's a window between initiation and calling WFAC. That's the big virtue of DTO; it separates running conditions from waiting, so you can put the initiation between them. If we were certain that a) we know what the starting conditions before initiation were, and b) we're certain we won't have a transient state that might go away before we wait (e.g. the select file dialog popping up when we've nulled out the actual dialog), we could use a single point waiter. Even if we could be certain of those two things, I do think DTO is more resilient to future code changes, but I certainly agree that it's more complex. It's all about having the window in which you're looking for transactions being able to straddle your initiation point. > > > > > > The downloads tests used to be lousy with races like this; I think one of > Andy's > > and my biggest accomplishments was rooting them all out. As you might > imagine, > > I want to be careful that any changes in waiting technology doesn't bring them > > back :-}.
http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.cc (right): http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:219: states_observed_.clear(); On 2012/03/08 20:56:34, ahendrickson wrote: > On 2012/03/08 18:25:31, rdsmith wrote: > > Completely up to you, but why duplicate this code in the derived classes > rather > > than in the base class constructor? > > This one is a bit tricky: In the base class constructor, you can't override the > functions using the virtual function table (it hasn't been set up yet), so it > would be calling its own IsDownloadInFinalState() instead of the derived class'. Ah, good catch. Yikes. Would you comment that? http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:237: ON_DANGEROUS_DOWNLOAD_FAIL) { On 2012/03/08 20:56:34, ahendrickson wrote: > On 2012/03/08 18:25:31, rdsmith wrote: > > Why FAIL rather than IGNORE? Dangerous downloads won't interfere with > > transitioning to IN_PROGRESS, so the class is more flexible without failing (I > > think?). > > I don't think a download class can be declared dangerous before it goes into > IN_PROGRESS, so I don't think it matters what I put here. My model of the transitions say that it can happen simultaneously (the first time we see IN_PROGRESS currently is at history addition, which is after several different types of danger evaluation) and FAIL will result in a failure call in OnDownloadUpdated. Accept's not ideal either, but I don't think it matters too much. But whatever you do, please comment the header file with details about what behavior will occur. > > There isn't an _IGNORE, but I could use _ACCEPT if you'd prefer. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:73: content::DownloadItem::DownloadState state) const; On 2012/03/08 20:56:34, ahendrickson wrote: > On 2012/03/08 18:25:31, rdsmith wrote: > > Up to you, but I'd be more inclined to leave this functionality out of the > class > > and let the user probe for it separately (maybe by a separate routine on > > DownloadTest). > > It's here specifically for IN_PROGRESS tests, which could transition out of that > into another state before checking in the tests. Ah, makes sense. Ok. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:85: scoped_refptr<content::DownloadManager> download_manager_; On 2012/03/08 21:28:55, ahendrickson wrote: > On 2012/03/08 20:30:02, benjhayden_chromium wrote: > > Would it be too much trouble to keep member fields private and provide > protected > > accessors as needed? 'Yes' is an acceptable answer, although 'not this CL' > would > > be preferable. It's just a readability issue. > > My preference would be to punt this to another CL, but if people feel strongly > enough about it I'll do it here. For this kind of issue, it's useful to look at whether there's something about the design that's problematic (i.e. assume it's a code smell--where's the cheese?). In this case, it looks to me as if the reason all of these member variables are accessed by the derived classes is because they're accessed in the derived class constructors, code that is duplicated between derived class constructors (another code smell, already noted). You pointed out that we needed that to be executed after the base class constructor was executed so that the virtual function table for the base class would be initialized (good catch, and nice bullet dodge). Well, is there any way we can execute that common code in the base class after base class construction? (I.e. I'm suggesting making an Init() base class method that's called from the derived class constructor.) To answer your basic issue above, I dislike letting real design/encapsulation issues in before landing. If it was just readability I'd be ok with a new CL, but based on my analysis above, it's not. Let me know if you disagree. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:168: // - A specified number of downloads change to the IN_PROGRESS state. On 2012/03/08 20:56:34, ahendrickson wrote: > On 2012/03/08 18:25:31, rdsmith wrote: > > You haven't specified the behavior on a dangerous download (which I'm > suggesting > > changing elsewhere) or on a select file prompt. The select file prompt > behavior > > I'm feeling iffy about--if the select file dialog is popped up, that will > block > > the download visibly transitioning to the IN_PROGRESS state, and if the select > > file dialog is nulled, a "finish_on_select_file" may result in an exit before > > the downloads really get to IN_PROGRESS. So you might want to actually give > the > > user control over select file dialog behavior? (I think I said something > > different than this earlier (and my apologies for the mixed message). That > > means that one of the two times I'm confused--I obviously believe it's the > > previous one, but you might want to check the code. I think that we don't see > > IN_PROGRESS downloads until after entry into history, which is delayed on the > > select file dialog--that's the important set of events to figure out.) > > I think we're in trouble if the select file dialog appears, no matter what. The > tests can specify to automatically accept the default and avoid them. > > I changed it to ignoring the select file dialog for IN_PROGRESS tests. Confused, and I'd like to be unconfused before we land this. I think we're in the same position with IN_PROGRESS that we've been previously, and that we want the full select functionality. Specifically: * If the test is testing to see if the select file dialog appears and wants to exit when it gets that dialog, ignoring won't be right (we may not have any IN_PROGRESS tests in this space, so this may be a comment about having options for future tests). * If the test is expecting the select file dialog and wants to blow past it, it'll null the dialog out. However, this will still fire the select file dialog notification (done from a lower level), so *not* ignoring it won't be right for these tests. So I think we need to port over the control logic from the old code. Do you see something I'm missing?
http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.cc (right): http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:219: states_observed_.clear(); On 2012/03/09 18:39:38, rdsmith wrote: > On 2012/03/08 20:56:34, ahendrickson wrote: > > On 2012/03/08 18:25:31, rdsmith wrote: > > > Completely up to you, but why duplicate this code in the derived classes > > rather > > > than in the base class constructor? > > > > This one is a bit tricky: In the base class constructor, you can't override > the > > functions using the virtual function table (it hasn't been set up yet), so it > > would be calling its own IsDownloadInFinalState() instead of the derived > class'. > > Ah, good catch. Yikes. Would you comment that? I *had* comments to this effect, but they seem to have disappeared. Done. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:237: ON_DANGEROUS_DOWNLOAD_FAIL) { On 2012/03/09 18:39:38, rdsmith wrote: > On 2012/03/08 20:56:34, ahendrickson wrote: > > On 2012/03/08 18:25:31, rdsmith wrote: > > > Why FAIL rather than IGNORE? Dangerous downloads won't interfere with > > > transitioning to IN_PROGRESS, so the class is more flexible without failing > (I > > > think?). > > > > I don't think a download class can be declared dangerous before it goes into > > IN_PROGRESS, so I don't think it matters what I put here. > > My model of the transitions say that it can happen simultaneously (the first > time we see IN_PROGRESS currently is at history addition, which is after several > different types of danger evaluation) and FAIL will result in a failure call in > OnDownloadUpdated. Accept's not ideal either, but I don't think it matters too > much. But whatever you do, please comment the header file with details about > what behavior will occur. Your model is correct. The header file has the comments for this class. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:85: scoped_refptr<content::DownloadManager> download_manager_; On 2012/03/09 18:39:38, rdsmith wrote: > On 2012/03/08 21:28:55, ahendrickson wrote: > > On 2012/03/08 20:30:02, benjhayden_chromium wrote: > > > Would it be too much trouble to keep member fields private and provide > > protected > > > accessors as needed? 'Yes' is an acceptable answer, although 'not this CL' > > would > > > be preferable. It's just a readability issue. > > > > My preference would be to punt this to another CL, but if people feel strongly > > enough about it I'll do it here. > > For this kind of issue, it's useful to look at whether there's something about > the design that's problematic (i.e. assume it's a code smell--where's the > cheese?). In this case, it looks to me as if the reason all of these member > variables are accessed by the derived classes is because they're accessed in the > derived class constructors, code that is duplicated between derived class > constructors (another code smell, already noted). You pointed out that we > needed that to be executed after the base class constructor was executed so that > the virtual function table for the base class would be initialized (good catch, > and nice bullet dodge). Well, is there any way we can execute that common code > in the base class after base class construction? (I.e. I'm suggesting making an > Init() base class method that's called from the derived class constructor.) > > To answer your basic issue above, I dislike letting real design/encapsulation > issues in before landing. If it was just readability I'd be ok with a new CL, > but based on my analysis above, it's not. Let me know if you disagree. OK, done. http://codereview.chromium.org/9568003/diff/31001/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:168: // - A specified number of downloads change to the IN_PROGRESS state. On 2012/03/09 18:39:38, rdsmith wrote: > On 2012/03/08 20:56:34, ahendrickson wrote: > > On 2012/03/08 18:25:31, rdsmith wrote: > > > You haven't specified the behavior on a dangerous download (which I'm > > suggesting > > > changing elsewhere) or on a select file prompt. The select file prompt > > behavior > > > I'm feeling iffy about--if the select file dialog is popped up, that will > > block > > > the download visibly transitioning to the IN_PROGRESS state, and if the > select > > > file dialog is nulled, a "finish_on_select_file" may result in an exit > before > > > the downloads really get to IN_PROGRESS. So you might want to actually give > > the > > > user control over select file dialog behavior? (I think I said something > > > different than this earlier (and my apologies for the mixed message). That > > > means that one of the two times I'm confused--I obviously believe it's the > > > previous one, but you might want to check the code. I think that we don't > see > > > IN_PROGRESS downloads until after entry into history, which is delayed on > the > > > select file dialog--that's the important set of events to figure out.) > > > > I think we're in trouble if the select file dialog appears, no matter what. > The > > tests can specify to automatically accept the default and avoid them. > > > > I changed it to ignoring the select file dialog for IN_PROGRESS tests. > > Confused, and I'd like to be unconfused before we land this. I think we're in > the same position with IN_PROGRESS that we've been previously, and that we want > the full select functionality. Specifically: > > * If the test is testing to see if the select file dialog appears and wants to > exit when it gets that dialog, ignoring won't be right (we may not have any > IN_PROGRESS tests in this space, so this may be a comment about having options > for future tests). > > * If the test is expecting the select file dialog and wants to blow past it, > it'll null the dialog out. However, this will still fire the select file dialog > notification (done from a lower level), so *not* ignoring it won't be right for > these tests. > > So I think we need to port over the control logic from the old code. Do you see > something I'm missing? OK, added the parameter to DownloadObserverTestInProgress, and updated the comments.
sky: PTAL at chrome/browser/ui/browser_close_browsertest.cc
http://codereview.chromium.org/9568003/diff/47003/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_close_browsertest.cc (right): http://codereview.chromium.org/9568003/diff/47003/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:131: DCHECK_EQ(count_downloads, Why are you using DCHECK in a test?
http://codereview.chromium.org/9568003/diff/47003/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_close_browsertest.cc (right): http://codereview.chromium.org/9568003/diff/47003/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:131: DCHECK_EQ(count_downloads, On 2012/03/09 20:20:28, sky wrote: > Why are you using DCHECK in a test? Oops, missed that one. Fixed.
LGTM
LGTM with comment changes below. http://codereview.chromium.org/9568003/diff/49004/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.cc (right): http://codereview.chromium.org/9568003/diff/49004/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:223: // You can't override virtual functions in a base class constructor; the wording nit: "You can't rely on overridden functions". The overriding occurs in the class definition, not in the constructor. http://codereview.chromium.org/9568003/diff/49004/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/49004/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:171: // IN_PROGRESS state, or a Select File Dialog has appeared. This comment about the select file dialog is no longer correct, correct?
http://codereview.chromium.org/9568003/diff/49004/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.cc (right): http://codereview.chromium.org/9568003/diff/49004/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:223: // You can't override virtual functions in a base class constructor; the On 2012/03/12 01:52:15, rdsmith wrote: > wording nit: "You can't rely on overridden functions". The overriding occurs in > the class definition, not in the constructor. Done. http://codereview.chromium.org/9568003/diff/49004/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/49004/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:171: // IN_PROGRESS state, or a Select File Dialog has appeared. On 2012/03/12 01:52:15, rdsmith wrote: > This comment about the select file dialog is no longer correct, correct? It is; I added a parameter to the constructor that allows it to finish if the select file dialog has appeared.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/9568003/51011
Change committed as 126099
http://codereview.chromium.org/9568003/diff/49004/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/9568003/diff/49004/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:171: // IN_PROGRESS state, or a Select File Dialog has appeared. On 2012/03/12 03:13:54, ahendrickson wrote: > On 2012/03/12 01:52:15, rdsmith wrote: > > This comment about the select file dialog is no longer correct, correct? > > It is; I added a parameter to the constructor that allows it to finish if the > select file dialog has appeared. Um ... that was my point. This says that the observer finishes when the select file dialog has appeared, but whether it has that behavior or not depends on the parameter, so the comment is incorrect. Am I confused? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
