Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7466033:
Fix warning prompting on closing a window that will cancel downloads. (Closed)
|
Created:
9 years, 5 months ago by Randy Smith (Not in Mondays) Modified:
9 years, 2 months ago Reviewers:
asanka, jennb, sky, Evan Stade, commit-bot: I haz the power, ahendrickson, Paweł Hajdan Jr., achuithb, Miranda Callahan CC:
chromium-reviews, asanka, rdsmith+dwatch_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix warning prompting on closing a window that will cancel downloads.
Handles two cases: Last window close (-> shuts download browser), and
last incognito window in an incognito profile (-> cancels downloads
on that profile).
Note that this doesn't cover the macintosh, which goes through different
code (http://crbug.com/88419) and the warning for incognito close is not
ideal (http://crbug.com/88421). This CL includes some modularization
to make resolving those issues easier.
BUG=61257
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105662
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106958
Patch Set 1 #Patch Set 2 : Fixed compile error. #Patch Set 3 : Removed unneeded friend decls and tweaked a comment. #
Total comments: 40
Patch Set 4 : Incorporated comments from sky & jennb. #Patch Set 5 : Fix compiler warning. #
Total comments: 4
Patch Set 6 : Incorporated comments, got test working. #Patch Set 7 : Parallel view cleanup to gtk. #
Total comments: 2
Patch Set 8 : Updated comments on tabbing. #Patch Set 9 : Various fixes related to try jobs. #
Total comments: 1
Patch Set 10 : Merged to LKGR after long hiatus. #Patch Set 11 : Merge fixes and comment update. #Patch Set 12 : Fixing trybot failures (chromeos specifically). #
Total comments: 12
Patch Set 13 : Sync'd to near TOT and adapted to new test setup requirements. #Patch Set 14 : Fix testing and choosing of entry browser to handle closes-in-progress on mac. #Patch Set 15 : Fixed Achuith's comments. #Patch Set 16 : Merged up to latest (mostly around DownloadService changes. #
Total comments: 45
Patch Set 17 : Incoporated comments, primarily from Achuith. #
Total comments: 2
Patch Set 18 : Last little things from Achuith and Miranda. #
Total comments: 6
Patch Set 19 : Removed test case specification hack. #Patch Set 20 : Removed some unneeded include files. #Patch Set 21 : Incorporated believed last set of comments. #Patch Set 22 : Fixed typo. #Patch Set 23 : Incorporated comments + doubled test instances. #Patch Set 24 : Merge to LKGR. #Patch Set 25 : Merged to LKGR. #Patch Set 26 : Make sure to use temporary download directory #Messages
Total messages: 45 (0 generated)
PTAL. This is an MS-14 issue (I know, down to the wire) so if you can review quickly I'd be very grateful. * ahendrickson: chrome/browser/download/* * sky: crhome/browser/ui/* (top level) & chrome/chrome_tests.gypi * mirandac: Use of ProfileManager in CheckDownloadsInProgress (browser.cc) * estade: Gtk modification. * dmazzoni: Views modification. I've also cc'd phajdan.jr in case he wants to comment on my breakout of the observer code, and jennb FHI on the breakout of the observer code. Note that the test I've written is disabled; it's dependent on some code from mirandac that I don't believe I'm going to get in time for MS-14. I've done manual testing, specifically: * Open download, close browser->correct warning. * Open incognito window, download in regular window, close regular window->no warning. Close incognito window->correct warning. * Incognito window, incognito download, close incognito window->correct warning. * New regular window, download in original regular window, close original regular window->no warning. Close new regular window->correct warning. * 2 Downloads, close window->Correct warning mentioning two downloads. * 2 Downloads, incognito window, close regular window, close incognito window->Correct warning mentioning two downloads. And I would like to commit the function change on the basis of that manual testing and getting the auto test in ASA I have the support code. Thanks much in advance!
On 2011/07/21 17:29:39, rdsmith wrote: > PTAL. This is an MS-14 issue (I know, down to the wire) so if you can review > quickly I'd be very grateful. > * ahendrickson: chrome/browser/download/* > * sky: crhome/browser/ui/* (top level) & chrome/chrome_tests.gypi > * mirandac: Use of ProfileManager in CheckDownloadsInProgress (browser.cc) > * estade: Gtk modification. > * dmazzoni: Views modification. > > I've also cc'd phajdan.jr in case he wants to comment on my breakout of the > observer code, and jennb FHI on the breakout of the observer code. > > Note that the test I've written is disabled; it's dependent on some code from > mirandac that I don't believe I'm going to get in time for MS-14. I've done > manual testing, specifically: > * Open download, close browser->correct warning. > * Open incognito window, download in regular window, close regular window->no > warning. Close incognito window->correct warning. > * Incognito window, incognito download, close incognito window->correct warning. > * New regular window, download in original regular window, close original > regular window->no warning. Close new regular window->correct warning. > * 2 Downloads, close window->Correct warning mentioning two downloads. > * 2 Downloads, incognito window, close regular window, close incognito > window->Correct warning mentioning two downloads. > And I would like to commit the function change on the basis of that manual > testing and getting the auto test in ASA I have the support code. > > Thanks much in advance! ProfileManager usage in CheckDownloadsInProgress LGTM. I'm making the test cases as we speak; if they aren't finished before this CL is committed, I can fix and pass to you for review.
On 2011/07/21 17:42:44, Miranda Callahan wrote: > ProfileManager usage in CheckDownloadsInProgress LGTM. Thanks! > I'm making the test > cases as we speak; if they aren't finished before this CL is committed, I can > fix and pass to you for review. I'll admit that I'm also concerned about the time to get the test working properly after I have the code--if possible, I'd like to have the ok to commit without getting the test working if that's how the time crunch occurs. I guess that's primarily up to sky by my parceling out of review responsibilities as an OWNER of chrome/browser/ui.
Thanks for the cc. Couple small nits. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/download/down... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/7466033/diff/4001/chrome/browser/download/down... chrome/browser/download/download_test_observer.h:14: #include "chrome/browser/download/download_item.h" alpha ordering http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.h#... chrome/browser/ui/browser.h:121: // Downloas associated with this incognito profile would be canceled. typo - Downloas
http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1061: // Let's figure out if we are the last window for our profile, and window -> tabbed window http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1083: || browser->is_attempting_to_close_browser_ nit: || on previous line http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1084: || !browser->is_type_tabbed()) How come non-tabbed windows don't count? http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1094: *type = BROWSER_SHUTDOWN; Since this case just means there are no more tabbed browsers, BROWSER_SHUTDOWN is misleading. How about something like DOWNLOAD_CLOSE_NO_MORE_TABBED_BROWSERS, or something. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1104: *type = LAST_WINDOW_IN_INCOGNITO_PROFILE; LAST_WINDOW -> LAST_TABBED_BROWSER http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:4470: void Browser::CheckDownloadsInProgress(int* total_download_count, This method feels like it should be two methods. One for total download count, and one for profile download count. Also, since the implementation is Profile specific and not Browser specific, it should be some where else (Browser already has enough state). Maybe it should be on Profile or a static method on DownloadManager that takes a Profile. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:4484: std::vector<Profile*>::const_iterator profile_it = nit: move this into the for loop http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.h#... chrome/browser/ui/browser.h:116: // OkToCloseWithInProgressDownloads; part of that function's interface. The last fragment of this sentence doesn't make sense to me. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.h#... chrome/browser/ui/browser.h:119: BROWSER_SHUTDOWN, Can you prefix this with something to make it clear they are related to DownloadClosePreventionType. Maybe DOWNLOAD_CLOSE_IN_SHUTDOWN and DOWNLOAD_CLOSE_LAST_WINDOW_IN_... Or something. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.h#... chrome/browser/ui/browser.h:372: bool OkToCloseWithInProgressDownloads(DownloadClosePreventionType* type, It's a little bizarre to have both a boolean return and an enum. Shouldn't the true state be expressed as another element of the enum? http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... File chrome/browser/ui/browser_close_browsertest.cc (right): http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... chrome/browser/ui/browser_close_browsertest.cc:23: Profile* CreateNewProfile() { Why is this here? http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... chrome/browser/ui/browser_close_browsertest.cc:28: void CreateStalledDownload(Browser* browser, int num_downloads) { Please add descriptions for the methods that aren't obvious, especially ones that have out params. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... chrome/browser/ui/browser_close_browsertest.cc:134: enum { REGULAR = 0, INCOGNITO = 1 } window_to_probe; enums should be first. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... chrome/browser/ui/browser_close_browsertest.cc:244: IN_PROC_BROWSER_TEST_F(BrowserCloseTest, DISABLED_DownloadsCloseCheck) { How come you're checking in a test that's disabled?
Next round. Miranda, based on sky's comments I've added methods to Profile and ProfileManager, could you review those? http://codereview.chromium.org/7466033/diff/4001/chrome/browser/download/down... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/7466033/diff/4001/chrome/browser/download/down... chrome/browser/download/download_test_observer.h:14: #include "chrome/browser/download/download_item.h" On 2011/07/21 18:35:33, jennb wrote: > alpha ordering Done. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1061: // Let's figure out if we are the last window for our profile, and On 2011/07/21 20:08:11, sky wrote: > window -> tabbed window Done. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1083: || browser->is_attempting_to_close_browser_ On 2011/07/21 20:08:11, sky wrote: > nit: || on previous line Done. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1084: || !browser->is_type_tabbed()) On 2011/07/21 20:08:11, sky wrote: > How come non-tabbed windows don't count? That's an excellent question and I don't know the answer; I was copying the code and only modifying the parts I was clear on. What we're doing here is trying to figure out ahead of time what will result in profile destruction (which means we're trying to keep separate code paths in sync, which is a losing battle, I just didn't see another way to do it). If the existence of a non-tabbed windows won't keep the associated profile alive we should skip them, and if they do, we shouldn't. What are the contexts in which we use non-tabbed windows, do you know? Does that summary spark any sense of why we might treat them specially? http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1094: *type = BROWSER_SHUTDOWN; On 2011/07/21 20:08:11, sky wrote: > Since this case just means there are no more tabbed browsers, BROWSER_SHUTDOWN > is misleading. How about something like DOWNLOAD_CLOSE_NO_MORE_TABBED_BROWSERS, > or something. Profiles are only killed on browser shutdown, so if non-tabbed windows keep the browser alive, the bug's in the code above, not here. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1104: *type = LAST_WINDOW_IN_INCOGNITO_PROFILE; On 2011/07/21 20:08:11, sky wrote: > LAST_WINDOW -> LAST_TABBED_BROWSER Holding off on this until we resolve the tabbed window issue. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:4470: void Browser::CheckDownloadsInProgress(int* total_download_count, On 2011/07/21 20:08:11, sky wrote: > This method feels like it should be two methods. One for total download count, > and one for profile download count. > > Also, since the implementation is Profile specific and not Browser specific, it > should be some where else (Browser already has enough state). Maybe it should be > on Profile or a static method on DownloadManager that takes a Profile. That makes sense, though I'm not sure where to put them--DownloadManager has the same problem as Browser. I've dumped them on Profile and ProfileManager, which brings mirandac back in as a reviewer. Miranda, could you take a look at the new situation? http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:4484: std::vector<Profile*>::const_iterator profile_it = On 2011/07/21 20:08:11, sky wrote: > nit: move this into the for loop The loop now matches similar loops in profile_manager.cc which uses indices; this is now moot. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.h#... chrome/browser/ui/browser.h:116: // OkToCloseWithInProgressDownloads; part of that function's interface. On 2011/07/21 20:08:11, sky wrote: > The last fragment of this sentence doesn't make sense to me. Removed. What I was trying to say was just "this enum was created purely to be part of that function's interface" but that's reasonably clear from context. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.h#... chrome/browser/ui/browser.h:119: BROWSER_SHUTDOWN, On 2011/07/21 20:08:11, sky wrote: > Can you prefix this with something to make it clear they are related to > DownloadClosePreventionType. Maybe DOWNLOAD_CLOSE_IN_SHUTDOWN and > DOWNLOAD_CLOSE_LAST_WINDOW_IN_... Or something. Done. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.h#... chrome/browser/ui/browser.h:121: // Downloas associated with this incognito profile would be canceled. On 2011/07/21 18:35:33, jennb wrote: > typo - Downloas Done. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.h#... chrome/browser/ui/browser.h:372: bool OkToCloseWithInProgressDownloads(DownloadClosePreventionType* type, On 2011/07/21 20:08:11, sky wrote: > It's a little bizarre to have both a boolean return and an enum. Shouldn't the > true state be expressed as another element of the enum? Whoops, not sure how I missed that. Right you are. Done. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... File chrome/browser/ui/browser_close_browsertest.cc (right): http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... chrome/browser/ui/browser_close_browsertest.cc:23: Profile* CreateNewProfile() { On 2011/07/21 20:08:11, sky wrote: > Why is this here? See my answer to the question about the disabled test, below. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... chrome/browser/ui/browser_close_browsertest.cc:28: void CreateStalledDownload(Browser* browser, int num_downloads) { On 2011/07/21 20:08:11, sky wrote: > Please add descriptions for the methods that aren't obvious, especially ones > that have out params. Done. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... chrome/browser/ui/browser_close_browsertest.cc:134: enum { REGULAR = 0, INCOGNITO = 1 } window_to_probe; On 2011/07/21 20:08:11, sky wrote: > enums should be first. This line declares a member variable window_to_probe using an anonymous enum; it's not really an enum declaration. If I move the enum first, I have to name it and create two lines where there was one before. If that's what you want, I'll do it, but I want to just confirm that? http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... chrome/browser/ui/browser_close_browsertest.cc:244: IN_PROC_BROWSER_TEST_F(BrowserCloseTest, DISABLED_DownloadsCloseCheck) { On 2011/07/21 20:08:11, sky wrote: > How come you're checking in a test that's disabled? So if you'd like me to make that a separate CL, I'm willing; this is just trying to write the automated testing for a change at the same time as I write the change. The code to support that test (the code to create a new profile for testing in multi-profile context, which would result in filling in CreateNewProfile() above) isn't written yet, and I'm worried about getting my change into MS-14 if I wait on it, so I just barreled ahead doing as much as I could. Shall I split this portion of the change out? If so, is it ok if I leave the refactor of DownloadTestObserver in? That's potentially useful for other people, and it was finicky enough to do that I don't look forward to reverting it and doing it again later. Without this test, it doesn't strictly speaking have much to do with this CL, but it doesn't change functionality either--it's just a refactor.
chrome/browser/download/*: LGTM.
lgtm http://codereview.chromium.org/7466033/diff/11001/chrome/browser/ui/gtk/downl... File chrome/browser/ui/gtk/download/download_in_progress_dialog_gtk.cc (right): http://codereview.chromium.org/7466033/diff/11001/chrome/browser/ui/gtk/downl... chrome/browser/ui/gtk/download/download_in_progress_dialog_gtk.cc:25: &download_count); looks like this would fit on the line above
Profiles LGTM, with nitty ternary-op suggestion. http://codereview.chromium.org/7466033/diff/11001/chrome/browser/profiles/pro... File chrome/browser/profiles/profile.cc (right): http://codereview.chromium.org/7466033/diff/11001/chrome/browser/profiles/pro... chrome/browser/profiles/profile.cc:859: return GetDownloadManager()->in_progress_count(); A tiny change, but maybe 2 lines instead of 4 here: return HasCreatedDownloadManager() ? GetDownloadManager()->in_progress_count() : 0;
http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1084: || !browser->is_type_tabbed()) On 2011/07/21 21:36:31, rdsmith wrote: > On 2011/07/21 20:08:11, sky wrote: > > How come non-tabbed windows don't count? > > That's an excellent question and I don't know the answer; I was copying the code > and only modifying the parts I was clear on. What we're doing here is trying to > figure out ahead of time what will result in profile destruction (which means > we're trying to keep separate code paths in sync, which is a losing battle, I > just didn't see another way to do it). If the existence of a non-tabbed windows > won't keep the associated profile alive we should skip them, and if they do, we > shouldn't. What are the contexts in which we use non-tabbed windows, do you > know? Does that summary spark any sense of why we might treat them specially? > Popups are the common case of a non-tabbed browser, and it keeps chrome running. App windows are the other case, but as common. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1094: *type = BROWSER_SHUTDOWN; On 2011/07/21 21:36:31, rdsmith wrote: > On 2011/07/21 20:08:11, sky wrote: > > Since this case just means there are no more tabbed browsers, BROWSER_SHUTDOWN > > is misleading. How about something like > DOWNLOAD_CLOSE_NO_MORE_TABBED_BROWSERS, > > or something. > > Profiles are only killed on browser shutdown, so if non-tabbed windows keep the > browser alive, the bug's in the code above, not here. non-tabbed windows definitely keep chrome running. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... File chrome/browser/ui/browser_close_browsertest.cc (right): http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... chrome/browser/ui/browser_close_browsertest.cc:134: enum { REGULAR = 0, INCOGNITO = 1 } window_to_probe; On 2011/07/21 21:36:31, rdsmith wrote: > On 2011/07/21 20:08:11, sky wrote: > > enums should be first. > > This line declares a member variable window_to_probe using an anonymous enum; > it's not really an enum declaration. If I move the enum first, I have to name > it and create two lines where there was one before. If that's what you want, > I'll do it, but I want to just confirm that? Fair enough. Leave it. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... chrome/browser/ui/browser_close_browsertest.cc:244: IN_PROC_BROWSER_TEST_F(BrowserCloseTest, DISABLED_DownloadsCloseCheck) { On 2011/07/21 21:36:31, rdsmith wrote: > On 2011/07/21 20:08:11, sky wrote: > > How come you're checking in a test that's disabled? > > So if you'd like me to make that a separate CL, I'm willing; this is just trying > to write the automated testing for a change at the same time as I write the > change. The code to support that test (the code to create a new profile for > testing in multi-profile context, which would result in filling in > CreateNewProfile() above) isn't written yet, and I'm worried about getting my > change into MS-14 if I wait on it, so I just barreled ahead doing as much as I > could. > > Shall I split this portion of the change out? If so, is it ok if I leave the > refactor of DownloadTestObserver in? That's potentially useful for other > people, and it was finicky enough to do that I don't look forward to reverting > it and doing it again later. Without this test, it doesn't strictly speaking > have much to do with this CL, but it doesn't change functionality either--it's > just a refactor. > My vote goes for separate cl with test in it.
Next round. The test works! Sky, this means I'm planning to commit this as a unit, having enabled the test. Andy: There's some changes in this last patchset for you to take a look at. I removed a check in AssertQueueStateConsistent, and fixed a bug in DownloadTestFlushObserver. Dominic: PTAL? http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1084: || !browser->is_type_tabbed()) On 2011/07/22 03:00:40, sky wrote: > On 2011/07/21 21:36:31, rdsmith wrote: > > On 2011/07/21 20:08:11, sky wrote: > > > How come non-tabbed windows don't count? > > > > That's an excellent question and I don't know the answer; I was copying the > code > > and only modifying the parts I was clear on. What we're doing here is trying > to > > figure out ahead of time what will result in profile destruction (which means > > we're trying to keep separate code paths in sync, which is a losing battle, I > > just didn't see another way to do it). If the existence of a non-tabbed > windows > > won't keep the associated profile alive we should skip them, and if they do, > we > > shouldn't. What are the contexts in which we use non-tabbed windows, do you > > know? Does that summary spark any sense of why we might treat them specially? > > > > Popups are the common case of a non-tabbed browser, and it keeps chrome running. > App windows are the other case, but as common. Ok, sounds like I shouldn't exclude non-tabbed stuff then. Done. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1094: *type = BROWSER_SHUTDOWN; On 2011/07/22 03:00:40, sky wrote: > On 2011/07/21 21:36:31, rdsmith wrote: > > On 2011/07/21 20:08:11, sky wrote: > > > Since this case just means there are no more tabbed browsers, > BROWSER_SHUTDOWN > > > is misleading. How about something like > > DOWNLOAD_CLOSE_NO_MORE_TABBED_BROWSERS, > > > or something. > > > > Profiles are only killed on browser shutdown, so if non-tabbed windows keep > the > > browser alive, the bug's in the code above, not here. > > non-tabbed windows definitely keep chrome running. Done. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... File chrome/browser/ui/browser_close_browsertest.cc (right): http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... chrome/browser/ui/browser_close_browsertest.cc:134: enum { REGULAR = 0, INCOGNITO = 1 } window_to_probe; On 2011/07/22 03:00:40, sky wrote: > On 2011/07/21 21:36:31, rdsmith wrote: > > On 2011/07/21 20:08:11, sky wrote: > > > enums should be first. > > > > This line declares a member variable window_to_probe using an anonymous enum; > > it's not really an enum declaration. If I move the enum first, I have to name > > it and create two lines where there was one before. If that's what you want, > > I'll do it, but I want to just confirm that? > > Fair enough. Leave it. Done. http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser_cl... chrome/browser/ui/browser_close_browsertest.cc:244: IN_PROC_BROWSER_TEST_F(BrowserCloseTest, DISABLED_DownloadsCloseCheck) { On 2011/07/22 03:00:40, sky wrote: > On 2011/07/21 21:36:31, rdsmith wrote: > > On 2011/07/21 20:08:11, sky wrote: > > > How come you're checking in a test that's disabled? > > > > So if you'd like me to make that a separate CL, I'm willing; this is just > trying > > to write the automated testing for a change at the same time as I write the > > change. The code to support that test (the code to create a new profile for > > testing in multi-profile context, which would result in filling in > > CreateNewProfile() above) isn't written yet, and I'm worried about getting my > > change into MS-14 if I wait on it, so I just barreled ahead doing as much as I > > could. > > > > Shall I split this portion of the change out? If so, is it ok if I leave the > > refactor of DownloadTestObserver in? That's potentially useful for other > > people, and it was finicky enough to do that I don't look forward to reverting > > it and doing it again later. Without this test, it doesn't strictly speaking > > have much to do with this CL, but it doesn't change functionality either--it's > > just a refactor. > > > > My vote goes for separate cl with test in it. The postponement of multi-profile has made committing this CL less urgent; I'll try to get the auto-tests running before commit, and fall back to a split if I need to commit without them. http://codereview.chromium.org/7466033/diff/11001/chrome/browser/profiles/pro... File chrome/browser/profiles/profile.cc (right): http://codereview.chromium.org/7466033/diff/11001/chrome/browser/profiles/pro... chrome/browser/profiles/profile.cc:859: return GetDownloadManager()->in_progress_count(); On 2011/07/21 23:04:17, Miranda Callahan wrote: > A tiny change, but maybe 2 lines instead of 4 here: > > return HasCreatedDownloadManager() ? > GetDownloadManager()->in_progress_count() : 0; > Done. http://codereview.chromium.org/7466033/diff/11001/chrome/browser/ui/gtk/downl... File chrome/browser/ui/gtk/download/download_in_progress_dialog_gtk.cc (right): http://codereview.chromium.org/7466033/diff/11001/chrome/browser/ui/gtk/downl... chrome/browser/ui/gtk/download/download_in_progress_dialog_gtk.cc:25: &download_count); On 2011/07/21 22:59:48, Evan Stade wrote: > looks like this would fit on the line above Yeah, not sure where that came from. Done.
driving by again... :-) http://codereview.chromium.org/7466033/diff/15019/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7466033/diff/15019/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:1077: // Only consider tabbed browser windows, not popups. Comments need updating. 1058-1071 might also need updates too.
http://codereview.chromium.org/7466033/diff/15019/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7466033/diff/15019/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:1077: // Only consider tabbed browser windows, not popups. On 2011/07/22 21:02:40, jennb wrote: > Comments need updating. 1058-1071 might also need updates too. Whoops; done.
Much nicer. browser/ui LGTM
chrome/browser/download/*: LGTM
Asanka, could you take a look at the views/ code? Dominic doesn't appear to be around.
Asanka? JennB? I think I'm getting close so I'm wanting to get approvals in place (I need a clean try job run, which is taking a few iterations.)
LGTM http://codereview.chromium.org/7466033/diff/21001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7466033/diff/21001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:1075: // this is not the last regular browser window. As mentioned offline, a browser with regular downloads with an incognito window is allowed to close its last regular window.
LGTM Sorry, didn't think you needed one from me!
Sorry for the long hiatus; vacation and priority shifts. I have all LGTMs and think all I need to do is satisfy the trybots gods, but I wanted to give folks a chance to glance at my (somewhat extensive) merge to top of trunk if they wanted to. Asanka, if you'd be willing to just look at my comment rewrite in my latest patch set in response to your comment I'd be grateful; I struggled a bit with getting the wording both correct and clear.
I'm sticked with my LGTM On Tue, Aug 23, 2011 at 4:49 PM, <rdsmith@chromium.org> wrote: > Sorry for the long hiatus; vacation and priority shifts. I have all LGTMs > and > think all I need to do is satisfy the trybots gods, but I wanted to give > folks a > chance to glance at my (somewhat extensive) merge to top of trunk if they > wanted > to. > > Asanka, if you'd be willing to just look at my comment rewrite in my latest > patch set in response to your comment I'd be grateful; I struggled a bit > with > getting the wording both correct and clear. > > > http://codereview.chromium.org/7466033/ >
LGTM. Thanks!
Achuith: Willing to glance at the latest changes to browser_close_browsertest.cc for ChromeOS?
LGTM on the cros downloads panel code. A few nits as I took a quick look at the test code. http://codereview.chromium.org/7466033/diff/37001/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_close_browsertest.cc (right): http://codereview.chromium.org/7466033/diff/37001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:33: struct { Isn't struct profile_a {} more readable than struct {} profile_a? http://codereview.chromium.org/7466033/diff/37001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:96: true, // Bail on select file nits: 2 space between comment and code? period at end of comment? http://codereview.chromium.org/7466033/diff/37001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:161: for (int w = 1; w < num_windows; w++) { nit: ++w http://codereview.chromium.org/7466033/diff/37001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:181: #if 0 I'm assuming you're working on fixing this? http://codereview.chromium.org/7466033/diff/37001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:238: for (size_t i = 0; i < arraysize(browsers); i++) { nit: ++i http://codereview.chromium.org/7466033/diff/37001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:421: Profile* profile_a = browser()->profile(); Any reason to not pull out the boiler-plate in DownloadsCloseCheck_* into a separate function?
Below comments are in response to Achuith's comments, but I've let this CL lie fallow (i.e. rot) for long enough that merging it up to the TOT made a lot of changes. I'd like to ask for re-reviews from Miranda and Scott for chrome/browser/ui (both of you were involved originally, and I'm not sure what the right distribution of responsibility is). The specific areas of change I'm concerned about are: * Pulled DownloadCount() and TotalDownloadCount() from the profile related stuff and put it into DownloadService. Specific question: Is it ok to make TotalDownloadCount() a static on DownloadService? * Quite a bit of refactoring inside of browser_close_browsertest.cc. On the bright side, I think I've (finally!) gotten the mac code correct. (The basic problem on Mac is that browser windows don't go away promptly when you close them, so you need to be careful about stepping around the "in-process-of-closing" windows as you go poking through the system lists.) PTAL. http://codereview.chromium.org/7466033/diff/37001/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_close_browsertest.cc (right): http://codereview.chromium.org/7466033/diff/37001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:33: struct { On 2011/08/29 17:19:10, achuith.bhandarkar wrote: > Isn't struct profile_a {} more readable than struct {} profile_a? Sure, but it doesn't mean the same thing. In the first, profile_a is a type name. In the second it's the name of the member variable of the containing structure. http://codereview.chromium.org/7466033/diff/37001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:96: true, // Bail on select file On 2011/08/29 17:19:10, achuith.bhandarkar wrote: > nits: 2 space between comment and code? period at end of comment? Done. http://codereview.chromium.org/7466033/diff/37001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:161: for (int w = 1; w < num_windows; w++) { On 2011/08/29 17:19:10, achuith.bhandarkar wrote: > nit: ++w Done. http://codereview.chromium.org/7466033/diff/37001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:181: #if 0 On 2011/08/29 17:19:10, achuith.bhandarkar wrote: > I'm assuming you're working on fixing this? Yep. It's gone now. http://codereview.chromium.org/7466033/diff/37001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:238: for (size_t i = 0; i < arraysize(browsers); i++) { On 2011/08/29 17:19:10, achuith.bhandarkar wrote: > nit: ++i Done. http://codereview.chromium.org/7466033/diff/37001/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:421: Profile* profile_a = browser()->profile(); On 2011/08/29 17:19:10, achuith.bhandarkar wrote: > Any reason to not pull out the boiler-plate in DownloadsCloseCheck_* into a > separate function? Nope; hadn't gotten to it. Done.
SLGTM http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:1079: browser->is_attempting_to_close_browser_) nit: move this up to the previous line. http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/views/dow... File chrome/browser/ui/views/download/download_in_progress_dialog_view.cc (right): http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/views/dow... chrome/browser/ui/views/download/download_in_progress_dialog_view.cc:37: DCHECK(type != Browser::DOWNLOAD_CLOSE_OK); DCHECK_NE
Sorry for the late comments. Almost all are nits/minor. The only real concern I have is the one @ browser.cc:1079 http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.cc (right): http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:16: void AcceptDangerousDownload(scoped_refptr<DownloadManager> download_manager, scoped_refptr because this function can get called after DownloadTestObserver goes away? I know the header file has some comment to that effect, but you should have a comment here. Ideally, I think these functions should be methods of DownloadTestObserver, which should be RefCountedThreadSafe, like the DownloadTestFlushObserver. You could then use a scoped_refptr instead of a scoped_ptr in BrowserCloseTest http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:69: >= wait_count_) >= should be on previous line. http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:239: std::vector<DownloadItem*>::iterator it = downloads.begin(); You do this in a few other places; I think it's a bit irregular. The scope of 'it' now unnecessarily extends past the end of the for loop. I think you should go with for (DownloadManager::DownloadVector::iterator it = downloads.begin(); it != downloads.end(); ++it) { etc http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.h (right): http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:55: bool select_file_dialog_seen() { return select_file_dialog_seen_; } const http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:61: bool IsFinished(); const http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:64: virtual void OnDownloadUpdated(DownloadItem* download); OVERRIDE here and for the other 4 virtual methods in this class http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:89: std::set<DownloadItem*> finished_downloads_; You use this particular set many times; a concise typedef (like DownloadSet) would be nice. http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... chrome/browser/download/download_test_observer.h:149: virtual void ModelChanged(); OVERRIDE here and other 2 functions in this class http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:1045: I'd set *num_downloads_blocking to 0 here since an uninitialized value is and could be passed in. http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:1057: Have you considered combining the above 3 conditions into 1 statement? Also, total_download_count seems unnecessary to me. http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:1072: int profile_window_count = 0; Does it make sense to move lines 1072-1085 into a separate function? Maybe in BrowserList? I feel like this function is rather long making it difficult to follow. http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:1079: browser->is_attempting_to_close_browser_) You lost the condition to guard against counting popups/non-tabbed browser windows. Was that intentional? http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4924: if (cancel_download_confirmation_state_ != NOT_PROMPTED) { I know this is not your code, but do you think you could change this to: if (cancel_download_confirmation_state_ != NOT_PROMPTED) return cancel_download_confirmation_state_ != WAITING_FOR_RESPONSE; It would make this more readable and concise. http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_close_browsertest.cc (right): http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:72: void SetUpOnMainThread() OVERRIDE { virtual? http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:198: LOG(WARNING) << "TotalUnclosedBrowsers: counted browser " << (*iter); Is this logging supposed to be here? http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_c... chrome/browser/ui/browser_close_browsertest.cc:235: // Debugging hack to make it easy to run a single case from the Are you checking this in? http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_l... File chrome/browser/ui/browser_list.cc (right): http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_l... chrome/browser/ui/browser_list.cc:192: if (browser_it == BrowserList::end()) Is this condition necessary? I believe the code should drop through the condition below and the for loop and return true at the bottom of this function. http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/gtk/downl... File chrome/browser/ui/gtk/download/download_in_progress_dialog_gtk.cc (right): http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/gtk/downl... chrome/browser/ui/gtk/download/download_in_progress_dialog_gtk.cc:31: DCHECK(type != Browser::DOWNLOAD_CLOSE_OK); DCHECK_NE
Incorporated comments from Achuith and Scott.
Miranda/Scott: Achuith's asked me to make a minor improvement for readability in
browser.cc, but I'm reluctant to do so without an owner's eye passing over it.
Search for "readable and concise" in the comments below and just tell me you
don't mind :-}.
Pawel, I've done a hack in browser_close_browsertest.cc I'd like your opinion on
as a test guru--I'm parsing an extra argument ("--test_argument") to allow me to
define which cases I want to run. I found that useful enough in debugging this
test that I'd like to leave it in, but I don't know if that's Chromely. WDYT?
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow...
File chrome/browser/download/download_test_observer.cc (right):
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow...
chrome/browser/download/download_test_observer.cc:16: void
AcceptDangerousDownload(scoped_refptr<DownloadManager> download_manager,
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> scoped_refptr because this function can get called after DownloadTestObserver
> goes away? I know the header file has some comment to that effect, but you
> should have a comment here.
Done.
> Ideally, I think these functions should be methods of DownloadTestObserver,
> which should be RefCountedThreadSafe, like the DownloadTestFlushObserver. You
> could then use a scoped_refptr instead of a scoped_ptr in BrowserCloseTest
Well, they're doing two different things. DownloadTestFlushObserver is
ping-ponging methods of the class over to a whole lot of different threads and
then rendez-vousing back with itself. Accept/DenyDangerousDownload is faking a
UI action from the user on the download system itself; it never rendezvous with
DownloadTestObserver. So a) I'd rather not keep DownloadTestObserver around,
and b) though I'm not as fanatical about it as I should be, I don't want to
create more RefCountedThreadSafe classes; they're usually a bad idea. I won't
worry about it too much for tests, but I don't want to do it unless there's some
good reason.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow...
chrome/browser/download/download_test_observer.cc:69: >= wait_count_)
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> >= should be on previous line.
Done.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow...
chrome/browser/download/download_test_observer.cc:239:
std::vector<DownloadItem*>::iterator it = downloads.begin();
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> You do this in a few other places; I think it's a bit irregular. The scope of
> 'it' now unnecessarily extends past the end of the for loop. I think you
should
> go with
> for (DownloadManager::DownloadVector::iterator it = downloads.begin();
> it != downloads.end(); ++it) {
> etc
Huh. Interesting. I agree with you--I wonder why I did that? Done.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow...
File chrome/browser/download/download_test_observer.h (right):
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow...
chrome/browser/download/download_test_observer.h:55: bool
select_file_dialog_seen() { return select_file_dialog_seen_; }
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> const
Done.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow...
chrome/browser/download/download_test_observer.h:61: bool IsFinished();
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> const
Done.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow...
chrome/browser/download/download_test_observer.h:64: virtual void
OnDownloadUpdated(DownloadItem* download);
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> OVERRIDE here and for the other 4 virtual methods in this class
Done for the other three; NumDangerousDownloadsSeen wasn't overriding anything
and is not longer a virtual.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow...
chrome/browser/download/download_test_observer.h:89: std::set<DownloadItem*>
finished_downloads_;
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> You use this particular set many times; a concise typedef (like DownloadSet)
> would be nice.
Done.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow...
chrome/browser/download/download_test_observer.h:149: virtual void
ModelChanged();
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> OVERRIDE here and other 2 functions in this class
Done.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1045:
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> I'd set *num_downloads_blocking to 0 here since an uninitialized value is and
> could be passed in.
Done.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1057:
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> Have you considered combining the above 3 conditions into 1 statement?
My preference is for it being the way it is now; I think it makes the logic
clearer. (And I had enough headaches trying to get the logic clear in my head
for this routine, albeit for the stuff below, that I don't want to make it less
clear intentionally :-}).
> Also, total_download_count seems unnecessary to me.
Syntactically or semantically? If syntactically, it's used below. If
semantically...well, I need to check it at some point in the code. I could
avoid the check if there are any windows around, but I can avoid the window
check if there aren't any downloads around. It seems like six of one, half a
dozen of the other. Do you see a reason to only computer the total download
count if we're at the last window, as opposed to doing it the other way?
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1072: int profile_window_count = 0;
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> Does it make sense to move lines 1072-1085 into a separate function? Maybe in
> BrowserList? I feel like this function is rather long making it difficult to
> follow.
Huh. I don't have that feeling about it. The *logic* of the function is
convoluted, I agree, but I don't actually think subroutinizing it would help
(I'd love to come up with better comment phrasing, though). But the function
doesn't seem that long to me--only 64 lines, 14 of which are a big old comment
in the middle.
I've played with the comments (eliminated the big one, actually); see if you're
more comfortable with the routine now.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1079: browser->is_attempting_to_close_browser_)
On 2011/10/13 20:23:43, sky wrote:
> nit: move this up to the previous line.
Done.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1079: browser->is_attempting_to_close_browser_)
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> You lost the condition to guard against counting popups/non-tabbed browser
> windows. Was that intentional?
Yes. See earlier comment on this CL @
http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc...
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4924: if (cancel_download_confirmation_state_ !=
NOT_PROMPTED) {
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> I know this is not your code, but do you think you could change this to:
>
> if (cancel_download_confirmation_state_ != NOT_PROMPTED)
> return cancel_download_confirmation_state_ != WAITING_FOR_RESPONSE;
>
> It would make this more readable and concise.
I agree with you, but I'm reluctant to make a change to code I don't own, when
also requested by a non-owner, in an unrelated CL. Miranda, Scott: If either of
you want to bless this request, I'm happy to do it.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_c...
File chrome/browser/ui/browser_close_browsertest.cc (right):
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_close_browsertest.cc:72: void SetUpOnMainThread()
OVERRIDE {
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> virtual?
Done.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_close_browsertest.cc:198: LOG(WARNING) <<
"TotalUnclosedBrowsers: counted browser " << (*iter);
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> Is this logging supposed to be here?
Oh, goodness. No, it's not; I thought I had gotten rid of all that. Done.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_close_browsertest.cc:235: // Debugging hack to make it
easy to run a single case from the
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> Are you checking this in?
Right, thanks for pointing this out; I wanted to get an opinion from a test
maven as to whether this was an ok thing to include or not. It made debugging
the test failure much easier, so if it's Chromely I'd like to leave it in. I'll
pull Pawel in for an opinion on this round.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_l...
File chrome/browser/ui/browser_list.cc (right):
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_list.cc:192: if (browser_it == BrowserList::end())
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> Is this condition necessary? I believe the code should drop through the
> condition below and the for loop and return true at the bottom of this
function.
It's pedantic, but I don't think so. If there aren't any browsers, but there
*are* downloads, removing the test would be a semantic change. And what's
currently there matches the semantics of the original routine. There can be no
browsers if Chrome is in service mode, and while I don't like canceling the
downloads in that case, I haven't thought of a better solution (and I don't
think it'd necessarily belong in this CL if I had).
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/gtk/downl...
File chrome/browser/ui/gtk/download/download_in_progress_dialog_gtk.cc (right):
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/gtk/downl...
chrome/browser/ui/gtk/download/download_in_progress_dialog_gtk.cc:31:
DCHECK(type != Browser::DOWNLOAD_CLOSE_OK);
On 2011/10/13 23:05:08, achuith.bhandarkar wrote:
> DCHECK_NE
Done.
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/views/dow...
File chrome/browser/ui/views/download/download_in_progress_dialog_view.cc
(right):
http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/views/dow...
chrome/browser/ui/views/download/download_in_progress_dialog_view.cc:37:
DCHECK(type != Browser::DOWNLOAD_CLOSE_OK);
On 2011/10/13 20:23:43, sky wrote:
> DCHECK_NE
Done.
The 'readable and concise' changes LGTM -Scott On Thu, Oct 13, 2011 at 5:37 PM, <rdsmith@chromium.org> wrote: > Incorporated comments from Achuith and Scott. > > Miranda/Scott: Achuith's asked me to make a minor improvement for > readability in > browser.cc, but I'm reluctant to do so without an owner's eye passing over > it. > Search for "readable and concise" in the comments below and just tell me you > don't mind :-}. > > Pawel, I've done a hack in browser_close_browsertest.cc I'd like your > opinion on > as a test guru--I'm parsing an extra argument ("--test_argument") to allow > me to > define which cases I want to run. I found that useful enough in debugging > this > test that I'd like to leave it in, but I don't know if that's Chromely. > WDYT? > > > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... > File chrome/browser/download/download_test_observer.cc (right): > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... > chrome/browser/download/download_test_observer.cc:16: void > AcceptDangerousDownload(scoped_refptr<DownloadManager> download_manager, > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> scoped_refptr because this function can get called after > > DownloadTestObserver >> >> goes away? I know the header file has some comment to that effect, but > > you >> >> should have a comment here. > > Done. > >> Ideally, I think these functions should be methods of > > DownloadTestObserver, >> >> which should be RefCountedThreadSafe, like the > > DownloadTestFlushObserver. You >> >> could then use a scoped_refptr instead of a scoped_ptr in > > BrowserCloseTest > > Well, they're doing two different things. DownloadTestFlushObserver is > ping-ponging methods of the class over to a whole lot of different > threads and then rendez-vousing back with itself. > Accept/DenyDangerousDownload is faking a UI action from the user on the > download system itself; it never rendezvous with DownloadTestObserver. > So a) I'd rather not keep DownloadTestObserver around, and b) though I'm > not as fanatical about it as I should be, I don't want to create more > RefCountedThreadSafe classes; they're usually a bad idea. I won't worry > about it too much for tests, but I don't want to do it unless there's > some good reason. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... > chrome/browser/download/download_test_observer.cc:69: >= wait_count_) > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> >= should be on previous line. > > Done. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... > chrome/browser/download/download_test_observer.cc:239: > std::vector<DownloadItem*>::iterator it = downloads.begin(); > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> You do this in a few other places; I think it's a bit irregular. The > > scope of >> >> 'it' now unnecessarily extends past the end of the for loop. I think > > you should >> >> go with >> for (DownloadManager::DownloadVector::iterator it = downloads.begin(); >> it != downloads.end(); ++it) { >> etc > > Huh. Interesting. I agree with you--I wonder why I did that? Done. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... > File chrome/browser/download/download_test_observer.h (right): > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... > chrome/browser/download/download_test_observer.h:55: bool > select_file_dialog_seen() { return select_file_dialog_seen_; } > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> const > > Done. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... > chrome/browser/download/download_test_observer.h:61: bool IsFinished(); > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> const > > Done. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... > chrome/browser/download/download_test_observer.h:64: virtual void > OnDownloadUpdated(DownloadItem* download); > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> OVERRIDE here and for the other 4 virtual methods in this class > > Done for the other three; NumDangerousDownloadsSeen wasn't overriding > anything and is not longer a virtual. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... > chrome/browser/download/download_test_observer.h:89: > std::set<DownloadItem*> finished_downloads_; > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> You use this particular set many times; a concise typedef (like > > DownloadSet) >> >> would be nice. > > Done. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... > chrome/browser/download/download_test_observer.h:149: virtual void > ModelChanged(); > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> OVERRIDE here and other 2 functions in this class > > Done. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.cc > File chrome/browser/ui/browser.cc (right): > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c... > chrome/browser/ui/browser.cc:1045: > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> I'd set *num_downloads_blocking to 0 here since an uninitialized value > > is and >> >> could be passed in. > > Done. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c... > chrome/browser/ui/browser.cc:1057: > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> Have you considered combining the above 3 conditions into 1 statement? > > My preference is for it being the way it is now; I think it makes the > logic clearer. (And I had enough headaches trying to get the logic > clear in my head for this routine, albeit for the stuff below, that I > don't want to make it less clear intentionally :-}). > >> Also, total_download_count seems unnecessary to me. > > Syntactically or semantically? If syntactically, it's used below. If > semantically...well, I need to check it at some point in the code. I > could avoid the check if there are any windows around, but I can avoid > the window check if there aren't any downloads around. It seems like > six of one, half a dozen of the other. Do you see a reason to only > computer the total download count if we're at the last window, as > opposed to doing it the other way? > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c... > chrome/browser/ui/browser.cc:1072: int profile_window_count = 0; > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> Does it make sense to move lines 1072-1085 into a separate function? > > Maybe in >> >> BrowserList? I feel like this function is rather long making it > > difficult to >> >> follow. > > Huh. I don't have that feeling about it. The *logic* of the function > is convoluted, I agree, but I don't actually think subroutinizing it > would help (I'd love to come up with better comment phrasing, though). > But the function doesn't seem that long to me--only 64 lines, 14 of > which are a big old comment in the middle. > > I've played with the comments (eliminated the big one, actually); see if > you're more comfortable with the routine now. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c... > chrome/browser/ui/browser.cc:1079: > browser->is_attempting_to_close_browser_) > On 2011/10/13 20:23:43, sky wrote: >> >> nit: move this up to the previous line. > > Done. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c... > chrome/browser/ui/browser.cc:1079: > browser->is_attempting_to_close_browser_) > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> You lost the condition to guard against counting popups/non-tabbed > > browser >> >> windows. Was that intentional? > > Yes. See earlier comment on this CL @ > http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc... > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c... > chrome/browser/ui/browser.cc:4924: if > (cancel_download_confirmation_state_ != NOT_PROMPTED) { > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> I know this is not your code, but do you think you could change this > > to: > >> if (cancel_download_confirmation_state_ != NOT_PROMPTED) >> return cancel_download_confirmation_state_ != WAITING_FOR_RESPONSE; > >> It would make this more readable and concise. > > I agree with you, but I'm reluctant to make a change to code I don't > own, when also requested by a non-owner, in an unrelated CL. Miranda, > Scott: If either of you want to bless this request, I'm happy to do it. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_c... > File chrome/browser/ui/browser_close_browsertest.cc (right): > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_c... > chrome/browser/ui/browser_close_browsertest.cc:72: void > SetUpOnMainThread() OVERRIDE { > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> virtual? > > Done. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_c... > chrome/browser/ui/browser_close_browsertest.cc:198: LOG(WARNING) << > "TotalUnclosedBrowsers: counted browser " << (*iter); > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> Is this logging supposed to be here? > > Oh, goodness. No, it's not; I thought I had gotten rid of all that. > Done. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_c... > chrome/browser/ui/browser_close_browsertest.cc:235: // Debugging hack to > make it easy to run a single case from the > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> Are you checking this in? > > Right, thanks for pointing this out; I wanted to get an opinion from a > test maven as to whether this was an ok thing to include or not. It > made debugging the test failure much easier, so if it's Chromely I'd > like to leave it in. I'll pull Pawel in for an opinion on this round. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_l... > File chrome/browser/ui/browser_list.cc (right): > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_l... > chrome/browser/ui/browser_list.cc:192: if (browser_it == > BrowserList::end()) > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> Is this condition necessary? I believe the code should drop through > > the >> >> condition below and the for loop and return true at the bottom of this > > function. > > It's pedantic, but I don't think so. If there aren't any browsers, but > there *are* downloads, removing the test would be a semantic change. > And what's currently there matches the semantics of the original > routine. There can be no browsers if Chrome is in service mode, and > while I don't like canceling the downloads in that case, I haven't > thought of a better solution (and I don't think it'd necessarily belong > in this CL if I had). > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/gtk/downl... > File chrome/browser/ui/gtk/download/download_in_progress_dialog_gtk.cc > (right): > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/gtk/downl... > chrome/browser/ui/gtk/download/download_in_progress_dialog_gtk.cc:31: > DCHECK(type != Browser::DOWNLOAD_CLOSE_OK); > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: >> >> DCHECK_NE > > Done. > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/views/dow... > File > chrome/browser/ui/views/download/download_in_progress_dialog_view.cc > (right): > > http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/views/dow... > chrome/browser/ui/views/download/download_in_progress_dialog_view.cc:37: > DCHECK(type != Browser::DOWNLOAD_CLOSE_OK); > On 2011/10/13 20:23:43, sky wrote: >> >> DCHECK_NE > > Done. > > http://codereview.chromium.org/7466033/ >
LGTM as well -- just caught a nitlet noted below... http://codereview.chromium.org/7466033/diff/67002/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7466033/diff/67002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:43: #include "chrome/browser/download/download_service.h" whoops -- this is already here. :-)
Incorporated last little comments. Waiting for final LGTM from Achuith and opinion from Pawel. If I don't hear from Pawel by 3pm ET (12pm PT) I'll yank the questionable code and commit without his vote. http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4924: if (cancel_download_confirmation_state_ != NOT_PROMPTED) { On 2011/10/14 00:37:41, rdsmith wrote: > On 2011/10/13 23:05:08, achuith.bhandarkar wrote: > > I know this is not your code, but do you think you could change this to: > > > > if (cancel_download_confirmation_state_ != NOT_PROMPTED) > > return cancel_download_confirmation_state_ != WAITING_FOR_RESPONSE; > > > > It would make this more readable and concise. > > I agree with you, but I'm reluctant to make a change to code I don't own, when > also requested by a non-owner, in an unrelated CL. Miranda, Scott: If either of > you want to bless this request, I'm happy to do it. Done. http://codereview.chromium.org/7466033/diff/67002/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7466033/diff/67002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:43: #include "chrome/browser/download/download_service.h" On 2011/10/14 12:55:43, Miranda Callahan wrote: > whoops -- this is already here. :-) Ooops. Done.
LGTM with 1 minor nit and 1 suggestion. Thanks for incorporating the comments - it's looks good! http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... File chrome/browser/download/download_test_observer.cc (right): http://codereview.chromium.org/7466033/diff/61002/chrome/browser/download/dow... chrome/browser/download/download_test_observer.cc:16: void AcceptDangerousDownload(scoped_refptr<DownloadManager> download_manager, > Well, they're doing two different things. DownloadTestFlushObserver is > ping-ponging methods of the class over to a whole lot of different threads and > then rendez-vousing back with itself. Accept/DenyDangerousDownload is faking a > UI action from the user on the download system itself; it never rendezvous with > DownloadTestObserver. So a) I'd rather not keep DownloadTestObserver around, > and b) though I'm not as fanatical about it as I should be, I don't want to > create more RefCountedThreadSafe classes; they're usually a bad idea. I won't > worry about it too much for tests, but I don't want to do it unless there's some > good reason. > SGTM. http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:1057: > My preference is for it being the way it is now; I think it makes the logic > clearer. (And I had enough headaches trying to get the logic clear in my head > for this routine, albeit for the stuff below, that I don't want to make it less > clear intentionally :-}). That's fine. > > Also, total_download_count seems unnecessary to me. > > Syntactically or semantically? If syntactically, it's used below. If > semantically...well, I need to check it at some point in the code. I could > avoid the check if there are any windows around, but I can avoid the window > check if there aren't any downloads around. It seems like six of one, half a > dozen of the other. Do you see a reason to only computer the total download > count if we're at the last window, as opposed to doing it the other way? > I was wrong - I didn't see it being used below. Sorry for the confusion. http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:1072: int profile_window_count = 0; > > Huh. I don't have that feeling about it. The *logic* of the function is > convoluted, I agree, but I don't actually think subroutinizing it would help > (I'd love to come up with better comment phrasing, though). But the function > doesn't seem that long to me--only 64 lines, 14 of which are a big old comment > in the middle. > > I've played with the comments (eliminated the big one, actually); see if you're > more comfortable with the routine now. Yup, definitely better since it fits on my 15" screen now. http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_l... File chrome/browser/ui/browser_list.cc (right): http://codereview.chromium.org/7466033/diff/61002/chrome/browser/ui/browser_l... chrome/browser/ui/browser_list.cc:192: if (browser_it == BrowserList::end()) > It's pedantic, but I don't think so. If there aren't any browsers, but there > *are* downloads, removing the test would be a semantic change. And what's > currently there matches the semantics of the original routine. There can be no > browsers if Chrome is in service mode, and while I don't like canceling the > downloads in that case, I haven't thought of a better solution (and I don't > think it'd necessarily belong in this CL if I had). Ok. http://codereview.chromium.org/7466033/diff/69001/chrome/browser/download/dow... File chrome/browser/download/download_service.cc (right): http://codereview.chromium.org/7466033/diff/69001/chrome/browser/download/dow... chrome/browser/download/download_service.cc:43: return manager_->in_progress_count(); nit: this is fine, but have you considered return download_manager_created_ ? manager_->in_progress_count() : 0; http://codereview.chromium.org/7466033/diff/69001/chrome/browser/download/dow... File chrome/browser/download/download_service.h (right): http://codereview.chromium.org/7466033/diff/69001/chrome/browser/download/dow... chrome/browser/download/download_service.h:33: int DownloadCount(); nit: const
Thanks, Achuith! Comments all incorporated. I consider myself good WRT reviews at this point, but I've seen a single example of a chromeos failure on these tests (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...) which I think I should track down before I commit. I'm going to try to reproduce locally. So this may or may not get in for MS-16. http://codereview.chromium.org/7466033/diff/69001/chrome/browser/download/dow... File chrome/browser/download/download_service.cc (right): http://codereview.chromium.org/7466033/diff/69001/chrome/browser/download/dow... chrome/browser/download/download_service.cc:43: return manager_->in_progress_count(); On 2011/10/14 19:22:10, achuith.bhandarkar wrote: > nit: this is fine, but have you considered > return download_manager_created_ ? manager_->in_progress_count() : 0; Done. http://codereview.chromium.org/7466033/diff/69001/chrome/browser/download/dow... File chrome/browser/download/download_service.h (right): http://codereview.chromium.org/7466033/diff/69001/chrome/browser/download/dow... chrome/browser/download/download_service.h:33: int DownloadCount(); On 2011/10/14 19:22:10, achuith.bhandarkar wrote: > nit: const Done.
On 2011/10/14 19:30:47, rdsmith wrote: > Thanks, Achuith! Comments all incorporated. I consider myself good WRT reviews > at this point, but I've seen a single example of a chromeos failure on these > tests > (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...) > which I think I should track down before I commit. I'm going to try to > reproduce locally. So this may or may not get in for MS-16. > That does look ominous. Log doesn't say much unfortunately.
http://codereview.chromium.org/7466033/diff/69001/chrome/browser/download/dow... File chrome/browser/download/download_service.h (right): http://codereview.chromium.org/7466033/diff/69001/chrome/browser/download/dow... chrome/browser/download/download_service.h:36: static int TotalDownloadCount(); nit: Unless there is an existing convention, I'd like the name to explicitly mention all profiles. "Total" might be a little bit vague.
On 2011/10/14 19:55:26, achuith.bhandarkar wrote: > On 2011/10/14 19:30:47, rdsmith wrote: > > Thanks, Achuith! Comments all incorporated. I consider myself good WRT > reviews > > at this point, but I've seen a single example of a chromeos failure on these > > tests > > > (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...) > > which I think I should track down before I commit. I'm going to try to > > reproduce locally. So this may or may not get in for MS-16. > > > > That does look ominous. Log doesn't say much unfortunately. Yeah, and I failed to reproduce locally with a --gtest_repeat=10 :-{. Analyzing the logs in a bit more detail: * The failure was a 45s timeout; the random error output shows about 40s worth of time. * In the same run, DownloadsCloseCheck_0 took a bit under 10s, and DownloadsCloseCheck_2 took 40s but finished successfully. * In my local runs, all three tests took 20-30s; 0 was usually around 20, and 1&2 were usually 25-30s. * In my local runs there was about 55 lines of random error output; in the failing test on the bots, there is 51. This looks to me like "something happened" on the bots to slow things down between DownloadsCloseCheck_0 and DownloadsCloseCheck_1 (since 0 was so quick and 1&2 were so slow), and that DownloadsCloseCheck_1 just ran long. On that basis I'll split the three tests into six tests, and check in (after I incorporate Pawel's comment on naming). Let me know if this sounds bad to you, Achuith.
Thinking out of the box, what do you think of:
#define DOWNLOADS_CLOSE_CHECK(n) \
IN_PROC_BROWSER_TEST_F(BrowserCloseTest, DownloadsCloseCheck_##n) {\
ExecuteDownloadCloseCheckCase(n);\
}
DOWNLOADS_CLOSE_CHECK(0)
DOWNLOADS_CLOSE_CHECK(1)
etc.
DOWNLOADS_CLOSE_CHECK(50)
#undef DOWNLOADS_CLOSE_CHECK
It uses a local macro and may be more lines overall, but I think it would be
more readable and would be at less risk of running into the time limit.
Otherwise, what you suggest is fine.
On 2011/10/14 21:59:50, achuith.bhandarkar wrote:
> Thinking out of the box, what do you think of:
>
> #define DOWNLOADS_CLOSE_CHECK(n) \
> IN_PROC_BROWSER_TEST_F(BrowserCloseTest, DownloadsCloseCheck_##n) {\
> ExecuteDownloadCloseCheckCase(n);\
> }
>
> DOWNLOADS_CLOSE_CHECK(0)
> DOWNLOADS_CLOSE_CHECK(1)
> etc.
> DOWNLOADS_CLOSE_CHECK(50)
>
> #undef DOWNLOADS_CLOSE_CHECK
>
> It uses a local macro and may be more lines overall, but I think it would be
> more readable and would be at less risk of running into the time limit.
>
> Otherwise, what you suggest is fine.
I thought about doing something like that on and off as I was writing this test,
and concluded that the number of tests didn't push me over my dislike of macros.
It also has disadvantages: a) It takes more time because you're paying the
browser startup and shutdown costs more often, and b) it makes it easier to add
a test case and not have it run. So my inclination is to keep what I have,
though the more individual tests I have to break things out into, the weaker the
inclination gets :-}.
http://codereview.chromium.org/7466033/diff/69001/chrome/browser/download/dow... File chrome/browser/download/download_service.h (right): http://codereview.chromium.org/7466033/diff/69001/chrome/browser/download/dow... chrome/browser/download/download_service.h:36: static int TotalDownloadCount(); On 2011/10/14 21:26:19, Paweł Hajdan Jr. wrote: > nit: Unless there is an existing convention, I'd like the name to explicitly > mention all profiles. "Total" might be a little bit vague. Done.
Requesting re-review from ahendrickson for changes in last patchset (browser_close_browertest.cc only). Background: I committed this last week, and babysat the tree until all tests returned green. About two days later, the tests started failing repeatedly, and eventually the sheriffs got sick of disabling tests and reverted the entire CL. When I dug into the problem, it turned out to be that I was leaving files in the ~/Downloads directory, and after I left 100 of those files, chrome started prompting me on the downloads. That had different results on different platforms; on linux it resulted in a test failure, and on mac it resulted in a timeout. Same root cause, though. Fix is to associate a temporary, deleted on exit directory with downloads for all profiles. Test is to ls ~/Downloads, run the test once, and do another ls and make sure that there aren't any turds left there. I'll (re)commit with Andy's review and a clean try bot run.
browser_close_browertest.cc LGTM. Andy On 2011/10/20 22:28:10, rdsmith wrote: > Requesting re-review from ahendrickson for changes in last patchset > (browser_close_browertest.cc only). > > Background: I committed this last week, and babysat the tree until all tests > returned green. About two days later, the tests started failing repeatedly, and > eventually the sheriffs got sick of disabling tests and reverted the entire CL. > When I dug into the problem, it turned out to be that I was leaving files in the > ~/Downloads directory, and after I left 100 of those files, chrome started > prompting me on the downloads. That had different results on different > platforms; on linux it resulted in a test failure, and on mac it resulted in a > timeout. Same root cause, though. > > Fix is to associate a temporary, deleted on exit directory with downloads for > all profiles. Test is to ls ~/Downloads, run the test once, and do another ls > and make sure that there aren't any turds left there. > > I'll (re)commit with Andy's review and a clean try bot run.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/7466033/89001
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is win_rel, revision is 106756, job name was 7466033-89001 (retry) (retry) (retry) (retry).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/7466033/89001
Change committed as 106958 |
