[cronet] make startNetLogToFile write to correct file

components/cronet/ios/cronet_environment.cc

Index: components/cronet/ios/cronet_environment.cc
diff --git a/components/cronet/ios/cronet_environment.cc b/components/cronet/ios/cronet_environment.cc
index 0fef4af2e2266ab840e1b76a457f213ba763f5a6..b23b61b8b1191274fd79fb418d17e04403fad1f6 100644
--- a/components/cronet/ios/cronet_environment.cc
+++ b/components/cronet/ios/cronet_environment.cc
@@ -146,34 +146,42 @@ void CronetEnvironment::Initialize() {
   g_network_change_notifier = net::NetworkChangeNotifier::Create();
 }
 
-void CronetEnvironment::StartNetLog(base::FilePath::StringType file_name,
+bool CronetEnvironment::StartNetLog(base::FilePath::StringType file_name,
                                     bool log_bytes) {
-  DCHECK(file_name.length());
-  PostToNetworkThread(FROM_HERE,
-                      base::Bind(&CronetEnvironment::StartNetLogOnNetworkThread,
-                                 base::Unretained(this), file_name, log_bytes));
-}
+  if (!file_name.length())
+    return false;
 
-void CronetEnvironment::StartNetLogOnNetworkThread(
-    const base::FilePath::StringType& file_name,
-    bool log_bytes) {
-  DCHECK(file_name.length());
-  DCHECK(net_log_);
+  base::FilePath full_path(file_name);
 
-  if (net_log_observer_)
-    return;
+  if (!full_path.IsAbsolute()) {
+    if (!PathService::Get(base::DIR_HOME, &full_path))

[kapishnikov, 2016/11/10 20:52:17]

What is the value of base::DIR_HOME on iOS? Can application write into it? Do we
expect that some other code rather than Cronet.cc will be calling this method?

[lilyhoughton, 2016/11/16 16:04:10]

iOS apps definitely can't write to it, per bug 616918; I left the if-clause here
so as to not break other functions that might want the old behavior, but if that
isn't used anywhere, then this should definitely be removed.

+      return false;
 
-  base::FilePath files_root;
-  if (!PathService::Get(base::DIR_HOME, &files_root))
-    return;
+    full_path = full_path.Append(file_name);
+  }
 
-  base::FilePath full_path = files_root.Append(file_name);
   base::ScopedFILE file(base::OpenFile(full_path, "w"));
   if (!file) {
     LOG(ERROR) << "Can not start NetLog to " << full_path.value();
-    return;
+    return false;
   }
 
+  LOG(WARNING) << "Starting NetLog to " << full_path.value();
+  PostToNetworkThread(
+      FROM_HERE,
+      base::Bind(&CronetEnvironment::StartNetLogOnNetworkThread,
+                 base::Unretained(this), base::Passed(&file), log_bytes));

[kapishnikov, 2016/11/10 20:52:17]

Why do we need base::Passes here?

[lilyhoughton, 2016/11/16 16:04:10]

Because `base::Bind` will not accept an undecorated `std::unique_ptr` (which is
what a ScopedFILE is)

+
+  return true;
+}
+
+void CronetEnvironment::StartNetLogOnNetworkThread(base::ScopedFILE file,
+                                                   bool log_bytes) {
+  DCHECK(net_log_);
+
+  if (net_log_observer_)
+    return;
+
   net::NetLogCaptureMode capture_mode =
       log_bytes ? net::NetLogCaptureMode::IncludeSocketBytes()
                 : net::NetLogCaptureMode::Default();
@@ -182,7 +190,8 @@ void CronetEnvironment::StartNetLogOnNetworkThread(
   net_log_observer_->set_capture_mode(capture_mode);
   net_log_observer_->StartObserving(main_context_->net_log(), std::move(file),
                                     nullptr, main_context_.get());
-  LOG(WARNING) << "Started NetLog to " << full_path.value();
+
+  LOG(WARNING) << "Started NetLog";

[kapishnikov, 2016/11/10 20:52:17]

We already logging a message in StartNetLog(). I think we should remove this
message or reduce the level to debug/verbose.

[lilyhoughton, 2016/11/16 16:04:10]

I wanted to include both whether or not `startNetLog()` succeeded and what file
it is writing too.  I think ideally the format is

Starting NetLog to <file>...OK

or

Starting NetLog to <file>...FAIL

but I don't know how to make LOG() continue on the same line.

[kapishnikov, 2016/11/18 22:17:27]

If StartNetLogOnNetworkThread is called, this line is printed unconditionally
(unless net_log_observer_ is NULL). I don't see value in this LOG since we print
the result in StartNetLog. I agree that we need to log if there is an unexpected
failure.

 }
 
 void CronetEnvironment::StopNetLog() {