Enable V4LocalDatabaseManager based on Finch.

Enable V4LocalDatabaseManager based on Finch.
This only downloads hash-prefixes (and then throws them away).
Config CL: http://cr/120974092

BUG=543161
Committed: https://crrev.com/7fb023f02679805c33ebad190fba009729afcd6f
Cr-Commit-Position: refs/heads/master@{#391302}

[vakh (use Gerrit instead), 2016-04-28 01:35:25]

Description was changed from

==========
Enable V4LocalDatabaseManager based on Finch.
This only downloads hash-prefixes (and then throws them away).
Config CL: http://cr/120974092

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

to

==========
Enable V4LocalDatabaseManager based on Finch.
This only downloads hash-prefixes (and then throws them away).
Config CL: http://cr/120974092

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

[vakh (use Gerrit instead), 2016-04-28 01:35:25]

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

[Nathan Parker, 2016-04-28 17:33:21]

https://codereview.chromium.org/1924983002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/1924983002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:406:
V4LocalDatabaseManager* SafeBrowsingService::CreateV4LocalDatabaseManager() {
This should probably go in the services_delegate[_impl].  That way it won't be
built/linked-in on Android.

(CreateDatabaseManager should go there as well, but it's not there yet).

https://codereview.chromium.org/1924983002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:524: if
(IsV4LocalDatabaseManagerEnabled()) {
Rather than checking the field trial again (it shouldn't have changed), you
could just check if the ptr is not null.

I guess this should go in a ServicesDelegate::StartOnIOThread, since if we
actually make this file not include the v4ldbmgr header, you won't be able to
call methods on it.

[vakh (use Gerrit instead), 2016-04-29 00:24:34]

Move all Pver4 LocalDBManager code to services_delegate

[vakh (use Gerrit instead), 2016-04-29 01:55:12]

Add implementation of StartOnIOThread and StopOnIOThread

[vakh (use Gerrit instead), 2016-04-29 18:06:47]

https://codereview.chromium.org/1924983002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/1924983002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:406:
V4LocalDatabaseManager* SafeBrowsingService::CreateV4LocalDatabaseManager() {
On 2016/04/28 17:33:21, Nathan Parker wrote:
> This should probably go in the services_delegate[_impl].  That way it won't be
> built/linked-in on Android.
> 
> (CreateDatabaseManager should go there as well, but it's not there yet).

Done.

https://codereview.chromium.org/1924983002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:524: if
(IsV4LocalDatabaseManagerEnabled()) {
On 2016/04/28 17:33:21, Nathan Parker wrote:
> Rather than checking the field trial again (it shouldn't have changed), you
> could just check if the ptr is not null.
> 
> I guess this should go in a ServicesDelegate::StartOnIOThread, since if we
> actually make this file not include the v4ldbmgr header, you won't be able to
> call methods on it.

Done.

[vakh (use Gerrit instead), 2016-04-29 19:02:51]

git fetch & pull

[Nathan Parker, 2016-04-30 00:22:29]

lgtm

https://codereview.chromium.org/1924983002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/1924983002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.cc:553:
services_delegate_->StopOnIOThread(shutdown);
Should we do this in inverse order?  If it's started first, maybe it should be
stopped last.

https://codereview.chromium.org/1924983002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/services_delegate.h (right):

https://codereview.chromium.org/1924983002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/services_delegate.h:27: class
DownloadProtectionService;
add

class V4ProtocolConfig;

and then remove the include

[vakh (use Gerrit instead), 2016-04-30 00:50:34]

Patchset #5 (id:80001) has been deleted

[vakh (use Gerrit instead), 2016-04-30 00:50:48]

Minor. CR feedback.

[vakh (use Gerrit instead), 2016-04-30 00:51:50]

rkaplow@: Could you please take a look at the changes in:
testing/variations/fieldtrial_testing_config_win.json

https://codereview.chromium.org/1924983002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/1924983002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.cc:553:
services_delegate_->StopOnIOThread(shutdown);
On 2016/04/30 00:22:29, Nathan Parker wrote:
> Should we do this in inverse order?  If it's started first, maybe it should be
> stopped last.

Done.

https://codereview.chromium.org/1924983002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/services_delegate.h (right):

https://codereview.chromium.org/1924983002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/services_delegate.h:27: class
DownloadProtectionService;
On 2016/04/30 00:22:29, Nathan Parker wrote:
> add
> 
> class V4ProtocolConfig;
> 
> and then remove the include

Done.

[rkaplow, 2016-05-02 16:27:35]

https://codereview.chromium.org/1924983002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/services_delegate_impl.cc (right):

https://codereview.chromium.org/1924983002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/services_delegate_impl.cc:180: const std::string
group_name = base::FieldTrialList::FindFullName(
if this is a new field trial we suggest using the new feature api.
go/feature-api for details

https://codereview.chromium.org/1924983002/diff/100001/testing/variations/fie...
File testing/variations/fieldtrial_testing_config_win.json (right):

https://codereview.chromium.org/1924983002/diff/100001/testing/variations/fie...
testing/variations/fieldtrial_testing_config_win.json:331: "group_name":
"Enabled"
you only needed one group here to be tested. Likely it should be enabled which
is the main group experimented on.

[vakh (use Gerrit instead), 2016-05-02 21:54:39]

CR feedback: Only one group, and use new API.

[vakh (use Gerrit instead), 2016-05-02 22:04:34]

Thanks for the feedback. PTAL.

https://codereview.chromium.org/1924983002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/services_delegate_impl.cc (right):

https://codereview.chromium.org/1924983002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/services_delegate_impl.cc:180: const std::string
group_name = base::FieldTrialList::FindFullName(
On 2016/05/02 16:27:35, rkaplow wrote:
> if this is a new field trial we suggest using the new feature api.
> go/feature-api for details

Done.

https://codereview.chromium.org/1924983002/diff/100001/testing/variations/fie...
File testing/variations/fieldtrial_testing_config_win.json (right):

https://codereview.chromium.org/1924983002/diff/100001/testing/variations/fie...
testing/variations/fieldtrial_testing_config_win.json:331: "group_name":
"Enabled"
On 2016/05/02 16:27:35, rkaplow wrote:
> you only needed one group here to be tested. Likely it should be enabled which
> is the main group experimented on.
> 

Done.

[rkaplow, 2016-05-03 15:24:04]

lgtm

[vakh (use Gerrit instead), 2016-05-03 18:10:41]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2016-05-03 18:10:42]

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/1924983002/#ps120001
(title: "CR feedback: Only one group, and use new API.")

[commit-bot: I haz the power, 2016-05-03 18:12:20]

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

[commit-bot: I haz the power, 2016-05-03 18:21:03]

Description was changed from

==========
Enable V4LocalDatabaseManager based on Finch.
This only downloads hash-prefixes (and then throws them away).
Config CL: http://cr/120974092

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

to

==========
Enable V4LocalDatabaseManager based on Finch.
This only downloads hash-prefixes (and then throws them away).
Config CL: http://cr/120974092

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

[commit-bot: I haz the power, 2016-05-03 18:21:04]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-05-03 18:22:49]

Description was changed from

==========
Enable V4LocalDatabaseManager based on Finch.
This only downloads hash-prefixes (and then throws them away).
Config CL: http://cr/120974092

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

to

==========
Enable V4LocalDatabaseManager based on Finch.
This only downloads hash-prefixes (and then throws them away).
Config CL: http://cr/120974092

BUG=543161

Committed: https://crrev.com/7fb023f02679805c33ebad190fba009729afcd6f
Cr-Commit-Position: refs/heads/master@{#391302}
==========

[commit-bot: I haz the power, 2016-05-03 18:22:50]

Patchset 6 (id:??) landed as
https://crrev.com/7fb023f02679805c33ebad190fba009729afcd6f
Cr-Commit-Position: refs/heads/master@{#391302}