Write a log list for uploaded WebRTC logs.

chrome/browser/media/webrtc_log_uploader.cc

Index: chrome/browser/media/webrtc_log_uploader.cc
diff --git a/chrome/browser/media/webrtc_log_uploader.cc b/chrome/browser/media/webrtc_log_uploader.cc
index f88e3821ada4ca34a3d27a3a5bc6fed882169957..a2db599dfa674a112479853a725aa4e35d81ba26 100644
--- a/chrome/browser/media/webrtc_log_uploader.cc
+++ b/chrome/browser/media/webrtc_log_uploader.cc
@@ -4,9 +4,16 @@
 
 #include "chrome/browser/media/webrtc_log_uploader.h"
 
+#include "base/files/file_path.h"
 #include "base/logging.h"
+#include "base/path_service.h"
 #include "base/shared_memory.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
+#include "base/time.h"
+#include "chrome/browser/media/webrtc_log_upload_list.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/partial_circular_buffer.h"
 #include "content/public/browser/browser_thread.h"
@@ -24,6 +31,7 @@ namespace {
 
 const int kLogCountLimit = 5;
 const uint32 kIntermediateCompressionBufferBytes = 256 * 1024;  // 256 KB
+const int kLogListLimitLines = 50;
 
 const char kUploadURL[] = "https://clients2.google.com/cr/report";
 const char kUploadContentType[] = "multipart/form-data";
@@ -41,6 +49,49 @@ WebRtcLogUploader::~WebRtcLogUploader() {
 
 void WebRtcLogUploader::OnURLFetchComplete(
     const net::URLFetcher* source) {
+  int response_code = source->GetResponseCode();

[tommi (sloooow) - chröme, 2013/06/25 12:57:15]

In general this code could use with some commenting on what's being done.

[Henrik Grunell, 2013/06/26 09:45:37]

Done, let me know if you think more is needed.

+  std::string response_string;
+  if (response_code != 200 ||
+      !source->GetResponseAsString(&response_string)) {
+    return;
+  }
+  base::FilePath log_dir_path;
+  PathService::Get(chrome::DIR_USER_DATA, &log_dir_path);
+  base::FilePath upload_log_path =
+      log_dir_path.AppendASCII(WebRtcLogUploadList::kWebRtcLogListFilename);
+
+  int flags = base::PLATFORM_FILE_OPEN_ALWAYS |
+              base::PLATFORM_FILE_READ |
+              base::PLATFORM_FILE_WRITE;
+  base::PlatformFileError error = base::PLATFORM_FILE_OK;
+  base::PlatformFile logs_uploaded_file =
+      base::CreatePlatformFile(upload_log_path, flags, NULL, &error);
+  if (error != base::PLATFORM_FILE_OK)
+    return;

[tommi (sloooow) - chröme, 2013/06/25 12:57:15]

log an error?

[Henrik Grunell, 2013/06/26 09:45:37]

Yes, can be good. Done.

+
+  std::string contents;
+  char contents_buf[5 * 1024];

[tommi (sloooow) - chröme, 2013/06/25 12:57:15]

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?

[Henrik Grunell, 2013/06/26 09:45:37]

Yes, done.
Done.

+  size_t processed = base::ReadPlatformFileAtCurrentPos(logs_uploaded_file,
+                                                        &contents_buf[0],
+                                                        sizeof(contents_buf));
+  DCHECK_LT(processed, sizeof(contents_buf));

[tommi (sloooow) - chröme, 2013/06/25 12:57:15]

nit: rename the |processed| variable to |read|.  nothing has yet been parsed or
"processed" in a sense.

[Henrik Grunell, 2013/06/26 09:45:37]

Done.

+  contents.append(contents_buf, processed);
+
+  std::vector<std::string> log_entries;
+  base::SplitStringAlongWhitespace(contents, &log_entries);

[tommi (sloooow) - chröme, 2013/06/25 12:57:15]

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).

[Henrik Grunell, 2013/06/26 09:45:37]

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.

[tommi (sloooow) - chröme, 2013/06/27 08:45:40]

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.

[Henrik Grunell, 2013/06/27 11:56:21]

Agree, done.

+  if (log_entries.size() >= kLogListLimitLines)
+    contents = contents.substr(contents.find('\n') + 1);

[tommi (sloooow) - chröme, 2013/06/25 12:57:15]

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?

[Henrik Grunell, 2013/06/26 09:45:37]

Absolutely, done.
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.
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.

+
+  base::Time time_now = base::Time::Now();
+  contents += base::DoubleToString(time_now.ToDoubleT()) +
+              "," + response_string + '\n';

[tommi (sloooow) - chröme, 2013/06/25 12:57:15]

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.

[Henrik Grunell, 2013/06/26 09:45:37]

Done. The response is the report id which is written to the log list file.
Nothing is sent to the server here.

[tommi (sloooow) - chröme, 2013/06/27 08:45:40]

OK, can you change the variable name to reflect this (and perhaps document this
part of the protocol)?  "response_string" isn't very descriptive.

[Henrik Grunell, 2013/06/27 11:56:21]

Done.

+
+  base::SeekPlatformFile(logs_uploaded_file, base::PLATFORM_FILE_FROM_BEGIN, 0);
+  processed = base::WritePlatformFileAtCurrentPos(logs_uploaded_file,
+                                                  contents.c_str(),
+                                                  contents.size());
+  DCHECK_EQ(processed, contents.size());

[tommi (sloooow) - chröme, 2013/06/25 12:57:15]

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.

[Henrik Grunell, 2013/06/26 09:45:37]

I don't understand. I want to write contents.size() bytes and check that that
amount really has been written.

+  DCHECK(base::ClosePlatformFile(logs_uploaded_file));

[tommi (sloooow) - chröme, 2013/06/25 12:57:15]

DCHECK code is removed in release builds, so you need to close the file outside
the macro.

[Henrik Grunell, 2013/06/26 09:45:37]

Oops. Fixed.

 }
 
 void WebRtcLogUploader::OnURLFetchUploadProgress(