Browser tests for using the new SafeBrowsing protocol (v4)

Adding browser tests for PVer4.

Part 1: http://crrev.com/2675063002 (this CL)
Part 2: http://crrev.com/2684663002
Part 3: http://crrev.com/2683813002

BUG=682495
Review-Url: https://codereview.chromium.org/2675063002
Cr-Original-Commit-Position: refs/heads/master@{#449703}
Committed: https://chromium.googlesource.com/chromium/src/+/d9bfc537eac0e031058b6df63b4290a645318cf8
Review-Url: https://codereview.chromium.org/2675063002
Cr-Commit-Position: refs/heads/master@{#450138}
Committed: https://chromium.googlesource.com/chromium/src/+/b397adc6b277d907e4c313797b44c78171d4d793

[vakh (use Gerrit instead), 2017-02-04 02:23:39]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-04 02:24:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-02-04 02:36:02]

Remove DCHECK from V4LDBM

[vakh (use Gerrit instead), 2017-02-04 02:36:04]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2017-02-04 02:36:16]

Description was changed from

==========
Adding browser test for PVer4.

Just adding some basic tests for now. More coming soon.

BUG=682495
==========

to

==========
Adding browser tests for PVer4.

Just adding some basic tests for now. More coming soon.

BUG=682495
==========

[commit-bot: I haz the power, 2017-02-04 02:36:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-04 03:02:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 03:03:00]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[vakh (use Gerrit instead), 2017-02-04 03:38:32]

Fix Android build

[vakh (use Gerrit instead), 2017-02-04 03:38:34]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-04 03:38:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-04 05:39:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 05:39:35]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_TIMED_OUT,
no build URL)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no
build URL)
  win_clang on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL)

[vakh (use Gerrit instead), 2017-02-04 20:35:15]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-04 20:35:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-04 22:35:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 22:35:59]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_TIMED_OUT,
no build URL)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no
build URL)
  win_clang on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL)

[vakh (use Gerrit instead), 2017-02-06 17:57:39]

Remove LOGs

[vakh (use Gerrit instead), 2017-02-06 17:57:41]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 17:58:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-02-06 18:22:23]

Simplify V4DB creation in product code and tests

[vakh (use Gerrit instead), 2017-02-06 18:22:25]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 18:23:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-02-06 18:25:07]

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

[vakh (use Gerrit instead), 2017-02-06 18:25:08]

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

https://codereview.chromium.org/2675063002/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_database.cc:215: LOG(ERROR) <<
"V4Database::GetStoresMatchingFullHash: store: "
Please ignore. Removed in the next patch.

[vakh (use Gerrit instead), 2017-02-06 18:25:49]

Patchset #2 (id:20001) has been deleted

[vakh (use Gerrit instead), 2017-02-06 18:26:04]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-02-06 19:26:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 19:26:48]

Dry run: This issue passed the CQ dry run.

[Scott Hess - ex-Googler, 2017-02-06 22:46:36]

nit-picker's review...

https://codereview.chromium.org/2675063002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

https://codereview.chromium.org/2675063002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.h:201: bool use_v4_only =
false);
This feels like a case where some well-chosen enum names would improve
readability.  Like V4_DISABLED, V4_ENABLED, and V4_ONLY or something like that.

https://codereview.chromium.org/2675063002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/2675063002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1848: <<
store_path.AsUTF8Unsafe();
Is there any reason to see this on the bots?  You may be aware that I'm rabid
about automatic opt-in to logging!

https://codereview.chromium.org/2675063002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1925: return
base::WrapUnique(pm_);
Man, this gives me the heebie-jeebies.  But ... I guess so.  Putting in
invalidation logic seems like overkill.

https://codereview.chromium.org/2675063002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:2094: }
I think this is fine, but I'd be wary of going too far down this route, since
you're trading parameterized test cases for more-complicated tests.  Sometimes
such things can evolve organically into horrors impossible to understand.

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

https://codereview.chromium.org/2675063002/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_database.cc:215: LOG(ERROR) <<
"V4Database::GetStoresMatchingFullHash: store: "
On 2017/02/06 18:25:08, vakh (Varun Khaneja) wrote:
> Please ignore. Removed in the next patch.

Acknowledged.

https://codereview.chromium.org/2675063002/diff/80001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2675063002/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:198: LOG(ERROR) <<
"V4LocalDatabaseManager::CheckBrowseUrl: url: " << url.spec();
Does everyone need to see these logs?

[vakh (use Gerrit instead), 2017-02-06 23:29:20]

shess@'s review

[vakh (use Gerrit instead), 2017-02-06 23:29:22]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2017-02-06 23:30:13]

https://codereview.chromium.org/2675063002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

https://codereview.chromium.org/2675063002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.h:201: bool use_v4_only =
false);
On 2017/02/06 22:46:35, Scott Hess wrote:
> This feels like a case where some well-chosen enum names would improve
> readability.  Like V4_DISABLED, V4_ENABLED, and V4_ONLY or something like
that.

Done.

https://codereview.chromium.org/2675063002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/2675063002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1848: <<
store_path.AsUTF8Unsafe();
On 2017/02/06 22:46:35, Scott Hess wrote:
> Is there any reason to see this on the bots?  You may be aware that I'm rabid
> about automatic opt-in to logging!

Done.

https://codereview.chromium.org/2675063002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1925: return
base::WrapUnique(pm_);
On 2017/02/06 22:46:35, Scott Hess wrote:
> Man, this gives me the heebie-jeebies.  But ... I guess so.  Putting in
> invalidation logic seems like overkill.

Acknowledged.

https://codereview.chromium.org/2675063002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:2094: }
On 2017/02/06 22:46:35, Scott Hess wrote:
> I think this is fine, but I'd be wary of going too far down this route, since
> you're trading parameterized test cases for more-complicated tests.  Sometimes
> such things can evolve organically into horrors impossible to understand.

I agree, but these new tests are an exact replica of PVer3 tests in this file
and the idea is to make it easier for the reviewers to see that the PVer4 tests
are only different from PVer3 ones in how they inject the hash prefix and cache
entry, and therefore they achieve the same test goals as PVer3.

https://codereview.chromium.org/2675063002/diff/80001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2675063002/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:198: LOG(ERROR) <<
"V4LocalDatabaseManager::CheckBrowseUrl: url: " << url.spec();
On 2017/02/06 22:46:36, Scott Hess wrote:
> Does everyone need to see these logs?

No, got uploaded by mistake. Removed.

[commit-bot: I haz the power, 2017-02-06 23:30:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 00:38:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 00:38:28]

Dry run: This issue passed the CQ dry run.

[vakh (use Gerrit instead), 2017-02-07 17:51:31]

Description was changed from

==========
Adding browser tests for PVer4.

Just adding some basic tests for now. More coming soon.

BUG=682495
==========

to

==========
Adding browser tests for PVer4.

Just adding some basic tests for now.
More coming soon (http://crrev.com/2684663002)

BUG=682495
==========

[Nathan Parker, 2017-02-08 01:22:54]

Mostly nits about adding comments.

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

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.cc:261: if
(safe_browsing::V4FeatureList::IsV4OnlyEnabled()) {
This block of code could go into a GetV4UsageStatus() function in
v4_features_list.

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.h:62: enum class
V4UsageStatus { V4_DISABLED, V4_INSTANTIATED, V4_ONLY };
Add a comment... could point to the v4_feature_list.h comments.  Or... maybe
this should live there, so we remember to remove it when V4 is the only thing.

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

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1873: if
(test_store) {
Should test_store always be non-null?  If so, you could skip the if().

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1883:
TestV4Database* Create(
Should this return a V4Database instead of TestV4Database?

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1895:
TestV4Database* v4_db_;
Add comment that this isn't owned.

I'm assuming we don't need to worry about a use-after-free here... but... it
would be good to add a comment as to why that's true, to convince me/yourself.

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1928:
TestV4GetHashProtocolManager* pm_;
Add comment that this isn't owned.  Similar question about use-after-free

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1937:
sb_factory_.reset(
nit:
sb_factory_ = std::make_unique<TestSafeBrowsingServiceFactory>(...)

And elsewhere (search for /new /)

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1944:
v4_get_hash_factory_ = v4_get_hash_factory.get();
Can skip the temp var here

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1949:
FullHashInfo GetFullHashInfo(const GURL& url, const ListIdentifier& list_id) {
Can you add some comments to these functions, as to what they should be used
for?

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1955:
base::Time::Now() + base::TimeDelta::FromMinutes(5));
Is this time actually used?  If not, make it (0).  Or consider if we should use
a fake clock.

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1984:
TestV4GetHashProtocolManagerFactory* v4_get_hash_factory_;
// Unowned
(I assume)

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1989:
IN_PROC_BROWSER_TEST_F(V4SafeBrowsingServiceTest, UnwantedImgIgnored) {
How about an UnwantedMainFrame, to confirm that we're just ignoring subresources
and not all UwS classifications?

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:2003: class
V4SafeBrowsingServiceMetadataTest
IMHO this file is big enough to merit some comment dividers between blocks of
tests

https://codereview.chromium.org/2675063002/diff/100001/components/safe_browsi...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/2675063002/diff/100001/components/safe_browsi...
components/safe_browsing_db/v4_database.h:81: virtual V4Database* Create(
Should this return a unique_ptr? That's the canonical pattern.

https://codereview.chromium.org/2675063002/diff/100001/components/safe_browsi...
components/safe_browsing_db/v4_database.h:98: // the same thread as it was
called.
// Uses the registered factory or a default implementation.

https://codereview.chromium.org/2675063002/diff/100001/components/safe_browsi...
File components/safe_browsing_db/v4_feature_list.h (right):

https://codereview.chromium.org/2675063002/diff/100001/components/safe_browsi...
components/safe_browsing_db/v4_feature_list.h:14: extern const base::Feature
kLocalDatabaseManagerEnabled;
Do these need to be accessed outside this .cc?  Why can't others just use the
Is*() functions here?

https://codereview.chromium.org/2675063002/diff/100001/components/safe_browsi...
File components/safe_browsing_db/v4_get_hash_protocol_manager.h (right):

https://codereview.chromium.org/2675063002/diff/100001/components/safe_browsi...
components/safe_browsing_db/v4_get_hash_protocol_manager.h:292: // A cache of
full hash results.
Is this protected for tests? It might be better to just make them friends.  Or
comment on why this is protected.

https://codereview.chromium.org/2675063002/diff/100001/components/safe_browsi...
File components/safe_browsing_db/v4_store.h (right):

https://codereview.chromium.org/2675063002/diff/100001/components/safe_browsi...
components/safe_browsing_db/v4_store.h:395: HashPrefixMap hash_prefix_map_;
Same question here... why protected?

[vakh (use Gerrit instead), 2017-02-08 17:41:27]

nparker@'s review

[vakh (use Gerrit instead), 2017-02-08 17:41:29]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 17:41:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 18:27:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 18:27:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vakh (use Gerrit instead), 2017-02-08 18:41:02]

nparker@'s review

[vakh (use Gerrit instead), 2017-02-08 18:41:04]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 18:41:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 19:44:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 19:44:37]

Dry run: This issue passed the CQ dry run.

[vakh (use Gerrit instead), 2017-02-08 21:27:49]

nparker@'s review

[vakh (use Gerrit instead), 2017-02-08 21:27:51]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 21:28:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-02-08 21:31:32]

rebase

[vakh (use Gerrit instead), 2017-02-08 21:31:34]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 21:32:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 22:44:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 22:44:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vakh (use Gerrit instead), 2017-02-09 03:03:18]

rebase + fix failures after rebase

[vakh (use Gerrit instead), 2017-02-09 03:03:22]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 03:04:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-02-09 03:30:18]

Nit: Add comments in tests

[vakh (use Gerrit instead), 2017-02-09 03:30:22]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2017-02-09 03:30:28]

All comments addressed. PTAL.

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

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.cc:261: if
(safe_browsing::V4FeatureList::IsV4OnlyEnabled()) {
On 2017/02/08 01:22:43, Nathan Parker wrote:
> This block of code could go into a GetV4UsageStatus() function in
> v4_features_list.

Done.

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service.h:62: enum class
V4UsageStatus { V4_DISABLED, V4_INSTANTIATED, V4_ONLY };
On 2017/02/08 01:22:43, Nathan Parker wrote:
> Add a comment... could point to the v4_feature_list.h comments.  Or... maybe
> this should live there, so we remember to remove it when V4 is the only thing.

Done.

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

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1873: if
(test_store) {
On 2017/02/08 01:22:53, Nathan Parker wrote:
> Should test_store always be non-null?  If so, you could skip the if().

Done.

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1883:
TestV4Database* Create(
On 2017/02/08 01:22:48, Nathan Parker wrote:
> Should this return a V4Database instead of TestV4Database?

Done.

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1895:
TestV4Database* v4_db_;
On 2017/02/08 01:22:54, Nathan Parker wrote:
> Add comment that this isn't owned.
> 
> I'm assuming we don't need to worry about a use-after-free here... but... it
> would be good to add a comment as to why that's true, to convince me/yourself.

Done.

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1928:
TestV4GetHashProtocolManager* pm_;
On 2017/02/08 01:22:48, Nathan Parker wrote:
> Add comment that this isn't owned.  Similar question about use-after-free

Done.

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1937:
sb_factory_.reset(
On 2017/02/08 01:22:46, Nathan Parker wrote:
> nit:
> sb_factory_ = std::make_unique<TestSafeBrowsingServiceFactory>(...)
> 
> And elsewhere (search for /new /)

Done.

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1944:
v4_get_hash_factory_ = v4_get_hash_factory.get();
On 2017/02/08 01:22:49, Nathan Parker wrote:
> Can skip the temp var here

Done.

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1949:
FullHashInfo GetFullHashInfo(const GURL& url, const ListIdentifier& list_id) {
On 2017/02/08 01:22:52, Nathan Parker wrote:
> Can you add some comments to these functions, as to what they should be used
> for?

Done.

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1955:
base::Time::Now() + base::TimeDelta::FromMinutes(5));
On 2017/02/08 01:22:45, Nathan Parker wrote:
> Is this time actually used?  If not, make it (0).  Or consider if we should
use
> a fake clock.

I tried using Time() instead but the test failed because then the cache entry is
found to be stale.
Using Now() should be OK, it doesn't make the test flaky since the test doesn't
depend on the actual value returned by Now()

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1984:
TestV4GetHashProtocolManagerFactory* v4_get_hash_factory_;
On 2017/02/08 01:22:44, Nathan Parker wrote:
> // Unowned
> (I assume)

Done.

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1989:
IN_PROC_BROWSER_TEST_F(V4SafeBrowsingServiceTest, UnwantedImgIgnored) {
On 2017/02/08 01:22:48, Nathan Parker wrote:
> How about an UnwantedMainFrame, to confirm that we're just ignoring
subresources
> and not all UwS classifications?

There are a lot of tests remaining to be written.
I'll add a ToDo for this and add it after I've added all existing PVer3 tests
for PVer4.

https://codereview.chromium.org/2675063002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:2003: class
V4SafeBrowsingServiceMetadataTest
On 2017/02/08 01:22:50, Nathan Parker wrote:
> IMHO this file is big enough to merit some comment dividers between blocks of
> tests

Done.

[commit-bot: I haz the power, 2017-02-09 03:31:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-02-09 03:31:14]

Patchset #6 (id:140001) has been deleted

[vakh (use Gerrit instead), 2017-02-09 03:31:28]

Patchset #5 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-02-09 05:18:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 05:18:34]

Dry run: This issue passed the CQ dry run.

[Nathan Parker, 2017-02-10 00:52:38]

lgtm

https://codereview.chromium.org/2675063002/diff/220001/chrome/browser/downloa...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/2675063002/diff/220001/chrome/browser/downloa...
chrome/browser/download/download_browsertest.cc:1088:
safe_browsing::V4FeatureList::V4UsageStatus::V4_DISABLED) {
(This seems like a rather deep namespace but... meh.)

https://codereview.chromium.org/2675063002/diff/220001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/services_delegate.h (right):

https://codereview.chromium.org/2675063002/diff/220001/chrome/browser/safe_br...
chrome/browser/safe_browsing/services_delegate.h:75: virtual void
Initialize(bool v4_enabled = false) = 0;
Possible idea: Is there any place that calls this w/o an arg?  If not, you can
remove the default -- then it's more explicit what's happening and lessens the
change of mistake.

https://codereview.chromium.org/2675063002/diff/220001/components/safe_browsi...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/2675063002/diff/220001/components/safe_browsi...
components/safe_browsing_db/v4_database.h:210: static
std::unique_ptr<V4DatabaseFactory> db_factory_;
(I'm not sure if this ever gets deleted... but I think it makes sense to have
unique_ptr since then the ownership is clear.)

https://codereview.chromium.org/2675063002/diff/220001/components/safe_browsi...
File components/safe_browsing_db/v4_feature_list.h (right):

https://codereview.chromium.org/2675063002/diff/220001/components/safe_browsi...
components/safe_browsing_db/v4_feature_list.h:12: namespace V4FeatureList {
I'm not sure this namespace adds much, other than verbiage.  Your choice.

[vakh (use Gerrit instead), 2017-02-10 01:08:14]

vakh@chromium.org changed reviewers:
+ asanka@chromium.org, csharrison@chromium.org

[vakh (use Gerrit instead), 2017-02-10 01:08:16]

asanka@chromium.org: Please review changes in 
chrome/browser/download/download_browsertest.cc

csharrison@chromium.org: Please review changes in
chrome/browser/loader/safe_browsing_resource_throttle.cc

https://codereview.chromium.org/2675063002/diff/220001/chrome/browser/downloa...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/2675063002/diff/220001/chrome/browser/downloa...
chrome/browser/download/download_browsertest.cc:1088:
safe_browsing::V4FeatureList::V4UsageStatus::V4_DISABLED) {
On 2017/02/10 00:52:37, Nathan Parker wrote:
> (This seems like a rather deep namespace but... meh.)

Acknowledged. I think I'll change that file to v4_usage_status.h and get rid of
V4FeatureList completely. Separately.

https://codereview.chromium.org/2675063002/diff/220001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/services_delegate.h (right):

https://codereview.chromium.org/2675063002/diff/220001/chrome/browser/safe_br...
chrome/browser/safe_browsing/services_delegate.h:75: virtual void
Initialize(bool v4_enabled = false) = 0;
On 2017/02/10 00:52:37, Nathan Parker wrote:
> Possible idea: Is there any place that calls this w/o an arg?  If not, you can
> remove the default -- then it's more explicit what's happening and lessens the
> change of mistake.

Yes, ServicesDelegateStub.

https://codereview.chromium.org/2675063002/diff/220001/components/safe_browsi...
File components/safe_browsing_db/v4_database.h (right):

https://codereview.chromium.org/2675063002/diff/220001/components/safe_browsi...
components/safe_browsing_db/v4_database.h:210: static
std::unique_ptr<V4DatabaseFactory> db_factory_;
On 2017/02/10 00:52:37, Nathan Parker wrote:
> (I'm not sure if this ever gets deleted... but I think it makes sense to have
> unique_ptr since then the ownership is clear.)

Acknowledged.

https://codereview.chromium.org/2675063002/diff/220001/components/safe_browsi...
File components/safe_browsing_db/v4_feature_list.h (right):

https://codereview.chromium.org/2675063002/diff/220001/components/safe_browsi...
components/safe_browsing_db/v4_feature_list.h:12: namespace V4FeatureList {
On 2017/02/10 00:52:37, Nathan Parker wrote:
> I'm not sure this namespace adds much, other than verbiage.  Your choice.

Acknowledged.

[Charlie Harrison, 2017-02-10 01:35:07]

Can you make the description/title a bit more descriptive? I didn't recognize
"PVer4" at first glance.

[vakh (use Gerrit instead), 2017-02-10 01:36:40]

On 2017/02/10 01:35:07, Charlie Harrison wrote:
> Can you make the description/title a bit more descriptive? I didn't recognize
> "PVer4" at first glance.

Done. PTAL. Thanks.

[Charlie Harrison, 2017-02-10 02:35:58]

c/b/l LGTM

[asanka, 2017-02-10 16:15:47]

/download/ lgtm

[vakh (use Gerrit instead), 2017-02-10 18:43:29]

The CQ bit was checked by vakh@chromium.org

[commit-bot: I haz the power, 2017-02-10 18:44:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Scott Hess - ex-Googler, 2017-02-10 18:53:32]

lgtm

[commit-bot: I haz the power, 2017-02-10 19:50:47]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1486752209371720,
"parent_rev": "36bf620f1e9240fea9df9ab0e07bd650bb392bcc", "commit_rev":
"d9bfc537eac0e031058b6df63b4290a645318cf8"}

[commit-bot: I haz the power, 2017-02-10 19:51:22]

Description was changed from

==========
Adding browser tests for PVer4.

Just adding some basic tests for now.
More coming soon (http://crrev.com/2684663002)

BUG=682495
==========

to

==========
Adding browser tests for PVer4.

Just adding some basic tests for now.
More coming soon (http://crrev.com/2684663002)

BUG=682495

Review-Url: https://codereview.chromium.org/2675063002
Cr-Commit-Position: refs/heads/master@{#449703}
Committed:
https://chromium.googlesource.com/chromium/src/+/d9bfc537eac0e031058b6df63b42...
==========

[commit-bot: I haz the power, 2017-02-10 19:51:24]

Committed patchset #8 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/d9bfc537eac0e031058b6df63b42...

[hiroshige, 2017-02-10 20:54:07]

A revert of this CL (patchset #8 id:220001) has been created in
https://codereview.chromium.org/2687023007/ by hiroshige@chromium.org.

The reason for reverting is: Suspected to cause sizes test failing due to static
initializers added to v4_database.cc.

BUG=691069
.

[vakh (use Gerrit instead), 2017-02-11 00:15:08]

Reverted due to failures here:
https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/32970

[vakh (use Gerrit instead), 2017-02-11 01:27:39]

Use static raw pointer instead of static smart pointer

[vakh (use Gerrit instead), 2017-02-11 01:29:56]

Description was changed from

==========
Adding browser tests for PVer4.

Just adding some basic tests for now.
More coming soon (http://crrev.com/2684663002)

BUG=682495

Review-Url: https://codereview.chromium.org/2675063002
Cr-Commit-Position: refs/heads/master@{#449703}
Committed:
https://chromium.googlesource.com/chromium/src/+/d9bfc537eac0e031058b6df63b42...
==========

to

==========
Adding browser tests for PVer4.

Just adding some basic tests for now.
More coming soon (http://crrev.com/2684663002)

BUG=682495

Review-Url: https://codereview.chromium.org/2675063002
Cr-Commit-Position: refs/heads/master@{#449703}
Committed:
https://chromium.googlesource.com/chromium/src/+/d9bfc537eac0e031058b6df63b42...
==========

[vakh (use Gerrit instead), 2017-02-11 01:31:08]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-11 01:31:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-11 08:22:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-11 08:22:57]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Nathan Parker, 2017-02-13 19:45:35]

lgtm

https://codereview.chromium.org/2675063002/diff/240001/components/safe_browsi...
File components/safe_browsing_db/v4_database.cc (right):

https://codereview.chromium.org/2675063002/diff/240001/components/safe_browsi...
components/safe_browsing_db/v4_database.cc:109: db_factory_ = factory.release();
I wonder if you'll get leak warnings for this. Should you add 
ANNOTATE_LEAKING_OBJECT_PTR(..)?

[vakh (use Gerrit instead), 2017-02-13 20:05:07]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 20:05:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-02-13 20:05:41]

rebase

[vakh (use Gerrit instead), 2017-02-13 20:05:46]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 20:06:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-02-13 20:07:03]

Use ANNOTATE_LEAKING_OBJECT_PTR

[vakh (use Gerrit instead), 2017-02-13 20:07:09]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 20:08:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-02-13 20:34:21]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2017-02-13 20:34:21]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
shess@chromium.org, csharrison@chromium.org, nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/2675063002/#ps280001
(title: "Use ANNOTATE_LEAKING_OBJECT_PTR")

[commit-bot: I haz the power, 2017-02-13 20:35:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-02-13 22:01:46]

Description was changed from

==========
Adding browser tests for PVer4.

Just adding some basic tests for now.
More coming soon (http://crrev.com/2684663002)

BUG=682495

Review-Url: https://codereview.chromium.org/2675063002
Cr-Commit-Position: refs/heads/master@{#449703}
Committed:
https://chromium.googlesource.com/chromium/src/+/d9bfc537eac0e031058b6df63b42...
==========

to

==========
Adding browser tests for PVer4.

Part 1: http://crrev.com/2675063002 (this CL)
Part 2: http://crrev.com/2684663002
Part 3: http://crrev.com/2683813002

BUG=682495

Review-Url: https://codereview.chromium.org/2675063002
Cr-Commit-Position: refs/heads/master@{#449703}
Committed:
https://chromium.googlesource.com/chromium/src/+/d9bfc537eac0e031058b6df63b42...
==========

[commit-bot: I haz the power, 2017-02-13 23:09:06]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1487018061029940,
"parent_rev": "c5968317007dbf2c9dba040f820d1f727c43a4cd", "commit_rev":
"b397adc6b277d907e4c313797b44c78171d4d793"}

[commit-bot: I haz the power, 2017-02-13 23:09:45]

Description was changed from

==========
Adding browser tests for PVer4.

Part 1: http://crrev.com/2675063002 (this CL)
Part 2: http://crrev.com/2684663002
Part 3: http://crrev.com/2683813002

BUG=682495

Review-Url: https://codereview.chromium.org/2675063002
Cr-Commit-Position: refs/heads/master@{#449703}
Committed:
https://chromium.googlesource.com/chromium/src/+/d9bfc537eac0e031058b6df63b42...
==========

to

==========
Adding browser tests for PVer4.

Part 1: http://crrev.com/2675063002 (this CL)
Part 2: http://crrev.com/2684663002
Part 3: http://crrev.com/2683813002

BUG=682495

Review-Url: https://codereview.chromium.org/2675063002
Cr-Original-Commit-Position: refs/heads/master@{#449703}
Committed:
https://chromium.googlesource.com/chromium/src/+/d9bfc537eac0e031058b6df63b42...
Review-Url: https://codereview.chromium.org/2675063002
Cr-Commit-Position: refs/heads/master@{#450138}
Committed:
https://chromium.googlesource.com/chromium/src/+/b397adc6b277d907e4c313797b44...
==========

[commit-bot: I haz the power, 2017-02-13 23:09:53]

Committed patchset #11 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/b397adc6b277d907e4c313797b44...