Skeleton of the overall design for the database for Pver4

Skeleton of the overall design for the database for Pver4
Design doc: https://goto.google.com/chromium-pver4-design-doc

BUG==543161

Committed: https://crrev.com/8354386e0eb264960f0ccdaa9e3e003ad72f0bcd
Cr-Commit-Position: refs/heads/master@{#392211}

[vakh (use Gerrit instead), 2016-05-05 16:52:21]

vakh@chromium.org changed reviewers:
+ nparker@chromium.org

[Nathan Parker, 2016-05-05 18:26:44]

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
no (c) here anymore

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.h:24: bool enable_malware_list,
Do you think we need the enable_* switches?  They are scattered around the old
code, but I wonder if they were there just for adding/testing new DBs.  When we
have 20+ lists, we'd need a different mechanism anyway.

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.h:32: // The on-disk databases are
shared among all profiles, as it doesn't contain
Maybe describe the relationship of this to Store's (ie. each store is a list,
and the database manages multiple stores)

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.h:52: virtual void Init(const
base::FilePath& filename) = 0;
"filename_prefix"?

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_database_impl.cc (right):

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database_impl.cc:15: class V4DatabaseFactoryImpl
: public V4DatabaseFactory {
I think you could do without a default factory, by having a static
V4DatabaseFactory::Create() method that checks if a global factory is set (would
be for tests) and if not, just does the new V4DatabaseImpl.  That makes it very
clear what the default behaviour is.

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_database_impl.h (right):

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database_impl.h:19: // The on-disk databases are
shared among all profiles, as it doesn't contain
nit: No need to duplicate the comment from the interface -- they'll likely get
out of sync any way.

Do you think you need the abstract interface + impl, or could you do with just
one class?  Simpler is better, as long as its sufficient for testing.  You could
still derive a mocked class from the concrete class, for testing.

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_local_database_manager.h (right):

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager.h:73:
base::hash_map<UpdateListIdentifier, std::string> current_list_states_;
Q: Will the V4Database also have a map of UpdateListIdentifiers-to-Stores? 
Should this go there?  It could be part of each store.

[vakh (use Gerrit instead), 2016-05-05 23:53:09]

Remove v4_database_impl.* and do all of that stuff in v4_database.* itself

[vakh (use Gerrit instead), 2016-05-06 00:22:01]

Add TODO comment about not storing current_list_states_ in
v4_local_database_manager

[vakh (use Gerrit instead), 2016-05-06 00:23:15]

Thanks for the feedback. PTAL.

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2016/05/05 18:26:44, Nathan Parker wrote:
> no (c) here anymore

Done.

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.h:24: bool enable_malware_list,
On 2016/05/05 18:26:44, Nathan Parker wrote:
> Do you think we need the enable_* switches?  They are scattered around the old
> code, but I wonder if they were there just for adding/testing new DBs.  When
we
> have 20+ lists, we'd need a different mechanism anyway.

Done.
I also realized that this morning. Changing it to a map that's passed as an
argument.

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.h:32: // The on-disk databases are
shared among all profiles, as it doesn't contain
On 2016/05/05 18:26:44, Nathan Parker wrote:
> Maybe describe the relationship of this to Store's (ie. each store is a list,
> and the database manages multiple stores)

Done.

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database.h:52: virtual void Init(const
base::FilePath& filename) = 0;
On 2016/05/05 18:26:44, Nathan Parker wrote:
> "filename_prefix"?

Done. Removed that part.
The DB maintains the store, but does not initialize them -- that is the job of
the factory so I'll add this code to the factory when I init Stores.

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_database_impl.cc (right):

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database_impl.cc:15: class V4DatabaseFactoryImpl
: public V4DatabaseFactory {
On 2016/05/05 18:26:44, Nathan Parker wrote:
> I think you could do without a default factory, by having a static
> V4DatabaseFactory::Create() method that checks if a global factory is set
(would
> be for tests) and if not, just does the new V4DatabaseImpl.  That makes it
very
> clear what the default behaviour is.

Done.

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_database_impl.h (right):

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_database_impl.h:19: // The on-disk databases are
shared among all profiles, as it doesn't contain
On 2016/05/05 18:26:44, Nathan Parker wrote:
> nit: No need to duplicate the comment from the interface -- they'll likely get
> out of sync any way.
> 
> Do you think you need the abstract interface + impl, or could you do with just
> one class?  Simpler is better, as long as its sufficient for testing.  You
could
> still derive a mocked class from the concrete class, for testing.

Done.

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_local_database_manager.h (right):

https://codereview.chromium.org/1952843003/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager.h:73:
base::hash_map<UpdateListIdentifier, std::string> current_list_states_;
On 2016/05/05 18:26:44, Nathan Parker wrote:
> Q: Will the V4Database also have a map of UpdateListIdentifiers-to-Stores? 
> Should this go there?  It could be part of each store.

Yes, it belongs to the Store, which is the source of truth for information about
that "list".
Added a TODO.
It needs to be removed from v4_local_database_manager and
v4_update_protocol_manager

[vakh (use Gerrit instead), 2016-05-06 00:41:36]

https://codereview.chromium.org/1952843003/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/1952843003/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.cc:12: ListInfoMap list_suffix_map) {
This should be list_info_map -- I have fixed it in the next CL. Please ignore
that for now.

[Nathan Parker, 2016-05-06 16:46:35]

https://codereview.chromium.org/1952843003/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/1952843003/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.h:18: typedef const
base::hash_map<UpdateListIdentifier,
Are the values here filename post-fixes?

https://codereview.chromium.org/1952843003/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.h:54: static V4Database* Create(
You probably want the factory stashed as a static private member, and make a
setter-method for it.  That way, a test can inject its own factory object and
not have to pass it down through SafeBrowsingService->V4DatabaseManager-> etc.

And then the factory can be pure-virtual, with no implementation other than in
the tests.  

An example:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...

[vakh (use Gerrit instead), 2016-05-06 21:26:35]

Pure virtual factory and a register method for tests

[vakh (use Gerrit instead), 2016-05-06 21:27:57]

https://codereview.chromium.org/1952843003/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/1952843003/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.h:18: typedef const
base::hash_map<UpdateListIdentifier,
On 2016/05/06 16:46:35, Nathan Parker wrote:
> Are the values here filename post-fixes?

Yes.

https://codereview.chromium.org/1952843003/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.h:54: static V4Database* Create(
On 2016/05/06 16:46:35, Nathan Parker wrote:
> You probably want the factory stashed as a static private member, and make a
> setter-method for it.  That way, a test can inject its own factory object and
> not have to pass it down through SafeBrowsingService->V4DatabaseManager-> etc.
> 
> And then the factory can be pure-virtual, with no implementation other than in
> the tests.  
> 
> An example:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...

Done.

[vakh (use Gerrit instead), 2016-05-06 21:32:21]

https://codereview.chromium.org/1952843003/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/1952843003/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_database.h:54: static V4Database* Create(
On 2016/05/06 21:27:57, vakh wrote:
> On 2016/05/06 16:46:35, Nathan Parker wrote:
> > You probably want the factory stashed as a static private member, and make a
> > setter-method for it.  That way, a test can inject its own factory object
and
> > not have to pass it down through SafeBrowsingService->V4DatabaseManager->
etc.
> > 
> > And then the factory can be pure-virtual, with no implementation other than
in
> > the tests.  
> > 
> > An example:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...
> 
> Done.

A lot of classes under safe_browsing[_db]/ use the factory the other way (that I
had earlier) so I did that to be consistent. But I like the virtual
implementation better since there's fewer redirection when running this code in
Chromium.

[Nathan Parker, 2016-05-06 22:47:12]

lgtm

https://codereview.chromium.org/1952843003/diff/60001/components/safe_browsin...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/1952843003/diff/60001/components/safe_browsin...
components/safe_browsing_db/v4_database.h:71: //This is used *only* by tests.
nit: space after //

[vakh (use Gerrit instead), 2016-05-06 22:50:53]

Add space in comment

[vakh (use Gerrit instead), 2016-05-06 22:51:17]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2016-05-06 22:51:17]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/1952843003/#ps80001
(title: "Add space in comment")

[vakh (use Gerrit instead), 2016-05-06 22:51:36]

https://codereview.chromium.org/1952843003/diff/60001/components/safe_browsin...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/1952843003/diff/60001/components/safe_browsin...
components/safe_browsing_db/v4_database.h:71: //This is used *only* by tests.
On 2016/05/06 22:47:11, Nathan Parker wrote:
> nit: space after //

Done.

[commit-bot: I haz the power, 2016-05-06 22:51:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1952843003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1952843003/80001

[commit-bot: I haz the power, 2016-05-07 00:07:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-07 00:08:18]

Description was changed from

==========
Skeleton of the overall design for the database for Pver4
Design doc: https://goto.google.com/chromium-pver4-design-doc

BUG==543161
==========

to

==========
Skeleton of the overall design for the database for Pver4
Design doc: https://goto.google.com/chromium-pver4-design-doc

BUG==543161

Committed: https://crrev.com/8354386e0eb264960f0ccdaa9e3e003ad72f0bcd
Cr-Commit-Position: refs/heads/master@{#392211}
==========

[commit-bot: I haz the power, 2016-05-07 00:08:19]

Patchset 5 (id:??) landed as
https://crrev.com/8354386e0eb264960f0ccdaa9e3e003ad72f0bcd
Cr-Commit-Position: refs/heads/master@{#392211}

[Nico, 2016-05-07 14:31:00]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1960893002/ by thakis@chromium.org.

The reason for reverting is: Speculative; lots of browser tests started failing
with leak reports, and all stacks contain "safe browsing". See
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2...,
e.g.

[444:555:0506/185642:ERROR:leak_tracker.h(100)] #0 0x7fd9c5c00d0e
base::debug::StackTrace::StackTrace()
#1 0x00000186cf6f base::debug::LeakTracker<>::LeakTracker()
#2 0x0000018635bb SystemURLRequestContextGetter::SystemURLRequestContextGetter()
#3 0x000001865c21 IOThread::InitSystemRequestContext()
#4 0x000001865af5 IOThread::system_url_request_context_getter()
#5 0x000001a08b90 BrowserProcessImpl::system_request_context()
#6 0x000001ef00c4 safe_browsing::SafeBrowsingService::Initialize()
#7 0x000001a0c1b0 BrowserProcessImpl::CreateSafeBrowsingService()
#8 0x000001a0c022 BrowserProcessImpl::safe_browsing_service()
.