Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9355050:
Added read-only file error test. (Closed)
|
Created:
8 years, 10 months ago by ahendrickson Modified:
8 years, 9 months ago CC:
chromium-reviews, rdsmith+dwatch_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded read-only file error test.
Depends on CLs 9568003 and 9570005.
BUG=None
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126541
Patch Set 1 #Patch Set 2 : Fixed merge errors. #Patch Set 3 : Merged with parent #Patch Set 4 : Merged with parent #Patch Set 5 : Fixed posix issues #Patch Set 6 : Merged with parent, fixed another posix error. #Patch Set 7 : Fixed some issues & consolidated code #
Total comments: 20
Patch Set 8 : Merged with parent #Patch Set 9 : Added comment. #
Total comments: 2
Patch Set 10 : Merged with trunk #Patch Set 11 : Created file/folder permissions restoring class #Patch Set 12 : Checking for download redirection to Documents folder. #
Total comments: 20
Patch Set 13 : Created DownloadId default constructor. Cleaned up per Chris' comments. #
Total comments: 4
Patch Set 14 : Minor changes. #Patch Set 15 : Merged with parent. #Patch Set 16 : Merged with parent. #Patch Set 17 : Merged with parent. #
Total comments: 1
Patch Set 18 : Merged with trunk. #Patch Set 19 : Merged with trunk. #
Messages
Total messages: 24 (0 generated)
PTAL
You should find someone who counts as an owner for base/test; I wouldn't think Chris would count, and I'm sure I don't. I'll review download_browsertest.cc
First pass. http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:693: NullSelectFile(browser()); // Needed for read-only tests. This makes me uncomfortable, as it changes the behavior of a helper routine (DownloadFileCheckErrors) in a pretty important way. I can't actually imagine how it could be a problem for these tests, but it's worthwhile calling out in the description of what this routine does that it'll inhibit the select file dialog. http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:2064: IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorReadonlyFolderDirect) { Shouldn't both of these tests actively confirm that the file went to the My Documents folder? It doesn't look like they do.
Pawel, please take a look at the changes in base/test. I added code that records the permissions of a file/folder, and other code that restores it. http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:693: NullSelectFile(browser()); // Needed for read-only tests. On 2012/02/24 19:16:15, rdsmith wrote: > This makes me uncomfortable, as it changes the behavior of a helper routine > (DownloadFileCheckErrors) in a pretty important way. I can't actually imagine > how it could be a problem for these tests, but it's worthwhile calling out in > the description of what this routine does that it'll inhibit the select file > dialog. This makes the behavior what I wanted it to be in the first place -- if a select file dialog appears, choose the default. Combined with changing the "Bail on select file" parameter below to false, the download goes to completion/interruption. I'll add the comment. http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:2064: IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorReadonlyFolderDirect) { On 2012/02/24 19:16:15, rdsmith wrote: > Shouldn't both of these tests actively confirm that the file went to the My > Documents folder? It doesn't look like they do. They do, in line 751.
http://codereview.chromium.org/9355050/diff/21010/base/test/test_file_util.h File base/test/test_file_util.h (right): http://codereview.chromium.org/9355050/diff/21010/base/test/test_file_util.h#... base/test/test_file_util.h:62: void* GetPermissionInfo(const FilePath& path, size_t* length); I don't like this interface. How about a scoped object instead?
http://codereview.chromium.org/9355050/diff/21010/base/test/test_file_util.h File base/test/test_file_util.h (right): http://codereview.chromium.org/9355050/diff/21010/base/test/test_file_util.h#... base/test/test_file_util.h:62: void* GetPermissionInfo(const FilePath& path, size_t* length); On 2012/02/27 09:09:25, Paweł Hajdan Jr. wrote: > I don't like this interface. How about a scoped object instead? Done.
http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:693: NullSelectFile(browser()); // Needed for read-only tests. On 2012/02/25 01:05:17, ahendrickson wrote: > On 2012/02/24 19:16:15, rdsmith wrote: > > This makes me uncomfortable, as it changes the behavior of a helper routine > > (DownloadFileCheckErrors) in a pretty important way. I can't actually imagine > > how it could be a problem for these tests, but it's worthwhile calling out in > > the description of what this routine does that it'll inhibit the select file > > dialog. > > This makes the behavior what I wanted it to be in the first place -- if a select > file dialog appears, choose the default. Combined with changing the "Bail on > select file" parameter below to false, the download goes to > completion/interruption. > > I'll add the comment. I don't see the comment; did you mean you'd add it later? (Fine if so; I'm just used to responses coming back with the code changes they refer to.) http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:723: observer->WaitForFinished(); If show_download_item isn't set, doesn't this mean we don't wait? And won't that produce races/flakiness if we get to the tests below before the download has really started? Unless I'm missing something, good race hygiene requires you find something to wait on so that whatever has happened is going to happen (ideally not something that presupposes that the result you're testing for actually has happened.) http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:727: GetDownloads(browser(), &download_items); This won't see downloads before they're put into the history; will that be a problem for any of the !show_download_item cases? http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:737: if (item->GetLastReason() == DOWNLOAD_INTERRUPT_REASON_NONE) { What does this test mean? That we weren't interrupted? Isn't that a bit more clear if you just check item status? http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:2064: IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorReadonlyFolderDirect) { On 2012/02/25 01:05:17, ahendrickson wrote: > On 2012/02/24 19:16:15, rdsmith wrote: > > Shouldn't both of these tests actively confirm that the file went to the My > > Documents folder? It doesn't look like they do. > > They do, in line 751. That test (presuming I'm reading it correctly) is "If it isn't in the Download folder, make sure it's in the My Documents folder". What I'm asking for is "If we're downloading to a read only folder, make sure the download went to the My Documents folder." In theory, we could fallover to the MyDocuments folder for a non-read only download, and wouldn't notice (or the reverse, but actually writing into the read only directory seems a little extreme to have to worry about, though it wouldn't hurt the test.).
http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:693: NullSelectFile(browser()); // Needed for read-only tests. On 2012/02/27 21:06:03, rdsmith wrote: > On 2012/02/25 01:05:17, ahendrickson wrote: > > On 2012/02/24 19:16:15, rdsmith wrote: > > > This makes me uncomfortable, as it changes the behavior of a helper routine > > > (DownloadFileCheckErrors) in a pretty important way. I can't actually > imagine > > > how it could be a problem for these tests, but it's worthwhile calling out > in > > > the description of what this routine does that it'll inhibit the select file > > > dialog. > > > > This makes the behavior what I wanted it to be in the first place -- if a > select > > file dialog appears, choose the default. Combined with changing the "Bail on > > select file" parameter below to false, the download goes to > > completion/interruption. > > > > I'll add the comment. > > I don't see the comment; did you mean you'd add it later? (Fine if so; I'm just > used to responses coming back with the code changes they refer to.) Yes, it got added after a merge with the parent. It's in the function description. http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:723: observer->WaitForFinished(); On 2012/02/27 21:06:03, rdsmith wrote: > If show_download_item isn't set, doesn't this mean we don't wait? And won't > that produce races/flakiness if we get to the tests below before the download > has really started? > > Unless I'm missing something, good race hygiene requires you find something to > wait on so that whatever has happened is going to happen (ideally not something > that presupposes that the result you're testing for actually has happened.) Any suggestions on what to wait for in this case? http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:727: GetDownloads(browser(), &download_items); On 2012/02/27 21:06:03, rdsmith wrote: > This won't see downloads before they're put into the history; will that be a > problem for any of the !show_download_item cases? The !show_download_item won't have anything inserted into the history, and won't have anything displayed on the shelf. http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:737: if (item->GetLastReason() == DOWNLOAD_INTERRUPT_REASON_NONE) { On 2012/02/27 21:06:03, rdsmith wrote: > What does this test mean? That we weren't interrupted? Isn't that a bit more > clear if you just check item status? It means that the download succeeded in the end. Specifically for these tests, they are put in the "My Documents" folder instead of the destination folder. http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:2064: IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorReadonlyFolderDirect) { On 2012/02/27 21:06:03, rdsmith wrote: > On 2012/02/25 01:05:17, ahendrickson wrote: > > On 2012/02/24 19:16:15, rdsmith wrote: > > > Shouldn't both of these tests actively confirm that the file went to the My > > > Documents folder? It doesn't look like they do. > > > > They do, in line 751. > > That test (presuming I'm reading it correctly) is "If it isn't in the Download > folder, make sure it's in the My Documents folder". What I'm asking for is "If > we're downloading to a read only folder, make sure the download went to the My > Documents folder." In theory, we could fallover to the MyDocuments folder for a > non-read only download, and wouldn't notice (or the reverse, but actually > writing into the read only directory seems a little extreme to have to worry > about, though it wouldn't hurt the test.). Ah, I see. I don't have access to the information in this function, but I added another parameter to DownloadInfo to test in DownloadFileCheckErrors().
http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util.h File base/test/test_file_util.h (right): http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util.h#... base/test/test_file_util.h:62: public: nit: indent +1 http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util.h#... base/test/test_file_util.h:66: private: nit: indent +1 http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util.h#... base/test/test_file_util.h:68: void* info_; nit: Comment those things, at least |info_| and |length_| are not obvious. http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util.h#... base/test/test_file_util.h:69: size_t length_; nit: DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util_po... File base/test/test_file_util_posix.cc (right): http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util_po... base/test/test_file_util_posix.cc:39: DCHECK(length != NULL); nit: Just DCHECK(length) if it compiles. http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util_po... base/test/test_file_util_posix.cc:48: *mode = stat_buf.st_mode; That also drags in the type of the thing, i.e. directory vs. file vs. special file and so on. How about &-ing it with ~S_IFMT? http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util_po... base/test/test_file_util_posix.cc:59: if (!info || !length) Don't treat |length| as a boolean. Do length == 0 explicitly. http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util_po... base/test/test_file_util_posix.cc:67: delete mode; // AKA info. nit: No need for that comment. http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util_po... base/test/test_file_util_posix.cc:182: info_ = GetPermissionInfo(path_, &length_); Why don't you just paste the body of GetPermissionInfo inline here? Should be easier to read. http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util_po... base/test/test_file_util_posix.cc:188: if (!RestorePermissionInfo(path_, info_, length_)) Same here.
http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:723: observer->WaitForFinished(); On 2012/02/28 03:59:25, ahendrickson wrote: > On 2012/02/27 21:06:03, rdsmith wrote: > > If show_download_item isn't set, doesn't this mean we don't wait? And won't > > that produce races/flakiness if we get to the tests below before the download > > has really started? > > > > Unless I'm missing something, good race hygiene requires you find something to > > wait on so that whatever has happened is going to happen (ideally not > something > > that presupposes that the result you're testing for actually has happened.) > > Any suggestions on what to wait for in this case? I've addressed this in CL 9426029. http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util.h File base/test/test_file_util.h (right): http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util.h#... base/test/test_file_util.h:62: public: On 2012/02/28 07:55:55, Paweł Hajdan Jr. wrote: > nit: indent +1 Visual Studio strikes again!! $-) Fixed. http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util.h#... base/test/test_file_util.h:66: private: On 2012/02/28 07:55:55, Paweł Hajdan Jr. wrote: > nit: indent +1 Done. http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util.h#... base/test/test_file_util.h:68: void* info_; On 2012/02/28 07:55:55, Paweł Hajdan Jr. wrote: > nit: Comment those things, at least |info_| and |length_| are not obvious. Done. http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util.h#... base/test/test_file_util.h:69: size_t length_; On 2012/02/28 07:55:55, Paweł Hajdan Jr. wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util_po... File base/test/test_file_util_posix.cc (right): http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util_po... base/test/test_file_util_posix.cc:39: DCHECK(length != NULL); On 2012/02/28 07:55:55, Paweł Hajdan Jr. wrote: > nit: Just DCHECK(length) if it compiles. Done. http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util_po... base/test/test_file_util_posix.cc:48: *mode = stat_buf.st_mode; On 2012/02/28 07:55:55, Paweł Hajdan Jr. wrote: > That also drags in the type of the thing, i.e. directory vs. file vs. special > file and so on. How about &-ing it with ~S_IFMT? Done. http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util_po... base/test/test_file_util_posix.cc:59: if (!info || !length) On 2012/02/28 07:55:55, Paweł Hajdan Jr. wrote: > Don't treat |length| as a boolean. Do length == 0 explicitly. Done. http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util_po... base/test/test_file_util_posix.cc:67: delete mode; // AKA info. On 2012/02/28 07:55:55, Paweł Hajdan Jr. wrote: > nit: No need for that comment. Done. http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util_po... base/test/test_file_util_posix.cc:182: info_ = GetPermissionInfo(path_, &length_); On 2012/02/28 07:55:55, Paweł Hajdan Jr. wrote: > Why don't you just paste the body of GetPermissionInfo inline here? Should be > easier to read. This seems pretty straightforward to me. http://codereview.chromium.org/9355050/diff/30001/base/test/test_file_util_po... base/test/test_file_util_posix.cc:188: if (!RestorePermissionInfo(path_, info_, length_)) On 2012/02/28 07:55:55, Paweł Hajdan Jr. wrote: > Same here. Ditto.
Have you uploaded the updated patchset?
On 2012/03/01 17:30:52, Paweł Hajdan Jr. wrote: > Have you uploaded the updated patchset? Oops! Uploaded now. Ignore the title of p#13. It was a cut & paste error.
Pretty darn close (commenting on download_browsertest.cc only; I haven't reviewed the other files). http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:693: NullSelectFile(browser()); // Needed for read-only tests. On 2012/02/28 03:59:25, ahendrickson wrote: > On 2012/02/27 21:06:03, rdsmith wrote: > > On 2012/02/25 01:05:17, ahendrickson wrote: > > > On 2012/02/24 19:16:15, rdsmith wrote: > > > > This makes me uncomfortable, as it changes the behavior of a helper > routine > > > > (DownloadFileCheckErrors) in a pretty important way. I can't actually > > imagine > > > > how it could be a problem for these tests, but it's worthwhile calling out > > in > > > > the description of what this routine does that it'll inhibit the select > file > > > > dialog. > > > > > > This makes the behavior what I wanted it to be in the first place -- if a > > select > > > file dialog appears, choose the default. Combined with changing the "Bail > on > > > select file" parameter below to false, the download goes to > > > completion/interruption. > > > > > > I'll add the comment. > > > > I don't see the comment; did you mean you'd add it later? (Fine if so; I'm > just > > used to responses coming back with the code changes they refer to.) > > Yes, it got added after a merge with the parent. > It's in the function description. Got it--thanks. http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:723: observer->WaitForFinished(); On 2012/03/01 15:18:18, ahendrickson wrote: > On 2012/02/28 03:59:25, ahendrickson wrote: > > On 2012/02/27 21:06:03, rdsmith wrote: > > > If show_download_item isn't set, doesn't this mean we don't wait? And won't > > > that produce races/flakiness if we get to the tests below before the > download > > > has really started? > > > > > > Unless I'm missing something, good race hygiene requires you find something > to > > > wait on so that whatever has happened is going to happen (ideally not > > something > > > that presupposes that the result you're testing for actually has happened.) > > > > Any suggestions on what to wait for in this case? > > I've addressed this in CL 9426029. And just confirming: That CL is going in first? I'll want to re-review this CL after that one goes in, to see how the fix relates in place. http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:737: if (item->GetLastReason() == DOWNLOAD_INTERRUPT_REASON_NONE) { On 2012/02/28 03:59:25, ahendrickson wrote: > On 2012/02/27 21:06:03, rdsmith wrote: > > What does this test mean? That we weren't interrupted? Isn't that a bit more > > clear if you just check item status? > > It means that the download succeeded in the end. Specifically for these tests, > they are put in the "My Documents" folder instead of the destination folder. Why not test against state COMPLETED? It seems clearer. http://codereview.chromium.org/9355050/diff/37003/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9355050/diff/37003/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:758: EXPECT_TRUE(download_info.should_redirect_to_documents); This feels unnecessary since you're inside of an if that tested exactly this.
LGTM with a nit. http://codereview.chromium.org/9355050/diff/37003/base/test/test_file_util.h File base/test/test_file_util.h (right): http://codereview.chromium.org/9355050/diff/37003/base/test/test_file_util.h#... base/test/test_file_util.h:69: size_t length_; // the length of the stored permission information. nit: "the" -> "The" (capital T)
http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:723: observer->WaitForFinished(); On 2012/03/01 20:37:14, rdsmith wrote: > On 2012/03/01 15:18:18, ahendrickson wrote: > > On 2012/02/28 03:59:25, ahendrickson wrote: > > > On 2012/02/27 21:06:03, rdsmith wrote: > > > > If show_download_item isn't set, doesn't this mean we don't wait? And > won't > > > > that produce races/flakiness if we get to the tests below before the > > download > > > > has really started? > > > > > > > > Unless I'm missing something, good race hygiene requires you find > something > > to > > > > wait on so that whatever has happened is going to happen (ideally not > > > something > > > > that presupposes that the result you're testing for actually has > happened.) > > > > > > Any suggestions on what to wait for in this case? > > > > I've addressed this in CL 9426029. > > And just confirming: That CL is going in first? I'll want to re-review this CL > after that one goes in, to see how the fix relates in place. You are correct. That CL has changed to 9570005 however. http://codereview.chromium.org/9355050/diff/20001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:737: if (item->GetLastReason() == DOWNLOAD_INTERRUPT_REASON_NONE) { On 2012/03/01 20:37:14, rdsmith wrote: > On 2012/02/28 03:59:25, ahendrickson wrote: > > On 2012/02/27 21:06:03, rdsmith wrote: > > > What does this test mean? That we weren't interrupted? Isn't that a bit > more > > > clear if you just check item status? > > > > It means that the download succeeded in the end. Specifically for these > tests, > > they are put in the "My Documents" folder instead of the destination folder. > > Why not test against state COMPLETED? It seems clearer. Done. http://codereview.chromium.org/9355050/diff/37003/base/test/test_file_util.h File base/test/test_file_util.h (right): http://codereview.chromium.org/9355050/diff/37003/base/test/test_file_util.h#... base/test/test_file_util.h:69: size_t length_; // the length of the stored permission information. On 2012/03/02 12:26:25, Paweł Hajdan Jr. wrote: > nit: "the" -> "The" (capital T) Done. http://codereview.chromium.org/9355050/diff/37003/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9355050/diff/37003/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:758: EXPECT_TRUE(download_info.should_redirect_to_documents); On 2012/03/01 20:37:14, rdsmith wrote: > This feels unnecessary since you're inside of an if that tested exactly this. Oops, you're right. Missed that in the refactoring.
This looks fine. I'd like to glance it at again after 9570005 goes in and is merged into this one, but I can't imagine that I'd have any major changes at that point.
On 2012/03/06 20:57:48, rdsmith wrote: > This looks fine. I'd like to glance it at again after 9570005 goes in and is > merged into this one, but I can't imagine that I'd have any major changes at > that point. 9570005 is in. PTAL.
LGTM. http://codereview.chromium.org/9355050/diff/54001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9355050/diff/54001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:705: observer->WaitForFinished(); Part of me's a little bothered that we're creating an object that in some contexts never gets used. If there's a clean way of avoiding creating the observer if you're not going to wait on it, you might want to think about doing it. Up to you, though.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/9355050/61005
Try job failure for 9355050-61005 (retry) (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/9355050/61005
Try job failure for 9355050-61005 (retry) on linux_rel for step "unit_tests". It's a second try, previously, step "unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/9355050/61005
Change committed as 126541 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
