Improve HttpStreamFactory NetLog events

net/http/http_stream_factory_impl.cc

Index: net/http/http_stream_factory_impl.cc
diff --git a/net/http/http_stream_factory_impl.cc b/net/http/http_stream_factory_impl.cc
index 68bab0f7b3f4c56abb723effcfae18efe5e63656..46e534cd4835d8996f7f1bbf9c34edacd1b5cd3b 100644
--- a/net/http/http_stream_factory_impl.cc
+++ b/net/http/http_stream_factory_impl.cc
@@ -159,14 +159,15 @@ HttpStreamRequest* HttpStreamFactoryImpl::RequestStreamInternal(
         websocket_handshake_stream_create_helper,
     HttpStreamRequest::StreamType stream_type,
     const NetLogWithSource& net_log) {
-  JobController* job_controller =
-      new JobController(this, delegate, session_, job_factory_.get());
-  job_controller_set_.insert(base::WrapUnique(job_controller));
-
+  auto job_controller = base::MakeUnique<JobController>(

[mmenke, 2017/01/26 20:47:43]

Am I the only one who can't stand this pattern?  :(  I find not putting types
before variables obfuscates types of variables (Yes, even when the type and
variable name are the same).  Would you mind doing:

std::unique_ptr<JobController> job_controller = base::MakeUnique...

[xunjieli, 2017/01/26 22:37:43]

Done.

[eroman, 2017/01/26 23:36:59]

Most chrome code (84%) uses "auto":

  verbose way: 149
  auto: 804

I personally find repeating the type to be unnecessarily verbose. Based on the
number I would say use of auto is more idiomatic, unless there was an explicit
style discussion somewhere.

[mmenke, 2017/01/27 15:45:17]

When I see a variable and want to know its type, I find skimming up and finding
auto and then looking over to MakeUnique<blah> and then mapping MakeUnique<blah>
to std::unique_ptr<blah> involves significantly more cognitive load than just
skimming up and seeing std::unique_ptr<blah>, and then going back to the code I
was looking at.

+      this, delegate, session_, job_factory_.get(),
+      /*is_preconnect=*/false);
+  job_controller->AssociateWithNetLogSource(net_log);
   Request* request = job_controller->Start(
       request_info, delegate, websocket_handshake_stream_create_helper, net_log,
       stream_type, priority, server_ssl_config, proxy_ssl_config);
 
+  job_controller_set_.insert(std::move(job_controller));
   return request;
 }
 
@@ -182,11 +183,12 @@ void HttpStreamFactoryImpl::PreconnectStreams(
 
   DCHECK(!for_websockets_);
 
-  JobController* job_controller =
-      new JobController(this, nullptr, session_, job_factory_.get());
-  job_controller_set_.insert(base::WrapUnique(job_controller));
+  auto job_controller = base::MakeUnique<JobController>(this, nullptr, session_,
+                                                        job_factory_.get(),
+                                                        /*is_preconnect=*/true);

[mmenke, 2017/01/26 20:47:43]

Same as above

[xunjieli, 2017/01/26 22:37:43]

Done.

   job_controller->Preconnect(num_streams, request_info, server_ssl_config,
                              proxy_ssl_config);
+  job_controller_set_.insert(std::move(job_controller));

[mmenke, 2017/01/26 20:47:43]

Are you sure Preconnect can't complete synchronously?  I guess the fact that
HttpStreamFactoryImpl::RequestStreamInternal can't indicates perhaps not, though
Preconnect doesn't actually create a stream, so it misses the posttask for the
final return.

[xunjieli, 2017/01/26 22:37:43]

Done. Sorry about that. I think we shouldn't make the assumption. Changed this
and the above one back.

 }
 
 const HostMappingRules* HttpStreamFactoryImpl::GetHostMappingRules() const {
@@ -200,8 +202,7 @@ void HttpStreamFactoryImpl::OnNewSpdySessionReady(
     const ProxyInfo& used_proxy_info,
     bool was_alpn_negotiated,
     NextProto negotiated_protocol,
-    bool using_spdy,
-    const NetLogWithSource& net_log) {
+    bool using_spdy) {
   while (true) {
     if (!spdy_session)
       break;