Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
381073002:
Move sqlite_channel_id_store from chrome/browser/net to net/extras. (Closed)
|
Created:
6 years, 5 months ago by mef Modified:
6 years, 3 months ago Reviewers:
droger, willchan no longer on Chromium, Ryan Sleevi, Lei Zhang, erikwright (departed), Ryan Hamilton, rvargas (doing something else), mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionMove sqlite_channel_id_store from chrome/browser/net to net/extras.
Application of special storage policy is split out into chrome/browser/net/quota_policy_channel_id_store.
TEST=net_unittests --gtest_filter=SQLiteChannelIDStoreTest*
TEST=unit_tests --gtest_filter=QuotaPolicyChannelIDStore*
BUG=397545
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289996
Committed: https://crrev.com/327a8e400da2c092955a707321c39212ff9a3de9
Cr-Commit-Position: refs/heads/master@{#292659}
Patch Set 1 #Patch Set 2 : Rebased to r282319 #Patch Set 3 : Fix net/BUILD.gn, remove third_party/sqlite dependency. #Patch Set 4 : Add special_storage_policy_delegate prototype. #
Total comments: 19
Patch Set 5 : Implement chrome_special_storage_policy_delegate. #
Total comments: 3
Patch Set 6 : Sync to ToT. #Patch Set 7 : Move to net/util and remove net:: prefix. #Patch Set 8 : Move net_util_sources list into net.gypi #Patch Set 9 : Added content/browser/net/quota_policyserver_bound_cert_store... #
Total comments: 10
Patch Set 10 : Sync to r286204 #Patch Set 11 : Fix compilation errors. #Patch Set 12 : Fix trybot errors and formatting. #Patch Set 13 : Rebased to r286605 #
Total comments: 22
Patch Set 14 : Implemented QuotaPolicyChannelIDStore, added unit tests. #
Total comments: 24
Patch Set 15 : Sync to r288058 #Patch Set 16 : Address review comments. #
Total comments: 4
Patch Set 17 : Add DeleteAll(server_identifiers) method. #
Total comments: 6
Patch Set 18 : Sync to r288793. #Patch Set 19 : Address Ryan's comments. #
Total comments: 14
Patch Set 20 : Address mmenke's comments. #
Total comments: 8
Patch Set 21 : Address review comments. #
Total comments: 13
Patch Set 22 : Address Eric's comments. #
Total comments: 1
Patch Set 23 : Sync to r289288 #Patch Set 24 : Move quota_policy_channel_id_store from content/browser/net to chrome/browser/net. #
Total comments: 1
Patch Set 25 : Sync to r289603 #Patch Set 26 : Fix flaky TestPersistence. #Patch Set 27 : Sync to f8b3fe9660d8dda318800f55d5e29799bbfd43f7 #Patch Set 28 : Change SpecialStoragePolicy namespace from quota to storage. #Patch Set 29 : Sync to r230888 #Patch Set 30 : Fix flaky TestDeleteAll. #Messages
Total messages: 66 (0 generated)
https://codereview.chromium.org/381073002/diff/60001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/381073002/diff/60001/net/net.gyp#newcode1048 net/net.gyp:1048: 'target_name': 'net_sqlite', net_sqlite_storage ? I think Will had stronger feelings about this than I do. https://codereview.chromium.org/381073002/diff/60001/net/sqlite/special_stora... File net/sqlite/special_storage_policy_delegate.h (right): https://codereview.chromium.org/381073002/diff/60001/net/sqlite/special_stora... net/sqlite/special_storage_policy_delegate.h:12: // Encapsulates methods of WebKit SpecialStoragePolicy used by net/sqlite. So, we would never mention this in a //net comment https://codereview.chromium.org/381073002/diff/60001/net/sqlite/special_stora... net/sqlite/special_storage_policy_delegate.h:14: : public base::RefCountedThreadSafe<SpecialStoragePolicyDelegate>{ 1) Why ref-counted? Make ownership explicit https://codereview.chromium.org/381073002/diff/60001/net/sqlite/special_stora... net/sqlite/special_storage_policy_delegate.h:21: virtual bool HasSessionOnlyOrigins() = 0; I'm not sure I grok how this fits into //net, but I'd need to understand more. That is, I understand there's a desire for some degree of this control, I just question whether it's appropriate to expose it *as this*, as opposed to an appropriate //net agnostication. https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... File net/sqlite/sqlite_server_bound_cert_store.h (right): https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... net/sqlite/sqlite_server_bound_cert_store.h:34: SpecialStoragePolicyDelegate* special_storage_policy); 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. https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... File net/sqlite/sqlite_server_bound_cert_store_unittest.cc (right): https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... net/sqlite/sqlite_server_bound_cert_store_unittest.cc:23: FILE_PATH_LITERAL("Origin Bound Certs"); pedantry: Doesn't need this constant, does it? You should just be able to use temp_dir_ directly.
+willchan to comment on 'net/sqlite' name. +mmenke as profile_io_impl owner +droger as fyi. Thanks, PTAL. https://codereview.chromium.org/381073002/diff/60001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/381073002/diff/60001/net/net.gyp#newcode1048 net/net.gyp:1048: 'target_name': 'net_sqlite', On 2014/07/10 20:20:25, Ryan Sleevi wrote: > net_sqlite_storage ? > > I think Will had stronger feelings about this than I do. net_storage? net_storage_sqlite? I'm completely open. https://codereview.chromium.org/381073002/diff/60001/net/sqlite/special_stora... File net/sqlite/special_storage_policy_delegate.h (right): https://codereview.chromium.org/381073002/diff/60001/net/sqlite/special_stora... net/sqlite/special_storage_policy_delegate.h:12: // Encapsulates methods of WebKit SpecialStoragePolicy used by net/sqlite. On 2014/07/10 20:20:25, Ryan Sleevi wrote: > So, we would never mention this in a //net comment Done. https://codereview.chromium.org/381073002/diff/60001/net/sqlite/special_stora... net/sqlite/special_storage_policy_delegate.h:14: : public base::RefCountedThreadSafe<SpecialStoragePolicyDelegate>{ On 2014/07/10 20:20:25, Ryan Sleevi wrote: > 1) Why ref-counted? Make ownership explicit Done. https://codereview.chromium.org/381073002/diff/60001/net/sqlite/special_stora... net/sqlite/special_storage_policy_delegate.h:21: virtual bool HasSessionOnlyOrigins() = 0; On 2014/07/10 20:20:25, Ryan Sleevi wrote: > I'm not sure I grok how this fits into //net, but I'd need to understand more. > > That is, I understand there's a desire for some degree of this control, I just > question whether it's appropriate to expose it *as this*, as opposed to an > appropriate //net agnostication. Ack. I've thrown this as straight copy of SpecialStoragePolicy methods in here to start a conversation. https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... File net/sqlite/sqlite_server_bound_cert_store.h (right): https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... net/sqlite/sqlite_server_bound_cert_store.h:34: SpecialStoragePolicyDelegate* special_storage_policy); On 2014/07/10 20:20:25, Ryan Sleevi wrote: > 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. 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? https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... File net/sqlite/sqlite_server_bound_cert_store_unittest.cc (right): https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... net/sqlite/sqlite_server_bound_cert_store_unittest.cc:23: FILE_PATH_LITERAL("Origin Bound Certs"); On 2014/07/10 20:20:25, Ryan Sleevi wrote: > pedantry: Doesn't need this constant, does it? You should just be able to use > temp_dir_ directly. Um, not sure I understand. SQLiteServerBoundCertStore takes path to the file, not to directory.
On 2014/07/11 13:29:24, mef wrote: > +willchan to comment on 'net/sqlite' name. > +mmenke as profile_io_impl owner > +droger as fyi. > > Thanks, PTAL. > > https://codereview.chromium.org/381073002/diff/60001/net/net.gyp > File net/net.gyp (right): > > https://codereview.chromium.org/381073002/diff/60001/net/net.gyp#newcode1048 > net/net.gyp:1048: 'target_name': 'net_sqlite', > On 2014/07/10 20:20:25, Ryan Sleevi wrote: > > net_sqlite_storage ? > > > > I think Will had stronger feelings about this than I do. > > net_storage? > net_storage_sqlite? > > I'm completely open. > > https://codereview.chromium.org/381073002/diff/60001/net/sqlite/special_stora... > File net/sqlite/special_storage_policy_delegate.h (right): > > https://codereview.chromium.org/381073002/diff/60001/net/sqlite/special_stora... > net/sqlite/special_storage_policy_delegate.h:12: // Encapsulates methods of > WebKit SpecialStoragePolicy used by net/sqlite. > On 2014/07/10 20:20:25, Ryan Sleevi wrote: > > So, we would never mention this in a //net comment > > Done. > > https://codereview.chromium.org/381073002/diff/60001/net/sqlite/special_stora... > net/sqlite/special_storage_policy_delegate.h:14: : public > base::RefCountedThreadSafe<SpecialStoragePolicyDelegate>{ > On 2014/07/10 20:20:25, Ryan Sleevi wrote: > > 1) Why ref-counted? Make ownership explicit > > Done. > > https://codereview.chromium.org/381073002/diff/60001/net/sqlite/special_stora... > net/sqlite/special_storage_policy_delegate.h:21: virtual bool > HasSessionOnlyOrigins() = 0; > On 2014/07/10 20:20:25, Ryan Sleevi wrote: > > I'm not sure I grok how this fits into //net, but I'd need to understand more. > > > > That is, I understand there's a desire for some degree of this control, I just > > question whether it's appropriate to expose it *as this*, as opposed to an > > appropriate //net agnostication. > Ack. I've thrown this as straight copy of SpecialStoragePolicy methods in here > to start a conversation. > > https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... > File net/sqlite/sqlite_server_bound_cert_store.h (right): > > https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... > net/sqlite/sqlite_server_bound_cert_store.h:34: SpecialStoragePolicyDelegate* > special_storage_policy); > On 2014/07/10 20:20:25, Ryan Sleevi wrote: > > 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. > 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? > > https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... > File net/sqlite/sqlite_server_bound_cert_store_unittest.cc (right): > > https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... > net/sqlite/sqlite_server_bound_cert_store_unittest.cc:23: > FILE_PATH_LITERAL("Origin Bound Certs"); > On 2014/07/10 20:20:25, Ryan Sleevi wrote: > > pedantry: Doesn't need this constant, does it? You should just be able to use > > temp_dir_ directly. > > Um, not sure I understand. SQLiteServerBoundCertStore takes path to the file, > not to directory. ping?
At some point we should make the call about whether or not we want to move the "core" code into net/core and leave this as is, or move stuff like this into a net/util subdirectory. I kinda think we should do that now. I don't necessarily think we need to block this CL, but I'd kinda like to commit to doing this sooner than later. And I tend to think we should do a net/util directory, just so we don't have to redo all the includes in the repository (ugh!). Within net/util, I am fine with sqlite. On Mon, Jul 14, 2014 at 12:06 PM, <mef@chromium.org> wrote: > On 2014/07/11 13:29:24, mef wrote: > >> +willchan to comment on 'net/sqlite' name. >> +mmenke as profile_io_impl owner >> +droger as fyi. >> > > Thanks, PTAL. >> > > https://codereview.chromium.org/381073002/diff/60001/net/net.gyp >> File net/net.gyp (right): >> > > https://codereview.chromium.org/381073002/diff/60001/net/ >> net.gyp#newcode1048 >> net/net.gyp:1048: 'target_name': 'net_sqlite', >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: >> > net_sqlite_storage ? >> > >> > I think Will had stronger feelings about this than I do. >> > > net_storage? >> net_storage_sqlite? >> > > I'm completely open. >> > > > https://codereview.chromium.org/381073002/diff/60001/net/ > sqlite/special_storage_policy_delegate.h > >> File net/sqlite/special_storage_policy_delegate.h (right): >> > > > https://codereview.chromium.org/381073002/diff/60001/net/ > sqlite/special_storage_policy_delegate.h#newcode12 > >> net/sqlite/special_storage_policy_delegate.h:12: // Encapsulates methods >> of >> WebKit SpecialStoragePolicy used by net/sqlite. >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: >> > So, we would never mention this in a //net comment >> > > Done. >> > > > https://codereview.chromium.org/381073002/diff/60001/net/ > sqlite/special_storage_policy_delegate.h#newcode14 > >> net/sqlite/special_storage_policy_delegate.h:14: : public >> base::RefCountedThreadSafe<SpecialStoragePolicyDelegate>{ >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: >> > 1) Why ref-counted? Make ownership explicit >> > > Done. >> > > > https://codereview.chromium.org/381073002/diff/60001/net/ > sqlite/special_storage_policy_delegate.h#newcode21 > >> net/sqlite/special_storage_policy_delegate.h:21: virtual bool >> HasSessionOnlyOrigins() = 0; >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: >> > I'm not sure I grok how this fits into //net, but I'd need to understand >> > more. > >> > >> > That is, I understand there's a desire for some degree of this control, >> I >> > just > >> > question whether it's appropriate to expose it *as this*, as opposed to >> an >> > appropriate //net agnostication. >> Ack. I've thrown this as straight copy of SpecialStoragePolicy methods in >> here >> to start a conversation. >> > > > https://codereview.chromium.org/381073002/diff/60001/net/ > sqlite/sqlite_server_bound_cert_store.h > >> File net/sqlite/sqlite_server_bound_cert_store.h (right): >> > > > https://codereview.chromium.org/381073002/diff/60001/net/ > sqlite/sqlite_server_bound_cert_store.h#newcode34 > >> net/sqlite/sqlite_server_bound_cert_store.h:34: >> SpecialStoragePolicyDelegate* >> special_storage_policy); >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: >> > 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. >> 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? >> > > > https://codereview.chromium.org/381073002/diff/60001/net/ > sqlite/sqlite_server_bound_cert_store_unittest.cc > >> File net/sqlite/sqlite_server_bound_cert_store_unittest.cc (right): >> > > > https://codereview.chromium.org/381073002/diff/60001/net/ > sqlite/sqlite_server_bound_cert_store_unittest.cc#newcode23 > >> net/sqlite/sqlite_server_bound_cert_store_unittest.cc:23: >> FILE_PATH_LITERAL("Origin Bound Certs"); >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: >> > pedantry: Doesn't need this constant, does it? You should just be able >> to >> > use > >> > temp_dir_ directly. >> > > Um, not sure I understand. SQLiteServerBoundCertStore takes path to the >> file, >> not to directory. >> > > ping? > > https://codereview.chromium.org/381073002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... File net/sqlite/sqlite_server_bound_cert_store.h (right): https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... net/sqlite/sqlite_server_bound_cert_store.h:34: SpecialStoragePolicyDelegate* special_storage_policy); On 2014/07/11 13:29:24, mef wrote: > On 2014/07/10 20:20:25, Ryan Sleevi wrote: > > 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. > 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? Can you explain why it feels wrong? https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... File net/sqlite/sqlite_server_bound_cert_store_unittest.cc (right): https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... net/sqlite/sqlite_server_bound_cert_store_unittest.cc:23: FILE_PATH_LITERAL("Origin Bound Certs"); On 2014/07/11 13:29:24, mef wrote: > On 2014/07/10 20:20:25, Ryan Sleevi wrote: > > pedantry: Doesn't need this constant, does it? You should just be able to use > > temp_dir_ directly. > > Um, not sure I understand. SQLiteServerBoundCertStore takes path to the file, > not to directory. Acknowledged. https://codereview.chromium.org/381073002/diff/80001/net/sqlite/sqlite_server... File net/sqlite/sqlite_server_bound_cert_store.cc (right): https://codereview.chromium.org/381073002/diff/80001/net/sqlite/sqlite_server... net/sqlite/sqlite_server_bound_cert_store.cc:592: if (!url.is_valid() || !special_storage_policy_->IsStorageSessionOnly(url)) So, to start the discussion: If we look at this, the goal of this special_storage_policy_ is to clean up all OBCs that are marked as temporary (vis-a-vis session-only storage). Question 1: Does this IsStorageSessionOnly change dynamically (from creation)? My understanding was/is "No", but it might be worth checking. If no: - Can we plumb whether or not a given origin is/should be temporary at creation, rather than at shutdown? Question 2: Does it make more sense to have the "purge temporaries" be an exposed bit of functionality, and let the higher layer (//content? //chrome? Does this affect iOS/CroNet embedders?) purge the temporaries. My gut feeling is that this distinction - between persistence and impermanence - may not be right for this class. For example, this only considers impermanence at shutdown. If we think IsStorageSessionOnly() may change for a URL, then it's possible we create an OBC when permanence is denied, but when it's allowed, and we shutdown, we still allow the OBC to be saved to disk. Note that in both cases, we *always* write it to the SQLite store, which equally seems... Odd. Just thinking out loud here.
On 2014/07/14 19:17:51, willchan wrote: > At some point we should make the call about whether or not we want to move > the "core" code into net/core and leave this as is, or move stuff like this > into a net/util subdirectory. I kinda think we should do that now. I don't > necessarily think we need to block this CL, but I'd kinda like to commit to > doing this sooner than later. And I tend to think we should do a net/util > directory, just so we don't have to redo all the includes in the repository > (ugh!). Within net/util, I am fine with sqlite. SGTM. Just to clarify, should the structure be something like: net/util/ net/util/special_storage_policy_delegate.h net/util/sqlite/ net/util/sqlite/sqlite_server_bound_cert_store.cc or we don't need an extra sqlite/ subdirectory? > > > On Mon, Jul 14, 2014 at 12:06 PM, <mailto:mef@chromium.org> wrote: > > > On 2014/07/11 13:29:24, mef wrote: > > > >> +willchan to comment on 'net/sqlite' name. > >> +mmenke as profile_io_impl owner > >> +droger as fyi. > >> > > > > Thanks, PTAL. > >> > > > > https://codereview.chromium.org/381073002/diff/60001/net/net.gyp > >> File net/net.gyp (right): > >> > > > > https://codereview.chromium.org/381073002/diff/60001/net/ > >> net.gyp#newcode1048 > >> net/net.gyp:1048: 'target_name': 'net_sqlite', > >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: > >> > net_sqlite_storage ? > >> > > >> > I think Will had stronger feelings about this than I do. > >> > > > > net_storage? > >> net_storage_sqlite? > >> > > > > I'm completely open. > >> > > > > > > https://codereview.chromium.org/381073002/diff/60001/net/ > > sqlite/special_storage_policy_delegate.h > > > >> File net/sqlite/special_storage_policy_delegate.h (right): > >> > > > > > > https://codereview.chromium.org/381073002/diff/60001/net/ > > sqlite/special_storage_policy_delegate.h#newcode12 > > > >> net/sqlite/special_storage_policy_delegate.h:12: // Encapsulates methods > >> of > >> WebKit SpecialStoragePolicy used by net/sqlite. > >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: > >> > So, we would never mention this in a //net comment > >> > > > > Done. > >> > > > > > > https://codereview.chromium.org/381073002/diff/60001/net/ > > sqlite/special_storage_policy_delegate.h#newcode14 > > > >> net/sqlite/special_storage_policy_delegate.h:14: : public > >> base::RefCountedThreadSafe<SpecialStoragePolicyDelegate>{ > >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: > >> > 1) Why ref-counted? Make ownership explicit > >> > > > > Done. > >> > > > > > > https://codereview.chromium.org/381073002/diff/60001/net/ > > sqlite/special_storage_policy_delegate.h#newcode21 > > > >> net/sqlite/special_storage_policy_delegate.h:21: virtual bool > >> HasSessionOnlyOrigins() = 0; > >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: > >> > I'm not sure I grok how this fits into //net, but I'd need to understand > >> > > more. > > > >> > > >> > That is, I understand there's a desire for some degree of this control, > >> I > >> > > just > > > >> > question whether it's appropriate to expose it *as this*, as opposed to > >> an > >> > appropriate //net agnostication. > >> Ack. I've thrown this as straight copy of SpecialStoragePolicy methods in > >> here > >> to start a conversation. > >> > > > > > > https://codereview.chromium.org/381073002/diff/60001/net/ > > sqlite/sqlite_server_bound_cert_store.h > > > >> File net/sqlite/sqlite_server_bound_cert_store.h (right): > >> > > > > > > https://codereview.chromium.org/381073002/diff/60001/net/ > > sqlite/sqlite_server_bound_cert_store.h#newcode34 > > > >> net/sqlite/sqlite_server_bound_cert_store.h:34: > >> SpecialStoragePolicyDelegate* > >> special_storage_policy); > >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: > >> > 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. > >> 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? > >> > > > > > > https://codereview.chromium.org/381073002/diff/60001/net/ > > sqlite/sqlite_server_bound_cert_store_unittest.cc > > > >> File net/sqlite/sqlite_server_bound_cert_store_unittest.cc (right): > >> > > > > > > https://codereview.chromium.org/381073002/diff/60001/net/ > > sqlite/sqlite_server_bound_cert_store_unittest.cc#newcode23 > > > >> net/sqlite/sqlite_server_bound_cert_store_unittest.cc:23: > >> FILE_PATH_LITERAL("Origin Bound Certs"); > >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: > >> > pedantry: Doesn't need this constant, does it? You should just be able > >> to > >> > > use > > > >> > temp_dir_ directly. > >> > > > > Um, not sure I understand. SQLiteServerBoundCertStore takes path to the > >> file, > >> not to directory. > >> > > > > ping? > > > > https://codereview.chromium.org/381073002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... File net/sqlite/sqlite_server_bound_cert_store.h (right): https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... net/sqlite/sqlite_server_bound_cert_store.h:34: SpecialStoragePolicyDelegate* special_storage_policy); On 2014/07/14 19:18:04, Ryan Sleevi wrote: > On 2014/07/11 13:29:24, mef wrote: > > On 2014/07/10 20:20:25, Ryan Sleevi wrote: > > > 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. > > 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? > > 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. OTOH keeping parameter type as a SpecialStoragePolicyDelegate* doesn't express the fact that it will be owned by SQLiteServerBoundCertStore... https://codereview.chromium.org/381073002/diff/80001/net/sqlite/sqlite_server... File net/sqlite/sqlite_server_bound_cert_store.cc (right): https://codereview.chromium.org/381073002/diff/80001/net/sqlite/sqlite_server... net/sqlite/sqlite_server_bound_cert_store.cc:592: if (!url.is_valid() || !special_storage_policy_->IsStorageSessionOnly(url)) On 2014/07/14 19:18:04, Ryan Sleevi wrote: > So, to start the discussion: > > If we look at this, the goal of this special_storage_policy_ is to clean up all > OBCs that are marked as temporary (vis-a-vis session-only storage). > > Question 1: Does this IsStorageSessionOnly change dynamically (from creation)? > My understanding was/is "No", but it might be worth checking. > > If no: > - Can we plumb whether or not a given origin is/should be temporary at creation, > rather than at shutdown? > > Question 2: Does it make more sense to have the "purge temporaries" be an > exposed bit of functionality, and let the higher layer (//content? //chrome? > Does this affect iOS/CroNet embedders?) purge the temporaries. > > > My gut feeling is that this distinction - between persistence and impermanence - > may not be right for this class. > > For example, this only considers impermanence at shutdown. If we think > IsStorageSessionOnly() may change for a URL, then it's possible we create an OBC > when permanence is denied, but when it's allowed, and we shutdown, we still > allow the OBC to be saved to disk. Note that in both cases, we *always* write it > to the SQLite store, which equally seems... Odd. > > Just thinking out loud here. I see your concern, although I kind of agree with bludell's suggestion to separate c14n/refactoring from functional changes. I also assume that it currently functions as intended. :-)
On Mon, Jul 14, 2014 at 1:02 PM, <mef@chromium.org> wrote: > On 2014/07/14 19:17:51, willchan wrote: > >> At some point we should make the call about whether or not we want to move >> the "core" code into net/core and leave this as is, or move stuff like >> this >> into a net/util subdirectory. I kinda think we should do that now. I don't >> necessarily think we need to block this CL, but I'd kinda like to commit >> to >> doing this sooner than later. And I tend to think we should do a net/util >> directory, just so we don't have to redo all the includes in the >> repository >> (ugh!). Within net/util, I am fine with sqlite. >> > > SGTM. Just to clarify, should the structure be something like: > > net/util/ > net/util/special_storage_policy_delegate.h > net/util/sqlite/ > net/util/sqlite/sqlite_server_bound_cert_store.cc > > or we don't need an extra sqlite/ subdirectory? I don't feel too strongly. I care mostly that it's under net/util. My personal preference would be to use subdirectories within net/util, so yeah, net/util/sqlite. But I don't care as much to argue that one. If others prefer another way, I'm open to that. > > > > > > > On Mon, Jul 14, 2014 at 12:06 PM, <mailto:mef@chromium.org> wrote: >> > > > On 2014/07/11 13:29:24, mef wrote: >> > >> >> +willchan to comment on 'net/sqlite' name. >> >> +mmenke as profile_io_impl owner >> >> +droger as fyi. >> >> >> > >> > Thanks, PTAL. >> >> >> > >> > https://codereview.chromium.org/381073002/diff/60001/net/net.gyp >> >> File net/net.gyp (right): >> >> >> > >> > https://codereview.chromium.org/381073002/diff/60001/net/ >> >> net.gyp#newcode1048 >> >> net/net.gyp:1048: 'target_name': 'net_sqlite', >> >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: >> >> > net_sqlite_storage ? >> >> > >> >> > I think Will had stronger feelings about this than I do. >> >> >> > >> > net_storage? >> >> net_storage_sqlite? >> >> >> > >> > I'm completely open. >> >> >> > >> > >> > https://codereview.chromium.org/381073002/diff/60001/net/ >> > sqlite/special_storage_policy_delegate.h >> > >> >> File net/sqlite/special_storage_policy_delegate.h (right): >> >> >> > >> > >> > https://codereview.chromium.org/381073002/diff/60001/net/ >> > sqlite/special_storage_policy_delegate.h#newcode12 >> > >> >> net/sqlite/special_storage_policy_delegate.h:12: // Encapsulates >> methods >> >> of >> >> WebKit SpecialStoragePolicy used by net/sqlite. >> >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: >> >> > So, we would never mention this in a //net comment >> >> >> > >> > Done. >> >> >> > >> > >> > https://codereview.chromium.org/381073002/diff/60001/net/ >> > sqlite/special_storage_policy_delegate.h#newcode14 >> > >> >> net/sqlite/special_storage_policy_delegate.h:14: : public >> >> base::RefCountedThreadSafe<SpecialStoragePolicyDelegate>{ >> >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: >> >> > 1) Why ref-counted? Make ownership explicit >> >> >> > >> > Done. >> >> >> > >> > >> > https://codereview.chromium.org/381073002/diff/60001/net/ >> > sqlite/special_storage_policy_delegate.h#newcode21 >> > >> >> net/sqlite/special_storage_policy_delegate.h:21: virtual bool >> >> HasSessionOnlyOrigins() = 0; >> >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: >> >> > I'm not sure I grok how this fits into //net, but I'd need to >> understand >> >> >> > more. >> > >> >> > >> >> > That is, I understand there's a desire for some degree of this >> control, >> >> I >> >> >> > just >> > >> >> > question whether it's appropriate to expose it *as this*, as opposed >> to >> >> an >> >> > appropriate //net agnostication. >> >> Ack. I've thrown this as straight copy of SpecialStoragePolicy methods >> in >> >> here >> >> to start a conversation. >> >> >> > >> > >> > https://codereview.chromium.org/381073002/diff/60001/net/ >> > sqlite/sqlite_server_bound_cert_store.h >> > >> >> File net/sqlite/sqlite_server_bound_cert_store.h (right): >> >> >> > >> > >> > https://codereview.chromium.org/381073002/diff/60001/net/ >> > sqlite/sqlite_server_bound_cert_store.h#newcode34 >> > >> >> net/sqlite/sqlite_server_bound_cert_store.h:34: >> >> SpecialStoragePolicyDelegate* >> >> special_storage_policy); >> >> >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: >> >> > 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. >> >> 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? >> >> >> > >> > >> > https://codereview.chromium.org/381073002/diff/60001/net/ >> > sqlite/sqlite_server_bound_cert_store_unittest.cc >> > >> >> File net/sqlite/sqlite_server_bound_cert_store_unittest.cc (right): >> >> >> > >> > >> > https://codereview.chromium.org/381073002/diff/60001/net/ >> >> > sqlite/sqlite_server_bound_cert_store_unittest.cc#newcode23 >> > >> >> net/sqlite/sqlite_server_bound_cert_store_unittest.cc:23: >> >> FILE_PATH_LITERAL("Origin Bound Certs"); >> >> On 2014/07/10 20:20:25, Ryan Sleevi wrote: >> >> > pedantry: Doesn't need this constant, does it? You should just be >> able >> >> to >> >> >> > use >> > >> >> > temp_dir_ directly. >> >> >> > >> > Um, not sure I understand. SQLiteServerBoundCertStore takes path to the >> >> file, >> >> not to directory. >> >> >> > >> > ping? >> > >> > https://codereview.chromium.org/381073002/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > https://codereview.chromium.org/381073002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... File net/sqlite/sqlite_server_bound_cert_store.h (right): https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... net/sqlite/sqlite_server_bound_cert_store.h:29: : public net::DefaultServerBoundCertStore::PersistentStore { drop the net::, it's cleaner that way :) Throughout this file, and the other files you're moving into //net. https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... net/sqlite/sqlite_server_bound_cert_store.h:34: SpecialStoragePolicyDelegate* special_storage_policy); 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.
willchan: I've split them into to net/util and net/util/sqlite. rsleevi: SGTM, I've dropped the net::. https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... File net/sqlite/sqlite_server_bound_cert_store.h (right): https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... net/sqlite/sqlite_server_bound_cert_store.h:29: : public net::DefaultServerBoundCertStore::PersistentStore { On 2014/07/14 20:35:26, Ryan Sleevi wrote: > drop the net::, it's cleaner that way :) > > Throughout this file, and the other files you're moving into //net. Done. https://codereview.chromium.org/381073002/diff/60001/net/sqlite/sqlite_server... net/sqlite/sqlite_server_bound_cert_store.h:34: SpecialStoragePolicyDelegate* special_storage_policy); 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.
Just to force the conversation: Consider this a "Not LGTM" until we can discuss the policy delegate. If you're not getting the answers in a timely manner on this review, feel free to raise it at net-dev@ so we can discuss solutions. I do feel like this is coupling a particular implementation aspect of the higher layers, and something we should figure out in the broader context (it applies to cookies too)
https://codereview.chromium.org/381073002/diff/80001/net/sqlite/sqlite_server... File net/sqlite/sqlite_server_bound_cert_store.cc (right): https://codereview.chromium.org/381073002/diff/80001/net/sqlite/sqlite_server... net/sqlite/sqlite_server_bound_cert_store.cc:592: if (!url.is_valid() || !special_storage_policy_->IsStorageSessionOnly(url)) On 2014/07/14 19:18:04, Ryan Sleevi wrote: > So, to start the discussion: > > If we look at this, the goal of this special_storage_policy_ is to clean up all > OBCs that are marked as temporary (vis-a-vis session-only storage). > > Question 1: Does this IsStorageSessionOnly change dynamically (from creation)? > My understanding was/is "No", but it might be worth checking. I think the answer is 'Maybe'. The quota::SpecialStoragePolicy is an interface, which doesn't mention static vs. dynamic requirement. It appears to have 4 implementations, only one of which is neither mock, nor trivial: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... in that it forwards the call to cookie_settings_->IsCookieSessionOnly(origin); Please correct me if I'm wrong, but I suppose that cookie settings could change dynamically during session, and therefore IsStorageSessionOnly cannot be guaranteed to be the same. > If no: > - Can we plumb whether or not a given origin is/should be temporary at creation, > rather than at shutdown? I'd say not always. > Question 2: Does it make more sense to have the "purge temporaries" be an > exposed bit of functionality, and let the higher layer (//content? //chrome? > Does this affect iOS/CroNet embedders?) purge the temporaries. Um, but how would that higher level do that? Do we need to add "purge temporaries" interface to lower level? > My gut feeling is that this distinction - between persistence and impermanence - > may not be right for this class. > > For example, this only considers impermanence at shutdown. If we think > IsStorageSessionOnly() may change for a URL, then it's possible we create an OBC > when permanence is denied, but when it's allowed, and we shutdown, we still > allow the OBC to be saved to disk. Note that in both cases, we *always* write it > to the SQLite store, which equally seems... Odd. Umm, so what's wrong in this scenario? Both cookie and its cert will or will not be saved according to current permanence setting. I admit that if permanence is denied and browser is killed before/without cleanup, then some data may remain in the database (depending on its flush points). We could add another in-memory-only layer for non-permanent storage, but maybe this is a job for higher layer. > Just thinking out loud here. I appreciate that and would like to hear more opinions on this topic.
Consider this a drive-by because I'm not looking at the code. Can we use net/extras instead of net/util? "util" doesn't express the correct internal layering.
That bikeshed color is not my first choice, but I don't care about the color too much. Fine by me. On Thu, Jul 24, 2014 at 12:59 PM, <rvargas@chromium.org> wrote: > Consider this a drive-by because I'm not looking at the code. > > Can we use net/extras instead of net/util? "util" doesn't express the > correct > internal layering. > > https://codereview.chromium.org/381073002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Mostly looking good, needs moar tests though https://codereview.chromium.org/381073002/diff/160001/content/browser/net/quo... File content/browser/net/quota_policy_server_bound_cert_store.cc (right): https://codereview.chromium.org/381073002/diff/160001/content/browser/net/quo... content/browser/net/quota_policy_server_bound_cert_store.cc:76: return !url.is_valid() || !special_storage_policy_->IsStorageSessionOnly(url); What about certs that the ssp decides later are IsStorageSessionOnly, but they were put into persistent_? You seemed to think that could happen? https://codereview.chromium.org/381073002/diff/160001/content/browser/net/quo... File content/browser/net/quota_policy_server_bound_cert_store.h (right): https://codereview.chromium.org/381073002/diff/160001/content/browser/net/quo... content/browser/net/quota_policy_server_bound_cert_store.h:24: class QuotaPolicyServerBoundCertStore Add comments about what makes this special (i.e. Document) https://codereview.chromium.org/381073002/diff/160001/content/browser/net/quo... content/browser/net/quota_policy_server_bound_cert_store.h:46: // True if everything should go to persistent store. Newline before the member vars. https://codereview.chromium.org/381073002/diff/160001/net/util/sqlite/DEPS File net/util/sqlite/DEPS (right): https://codereview.chromium.org/381073002/diff/160001/net/util/sqlite/DEPS#ne... net/util/sqlite/DEPS:2: "+sql", +net + base right? https://codereview.chromium.org/381073002/diff/160001/net/util/sqlite/sqlite_... File net/util/sqlite/sqlite_server_bound_cert_store.h (right): https://codereview.chromium.org/381073002/diff/160001/net/util/sqlite/sqlite_... net/util/sqlite/sqlite_server_bound_cert_store.h:43: virtual ~SQLiteServerBoundCertStore(); Private?
On 2014/07/24 23:47:33, Ryan Sleevi wrote: > Mostly looking good, needs moar tests though > > https://codereview.chromium.org/381073002/diff/160001/content/browser/net/quo... > File content/browser/net/quota_policy_server_bound_cert_store.cc (right): > > https://codereview.chromium.org/381073002/diff/160001/content/browser/net/quo... > content/browser/net/quota_policy_server_bound_cert_store.cc:76: return > !url.is_valid() || !special_storage_policy_->IsStorageSessionOnly(url); > What about certs that the ssp decides later are IsStorageSessionOnly, but they > were put into persistent_? You seemed to think that could happen? > > https://codereview.chromium.org/381073002/diff/160001/content/browser/net/quo... > File content/browser/net/quota_policy_server_bound_cert_store.h (right): > > https://codereview.chromium.org/381073002/diff/160001/content/browser/net/quo... > content/browser/net/quota_policy_server_bound_cert_store.h:24: class > QuotaPolicyServerBoundCertStore > Add comments about what makes this special (i.e. Document) > > https://codereview.chromium.org/381073002/diff/160001/content/browser/net/quo... > content/browser/net/quota_policy_server_bound_cert_store.h:46: // True if > everything should go to persistent store. > Newline before the member vars. > > https://codereview.chromium.org/381073002/diff/160001/net/util/sqlite/DEPS > File net/util/sqlite/DEPS (right): > > https://codereview.chromium.org/381073002/diff/160001/net/util/sqlite/DEPS#ne... > net/util/sqlite/DEPS:2: "+sql", > +net + base right? > > https://codereview.chromium.org/381073002/diff/160001/net/util/sqlite/sqlite_... > File net/util/sqlite/sqlite_server_bound_cert_store.h (right): > > https://codereview.chromium.org/381073002/diff/160001/net/util/sqlite/sqlite_... > net/util/sqlite/sqlite_server_bound_cert_store.h:43: virtual > ~SQLiteServerBoundCertStore(); > Private? Ricardo: sgtm Ryan: Yes, this WIP, I'm side tracked to network triage and other high priority issues ATM. It almost feels like temporary store should be just a map in memory as the 'temp db' seems overkill.
Should be a bug for this.
On 2014/07/25 15:19:32, mmenke wrote: > Should be a bug for this. There is now.
It seems that the way interface is defined now the QuotaPolicyChannelIDStore should actually keep the list of ChannelIDs in memory (collected from LoadedCallback) and apply policy in destructor, which smells funky. Alternatively DefaultChannelIDStore::PersistentStore could expose a method to enumerate stored items with callback for keep/delete decision. WDYT? I'll add tests once we decide how is it supposed to work. https://codereview.chromium.org/381073002/diff/160001/content/browser/net/quo... File content/browser/net/quota_policy_server_bound_cert_store.cc (right): https://codereview.chromium.org/381073002/diff/160001/content/browser/net/quo... content/browser/net/quota_policy_server_bound_cert_store.cc:76: return !url.is_valid() || !special_storage_policy_->IsStorageSessionOnly(url); On 2014/07/24 23:47:32, Ryan Sleevi wrote: > What about certs that the ssp decides later are IsStorageSessionOnly, but they > were put into persistent_? You seemed to think that could happen? Good question, I actually don't know. I imagine that things could change during session (for example user changes cookie settings), but it seems that our current interface/model doesn't account for that. Interestingly, I suppose it could also to certs that are already in the database, and therefore keeping the list of added certs is not a solution. It seems to go back to the idea of adding an explicit method to iterate all items in the database with callback to delete them as needed. https://codereview.chromium.org/381073002/diff/160001/content/browser/net/quo... File content/browser/net/quota_policy_server_bound_cert_store.h (right): https://codereview.chromium.org/381073002/diff/160001/content/browser/net/quo... content/browser/net/quota_policy_server_bound_cert_store.h:24: class QuotaPolicyServerBoundCertStore On 2014/07/24 23:47:33, Ryan Sleevi wrote: > Add comments about what makes this special (i.e. Document) Done. https://codereview.chromium.org/381073002/diff/160001/content/browser/net/quo... content/browser/net/quota_policy_server_bound_cert_store.h:46: // True if everything should go to persistent store. On 2014/07/24 23:47:32, Ryan Sleevi wrote: > Newline before the member vars. Done. https://codereview.chromium.org/381073002/diff/160001/net/util/sqlite/DEPS File net/util/sqlite/DEPS (right): https://codereview.chromium.org/381073002/diff/160001/net/util/sqlite/DEPS#ne... net/util/sqlite/DEPS:2: "+sql", On 2014/07/24 23:47:33, Ryan Sleevi wrote: > +net + base right? Done. https://codereview.chromium.org/381073002/diff/160001/net/util/sqlite/sqlite_... File net/util/sqlite/sqlite_server_bound_cert_store.h (right): https://codereview.chromium.org/381073002/diff/160001/net/util/sqlite/sqlite_... net/util/sqlite/sqlite_server_bound_cert_store.h:43: virtual ~SQLiteServerBoundCertStore(); On 2014/07/24 23:47:33, Ryan Sleevi wrote: > Private? Done.
Bunch of cleanup nits about the pre-existing code. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:62: ScopedVector<DefaultChannelIDStore::ChannelID>* channel_ids); Should include the header for ScopedVector https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:69: DCHECK(num_pending_ == 0 && pending_.empty()); nit: This should be split into two DCHECKs. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:70: } Destructor should go before other methods in this section. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:77: typedef enum { CHANNEL_ID_ADD, CHANNEL_ID_DELETE } OperationType; nit: While here, enums are generally defined using C++ style in net/ https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:107: base::FilePath path_; const? https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:120: std::set<std::string> channel_id_origins_; Should include <string> https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:132: static const int kCompatibleVersionNumber = 1; Get rid of the static, and move into the namespace below, maybe? https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:134: namespace { This should go at the top of the file. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:323: sql::Statement smt( "smt" seems to violate the naming guidelines. Suggest just writing out statement. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.h (right): https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.h:20: namespace net { Should be a blank line after the namespace line. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.h:46: DISALLOW_COPY_AND_ASSIGN(SQLiteChannelIDStore); Know this was an old bug, but should include the header for this.
I've changed QuotaPolicyChannelIDStore to remove session only channel ids on shutdown, so it works the same way as before. I've also added moar tests! https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:62: ScopedVector<DefaultChannelIDStore::ChannelID>* channel_ids); On 2014/07/31 15:24:07, mmenke wrote: > Should include the header for ScopedVector Done. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:69: DCHECK(num_pending_ == 0 && pending_.empty()); On 2014/07/31 15:24:07, mmenke wrote: > nit: This should be split into two DCHECKs. Done. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:70: } On 2014/07/31 15:24:06, mmenke wrote: > Destructor should go before other methods in this section. Done. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:77: typedef enum { CHANNEL_ID_ADD, CHANNEL_ID_DELETE } OperationType; On 2014/07/31 15:24:07, mmenke wrote: > nit: While here, enums are generally defined using C++ style in net/ Done. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:107: base::FilePath path_; On 2014/07/31 15:24:06, mmenke wrote: > const? Done. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:120: std::set<std::string> channel_id_origins_; On 2014/07/31 15:24:07, mmenke wrote: > Should include <string> Done. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:132: static const int kCompatibleVersionNumber = 1; On 2014/07/31 15:24:06, mmenke wrote: > Get rid of the static, and move into the namespace below, maybe? Done. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:134: namespace { On 2014/07/31 15:24:06, mmenke wrote: > This should go at the top of the file. Done. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:323: sql::Statement smt( On 2014/07/31 15:24:06, mmenke wrote: > "smt" seems to violate the naming guidelines. Suggest just writing out > statement. Done. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.h (right): https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.h:20: namespace net { On 2014/07/31 15:24:07, mmenke wrote: > Should be a blank line after the namespace line. Done. https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.h:46: DISALLOW_COPY_AND_ASSIGN(SQLiteChannelIDStore); On 2014/07/31 15:24:07, mmenke wrote: > Know this was an old bug, but should include the header for this. Done.
On 2014/07/31 21:12:23, mef wrote: > I've changed QuotaPolicyChannelIDStore to remove session only channel ids on > shutdown, so it works the same way as before. > > I've also added moar tests! > > https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... > File net/extras/sqlite/sqlite_channel_id_store.cc (right): > > https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... > net/extras/sqlite/sqlite_channel_id_store.cc:62: > ScopedVector<DefaultChannelIDStore::ChannelID>* channel_ids); > On 2014/07/31 15:24:07, mmenke wrote: > > Should include the header for ScopedVector > > Done. > > https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... > net/extras/sqlite/sqlite_channel_id_store.cc:69: DCHECK(num_pending_ == 0 && > pending_.empty()); > On 2014/07/31 15:24:07, mmenke wrote: > > nit: This should be split into two DCHECKs. > > Done. > > https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... > net/extras/sqlite/sqlite_channel_id_store.cc:70: } > On 2014/07/31 15:24:06, mmenke wrote: > > Destructor should go before other methods in this section. > > Done. > > https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... > net/extras/sqlite/sqlite_channel_id_store.cc:77: typedef enum { CHANNEL_ID_ADD, > CHANNEL_ID_DELETE } OperationType; > On 2014/07/31 15:24:07, mmenke wrote: > > nit: While here, enums are generally defined using C++ style in net/ > > Done. > > https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... > net/extras/sqlite/sqlite_channel_id_store.cc:107: base::FilePath path_; > On 2014/07/31 15:24:06, mmenke wrote: > > const? > > Done. > > https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... > net/extras/sqlite/sqlite_channel_id_store.cc:120: std::set<std::string> > channel_id_origins_; > On 2014/07/31 15:24:07, mmenke wrote: > > Should include <string> > > Done. > > https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... > net/extras/sqlite/sqlite_channel_id_store.cc:132: static const int > kCompatibleVersionNumber = 1; > On 2014/07/31 15:24:06, mmenke wrote: > > Get rid of the static, and move into the namespace below, maybe? > > Done. > > https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... > net/extras/sqlite/sqlite_channel_id_store.cc:134: namespace { > On 2014/07/31 15:24:06, mmenke wrote: > > This should go at the top of the file. > > Done. > > https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... > net/extras/sqlite/sqlite_channel_id_store.cc:323: sql::Statement smt( > On 2014/07/31 15:24:06, mmenke wrote: > > "smt" seems to violate the naming guidelines. Suggest just writing out > > statement. > > Done. > > https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... > File net/extras/sqlite/sqlite_channel_id_store.h (right): > > https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... > net/extras/sqlite/sqlite_channel_id_store.h:20: namespace net { > On 2014/07/31 15:24:07, mmenke wrote: > > Should be a blank line after the namespace line. > > Done. > > https://codereview.chromium.org/381073002/diff/240001/net/extras/sqlite/sqlit... > net/extras/sqlite/sqlite_channel_id_store.h:46: > DISALLOW_COPY_AND_ASSIGN(SQLiteChannelIDStore); > On 2014/07/31 15:24:07, mmenke wrote: > > Know this was an old bug, but should include the header for this. > > Done. Um, ping?
Mostly cool, except I think you've got a pretty nasty threading bug hiding here. https://codereview.chromium.org/381073002/diff/260001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/381073002/diff/260001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:848: 'browser/net/spdyproxy/data_reduction_proxy_settings_factory_android.h', weird diff. Unrelated? https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:45: &ApplyPolicyOnShutdown, shutdown_store, special_storage_policy_)); Doesn't this create a race condition? That is: { QuotaPolicyChannelIDStore a("/foo", ...); a->Load(...); // stuff } { QuotaPolicyChannelIDStore b("/foo", ...); b->Load(...); } Then isn't b->Load() race with shutdown_store->Load()? Even if they used the same SequencedTaskRunner, if shutdown_store->Load internally decides to use PostTask (to process loading), then it's possible it will 'yield' control of the STR before finishing, and allow b->Load(...) to run. https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:67: /* static */ void QuotaPolicyChannelIDStore::ApplyPolicyOnShutdown( Delete the /* static */ Chromium style is typically // static void Class::Stuff(...) for cases like this. https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.h (right): https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:35: quota::SpecialStoragePolicy* special_storage_policy); Document https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:45: private: Unnecessary double private. Combine with 48 https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:51: scoped_refptr<net::DefaultChannelIDStore::PersistentStore> shutdown_store, const scoped_refptr<...>& https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:53: scoped_ptr<ChannelIDVector> channel_ids); This doesn't need to be a class static, does it? Seems to be accessing all public methods. Move it into the .cc in an unnamed namespace? https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:60: scoped_refptr<net::DefaultChannelIDStore::PersistentStore> persistent_; naming: persistent_store_ https://codereview.chromium.org/381073002/diff/260001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/260001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:94: DCHECK(num_pending_ == 0); DCHECK_EQ(0u, num_pending_) ? https://codereview.chromium.org/381073002/diff/260001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:101: friend class base::RefCountedThreadSafe<SQLiteChannelIDStore::Backend>; STYLE *nit*: "as in life, put your friends first" - move this to line 91 https://codereview.chromium.org/381073002/diff/260001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.h (right): https://codereview.chromium.org/381073002/diff/260001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.h:26: // |DefaultChannelIDStore::PersistentCertStore|. STYLE: We don't use || around class names. https://codereview.chromium.org/381073002/diff/260001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.h:31: const scoped_refptr<base::SequencedTaskRunner>& background_task_runner); Can/should document these - this isn't part of the PersistentStore API requirements.
Thanks, Ryan. You are right. Previously policy-based cleanup was done inside of transaction, so consecutive load would get consistent view. Changing it to loop of DeleteChannelID removes transaction boundaries and in fact add delays for operation batching. I think correct implementation would actually require adding an explicit method to iterate PersistentStore with callback and delete some items. WDYT? https://codereview.chromium.org/381073002/diff/260001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/381073002/diff/260001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:848: 'browser/net/spdyproxy/data_reduction_proxy_settings_factory_android.h', On 2014/08/06 22:39:58, Ryan Sleevi wrote: > weird diff. Unrelated? Done. https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:45: &ApplyPolicyOnShutdown, shutdown_store, special_storage_policy_)); On 2014/08/06 22:39:58, Ryan Sleevi wrote: > Doesn't this create a race condition? > > That is: > { > QuotaPolicyChannelIDStore a("/foo", ...); > a->Load(...); > // stuff > } > { > QuotaPolicyChannelIDStore b("/foo", ...); > b->Load(...); > } > > Then isn't b->Load() race with shutdown_store->Load()? > Even if they used the same SequencedTaskRunner, if shutdown_store->Load > internally decides to use PostTask (to process loading), then it's possible it > will 'yield' control of the STR before finishing, and allow b->Load(...) to run. Yeah, you are right. In current SQLiteChannelIDStore implementation b->Load would also post to the same STR, but DeleteChannelIDs will be batched and delay-posted for 30 seconds. I wonder why I don't see this happening in QuotaPolicyChannelIDStoreTest::TestPolicy https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:67: /* static */ void QuotaPolicyChannelIDStore::ApplyPolicyOnShutdown( On 2014/08/06 22:39:58, Ryan Sleevi wrote: > Delete the /* static */ > > Chromium style is typically > > // static > void Class::Stuff(...) > > for cases like this. Done. https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.h (right): https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:35: quota::SpecialStoragePolicy* special_storage_policy); On 2014/08/06 22:39:58, Ryan Sleevi wrote: > Document Done. I think. If not, could you elaborate? https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:45: private: On 2014/08/06 22:39:58, Ryan Sleevi wrote: > Unnecessary double private. Combine with 48 Done. https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:51: scoped_refptr<net::DefaultChannelIDStore::PersistentStore> shutdown_store, On 2014/08/06 22:39:58, Ryan Sleevi wrote: > const scoped_refptr<...>& Done. https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:53: scoped_ptr<ChannelIDVector> channel_ids); On 2014/08/06 22:39:58, Ryan Sleevi wrote: > This doesn't need to be a class static, does it? Seems to be accessing all > public methods. Move it into the .cc in an unnamed namespace? Done. https://codereview.chromium.org/381073002/diff/260001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:60: scoped_refptr<net::DefaultChannelIDStore::PersistentStore> persistent_; On 2014/08/06 22:39:58, Ryan Sleevi wrote: > naming: persistent_store_ Done. https://codereview.chromium.org/381073002/diff/260001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/260001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:94: DCHECK(num_pending_ == 0); On 2014/08/06 22:39:58, Ryan Sleevi wrote: > DCHECK_EQ(0u, num_pending_) ? Done. https://codereview.chromium.org/381073002/diff/260001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:101: friend class base::RefCountedThreadSafe<SQLiteChannelIDStore::Backend>; On 2014/08/06 22:39:58, Ryan Sleevi wrote: > STYLE *nit*: "as in life, put your friends first" - move this to line 91 Done. https://codereview.chromium.org/381073002/diff/260001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.h (right): https://codereview.chromium.org/381073002/diff/260001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.h:26: // |DefaultChannelIDStore::PersistentCertStore|. On 2014/08/06 22:39:58, Ryan Sleevi wrote: > STYLE: We don't use || around class names. Done. https://codereview.chromium.org/381073002/diff/260001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.h:31: const scoped_refptr<base::SequencedTaskRunner>& background_task_runner); On 2014/08/06 22:39:58, Ryan Sleevi wrote: > Can/should document these - this isn't part of the PersistentStore API > requirements. Done.
On 2014/08/07 17:40:59, mef wrote:
> Thanks, Ryan.
>
> You are right. Previously policy-based cleanup was done inside of transaction,
> so consecutive load would get consistent view.
> Changing it to loop of DeleteChannelID removes transaction boundaries and in
> fact add delays for operation batching.
>
> I think correct implementation would actually require adding an explicit
method
> to iterate PersistentStore with callback and delete some items.
>
> WDYT?
That doesn't sound right/desirable.
Consider how the existing implementation works. When load is called, it creates
a vector of all the channel IDs it has. Whenever it adds or removes a channel
ID, it updates said vector. Finally, when it's time to nuke, it uses that vector
to iterate with the storage policy, deleting all of those within a transaction.
You could easily model this with your two-split as such:
1) move channel_id_origins_ from being part of your SQLite backend (where it's
unused) to being part of your Quota class
2) Update your quota class to add/remove to the channel_id_origins_ as
appropriate (Load, Delete, Add)
3) When it's time for you to shutdown
vector<...> to_remove;
for (it = origins_.begin(); ...) {
if (should_remove(*it))
to_remove.push_back(*it);
}
DeleteAll(to_remove);
4) Where DeleteAll is a method on the SQLite store, that takes a vector of
channel ID indices/origins/whatever, and in a transaction, nukes them all (more
aptly, I presume it posts a background task to immediately commit on the backend
a statement that nukes them all)
The way things are structured today (as in, pre-your change), the Backend seems
like it can outlive the Store, in order to support these async shutdown ops, so
you "should" be fine with respect to ordering. The 'only' issue would be if one
creates a new Quota store with a *new* (as in, different) background task
runner, because then you get into ordering issues. But that issue *already*
exists in today's SQLite store, so it doesn't seem a dealbreaker from the API
perspective.
https://codereview.chromium.org/381073002/diff/300001/net/extras/sqlite/sqlit...
File net/extras/sqlite/sqlite_channel_id_store.cc (right):
https://codereview.chromium.org/381073002/diff/300001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store.cc:151: std::set<std::string>
channel_id_origins_;
With the backend deletion code removed, you ever actually use this anymore.
That is, it existed as a cache to prevent you from having to read in all the
channel IDs, but with the new code, you're calling Load(), and you always read
these in.
https://codereview.chromium.org/381073002/diff/300001/net/extras/sqlite/sqlit...
File net/extras/sqlite/sqlite_channel_id_store.h (right):
https://codereview.chromium.org/381073002/diff/300001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store.h:40: const
DefaultChannelIDStore::ChannelID& channel_idx) OVERRIDE;
Worth noting that these two parameters have different names. Is one an ID and
one an index?
Ryan, thanks, PTAL. I'm not very happy about separate tracking of server_identifiers in quota_policy store, but I'll get over it as it doesn't seem much worse than it used to be. :) https://codereview.chromium.org/381073002/diff/300001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/300001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:151: std::set<std::string> channel_id_origins_; On 2014/08/07 22:00:22, Ryan Sleevi wrote: > With the backend deletion code removed, you ever actually use this anymore. > > That is, it existed as a cache to prevent you from having to read in all the > channel IDs, but with the new code, you're calling Load(), and you always read > these in. Done. https://codereview.chromium.org/381073002/diff/300001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.h (right): https://codereview.chromium.org/381073002/diff/300001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.h:40: const DefaultChannelIDStore::ChannelID& channel_idx) OVERRIDE; On 2014/08/07 22:00:22, Ryan Sleevi wrote: > Worth noting that these two parameters have different names. Is one an ID and > one an index? Nope, both are ChannelID, seems like a spurious x.
I think this LGTM, mod our discussion on gchat that the DeleteAll method only needs to be on the SQLite store, and not the Default store (and doesn't need to be virtual either - just a concrete instance method only applicable to SQLite stores) Please get mmenke@ to make a once-over as well, since he's been doing a good bit of reviewing. https://codereview.chromium.org/381073002/diff/320001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/320001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:50: if (!session_only_server_identifiers.empty()) { No braces for one-liners? Or is this //content style? https://codereview.chromium.org/381073002/diff/320001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/320001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:440: // Must close the backend on the background thread. Update comment https://codereview.chromium.org/381073002/diff/320001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:590: LOG(WARNING) << "Unable to delete channel ids on shutdown."; Update comment (not guaranteed to be in shutdown anymore)
sleevi@ - thanks! mmenke@ - could you do a once over before I go out to OWNERS of sql/ and content/browser/net/? https://codereview.chromium.org/381073002/diff/320001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/320001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:50: if (!session_only_server_identifiers.empty()) { On 2014/08/11 18:33:12, Ryan Sleevi wrote: > No braces for one-liners? Or is this //content style? Done. https://codereview.chromium.org/381073002/diff/320001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/320001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:440: // Must close the backend on the background thread. On 2014/08/11 18:33:12, Ryan Sleevi wrote: > Update comment Done. https://codereview.chromium.org/381073002/diff/320001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:590: LOG(WARNING) << "Unable to delete channel ids on shutdown."; On 2014/08/11 18:33:12, Ryan Sleevi wrote: > Update comment (not guaranteed to be in shutdown anymore) Done.
These are just from a quick skim, want to spend some more time digging through this, but may not have time today. https://codereview.chromium.org/381073002/diff/360001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/360001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:46: const GURL url(net::cookie_util::CookieOriginToURL(*it, true)); nit: const is generally not used https://codereview.chromium.org/381073002/diff/360001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:50: if (!session_only_server_identifiers.empty()) Would it make sense to put this logic in DeleteAll instead? https://codereview.chromium.org/381073002/diff/360001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store_unittest.cc (right): https://codereview.chromium.org/381073002/diff/360001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store_unittest.cc:49: static base::Time GetTestCertExpirationTime() { include time header https://codereview.chromium.org/381073002/diff/360001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/360001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:9: #include <string> Not needed once this is in header https://codereview.chromium.org/381073002/diff/360001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:97: ~Backend() { Classes that inherit from other classes should have virtual destructors. https://codereview.chromium.org/381073002/diff/360001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.h (right): https://codereview.chromium.org/381073002/diff/360001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.h:44: void DeleteAll(const std::vector<std::string>& server_identifiers); Need to include vector and string headers. https://codereview.chromium.org/381073002/diff/360001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.h:44: void DeleteAll(const std::vector<std::string>& server_identifiers); optional nit: I think "DeleteAll" is a little confusing, since it doesn't delete all, but deletes everything from a passed in list. Maybe just DeleteByIdentifier or even just Delete.
Also, I think your description of this issue is out of date.
Thanks Matt! I would also like to humbly ask for OWNERS approval: erikwright - content/browser/net/quota_policy_channel_id_store* and adding /sql to /net/extras/sqlite/DEPS. thestig - adding adding /content/browser/net to chrome/browser/net/DEPS. thanks, -m https://codereview.chromium.org/381073002/diff/360001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/360001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:46: const GURL url(net::cookie_util::CookieOriginToURL(*it, true)); On 2014/08/12 17:12:34, mmenke wrote: > nit: const is generally not used Done. https://codereview.chromium.org/381073002/diff/360001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:50: if (!session_only_server_identifiers.empty()) On 2014/08/12 17:12:34, mmenke wrote: > Would it make sense to put this logic in DeleteAll instead? Done. https://codereview.chromium.org/381073002/diff/360001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store_unittest.cc (right): https://codereview.chromium.org/381073002/diff/360001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store_unittest.cc:49: static base::Time GetTestCertExpirationTime() { On 2014/08/12 17:12:34, mmenke wrote: > include time header Done. https://codereview.chromium.org/381073002/diff/360001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/360001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:9: #include <string> On 2014/08/12 17:12:34, mmenke wrote: > Not needed once this is in header Done. https://codereview.chromium.org/381073002/diff/360001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:97: ~Backend() { On 2014/08/12 17:12:34, mmenke wrote: > Classes that inherit from other classes should have virtual destructors. Done. https://codereview.chromium.org/381073002/diff/360001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.h (right): https://codereview.chromium.org/381073002/diff/360001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.h:44: void DeleteAll(const std::vector<std::string>& server_identifiers); On 2014/08/12 17:12:34, mmenke wrote: > Need to include vector and string headers. Done. https://codereview.chromium.org/381073002/diff/360001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.h:44: void DeleteAll(const std::vector<std::string>& server_identifiers); On 2014/08/12 17:12:35, mmenke wrote: > optional nit: I think "DeleteAll" is a little confusing, since it doesn't > delete all, but deletes everything from a passed in list. Maybe just > DeleteByIdentifier or even just Delete. Done.
What happens in the case of an unclean shutdown? What is intended to happen? How does this compare to other session-only state that is persisted to disk and potentially spans browser process lifetimes? I believe other data types can intentionally be persisted across a process relaunch (in the case of an upgrade, for example). Does your data do this/need to do this? https://codereview.chromium.org/381073002/diff/380001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.h (right): https://codereview.chromium.org/381073002/diff/380001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:34: : public net::DefaultChannelIDStore::PersistentStore { Shouldn't you include "net/ssl/default_channel_id_store.h"?
LGTM, just a couple nits. https://codereview.chromium.org/381073002/diff/380001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/380001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:8: #include <set> nit: set included in header, so not needed here. https://codereview.chromium.org/381073002/diff/380001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:72: force_keep_session_state_ = true; Doe sit make more sense just to clear the special_storage_policy_, instead of adding a bool for this? I'm fine with the bool, just wondering if it's needed. https://codereview.chromium.org/381073002/diff/380001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/380001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:49: return false; nit: Use braces
Matt, thanks! Eric, this CL preserves existing behavior (just splitting sql persistance and policy application), meaning that it applies policy only on clean shutdown. On unclean shutdown all channel ids are left in the persistent store to be cleaned up on next shutdown. We were discussing the idea of storing session-only items in memory or in temporary store, but that didn't support change of policy during session (e.g. user changing cookie settings). https://codereview.chromium.org/381073002/diff/380001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/380001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:8: #include <set> On 2014/08/12 19:58:40, mmenke wrote: > nit: set included in header, so not needed here. Done. https://codereview.chromium.org/381073002/diff/380001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:72: force_keep_session_state_ = true; On 2014/08/12 19:58:40, mmenke wrote: > Doe sit make more sense just to clear the special_storage_policy_, instead of > adding a bool for this? I'm fine with the bool, just wondering if it's needed. Done. It was moved from original code. https://codereview.chromium.org/381073002/diff/380001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.h (right): https://codereview.chromium.org/381073002/diff/380001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:34: : public net::DefaultChannelIDStore::PersistentStore { On 2014/08/12 19:48:09, erikwright wrote: > Shouldn't you include "net/ssl/default_channel_id_store.h"? Done. https://codereview.chromium.org/381073002/diff/380001/net/extras/sqlite/sqlit... File net/extras/sqlite/sqlite_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/380001/net/extras/sqlite/sqlit... net/extras/sqlite/sqlite_channel_id_store.cc:49: return false; On 2014/08/12 19:58:40, mmenke wrote: > nit: Use braces Done.
https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:29: : path_(path), why is this member needed? https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:30: background_task_runner_(background_task_runner), also not needed https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:32: persistent_store_( Consider accepting a DefaultChannelIDStore::PersistentStore to delegate to rather than accepting the constructor parameters and hardcoding the implementation class. This would simplify this class and make it easier to test (if you chose, you could have a dumb in-memory single-threaded backing store rather than having to verify round-trips to disk, deal with message loops, etc). Cost is that the caller needs to explicitly reference SQLiteChannelIDStore, but that seems reasonable to me. https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:39: special_storage_policy_->HasSessionOnlyOrigins()) { consider reversing the logic and returning early if possible, to decrease nesting. https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.h (right): https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:64: scoped_refptr<net::SQLiteChannelIDStore> persistent_store_; Does this need to be stored as the derived type, or is the base type sufficient? https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:68: DISALLOW_COPY_AND_ASSIGN(QuotaPolicyChannelIDStore); #include "base/macros.h"
Thanks Eric, PTAL. https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:29: : path_(path), On 2014/08/12 20:56:11, erikwright wrote: > why is this member needed? Done. https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:30: background_task_runner_(background_task_runner), On 2014/08/12 20:56:12, erikwright wrote: > also not needed Done. https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:32: persistent_store_( On 2014/08/12 20:56:12, erikwright wrote: > Consider accepting a DefaultChannelIDStore::PersistentStore to delegate to > rather than accepting the constructor parameters and hardcoding the > implementation class. > > This would simplify this class and make it easier to test (if you chose, you > could have a dumb in-memory single-threaded backing store rather than having to > verify round-trips to disk, deal with message loops, etc). Cost is that the > caller needs to explicitly reference SQLiteChannelIDStore, but that seems > reasonable to me. Ack. The patchset #17 (https://codereview.chromium.org/381073002/#ps320001) added DeleteAll method to PersistentStore interface, but per discussion with sleevi@ (see message #27) I removed it from the interface, so now it only works with SQLiteChannelIDStore. I think it is fine as we can refactor it if needed in the future, but please let me know if you feel strongly about it. https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:39: special_storage_policy_->HasSessionOnlyOrigins()) { On 2014/08/12 20:56:12, erikwright wrote: > consider reversing the logic and returning early if possible, to decrease > nesting. Done. https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.h (right): https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:64: scoped_refptr<net::SQLiteChannelIDStore> persistent_store_; On 2014/08/12 20:56:12, erikwright wrote: > Does this need to be stored as the derived type, or is the base type sufficient? DefaultChannelIDStore::PersistentStore doesn't have DeleteAllInList method. https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.h:68: DISALLOW_COPY_AND_ASSIGN(QuotaPolicyChannelIDStore); On 2014/08/12 20:56:12, erikwright wrote: > #include "base/macros.h" Done.
https://codereview.chromium.org/381073002/diff/420001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/381073002/diff/420001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:35: #include "content/browser/net/quota_policy_channel_id_store.h" Don't you have to go through content/public ?
On 2014/08/12 21:27:20, Lei Zhang wrote: > https://codereview.chromium.org/381073002/diff/420001/chrome/browser/profiles... > File chrome/browser/profiles/profile_impl_io_data.cc (right): > > https://codereview.chromium.org/381073002/diff/420001/chrome/browser/profiles... > chrome/browser/profiles/profile_impl_io_data.cc:35: #include > "content/browser/net/quota_policy_channel_id_store.h" > Don't you have to go through content/public ? Mea culpa, I guess I do have to go through content/public. I have a question though. Reading http://www.chromium.org/developers/content-module/content-api suggests that content/public should only contain the interface, but QuotaPolicyChannelIDStore is actually just implementing net::DefaultChannelIDStore::PersistentStore, but takes quota::SpecialStoragePolicy in the constructor. Should I just have quota_policy_channel_id_store.h with such constructor and rename content/browser/net/quota_policy_channel_id_store.* into quota_policy_channel_id_store_impl.* ?
On 2014/08/12 21:39:17, mef wrote: > Mea culpa, I guess I do have to go through content/public. Can I ask why you want to put this code in content/ in the first place? I tried following the review, but I couldn't figure out what happened between patch sets 8 and 9.
On 2014/08/12 21:50:36, Lei Zhang wrote: > On 2014/08/12 21:39:17, mef wrote: > > Mea culpa, I guess I do have to go through content/public. > > Can I ask why you want to put this code in content/ in the first place? I tried > following the review, but I couldn't figure out what happened between patch sets > 8 and 9. This CL is part of componetization effort to move some persistence classes from chrome/browser/net into net, so they would be available without Chrome (e.g. to Cronet). The reason for the split between SQLiteChannelIDStore and QuotaPolicyChannelIDStore is that we don't want net/ to know about storage policy, so content/browser/net seemed like a good choice. :) Now that I wrote this maybe QuotaPolicyChannelIDStore should stay in chrome/browser/net if it doesn't belong to content/?
On 2014/08/12 22:00:38, mef wrote: > On 2014/08/12 21:50:36, Lei Zhang wrote: > > On 2014/08/12 21:39:17, mef wrote: > > > Mea culpa, I guess I do have to go through content/public. > > > > Can I ask why you want to put this code in content/ in the first place? I > tried > > following the review, but I couldn't figure out what happened between patch > sets > > 8 and 9. > > This CL is part of componetization effort to move some persistence classes from > chrome/browser/net into net, so they would be available without Chrome (e.g. to > Cronet). > The reason for the split between SQLiteChannelIDStore and > QuotaPolicyChannelIDStore is that we don't want net/ to know about storage > policy, so content/browser/net seemed like a good choice. :) > > Now that I wrote this maybe QuotaPolicyChannelIDStore should stay in > chrome/browser/net if it doesn't belong to content/? You probably should ask content/ OWNERS and/or content/browser/net OWNERS about where this code belongs. I was just curious. If you do put it in content, all you probably need to do is put a static function in content/public/browser that calls QuotaPolicyChannelIDStore's ctor, revert the chrome/browser/DEPS change and update the callers.
Lei, thanks! Per your suggestion and chat with Ryan, I've moved QuotaPolicyChannelIDStore from content/browser/net to chrome/browser/net. I would appreciate OWNER approval of net/extras/sqlite/DEPS: erikwright: +sql willchan: +base thanks, -m On 2014/08/12 22:14:57, Lei Zhang wrote: > On 2014/08/12 22:00:38, mef wrote: > > On 2014/08/12 21:50:36, Lei Zhang wrote: > > > On 2014/08/12 21:39:17, mef wrote: > > > > Mea culpa, I guess I do have to go through content/public. > > > > > > Can I ask why you want to put this code in content/ in the first place? I > > tried > > > following the review, but I couldn't figure out what happened between patch > > sets > > > 8 and 9. > > > > This CL is part of componetization effort to move some persistence classes > from > > chrome/browser/net into net, so they would be available without Chrome (e.g. > to > > Cronet). > > The reason for the split between SQLiteChannelIDStore and > > QuotaPolicyChannelIDStore is that we don't want net/ to know about storage > > policy, so content/browser/net seemed like a good choice. :) > > > > Now that I wrote this maybe QuotaPolicyChannelIDStore should stay in > > chrome/browser/net if it doesn't belong to content/? > > You probably should ask content/ OWNERS and/or content/browser/net OWNERS about > where this code belongs. I was just curious. > > If you do put it in content, all you probably need to do is put a static > function in content/public/browser that calls QuotaPolicyChannelIDStore's ctor, > revert the chrome/browser/DEPS change and update the callers.
You aren't adding base/ - it was already in net/extras/sqlite/DEPS, no? You're adding net/ and sql/. On 2014/08/13 15:35:35, mef wrote: > Lei, thanks! Per your suggestion and chat with Ryan, I've moved > QuotaPolicyChannelIDStore from content/browser/net to chrome/browser/net. > > I would appreciate OWNER approval of net/extras/sqlite/DEPS: > > erikwright: +sql > willchan: +base > > thanks, > -m > > On 2014/08/12 22:14:57, Lei Zhang wrote: > > On 2014/08/12 22:00:38, mef wrote: > > > On 2014/08/12 21:50:36, Lei Zhang wrote: > > > > On 2014/08/12 21:39:17, mef wrote: > > > > > Mea culpa, I guess I do have to go through content/public. > > > > > > > > Can I ask why you want to put this code in content/ in the first place? I > > > tried > > > > following the review, but I couldn't figure out what happened between > patch > > > sets > > > > 8 and 9. > > > > > > This CL is part of componetization effort to move some persistence classes > > from > > > chrome/browser/net into net, so they would be available without Chrome (e.g. > > to > > > Cronet). > > > The reason for the split between SQLiteChannelIDStore and > > > QuotaPolicyChannelIDStore is that we don't want net/ to know about storage > > > policy, so content/browser/net seemed like a good choice. :) > > > > > > Now that I wrote this maybe QuotaPolicyChannelIDStore should stay in > > > chrome/browser/net if it doesn't belong to content/? > > > > You probably should ask content/ OWNERS and/or content/browser/net OWNERS > about > > where this code belongs. I was just curious. > > > > If you do put it in content, all you probably need to do is put a static > > function in content/public/browser that calls QuotaPolicyChannelIDStore's > ctor, > > revert the chrome/browser/DEPS change and update the callers.
As mef just pointed out, I was wrong about that. I was confused by the DEPS file of the new directory being considered a modified version of a DEPS file in some other random directory. On 2014/08/13 15:38:49, mmenke wrote: > You aren't adding base/ - it was already in net/extras/sqlite/DEPS, no? You're > adding net/ and sql/. > > On 2014/08/13 15:35:35, mef wrote: > > Lei, thanks! Per your suggestion and chat with Ryan, I've moved > > QuotaPolicyChannelIDStore from content/browser/net to chrome/browser/net. > > > > I would appreciate OWNER approval of net/extras/sqlite/DEPS: > > > > erikwright: +sql > > willchan: +base > > > > thanks, > > -m > > > > On 2014/08/12 22:14:57, Lei Zhang wrote: > > > On 2014/08/12 22:00:38, mef wrote: > > > > On 2014/08/12 21:50:36, Lei Zhang wrote: > > > > > On 2014/08/12 21:39:17, mef wrote: > > > > > > Mea culpa, I guess I do have to go through content/public. > > > > > > > > > > Can I ask why you want to put this code in content/ in the first place? > I > > > > tried > > > > > following the review, but I couldn't figure out what happened between > > patch > > > > sets > > > > > 8 and 9. > > > > > > > > This CL is part of componetization effort to move some persistence classes > > > from > > > > chrome/browser/net into net, so they would be available without Chrome > (e.g. > > > to > > > > Cronet). > > > > The reason for the split between SQLiteChannelIDStore and > > > > QuotaPolicyChannelIDStore is that we don't want net/ to know about storage > > > > policy, so content/browser/net seemed like a good choice. :) > > > > > > > > Now that I wrote this maybe QuotaPolicyChannelIDStore should stay in > > > > chrome/browser/net if it doesn't belong to content/? > > > > > > You probably should ask content/ OWNERS and/or content/browser/net OWNERS > > about > > > where this code belongs. I was just curious. > > > > > > If you do put it in content, all you probably need to do is put a static > > > function in content/public/browser that calls QuotaPolicyChannelIDStore's > > ctor, > > > revert the chrome/browser/DEPS change and update the callers.
net/extras/sqlite/DEPS is a new file as net/extras/sqlite is a new directory. I guess that DEPS inheritance doesn't work here because there is no net/extras/DEPS file. On 2014/08/13 15:38:49, mmenke wrote: > You aren't adding base/ - it was already in net/extras/sqlite/DEPS, no? You're > adding net/ and sql/. > > On 2014/08/13 15:35:35, mef wrote: > > Lei, thanks! Per your suggestion and chat with Ryan, I've moved > > QuotaPolicyChannelIDStore from content/browser/net to chrome/browser/net. > > > > I would appreciate OWNER approval of net/extras/sqlite/DEPS: > > > > erikwright: +sql > > willchan: +base > > > > thanks, > > -m > > > > On 2014/08/12 22:14:57, Lei Zhang wrote: > > > On 2014/08/12 22:00:38, mef wrote: > > > > On 2014/08/12 21:50:36, Lei Zhang wrote: > > > > > On 2014/08/12 21:39:17, mef wrote: > > > > > > Mea culpa, I guess I do have to go through content/public. > > > > > > > > > > Can I ask why you want to put this code in content/ in the first place? > I > > > > tried > > > > > following the review, but I couldn't figure out what happened between > > patch > > > > sets > > > > > 8 and 9. > > > > > > > > This CL is part of componetization effort to move some persistence classes > > > from > > > > chrome/browser/net into net, so they would be available without Chrome > (e.g. > > > to > > > > Cronet). > > > > The reason for the split between SQLiteChannelIDStore and > > > > QuotaPolicyChannelIDStore is that we don't want net/ to know about storage > > > > policy, so content/browser/net seemed like a good choice. :) > > > > > > > > Now that I wrote this maybe QuotaPolicyChannelIDStore should stay in > > > > chrome/browser/net if it doesn't belong to content/? > > > > > > You probably should ask content/ OWNERS and/or content/browser/net OWNERS > > about > > > where this code belongs. I was just curious. > > > > > > If you do put it in content, all you probably need to do is put a static > > > function in content/public/browser that calls QuotaPolicyChannelIDStore's > > ctor, > > > revert the chrome/browser/DEPS change and update the callers.
net/extras/sqlite/DEPS LGTM https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... File content/browser/net/quota_policy_channel_id_store.cc (right): https://codereview.chromium.org/381073002/diff/400001/content/browser/net/quo... content/browser/net/quota_policy_channel_id_store.cc:32: persistent_store_( On 2014/08/12 21:11:22, mef wrote: > On 2014/08/12 20:56:12, erikwright wrote: > > Consider accepting a DefaultChannelIDStore::PersistentStore to delegate to > > rather than accepting the constructor parameters and hardcoding the > > implementation class. > > > > This would simplify this class and make it easier to test (if you chose, you > > could have a dumb in-memory single-threaded backing store rather than having > to > > verify round-trips to disk, deal with message loops, etc). Cost is that the > > caller needs to explicitly reference SQLiteChannelIDStore, but that seems > > reasonable to me. > > Ack. The patchset #17 (https://codereview.chromium.org/381073002/#ps320001) > added DeleteAll method to PersistentStore interface, but per discussion with > sleevi@ (see message #27) I removed it from the interface, so now it only works > with SQLiteChannelIDStore. > I think it is fine as we can refactor it if needed in the future, but please let > me know if you feel strongly about it. It's your choice. I think you will remove dozens of lines and much complexity from the unit test if you add that method to the interface and follow my recommendation.
On 2014/08/13 15:49:09, erikwright wrote: > net/extras/sqlite/DEPS LGTM > It's your choice. I think you will remove dozens of lines and much complexity > from the unit test if you add that method to the interface and follow my > recommendation. I think when faced with interface complexity vs test complexity, we should prefer test complexity (i.e. if it keeps the interface cleaner). There's no good/justifiable layering reason for it to be part of the interface - its purely to satisfy some higher layer's need. Because of that, we should keep it as close to the code it's used - even if that means a little more complexity testing. I view it the same as testing code in //chrome vs //net - yeah, its more complex, but it keeps our layering clean. That said, if we ever have need (beyond testing) of this functionally, totally cool with pushing it down, same as moving code from //chrome to //net :)
chrome/ lgtm with the comment below addressed. https://codereview.chromium.org/381073002/diff/460001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/381073002/diff/460001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:2972: '../net/net.gyp:net_extras', Please update chrome/browser/BUILD.gn to match this.
The base dependency in the new DEPS file lgtm.
The CQ bit was checked by willchan@chromium.org
lgtm mef already left the office, but i checked with him earlier to see if i should cq this if i approved. he said yes. i'm sending this to cq.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/381073002/460001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/381073002/500001
Message was sent while issue was closed.
Committed patchset #26 (500001) as 289996
Message was sent while issue was closed.
A revert of this CL (patchset #26) has been created in https://codereview.chromium.org/477253002/ by miu@chromium.org. The reason for reverting is: Closed the tree on failing net_unittests: http://build.chromium.org/p/chromium.linux/buildstatus?builder=Linux%20Tests%....
Message was sent while issue was closed.
On 2014/08/15 22:51:58, miu_OOO_until_Aug_25 wrote: > A revert of this CL (patchset #26) has been created in > https://codereview.chromium.org/477253002/ by mailto:miu@chromium.org. > > The reason for reverting is: Closed the tree on failing net_unittests: > > http://build.chromium.org/p/chromium.linux/buildstatus?builder=Linux%20Tests%.... mef@: What's the status on relanding this? Thanks!
Message was sent while issue was closed.
On 2014/08/25 08:33:33, blundell wrote: > On 2014/08/15 22:51:58, miu_OOO_until_Aug_25 wrote: > > A revert of this CL (patchset #26) has been created in > > https://codereview.chromium.org/477253002/ by mailto:miu@chromium.org. > > > > The reason for reverting is: Closed the tree on failing net_unittests: > > > > > http://build.chromium.org/p/chromium.linux/buildstatus?builder=Linux%20Tests%.... > > mef@: What's the status on relanding this? Thanks! I was OOO on vacation, will try to reland this week.
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/381073002/580001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
Message was sent while issue was closed.
Committed patchset #30 (id:580001) as 7269fc84fe9910a7e687e7d00bdc5744a0cb1229
Message was sent while issue was closed.
Patchset 30 (id:??) landed as https://crrev.com/327a8e400da2c092955a707321c39212ff9a3de9 Cr-Commit-Position: refs/heads/master@{#292659} |
