net/sqlite/sqlite_server_bound_cert_store.h - Issue 381073002: Move sqlite_channel_id_store from chrome/browser/net to net/extras.

Unified Diff: net/sqlite/sqlite_server_bound_cert_store.h

Issue 381073002: Move sqlite_channel_id_store from chrome/browser/net to net/extras. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Add special_storage_policy_delegate prototype. Created 6 years, 5 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« net/sqlite/special_storage_policy_delegate.h ('K') | « net/sqlite/special_storage_policy_delegate.h ('k') | net/sqlite/sqlite_server_bound_cert_store.cc » ('j') | net/sqlite/sqlite_server_bound_cert_store_unittest.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: net/sqlite/sqlite_server_bound_cert_store.h

diff --git a/chrome/browser/net/sqlite_server_bound_cert_store.h b/net/sqlite/sqlite_server_bound_cert_store.h

similarity index 84%

rename from chrome/browser/net/sqlite_server_bound_cert_store.h

rename to net/sqlite/sqlite_server_bound_cert_store.h

index d6777b0f1294daac502731a30e43f16ff3eb1c96..0e4d8e55467f83881529b25c4e95df9b9a1d7adf 100644

--- a/chrome/browser/net/sqlite_server_bound_cert_store.h

+++ b/net/sqlite/sqlite_server_bound_cert_store.h

@@ -2,8 +2,8 @@

// 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"

@@ -15,9 +15,9 @@ 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

@@ -31,7 +31,7 @@ class SQLiteServerBoundCertStore

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 con

mef 2014/07/11 13:29:24 FWIW quota::SpecialStoragePolicy is refcounted, bu

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

Ryan Sleevi 2014/07/14 20:35:26 1) We should not have new code using Singletons, a

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_

On 2014/07/14 20:35:26, Ryan Sleevi wrote: > On 2014/07/14 20:18:02, mef wrote: > > > Can you explain why it feels wrong? > > 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. > > 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) > > > OTOH keeping parameter type as a SpecialStoragePolicyDelegate* doesn't express > > the fact that it will be owned by SQLiteServerBoundCertStore... > > 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.

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;

@@ -52,4 +52,6 @@ class SQLiteServerBoundCertStore

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_