Move sqlite_channel_id_store from chrome/browser/net to net/extras.

net/sqlite/sqlite_server_bound_cert_store.h

 // Copyright (c) 2012 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 CHROME_BROWSER_NET_SQLITE_SERVER_BOUND_CERT_STORE_H_
-#define CHROME_BROWSER_NET_SQLITE_SERVER_BOUND_CERT_STORE_H_
+#ifndef NET_SQLITE_SQLITE_SERVER_BOUND_CERT_STORE_H_
+#define NET_SQLITE_SQLITE_SERVER_BOUND_CERT_STORE_H_
 
 #include "base/callback_forward.h"
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "net/ssl/default_server_bound_cert_store.h"
 
 namespace base {
 class FilePath;
 class SequencedTaskRunner;
 }
 
-namespace quota {
-class SpecialStoragePolicy;
-}
+namespace net {
+
+class SpecialStoragePolicyDelegate;
 
 // Implements the net::DefaultServerBoundCertStore::PersistentStore interface
 // in terms of a SQLite database. For documentation about the actual member
 // functions consult the documentation of the parent class
 // |net::DefaultServerBoundCertStore::PersistentCertStore|.
 // If provided, a |SpecialStoragePolicy| is consulted when the SQLite database
 // is closed to decide which certificates to keep.
 class SQLiteServerBoundCertStore
     : public net::DefaultServerBoundCertStore::PersistentStore {

[Ryan Sleevi, 2014/07/14 20:35:26]

drop the net::, it's cleaner that way :)

Throughout this file, and the other files you're moving into //net.

[mef, 2014/07/14 21:36:53]

Done.

  public:
   SQLiteServerBoundCertStore(
       const base::FilePath& path,
       const scoped_refptr<base::SequencedTaskRunner>& background_task_runner,
-      quota::SpecialStoragePolicy* special_storage_policy);
+      SpecialStoragePolicyDelegate* special_storage_policy);

[Ryan Sleevi, 2014/07/10 20:20:25]

this would/should be a scoped_refptr<>, if you continue with refcounting,
because ownership is transferred/shared.

Better yet would not be not ref-counting.

[mef, 2014/07/11 13:29:24]

FWIW quota::SpecialStoragePolicy is refcounted, but I don't think that delegate
needs to be.
I've changed it to scoped_ptr, but this feels somewhat wrong. 
Maybe SQLiteServerBoundCertStore shouldn't own it, but then how do I express
ownership?

[Ryan Sleevi, 2014/07/14 19:18:04]

Can you explain why it feels wrong?

[mef, 2014/07/14 20:18:02]

Because it means that Store owns a delegate, which it does not, as delegate is
provided by the caller, and may very well be a singleton or even omitted. In
fact per chat with droger@ we pass in NULL on some platforms, and making
parameter a scoped_ptr<> to express ownership transfer seems to require caller
to allocate and pass a dummy instance.
OTOH keeping parameter type as a SpecialStoragePolicyDelegate* doesn't express
the fact that it will be owned by SQLiteServerBoundCertStore...

[Ryan Sleevi, 2014/07/14 20:35:26]

1) We should not have new code using Singletons, although I appreciate the
concern
2) We're not talking about doing new DummySpecialStoragePolicyDelegate, are we?
Because that's not required. If we're just talking about the stack allocation
overhead of a scoped_ptr<>, scoped_ptr<> is able to use the empty-base-object
optimization to devolve into simple sizeof(ptr), and with RVO and friends, can
devolve into a simple pointer swap into the SQLiteSBCS. So I do think we're both
worrying about something overly pessimistically (cost of a pointer allocation),
especially when it doesn't help clarity.
3) For situations like that, if they do happen, the expectation is the caller
should create their own PolicyDelegate that wraps their (singleton, ref-counted
object, foo magic here). While we explicitly discourage multiple base class
implementations, we (as in Willchan and I) try to discourage multiple
*interface* implementations, under the similar logic of too much coupling.
Having a wrapper class in a .cc to wrap your singleton-y object and expose the
PolicyDelegate class is not the end of the world (and matches our advice w/r/t
RefCounting and WeakPtrs, which is similarly about internal helper classes)
If it's owned, it should be scoped_ptr<>.
If it just has to *outlive* the SBCS, then we can document that, and pass a
naked pointer. However, the behaviour of the ::Backend() suggests that it may be
difficult to reason about such a lifetime, and it may be thus beneficial to
think of it as ownership transfer.

[mef, 2014/07/14 21:36:53]

Acknowledged. Looking at test code passing
scoped_ptr<SpecialStoragePolicyDelegate>() may actually look cleaner than
passing NULL as it is clear what parameter is omitted.

 
   // net::DefaultServerBoundCertStore::PersistentStore:
   virtual void Load(const LoadedCallback& loaded_callback) OVERRIDE;
   virtual void AddServerBoundCert(
       const net::DefaultServerBoundCertStore::ServerBoundCert& cert) OVERRIDE;
   virtual void DeleteServerBoundCert(
       const net::DefaultServerBoundCertStore::ServerBoundCert& cert) OVERRIDE;
   virtual void SetForceKeepSessionState() OVERRIDE;
 
  protected:
   virtual ~SQLiteServerBoundCertStore();
 
  private:
   class Backend;
 
   scoped_refptr<Backend> backend_;
 
   DISALLOW_COPY_AND_ASSIGN(SQLiteServerBoundCertStore);
 };
 
-#endif  // CHROME_BROWSER_NET_SQLITE_SERVER_BOUND_CERT_STORE_H_
+}  // namespace net
+
+#endif  // NET_SQLITE_SQLITE_SERVER_BOUND_CERT_STORE_H_