Create test fixture for SafeBrowsingService

Create test fixture for SafeBrowsingService and its factory
to simply test writing.

Also squeeze in a minor fix in DownloadDangerPrompt classes
to prevent download report sent for a already terminated or
not dangerous download.

BUG=594683
Committed: https://crrev.com/7526f825c981ac645f6287b0b11b24e48a336cae
Cr-Commit-Position: refs/heads/master@{#395183}

[Jialiu Lin, 2016-05-05 20:39:14]

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

[Jialiu Lin, 2016-05-05 20:39:15]

Add test_safe_browsing_service.h and .cc file to define a configurable testing
interface for SafeBrowsingService.
Modified 9 unit_tests/browser_tests to use this new test fixture.

[Nathan Parker, 2016-05-07 00:05:33]

Lots of semi-duplicated code removed!  Nice.

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_danger_prompt.cc (right):

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_danger_prompt.cc:337: default:  // Don't send
report for any other danger types.
Does this change the behaviour?  Isn't test code...

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:744: #if
defined(SAFE_BROWSING_DB_LOCAL) || defined(SAFE_BROWSING_DB_REMOTE)
Is this file included in Android tests?  If so, then this will now test on
Android when it didn't before, which is good.  And... I think this #if statement
will be true on all platforms (except iOS, which doesn't use /chrome/**), which
could mean you could remove all the ifdefs!

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:762: if
(badurls_.find(gurl.spec())==badurls_.end() ||
nit: spaces around ==

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:1157:
DCHECK(safe_browsing_factory_->test_safe_browsing_service());

I'm not not sure how the comment applies to this DCHECK.  Is it a todo?

ASSERT_TRUE might be better, so it'll check in non-debug builds as well.

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

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/test_safe_browsing_service.cc:16:
TestSafeBrowsingUIManager::TestSafeBrowsingUIManager()
nit: Can you add a comment/header between the different class groupings of
methods?  Makes it easier to read.

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/test_safe_browsing_service.cc:79: return
SafeBrowsingService::CreateDatabaseManager();
Should this store the created object?  The function below creates and stores.

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/test_safe_browsing_service.cc:101:
SafeBrowsingProtocolConfig* protocol_config) {
Would this be simpler as a pass- and store-by-value? That'd make it easier to
manage ownership of the thing.

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/test_safe_browsing_service.cc:127: return
test_safe_browsing_service_;
Does the factory need to retain a copy of this ptr?  Or if it does, should it
instead be unique_ptr so it owns it?

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/test_safe_browsing_service.h (right):

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/test_safe_browsing_service.h:109: // This is an
implmentation of SafeBrowsingUIManager without actually
nit: implementation

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

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/unverified_download_policy_unittest.cc:83:
testing_safe_browsing_service_factory_.test_safe_browsing_service()
Shouldn't this set the FakeDatabaseManager on the factory obj, and then let it
create/initialize it normally?

Also, do you need to register the factory if you're then invoking it directly?

[Jialiu Lin, 2016-05-10 21:52:37]

Patchset #3 (id:40001) has been deleted

[Jialiu Lin, 2016-05-11 00:18:11]

Patchset #4 (id:80001) has been deleted

[Jialiu Lin, 2016-05-12 20:21:57]

Patchset #5 (id:120001) has been deleted

[Jialiu Lin, 2016-05-12 20:22:08]

Patchset #4 (id:100001) has been deleted

[Jialiu Lin, 2016-05-12 20:52:48]

Description was changed from

==========
Create test fixture for SafeBrowsingService and its factory to simply
test writing.

BUG=594683
==========

to

==========
Create test fixture for SafeBrowsingService and its factory to simply test
writing.

Also squeeze in a minor fix in DownloadDangerPrompt classes to prevent download
report sent for a already terminated or not dangerous download.  

BUG=594683
==========

[Jialiu Lin, 2016-05-12 20:54:16]

Description was changed from

==========
Create test fixture for SafeBrowsingService and its factory to simply test
writing.

Also squeeze in a minor fix in DownloadDangerPrompt classes to prevent download
report sent for a already terminated or not dangerous download.  

BUG=594683
==========

to

==========
Create test fixture for SafeBrowsingService and its factory 
to simply test writing.

Also squeeze in a minor fix in DownloadDangerPrompt classes
to prevent download report sent for a already terminated or
not dangerous download.  

BUG=594683
==========

[Jialiu Lin, 2016-05-12 21:33:47]

Patchset #4 (id:140001) has been deleted

[Jialiu Lin, 2016-05-12 21:34:21]

Patchset #3 (id:60001) has been deleted

[Jialiu Lin, 2016-05-12 21:53:58]

Thanks, nparker@!
Your comments are all addressed. 

+asanka@
for owner review on download related code.

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_danger_prompt.cc (right):

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_danger_prompt.cc:337: default:  // Don't send
report for any other danger types.
On 2016/05/07 at 00:05:32, Nathan Parker wrote:
> Does this change the behaviour?  Isn't test code...

Oops, added a test in download_danger_prompt_browsertest.cc

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:744: #if
defined(SAFE_BROWSING_DB_LOCAL) || defined(SAFE_BROWSING_DB_REMOTE)
On 2016/05/07 at 00:05:32, Nathan Parker wrote:
> Is this file included in Android tests?  If so, then this will now test on
Android when it didn't before, which is good.  And... I think this #if statement
will be true on all platforms (except iOS, which doesn't use /chrome/**), which
could mean you could remove all the ifdefs!

Good to know. ifdefs removed. Thanks!

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:762: if
(badurls_.find(gurl.spec())==badurls_.end() ||
On 2016/05/07 00:05:32, Nathan Parker wrote:
> nit: spaces around ==

Done.

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:1157:
DCHECK(safe_browsing_factory_->test_safe_browsing_service());
On 2016/05/07 at 00:05:32, Nathan Parker wrote:
> I'm not not sure how the comment applies to this DCHECK.  Is it a todo?
> 
> ASSERT_TRUE might be better, so it'll check in non-debug builds as well.

Oops, sorry it is a todo for myself. Comments removed and changed to Assert_true

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

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/test_safe_browsing_service.cc:16:
TestSafeBrowsingUIManager::TestSafeBrowsingUIManager()
On 2016/05/07 00:05:32, Nathan Parker wrote:
> nit: Can you add a comment/header between the different class groupings of
> methods?  Makes it easier to read.
Comments added. Also moved TestSafeBrowsingUIManager::* functions to the end to
make the order consistent with functions .h files

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/test_safe_browsing_service.cc:79: return
SafeBrowsingService::CreateDatabaseManager();
On 2016/05/07 at 00:05:32, Nathan Parker wrote:
> Should this store the created object?  The function below creates and stores.
Since the only place CreateDatabaseManager() called is  "database_manager_ =
CreateDatabaseManager();", don't need to store the created object. 
CreateUIManager should be similar.  I modified CreateUIManager instead.

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/test_safe_browsing_service.cc:101:
SafeBrowsingProtocolConfig* protocol_config) {
On 2016/05/07 at 00:05:32, Nathan Parker wrote:
> Would this be simpler as a pass- and store-by-value? That'd make it easier to
manage ownership of the thing.
Currently, TestSafeBrowsingService relies on if(protocol_config_) to determine
if use customized protocol config or not. If it is stored by value, it is hard
to check protocol_config_ has been set of not.

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/test_safe_browsing_service.cc:127: return
test_safe_browsing_service_;
On 2016/05/07 at 00:05:32, Nathan Parker wrote:
> Does the factory need to retain a copy of this ptr?  Or if it does, should it
instead be unique_ptr so it owns it?

hmm, it actually makes things more complicated. Since this object will be passed
to browser process and hold in scope_refptr. Keeping a ownership here make
destruction procedure super convoluted.

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/test_safe_browsing_service.h (right):

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/test_safe_browsing_service.h:109: // This is an
implmentation of SafeBrowsingUIManager without actually
On 2016/05/07 at 00:05:32, Nathan Parker wrote:
> nit: implementation

done.

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

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/unverified_download_policy_unittest.cc:83:
testing_safe_browsing_service_factory_.test_safe_browsing_service()
On 2016/05/07 at 00:05:32, Nathan Parker wrote:
> Shouldn't this set the FakeDatabaseManager on the factory obj, and then let it
create/initialize it normally?
> 
> Also, do you need to register the factory if you're then invoking it directly?

Ah, this is not a InProcessBrowserTest but rather a unit tests, and browser
process is already created. So we don't have to register the factory, but
calling browser_process->SetSafeBrowsingService() to force instantiate a
TestSafeBrowsingService instance. 
Though it is good idea to make it more consistent with other tests.

Used SetTestSafeBrowsingService() on factory obj instead.

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/unverified_download_policy_unittest.cc:98:
browser_process->SetSafeBrowsingService(nullptr);
No need this line, since TestSafeBrowsingFactory destructor takes care of it.

[Jialiu Lin, 2016-05-16 17:31:51]

jialiul@chromium.org changed reviewers:
+ asanka@chromium.org

[Jialiu Lin, 2016-05-16 17:31:52]

nparker@, 
Your comments are all addressed. Thanks!

asanka@, 
Let me know how it looks to you. And also need your owner review of download
related source code.

[Nathan Parker, 2016-05-16 19:56:39]

lgtm

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

https://codereview.chromium.org/1943993006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/test_safe_browsing_service.cc:101:
SafeBrowsingProtocolConfig* protocol_config) {
On 2016/05/12 21:53:58, JialiuLin wrote:
> On 2016/05/07 at 00:05:32, Nathan Parker wrote:
> > Would this be simpler as a pass- and store-by-value? That'd make it easier
to
> manage ownership of the thing.
> Currently, TestSafeBrowsingService relies on if(protocol_config_) to determine
> if use customized protocol config or not. If it is stored by value, it is hard
> to check protocol_config_ has been set of not. 
> 

Makes sense.

I'm not advocating it, but I've been looking for an excuse to use the new
base::Optional.  Maybe for new things it'd be better.

[asanka, 2016-05-18 17:08:47]

https://codereview.chromium.org/1943993006/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/test_safe_browsing_service.h (right):

https://codereview.chromium.org/1943993006/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/test_safe_browsing_service.h:64: void
SetV4ProtocolConfig(V4ProtocolConfig* v4_protocol_config);
At the moment we have two sets of SB satellite services. One set mediated by
ServicesDelegate, and allowing test overrides via ServiceCreator. Another set is
managed here. Shall we try to consolidate these?

[Jialiu Lin, 2016-05-18 19:06:55]

https://codereview.chromium.org/1943993006/diff/160001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/test_safe_browsing_service.h (right):

https://codereview.chromium.org/1943993006/diff/160001/chrome/browser/safe_br...
chrome/browser/safe_browsing/test_safe_browsing_service.h:64: void
SetV4ProtocolConfig(V4ProtocolConfig* v4_protocol_config);
On 2016/05/18 17:08:47, asanka wrote:
> At the moment we have two sets of SB satellite services. One set mediated by
> ServicesDelegate, and allowing test overrides via ServiceCreator. Another set
is
> managed here. Shall we try to consolidate these?

I tried to consolidate these two cases, but found it overly complicate the code.
I plan to leave download_browsertest, and download_protection_unittests (and
later the PPAPI download test once your CL is landed) to implement
ServicesDelegate. If I see more and more tests using the serviceDelegate, i will
add another test fixture for them.

[asanka, 2016-05-19 19:31:29]

lgtm.

Consolidating the service management will introduce a substantial amount of
churn. There are other approaches right in the Chromium tree like
/components/keyed_service which also handles inter-service dependencies, two
phase shutdown, and convenient methods for overriding service factories for
testing. I don't know how hard it would be to move to something like that.

[Jialiu Lin, 2016-05-19 21:39:21]

Patchset #4 (id:180001) has been deleted

[Jialiu Lin, 2016-05-19 23:58:22]

jialiul@chromium.org changed reviewers:
+ davidben@chromium.org, msw@chromium.org, sky@chromium.org

[Jialiu Lin, 2016-05-19 23:58:23]

Thanks, asanka@! 
+davidben@, msw@, sky@

davidben@chromium.org: Please review changes in
chrome/browser/prerender/prerender_browsertest.cc

msw@chromium.org: Please review changes in 
chrome/browser/ui/views/download/download_danger_prompt_views.cc
Change limits the scope of sending download recovery report. Same as in
chrome/browser/download/download_danger_prompt.cc

sky@chromium.org: Please review changes in
chrome/test/ppapi/ppapi_filechooser_browsertest.cc

[sky, 2016-05-20 15:51:07]

sky@chromium.org changed reviewers:
+ bbudge@chromium.org
- sky@chromium.org

[sky, 2016-05-20 15:51:08]

sky->bbudge for chrome/test/ppapi/ppapi_filechooser_browsertest.cc

[msw, 2016-05-20 16:50:19]

https://codereview.chromium.org/1943993006/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/download/download_danger_prompt_views.cc (right):

https://codereview.chromium.org/1943993006/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_danger_prompt_views.cc:333: const bool
accept = action == DownloadDangerPrompt::ACCEPT;
nit: |accept| is only used in the block below, move it there.

https://codereview.chromium.org/1943993006/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_danger_prompt_views.cc:334: if
(download_->IsDangerous() && !download_->IsDone()) {
IsDone also returns true for completed downloads, should you instead check the
download state for cancelled (and interrupted, in-progress, etc.)?

[Jialiu Lin, 2016-05-20 17:43:32]

Thanks, msw@!

https://codereview.chromium.org/1943993006/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/download/download_danger_prompt_views.cc (right):

https://codereview.chromium.org/1943993006/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_danger_prompt_views.cc:333: const bool
accept = action == DownloadDangerPrompt::ACCEPT;
On 2016/05/20 16:50:19, msw wrote:
> nit: |accept| is only used in the block below, move it there.
Thanks for catching this.

https://codereview.chromium.org/1943993006/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_danger_prompt_views.cc:334: if
(download_->IsDangerous() && !download_->IsDone()) {
On 2016/05/20 16:50:19, msw wrote:
> IsDone also returns true for completed downloads, should you instead check the
> download state for cancelled (and interrupted, in-progress, etc.)?
Here, we only need to exclude the canceled and completed states. This is to
prevent download danger prompt send report when it is already completed or
canceled. 
If a download is in the completed/canceled state, there is no need to recover it
anyway, so the download danger prompt should dismiss itself (this is handled in
OnDownloadUpdated), and no report should be sent either.

[msw, 2016-05-20 18:08:02]

https://codereview.chromium.org/1943993006/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/download/download_danger_prompt_views.cc (right):

https://codereview.chromium.org/1943993006/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/download/download_danger_prompt_views.cc:334: if
(download_->IsDangerous() && !download_->IsDone()) {
On 2016/05/20 17:43:32, JialiuLin wrote:
> On 2016/05/20 16:50:19, msw wrote:
> > IsDone also returns true for completed downloads, should you instead check
the
> > download state for cancelled (and interrupted, in-progress, etc.)?
> Here, we only need to exclude the canceled and completed states. This is to
> prevent download danger prompt send report when it is already completed or
> canceled. 
> If a download is in the completed/canceled state, there is no need to recover
it
> anyway, so the download danger prompt should dismiss itself (this is handled
in
> OnDownloadUpdated), and no report should be sent either.

Acknowledged, but perhaps I don't understand the motivation. Why avoid recording
that the prompt was shown if the download completes? Shouldn't it still record
that the prompt was shown and then perhaps record that it was ignored (no
ACCEPT/CANCEL/DISMISS), because the download finished or was otherwise somehow
cancelled/interrupted? Perhaps add an explanatory comment. Hopefully someone
more familiar with downloads has reviewed and approved this aspect of the
change, and future readers will understand the motivation.

[Jialiu Lin, 2016-05-20 19:07:11]

On 2016/05/20 18:08:02, msw wrote:
>
https://codereview.chromium.org/1943993006/diff/220001/chrome/browser/ui/view...
> File chrome/browser/ui/views/download/download_danger_prompt_views.cc (right):
> 
>
https://codereview.chromium.org/1943993006/diff/220001/chrome/browser/ui/view...
> chrome/browser/ui/views/download/download_danger_prompt_views.cc:334: if
> (download_->IsDangerous() && !download_->IsDone()) {
> On 2016/05/20 17:43:32, JialiuLin wrote:
> > On 2016/05/20 16:50:19, msw wrote:
> > > IsDone also returns true for completed downloads, should you instead check
> the
> > > download state for cancelled (and interrupted, in-progress, etc.)?
> > Here, we only need to exclude the canceled and completed states. This is to
> > prevent download danger prompt send report when it is already completed or
> > canceled. 
> > If a download is in the completed/canceled state, there is no need to
recover
> it
> > anyway, so the download danger prompt should dismiss itself (this is handled
> in
> > OnDownloadUpdated), and no report should be sent either.
> 
> Acknowledged, but perhaps I don't understand the motivation. Why avoid
recording
> that the prompt was shown if the download completes? Shouldn't it still record
> that the prompt was shown and then perhaps record that it was ignored (no
> ACCEPT/CANCEL/DISMISS), because the download finished or was otherwise somehow
> cancelled/interrupted? Perhaps add an explanatory comment. Hopefully someone
> more familiar with downloads has reviewed and approved this aspect of the
> change, and future readers will understand the motivation.
I see your confusion here. We record both shown and user actions in order to
calculate
click through rate. It is pretty straight forward for explicity
ACEPT/DENY/DIMISS cases.
If user totally ignored this prompt (not give any explicit action), when
this tab closes, a dismiss action will be automatically triggered and at that
moment we will record this prompt
is shown and dismissed.

The change here is actually to guard some rare cases. For example, when a user
tries to recover a blocked
downloads by clicking on the download recovery button, and sees the prompt(let's
call it prompt #1).
Without acting on this prompt, this user gets distracted and goes somewhere
else.
later the user opens another chrome://downloads page, and recovers
the same download through prompt #2, and this download completes. At this time,
prompt #1 should
dismiss itself without sending a report,because all the necessary reporting is
already
done by prompt#2. We don't want to double count user action or prompt shown
events.

I have added some comments added to make it more clear.

[Jialiu Lin, 2016-05-20 19:46:04]

jialiul@chromium.org changed reviewers:
- davidben@chromium.org

[Jialiu Lin, 2016-05-20 19:47:08]

jialiul@chromium.org changed reviewers:
+ mmenke@chromium.org

[Jialiu Lin, 2016-05-20 19:47:08]

mmenke@chromium.org: Could your please review changes in
prerender_browsertests.cc

[mmenke, 2016-05-20 20:08:38]

prerender LGTM

https://codereview.chromium.org/1943993006/diff/260001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/1943993006/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:754: return true;
nit:  Use braces when any part of an if spans more than one line.

https://codereview.chromium.org/1943993006/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:775: bool
CheckExtensionIDs(const std::set<std::string>& extension_ids,
include <std>

https://codereview.chromium.org/1943993006/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:799:
base::hash_map<std::string, SBThreatType> badurls_;
bad_urls_?

https://codereview.chromium.org/1943993006/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:799:
base::hash_map<std::string, SBThreatType> badurls_;
include base/hash_map, or wherever this is declared.

https://codereview.chromium.org/1943993006/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:1083: new
safe_browsing::TestSafeBrowsingServiceFactory()),
Do we get anything from making this a unique_ptr?  If we just call its
constructor on test fixture creation and its normal destructor when we're
destroyed, doesn't seem like there's any reason to use a smart pointer.

[msw, 2016-05-20 20:25:06]

Thanks for the comment and thoughtful response; lgtm.

[mmenke, 2016-05-20 20:25:59]

https://codereview.chromium.org/1943993006/diff/260001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/1943993006/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:775: bool
CheckExtensionIDs(const std::set<std::string>& extension_ids,
On 2016/05/20 20:08:37, mmenke wrote:
> include <std>

Erm..set, rather

[Jialiu Lin, 2016-05-20 20:33:30]

Thanks, mmenke@!

https://codereview.chromium.org/1943993006/diff/260001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/1943993006/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:754: return true;
On 2016/05/20 20:08:37, mmenke wrote:
> nit:  Use braces when any part of an if spans more than one line.

Done.

https://codereview.chromium.org/1943993006/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:775: bool
CheckExtensionIDs(const std::set<std::string>& extension_ids,
On 2016/05/20 20:08:37, mmenke wrote:
> include <std>

Done.

https://codereview.chromium.org/1943993006/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:775: bool
CheckExtensionIDs(const std::set<std::string>& extension_ids,
On 2016/05/20 20:25:59, mmenke wrote:
> On 2016/05/20 20:08:37, mmenke wrote:
> > include <std>
> 
> Erm..set, rather
I figured. :-)

https://codereview.chromium.org/1943993006/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:799:
base::hash_map<std::string, SBThreatType> badurls_;
On 2016/05/20 20:08:37, mmenke wrote:
> include base/hash_map, or wherever this is declared.
changed to std::unordered_map since hash_map is deprecated. 
Also added corresponding include.

https://codereview.chromium.org/1943993006/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:799:
base::hash_map<std::string, SBThreatType> badurls_;
On 2016/05/20 20:08:37, mmenke wrote:
> bad_urls_?

Done.

https://codereview.chromium.org/1943993006/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:1083: new
safe_browsing::TestSafeBrowsingServiceFactory()),
On 2016/05/20 20:08:37, mmenke wrote:
> Do we get anything from making this a unique_ptr?  If we just call its
> constructor on test fixture creation and its normal destructor when we're
> destroyed, doesn't seem like there's any reason to use a smart pointer.
agreed... I just tried to keep minimum change to the test file. But you're
right, there is no reason to use a smart pointer.

[Jialiu Lin, 2016-05-20 20:34:09]

jialiul@chromium.org changed reviewers:
- bbudge@chromium.org

[Jialiu Lin, 2016-05-20 22:16:42]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2016-05-20 22:16:42]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
nparker@chromium.org, mmenke@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1943993006/#ps280001
(title: "nits")

[commit-bot: I haz the power, 2016-05-20 22:17:04]

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

[commit-bot: I haz the power, 2016-05-20 22:22:37]

Description was changed from

==========
Create test fixture for SafeBrowsingService and its factory 
to simply test writing.

Also squeeze in a minor fix in DownloadDangerPrompt classes
to prevent download report sent for a already terminated or
not dangerous download.  

BUG=594683
==========

to

==========
Create test fixture for SafeBrowsingService and its factory 
to simply test writing.

Also squeeze in a minor fix in DownloadDangerPrompt classes
to prevent download report sent for a already terminated or
not dangerous download.  

BUG=594683
==========

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

Committed patchset #8 (id:280001)

[commit-bot: I haz the power, 2016-05-20 22:25:03]

Description was changed from

==========
Create test fixture for SafeBrowsingService and its factory 
to simply test writing.

Also squeeze in a minor fix in DownloadDangerPrompt classes
to prevent download report sent for a already terminated or
not dangerous download.  

BUG=594683
==========

to

==========
Create test fixture for SafeBrowsingService and its factory
to simply test writing.

Also squeeze in a minor fix in DownloadDangerPrompt classes
to prevent download report sent for a already terminated or
not dangerous download.

BUG=594683

Committed: https://crrev.com/7526f825c981ac645f6287b0b11b24e48a336cae
Cr-Commit-Position: refs/heads/master@{#395183}
==========

[commit-bot: I haz the power, 2016-05-20 22:25:05]

Patchset 8 (id:??) landed as
https://crrev.com/7526f825c981ac645f6287b0b11b24e48a336cae
Cr-Commit-Position: refs/heads/master@{#395183}