Server push cancellation: add NetLogs to track cache lookup transaction

net/http/http_cache_lookup_manager.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_HTTP_HTTP_CACHE_LOOKUP_MANAGER_H_
 #define NET_HTTP_HTTP_CACHE_LOOKUP_MANAGER_H_
 
 #include "net/base/net_export.h"
 #include "net/http/http_cache.h"
 #include "net/http/http_cache_transaction.h"
 #include "net/spdy/server_push_delegate.h"
 
 namespace net {
 
 // An implementation of ServerPushDelegate that issues an HttpCache::Transaction
 // to lookup whether the response to the pushed URL is cached and cancel the
 // push in that case.
 class NET_EXPORT_PRIVATE HttpCacheLookupManager : public ServerPushDelegate {
  public:
   // |http_cache| MUST outlive the HttpCacheLookupManager.
-  HttpCacheLookupManager(HttpCache* http_cache,
-                         const NetLogWithSource& net_log);
+  HttpCacheLookupManager(HttpCache* http_cache, NetLog* net_log);

[eroman, 2017/02/10 23:57:47]

I don't believe this |net_log| parameter is needed, nor is the member variable
HttpCacheLookupManager::net_log_.

The only place that is using this parameter is HttpCacheLookupManager::OnPush(),
in order to create a new NetLogWithSource.

The callsite in OnPush() can instead use |source_net_log.net_log()|.

[Zhongyi Shi, 2017/02/11 02:21:11]

Done.

   ~HttpCacheLookupManager() override;
 
-  void OnPush(std::unique_ptr<ServerPushHelper> push_helper) override;
+  // ServerPushDelegate implementation.
+  // |source_net_log| is owned by the caller of this method.

[eroman, 2017/02/10 23:57:47]

Can you re-phrase the comment a bit? "owned" doesn't seem like the right choice
of words as this is a copiable object.

As far as parameter name, |source_net_log| is reasonable as it identifies the
"source" of the push. Although given the type is a NetLogWithSource it may be a
bit ambiguous.

Would it make sense to call this |session_net_log| instead? (as I believe this
is always going to identify a session that the push delegate was associated
with)

[Zhongyi Shi, 2017/02/11 02:21:10]

Yup, this will always be called from a session: either spdy session or a quic
session.

+  void OnPush(std::unique_ptr<ServerPushHelper> push_helper,
+              const NetLogWithSource& source_net_log) override;
 
   // Invoked when the HttpCache::Transaction for |url| finishes to cancel the
   // server push if the response to the server push is found cached.
   void OnLookupComplete(const GURL& url, int rv);
 
  private:
   // A class that takes the ownership of ServerPushHelper, issues and owns an
   // HttpCache::Transaction which lookups the response in cache for the server
   // push.
   class LookupTransaction {
    public:
-    LookupTransaction(std::unique_ptr<ServerPushHelper> push_helper);
+    LookupTransaction(std::unique_ptr<ServerPushHelper> push_helper,
+                      NetLog* net_log);
     ~LookupTransaction();
 
     // Issues an HttpCache::Transaction to lookup whether the response is cached
     // without header validation.
+    // |source_net_log| is owned by the requester of server push cache lookup.

[eroman, 2017/02/10 23:57:47]

Same comment as earlier regarding comment + parameter name.

[Zhongyi Shi, 2017/02/11 02:21:10]

Done.

     int StartLookup(HttpCache* cache,
                     const CompletionCallback& callback,
-                    const NetLogWithSource& net_log);
+                    const NetLogWithSource& source_net_log);
 
-    void CancelPush();
+    void OnLookupComplete(bool found_in_cache);
 
    private:
     std::unique_ptr<ServerPushHelper> push_helper_;
     std::unique_ptr<HttpRequestInfo> request_;
     std::unique_ptr<HttpTransaction> transaction_;
+    const NetLogWithSource net_log_;
   };
 
-  NetLogWithSource net_log_;
+  NetLog* net_log_;

[eroman, 2017/02/10 23:57:47]

Per comment above, I believe we delete this.

[Zhongyi Shi, 2017/02/11 02:21:10]

Done.

   // HttpCache must outlive the HttpCacheLookupManager.
   HttpCache* http_cache_;
   std::map<GURL, std::unique_ptr<LookupTransaction>> lookup_transactions_;
   base::WeakPtrFactory<HttpCacheLookupManager> weak_factory_;
 };
 
 }  // namespace net
 
 #endif  // NET_HTTP_HTTP_CACHE_LOOKUP_MANAGER_H_