Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17589014:
Write a log list for uploaded WebRTC logs. (Closed)
|
Created:
7 years, 6 months ago by Henrik Grunell Modified:
7 years, 5 months ago Reviewers:
tommi (sloooow) - chröme CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWrite a log list for uploaded WebRTC logs.
CL for show a list of uploaded logs:
https://chromiumcodereview.appspot.com/17063004/
BUG=254329
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210312
Patch Set 1 #
Total comments: 22
Patch Set 2 : Code review. #
Total comments: 6
Patch Set 3 : Code review. #Patch Set 4 : Nicer line limitation code. #
Total comments: 2
Patch Set 5 : Code review; added basic unit test #Patch Set 6 : Close file + explicit ctor. #Patch Set 7 : Some unit test changes. #
Total comments: 21
Patch Set 8 : Code review. #Patch Set 9 : Rebase #Patch Set 10 : Fixed error caused by using wrong read file function + a test assert change. #Patch Set 11 : Using expect + return instead of a macro to get better fail output. #Patch Set 12 : Using ReadFileToString and WriteFile instead of platform file. #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/17589014/diff/1/chrome/browser/media/webrtc_l... File chrome/browser/media/webrtc_log_uploader.cc (right): https://codereview.chromium.org/17589014/diff/1/chrome/browser/media/webrtc_l... chrome/browser/media/webrtc_log_uploader.cc:52: int response_code = source->GetResponseCode(); In general this code could use with some commenting on what's being done. https://codereview.chromium.org/17589014/diff/1/chrome/browser/media/webrtc_l... chrome/browser/media/webrtc_log_uploader.cc:70: return; log an error? https://codereview.chromium.org/17589014/diff/1/chrome/browser/media/webrtc_l... chrome/browser/media/webrtc_log_uploader.cc:73: char contents_buf[5 * 1024]; instead of having two buffers for the file contents, can we pre-allocate memory in the contents string and resize after reading? Also, can you add a comment that explains the 5KB? https://codereview.chromium.org/17589014/diff/1/chrome/browser/media/webrtc_l... chrome/browser/media/webrtc_log_uploader.cc:77: DCHECK_LT(processed, sizeof(contents_buf)); nit: rename the |processed| variable to |read|. nothing has yet been parsed or "processed" in a sense. https://codereview.chromium.org/17589014/diff/1/chrome/browser/media/webrtc_l... chrome/browser/media/webrtc_log_uploader.cc:81: base::SplitStringAlongWhitespace(contents, &log_entries); if I'm understanding this correctly, you're splitting the contents by whitespace here but then comparing the resulting list size against a "lines limit". How do you know that there won't be any whitespace in any of the lines? It seems to me that you should instead split the contents based on \n alone (assuming there'll not be any \r in there either). https://codereview.chromium.org/17589014/diff/1/chrome/browser/media/webrtc_l... chrome/browser/media/webrtc_log_uploader.cc:83: contents = contents.substr(contents.find('\n') + 1); this is unnecessarily inefficient. you're searching a string to create a new substring and then assign that to the original string. Instead just use std::string::erase to truncate the string. Also, it looks like you're relying on std::string::npos to be equivalent to max size_t. This doesn't feel right since you're +1-ing an unsigned value and counting on integer overflow to give you 0 if \n can't be found. Instead I'd prefer to check the return value from find(). Also - is it correct behavior to truncate the contents at the first \n if the number of spaces found is larger than kLogListLimitLines? https://codereview.chromium.org/17589014/diff/1/chrome/browser/media/webrtc_l... chrome/browser/media/webrtc_log_uploader.cc:87: "," + response_string + '\n'; can you add a comment on why the response string is needed here? Since this is already a response from the server it feels a bit odd to give it back to the server. https://codereview.chromium.org/17589014/diff/1/chrome/browser/media/webrtc_l... chrome/browser/media/webrtc_log_uploader.cc:93: DCHECK_EQ(processed, contents.size()); this will likely fail if log_entries.size() >= kLogListLimitLines (if find() returns something else than std::string::npos that is) so either the above check for kLogListLimitLines is incorrect or this DCHECK is. https://codereview.chromium.org/17589014/diff/1/chrome/browser/media/webrtc_l... chrome/browser/media/webrtc_log_uploader.cc:94: DCHECK(base::ClosePlatformFile(logs_uploaded_file)); DCHECK code is removed in release builds, so you need to close the file outside the macro.
https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... File chrome/browser/media/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... chrome/browser/media/webrtc_log_uploader.cc:52: int response_code = source->GetResponseCode(); On 2013/06/25 12:57:15, tommi wrote: > In general this code could use with some commenting on what's being done. Done, let me know if you think more is needed. https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... chrome/browser/media/webrtc_log_uploader.cc:70: return; On 2013/06/25 12:57:15, tommi wrote: > log an error? Yes, can be good. Done. https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... chrome/browser/media/webrtc_log_uploader.cc:73: char contents_buf[5 * 1024]; On 2013/06/25 12:57:15, tommi wrote: > instead of having two buffers for the file contents, can we pre-allocate memory > in the contents string and resize after reading? Yes, done. > Also, can you add a comment that explains the 5KB? Done. https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... chrome/browser/media/webrtc_log_uploader.cc:77: DCHECK_LT(processed, sizeof(contents_buf)); On 2013/06/25 12:57:15, tommi wrote: > nit: rename the |processed| variable to |read|. nothing has yet been parsed or > "processed" in a sense. Done. https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... chrome/browser/media/webrtc_log_uploader.cc:81: base::SplitStringAlongWhitespace(contents, &log_entries); On 2013/06/25 12:57:15, tommi wrote: > if I'm understanding this correctly, you're splitting the contents by whitespace > here but then comparing the resulting list size against a "lines limit". How do > you know that there won't be any whitespace in any of the lines? It seems to me > that you should instead split the contents based on \n alone (assuming there'll > not be any \r in there either). If there's another whitespace there's an error in this function. :) (Below where data is written to it.) Actually, the parsing of the file uses this when reading (see chrome/browser/upload_list.cc) so I figured I could simply do the same here. https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... chrome/browser/media/webrtc_log_uploader.cc:83: contents = contents.substr(contents.find('\n') + 1); On 2013/06/25 12:57:15, tommi wrote: > this is unnecessarily inefficient. you're searching a string to create a new > substring and then assign that to the original string. Instead just use > std::string::erase to truncate the string. Absolutely, done. > Also, it looks like you're relying on std::string::npos to be equivalent to max > size_t. This doesn't feel right since you're +1-ing an unsigned value and > counting on integer overflow to give you 0 if \n can't be found. Instead I'd > prefer to check the return value from find(). The assumption is that the only whitespaces are '\n'; then find will not return npos. Anyway, I've rewritten this part based on the next question. > > Also - is it correct behavior to truncate the contents at the first \n if the > number of spaces found is larger than kLogListLimitLines? No it's an error. Rewrote it to keep only the last 49 entries. Also used the vector to get the string to search for when finding the cut position. https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... chrome/browser/media/webrtc_log_uploader.cc:87: "," + response_string + '\n'; On 2013/06/25 12:57:15, tommi wrote: > can you add a comment on why the response string is needed here? > Since this is already a response from the server it feels a bit odd to give it > back to the server. Done. The response is the report id which is written to the log list file. Nothing is sent to the server here. https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... chrome/browser/media/webrtc_log_uploader.cc:93: DCHECK_EQ(processed, contents.size()); On 2013/06/25 12:57:15, tommi wrote: > this will likely fail if log_entries.size() >= kLogListLimitLines (if find() > returns something else than std::string::npos that is) so either the above check > for kLogListLimitLines is incorrect or this DCHECK is. I don't understand. I want to write contents.size() bytes and check that that amount really has been written. https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... chrome/browser/media/webrtc_log_uploader.cc:94: DCHECK(base::ClosePlatformFile(logs_uploaded_file)); On 2013/06/25 12:57:15, tommi wrote: > DCHECK code is removed in release builds, so you need to close the file outside > the macro. Oops. Fixed.
https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... File chrome/browser/media/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... chrome/browser/media/webrtc_log_uploader.cc:81: base::SplitStringAlongWhitespace(contents, &log_entries); On 2013/06/26 09:45:37, Henrik Grunell wrote: > On 2013/06/25 12:57:15, tommi wrote: > > if I'm understanding this correctly, you're splitting the contents by > whitespace > > here but then comparing the resulting list size against a "lines limit". How > do > > you know that there won't be any whitespace in any of the lines? It seems to > me > > that you should instead split the contents based on \n alone (assuming > there'll > > not be any \r in there either). > > If there's another whitespace there's an error in this function. :) (Below where > data is written to it.) Actually, the parsing of the file uses this when reading > (see chrome/browser/upload_list.cc) so I figured I could simply do the same > here. OK, but I still feel that it's incorrect to mix the concept of "lines" and "whitespace". If it's really lines that we want to split the contents by, then I'd rather that we split only by \n and not tab, space etc. https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... chrome/browser/media/webrtc_log_uploader.cc:87: "," + response_string + '\n'; On 2013/06/26 09:45:37, Henrik Grunell wrote: > On 2013/06/25 12:57:15, tommi wrote: > > can you add a comment on why the response string is needed here? > > Since this is already a response from the server it feels a bit odd to give it > > back to the server. > > Done. The response is the report id which is written to the log list file. > Nothing is sent to the server here. OK, can you change the variable name to reflect this (and perhaps document this part of the protocol)? "response_string" isn't very descriptive. https://chromiumcodereview.appspot.com/17589014/diff/6001/chrome/browser/medi... File chrome/browser/media/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/17589014/diff/6001/chrome/browser/medi... chrome/browser/media/webrtc_log_uploader.cc:94: size_t new_first_entry_pos = contents.find(new_first_entry); Is this guaranteed to be correct? What if kLogListLimitLines is 3 and contents is: "abc def foo abc bar"? Actually, skimming over this again I see that the only purpose of the log_entries vector is to count the number of words in |contents| and then potentially trim |contents| based on that number. (please correct me if that's not the case) What about this approach instead: Go backwards through |contents|, counting the spaces along the way. Once we reach either the beginning of the string or the maximum number of words, we stop. If we're not at the beginning of the string, we delete the front of the string up until the current pos. This way, we don't have to allocate the array and each word potentially multiple times. https://chromiumcodereview.appspot.com/17589014/diff/6001/chrome/browser/medi... chrome/browser/media/webrtc_log_uploader.cc:110: DCHECK(ret); FYI - there is also DPCHECK which will print out any error values related to system calls (GetLastError() on Windows, errno on linux) https://chromiumcodereview.appspot.com/17589014/diff/6001/chrome/browser/medi... chrome/browser/media/webrtc_log_uploader.cc:113: DCHECK(ret); you could use that here too
https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... File chrome/browser/media/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... chrome/browser/media/webrtc_log_uploader.cc:81: base::SplitStringAlongWhitespace(contents, &log_entries); On 2013/06/27 08:45:40, tommi wrote: > On 2013/06/26 09:45:37, Henrik Grunell wrote: > > On 2013/06/25 12:57:15, tommi wrote: > > > if I'm understanding this correctly, you're splitting the contents by > > whitespace > > > here but then comparing the resulting list size against a "lines limit". > How > > do > > > you know that there won't be any whitespace in any of the lines? It seems > to > > me > > > that you should instead split the contents based on \n alone (assuming > > there'll > > > not be any \r in there either). > > > > If there's another whitespace there's an error in this function. :) (Below > where > > data is written to it.) Actually, the parsing of the file uses this when > reading > > (see chrome/browser/upload_list.cc) so I figured I could simply do the same > > here. > > OK, but I still feel that it's incorrect to mix the concept of "lines" and > "whitespace". If it's really lines that we want to split the contents by, then > I'd rather that we split only by \n and not tab, space etc. Agree, done. https://chromiumcodereview.appspot.com/17589014/diff/1/chrome/browser/media/w... chrome/browser/media/webrtc_log_uploader.cc:87: "," + response_string + '\n'; On 2013/06/27 08:45:40, tommi wrote: > On 2013/06/26 09:45:37, Henrik Grunell wrote: > > On 2013/06/25 12:57:15, tommi wrote: > > > can you add a comment on why the response string is needed here? > > > Since this is already a response from the server it feels a bit odd to give > it > > > back to the server. > > > > Done. The response is the report id which is written to the log list file. > > Nothing is sent to the server here. > > OK, can you change the variable name to reflect this (and perhaps document this > part of the protocol)? "response_string" isn't very descriptive. Done. https://chromiumcodereview.appspot.com/17589014/diff/6001/chrome/browser/medi... File chrome/browser/media/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/17589014/diff/6001/chrome/browser/medi... chrome/browser/media/webrtc_log_uploader.cc:94: size_t new_first_entry_pos = contents.find(new_first_entry); On 2013/06/27 08:45:40, tommi wrote: > Is this guaranteed to be correct? > > What if kLogListLimitLines is 3 and contents is: > "abc def foo abc bar"? > > Actually, skimming over this again I see that the only purpose of the > log_entries vector is to count the number of words in |contents| and then > potentially trim |contents| based on that number. (please correct me if that's > not the case) > > What about this approach instead: > Go backwards through |contents|, counting the spaces along the way. Once we > reach either the beginning of the string or the maximum number of words, we > stop. If we're not at the beginning of the string, we delete the front of the > string up until the current pos. > > This way, we don't have to allocate the array and each word potentially multiple > times. The lines will not contain other whitespaces than '\n'. It doesn't matter though due to next paragraph. I also considered searching backwards but for some reason I thought this was better. (Probably I though the code was easier to read before the last change I made.) Now however I also think searching backwards is better for all reasons. Changed. https://chromiumcodereview.appspot.com/17589014/diff/6001/chrome/browser/medi... chrome/browser/media/webrtc_log_uploader.cc:110: DCHECK(ret); On 2013/06/27 08:45:40, tommi wrote: > FYI - there is also DPCHECK which will print out any error values related to > system calls (GetLastError() on Windows, errno on linux) OK, good to know. Changed. https://chromiumcodereview.appspot.com/17589014/diff/6001/chrome/browser/medi... chrome/browser/media/webrtc_log_uploader.cc:113: DCHECK(ret); On 2013/06/27 08:45:40, tommi wrote: > you could use that here too Done.
Rewrote the line limitation code.
I think we need a unit test for the class. At the very least the file parsing code needs a test. https://chromiumcodereview.appspot.com/17589014/diff/15001/chrome/browser/med... File chrome/browser/media/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/17589014/diff/15001/chrome/browser/med... chrome/browser/media/webrtc_log_uploader.cc:105: contents.erase(0, i + 2); I think we'll then also need to add a check if i > 0 before we do this. The reason is that if we don't reach the limit, i will be -1 after the loop, we increment to 1 and delete the first character of the string. ... thinking about that a bit more though, I think we need to do this slightly differently assuming that we only want to trim whole lines. I think you need to add a new variable to keep track of the last eol you found. Then modify the condition in the loop to be something like: if (contents[i] == '\n') { ++lf_count; eol_pos = i; } After the loop you can then do: if (lf_count >= kLogListLimitLines) contents.erase(0, eol_pos + 1);
Added unit test + some refactoring for that. Added Jochen as reviewer, please review chrome/browser/browser_process_impl.cc https://chromiumcodereview.appspot.com/17589014/diff/15001/chrome/browser/med... File chrome/browser/media/webrtc_log_uploader.cc (right): https://chromiumcodereview.appspot.com/17589014/diff/15001/chrome/browser/med... chrome/browser/media/webrtc_log_uploader.cc:105: contents.erase(0, i + 2); On 2013/06/28 11:00:05, tommi wrote: > I think we'll then also need to add a check if i > 0 before we do this. > The reason is that if we don't reach the limit, i will be -1 after the loop, we > increment to 1 and delete the first character of the string. > > ... thinking about that a bit more though, I think we need to do this slightly > differently assuming that we only want to trim whole lines. > > I think you need to add a new variable to keep track of the last eol you found. > Then modify the condition in the loop to be something like: > if (contents[i] == '\n') { > ++lf_count; > eol_pos = i; > } > > After the loop you can then do: > > if (lf_count >= kLogListLimitLines) > contents.erase(0, eol_pos + 1); Good point. Added check.
Some unit test changes + rebase.
https://chromiumcodereview.appspot.com/17589014/diff/32001/chrome/browser/bro... File chrome/browser/browser_process_impl.cc (right): https://chromiumcodereview.appspot.com/17589014/diff/32001/chrome/browser/bro... chrome/browser/browser_process_impl.cc:641: webrtc_log_uploader_.reset(new WebRtcLogUploader(empty_path)); nit. Doesn't WebRtcLogUploader(base::FilePath()) work?
https://codereview.chromium.org/17589014/diff/32001/chrome/browser/browser_pr... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/17589014/diff/32001/chrome/browser/browser_pr... chrome/browser/browser_process_impl.cc:641: webrtc_log_uploader_.reset(new WebRtcLogUploader(empty_path)); instead of requiring an empty path can we have two constructors, one default one and separate explicit one that accepts a non-empty path? https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... File chrome/browser/media/webrtc_log_uploader.cc (right): https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader.cc:213: void WebRtcLogUploader::AddUploadedLogInfoToUploadListFile( Since there's no locking in this class can we add thread checks that all of these calls happen on the same thread? https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader.cc:229: LOG(WARNING) << "Could not open WebRTC log list file."; nit: also include the error value https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader.cc:239: DCHECK_LT(read, contents.size()); isn't it possible that read can be equal to content.size()? https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader.cc:245: if (!contents.empty()) { nit: this condition isn't really necessary https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader.cc:252: if (lf_count <= kLogListLimitLines) { shouldn't this be >= ? Right now it looks like you'll always delete everything otherwise and if the file is too big you don't trim it. https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... File chrome/browser/media/webrtc_log_uploader.h (right): https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader.h:38: explicit WebRtcLogUploader(base::FilePath& upload_list_path); instead of changing the constructor and add a FilePath dependency to the production code, let's add a an inline method that tests can use to set the path. E.g. void SetUploadPathForTesting(const base::FilePath& path) { upload_list_path_ = path; } https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... File chrome/browser/media/webrtc_log_uploader_unittest.cc (right): https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader_unittest.cc:35: EXPECT_EQ(expected_lines + 1, static_cast<int>(lines.size())); maybe this should be ASSERT_EQ to avoid executing the rest if that fails. https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader_unittest.cc:43: ASSERT_EQ(2u, line_parts.size()); ... like you do here :) https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader_unittest.cc:76: TEST_F(WebRtcLogUploaderTest, Basic) { does this test the trimming (or not) of the file?
Jochen only a gypi file for you now. :) https://codereview.chromium.org/17589014/diff/32001/chrome/browser/browser_pr... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/17589014/diff/32001/chrome/browser/browser_pr... chrome/browser/browser_process_impl.cc:641: webrtc_log_uploader_.reset(new WebRtcLogUploader(empty_path)); On 2013/07/01 12:01:01, tommi wrote: > instead of requiring an empty path can we have two constructors, one default one > and separate explicit one that accepts a non-empty path? As per the next comment, I've removed the argument and added a function to set for testing. https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... File chrome/browser/media/webrtc_log_uploader.cc (right): https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader.cc:213: void WebRtcLogUploader::AddUploadedLogInfoToUploadListFile( On 2013/07/01 12:01:01, tommi wrote: > Since there's no locking in this class can we add thread checks that all of > these calls happen on the same thread? As discussed offline, not done. Problems for test. Only data to protect is upload_list_path_ which is accessed in ctor and here. https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader.cc:229: LOG(WARNING) << "Could not open WebRTC log list file."; On 2013/07/01 12:01:01, tommi wrote: > nit: also include the error value Done. https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader.cc:239: DCHECK_LT(read, contents.size()); On 2013/07/01 12:01:01, tommi wrote: > isn't it possible that read can be equal to content.size()? Is should be ~ a factor 3 smaller, but I can check for LE instead if you think? Equal is OK in terms of not writing out of bounds. https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader.cc:245: if (!contents.empty()) { On 2013/07/01 12:01:01, tommi wrote: > nit: this condition isn't really necessary That's right. Removed. https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader.cc:252: if (lf_count <= kLogListLimitLines) { On 2013/07/01 12:01:01, tommi wrote: > shouldn't this be >= ? Right now it looks like you'll always delete everything > otherwise and if the file is too big you don't trim it. Yes it should, thanks. https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... File chrome/browser/media/webrtc_log_uploader.h (right): https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader.h:38: explicit WebRtcLogUploader(base::FilePath& upload_list_path); On 2013/07/01 12:01:01, tommi wrote: > instead of changing the constructor and add a FilePath dependency to the > production code, let's add a an inline method that tests can use to set the > path. E.g. > > void SetUploadPathForTesting(const base::FilePath& path) { > upload_list_path_ = path; > } Done. https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... File chrome/browser/media/webrtc_log_uploader_unittest.cc (right): https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader_unittest.cc:35: EXPECT_EQ(expected_lines + 1, static_cast<int>(lines.size())); On 2013/07/01 12:01:01, tommi wrote: > maybe this should be ASSERT_EQ to avoid executing the rest if that fails. Done. https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader_unittest.cc:43: ASSERT_EQ(2u, line_parts.size()); On 2013/07/01 12:01:01, tommi wrote: > ... like you do here :) :) https://codereview.chromium.org/17589014/diff/32001/chrome/browser/media/webr... chrome/browser/media/webrtc_log_uploader_unittest.cc:76: TEST_F(WebRtcLogUploaderTest, Basic) { On 2013/07/01 12:01:01, tommi wrote: > does this test the trimming (or not) of the file? Yes it does, the conditional error in the tested function only manifested itself by removing the first char in the first line when it shouldn't.
On 2013/07/01 13:26:21, Henrik Grunell wrote: > Jochen only a gypi file for you now. :) for the gypi file, you don't need owners approval
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grunell@chromium.org/17589014/42001
Retried try job too often on win_x64_rel for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grunell@chromium.org/17589014/54002
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
PTAL again. - Fixed the win_rel and win7_aura bots error. Caused by using wrong read file function... - An test assert change in the test support functions. It now uses expect and returns false if fails. Code isn't super neat, but it's just a few places. Let me know if you think I should e.g. set up macros. The main test function then asserts on the return value. - Renamed test to something better.
New patch.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grunell@chromium.org/17589014/82001
Message was sent while issue was closed.
Change committed as 210312 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
