Componentize SafeBrowsingBlockingPage for WebView use

Componentize SafeBrowsingBlockingPage for WebView use

1. Create a BaseBlockingPage in components/safe_browsing, and move
   basic user interaction handling logic into this class
2. Implement BaseUIManager::DisplayBlockingPage to work with
   BaseBlockingPage
3. Make security_interstitials::MetricsHelper a non-abstract class,
   such that it can be instantiated by BaseBlockingPage
4. Adjust unit_tests and browser_tests
5. Rename BaseSafeBrowsingUIManager to BaseUIManager,
   BaseSafeBrowsingResourceThrottle to safe_browsing::ResourceThrottle

Overall, no logic change in 
chrome/browser/safe_browsing/SafeBrowsingBlockingPage class

BUG=666100
Review-Url: https://codereview.chromium.org/2623733002
Cr-Commit-Position: refs/heads/master@{#443087}
Committed: https://chromium.googlesource.com/chromium/src/+/3d6032edf2e884a270692a0fba98637bf525a52b

[Jialiu Lin, 2017-01-10 00:06:25]

Description was changed from

==========
Componentize SafeBrowsingBlockingPage for WebView use

1. Create a BaseSafeBrowsingBlockingPage in components/safe_browsing,
  and move basic user interaction handling logic into this class
2. Implement BaseSafeBrowsingUIManager::DisplayBlockingPage to work
   with BaseSafeBrowsingBlockingPage
3. Make security_interstitials::MetricsHelper a non-abstract class,
   such that it can be instantiated by BaseSafeBrowsingBlockingPage
4. Adjust unit_tests and browser_tests

BUG=666100
==========

to

==========
Componentize SafeBrowsingBlockingPage for WebView use

1. Create a BaseSafeBrowsingBlockingPage in components/safe_browsing,
  and move basic user interaction handling logic into this class
2. Implement BaseSafeBrowsingUIManager::DisplayBlockingPage to work
   with BaseSafeBrowsingBlockingPage
3. Make security_interstitials::MetricsHelper a non-abstract class,
   such that it can be instantiated by BaseSafeBrowsingBlockingPage
4. Adjust unit_tests and browser_tests

BUG=666100
==========

[Jialiu Lin, 2017-01-10 00:07:46]

Description was changed from

==========
Componentize SafeBrowsingBlockingPage for WebView use

1. Create a BaseSafeBrowsingBlockingPage in components/safe_browsing,
  and move basic user interaction handling logic into this class
2. Implement BaseSafeBrowsingUIManager::DisplayBlockingPage to work
   with BaseSafeBrowsingBlockingPage
3. Make security_interstitials::MetricsHelper a non-abstract class,
   such that it can be instantiated by BaseSafeBrowsingBlockingPage
4. Adjust unit_tests and browser_tests

BUG=666100
==========

to

==========
Componentize SafeBrowsingBlockingPage for WebView use

1. Create a BaseSafeBrowsingBlockingPage in components/safe_browsing,
  and move basic user interaction handling logic into this class
2. Implement BaseSafeBrowsingUIManager::DisplayBlockingPage to work
   with BaseSafeBrowsingBlockingPage
3. Make security_interstitials::MetricsHelper a non-abstract class,
   such that it can be instantiated by BaseSafeBrowsingBlockingPage
4. Adjust unit_tests and browser_tests

Overall, no logic change in 
chrome/browser/safe_browsing/SafeBrowsingBlockingPage class

BUG=666100
==========

[Jialiu Lin, 2017-01-10 00:07:56]

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

[commit-bot: I haz the power, 2017-01-10 00:09:22]

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

[Jialiu Lin, 2017-01-10 00:10:24]

jialiul@chromium.org changed reviewers:
+ meacer@chromium.org, ntfschr@chromium.org

[Jialiu Lin, 2017-01-10 00:10:25]

+ntfschr@ for overall refactoring
+meacer@ for owner review of components/security_interstitials/

Thanks,
Jialiu

[Jialiu Lin, 2017-01-10 00:11:24]

+cc nparker@, lpz@, and vakh@

[Nate Fischer, 2017-01-10 01:21:29]

lgtm

Overall, LGTM. Great work! I left a couple comments in-line.

https://codereview.chromium.org/2623733002/diff/1/components/safe_browsing/ba...
File components/safe_browsing/base_safe_browsing_blocking_page.h (right):

https://codereview.chromium.org/2623733002/diff/1/components/safe_browsing/ba...
components/safe_browsing/base_safe_browsing_blocking_page.h:109: // is invoked,
-1 if not entry should be removed.
nit: might read better as "-1 if entry should not be removed"

https://codereview.chromium.org/2623733002/diff/1/components/safe_browsing/ba...
File components/safe_browsing/base_ui_manager.cc (right):

https://codereview.chromium.org/2623733002/diff/1/components/safe_browsing/ba...
components/safe_browsing/base_ui_manager.cc:178: LOG(ERROR) << "This should not
be called: "
Why is this log here?

[Jialiu Lin, 2017-01-10 01:54:29]

Thanks!

https://codereview.chromium.org/2623733002/diff/1/components/safe_browsing/ba...
File components/safe_browsing/base_safe_browsing_blocking_page.h (right):

https://codereview.chromium.org/2623733002/diff/1/components/safe_browsing/ba...
components/safe_browsing/base_safe_browsing_blocking_page.h:109: // is invoked,
-1 if not entry should be removed.
On 2017/01/10 01:21:29, Nate Fischer wrote:
> nit: might read better as "-1 if entry should not be removed"

Done.

https://codereview.chromium.org/2623733002/diff/1/components/safe_browsing/ba...
File components/safe_browsing/base_ui_manager.cc (right):

https://codereview.chromium.org/2623733002/diff/1/components/safe_browsing/ba...
components/safe_browsing/base_ui_manager.cc:178: LOG(ERROR) << "This should not
be called: "
On 2017/01/10 01:21:29, Nate Fischer wrote:
> Why is this log here?
Oops, for my local testing use. should not be here. Deleted.

[vakh (use Gerrit instead), 2017-01-10 20:44:25]

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

[vakh (use Gerrit instead), 2017-01-10 20:44:27]

Nice refactor!

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
File components/safe_browsing/base_safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:52: sb_error_ui_ =
base::MakeUnique<SafeBrowsingErrorUI>(
To avoid repetition, re-write as:

sb_error_ui_ = base::MakeUnique<SafeBrowsingErrorUI>(
    unsafe_resources_[0].url, main_frame_url_,
    GetInterstitialReason(unsafe_resources_),
    display_options ? *display_options :
CreateDefaultDisplayOptions(unsafe_resources),
    ui_manager_->app_locale(), base::Time::NowFromSystemTime(),
    controller());

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:82:
std::vector<UnsafeResource> resources;
This should work and avoid the explicit call to push_back:

std::vector<UnsafeResource> resources {resource};

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:97: DVLOG(1) <<
__func__ << " " << unsafe_resource.url.spec();
Remove DVLOG?

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:131:
ui_manager_->OnBlockingPageDone(unsafe_resources_, true, web_contents(),
Add: /* proceed */

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:150:
ui_manager_->OnBlockingPageDone(unsafe_resources_, false, web_contents(),
Add: /* proceed */

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:280: malware =
true;
You could shortcut the method here with: return
SafeBrowsingErrorUI::SB_REASON_MALWARE;

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:307: GURL
request_url(unsafe_resources[0].url);
request_url is only being used in one place so we should not create that
temporary, just use unsafe_resources[0].url directly.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:333: return
display_options;
directly return?

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
File components/safe_browsing/base_safe_browsing_blocking_page.h (right):

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.h:26: class
BaseSafeBrowsingBlockingPage
Since this is already in the safe_browsing namespace and in
components/safe_browsing, we can drop SafeBrowsing from the name. That would
also make it consistent with the other class names in
components/safe_browsing_db.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.h:32: typedef
std::map<content::WebContents*, UnsafeResourceList> UnsafeResourceMap;
Do we need the map to be sorted? If not, we can use std::unordered_set

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.h:39:
BaseSafeBrowsingUIManager* ui_manager,
Ideally this should also have been called BaseUIManager but let's do that later.
The redundancy has no value.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.h:115: bool
proceeded_;
Please add a comment explaining the purpose.

[meacer, 2017-01-10 21:18:21]

Looking good! I added a bunch of renaming/refactoring suggestions.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
File components/safe_browsing/base_safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:33:
g_unsafe_resource_map = LAZY_INSTANCE_INITIALIZER;
You can make this leaky so that it's not destructed during shutdown.

base::LazyInstance<BaseSafeBrowsingBlockingPage::UnsafeResourceMap>::Leaky
    g_unsafe_resource_map = LAZY_INSTANCE_INITIALIZER;

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:45:
unsafe_resources[0].url,
nit: While you are at it, I wonder if it makes sense to refactor unsafe
resources code so that it doesn't necessarily rely on the first item.

E.g. something like:

struct UnsafeResourceList {
  GURL page_url;
  vector<UnsafeResource> resources;
}

I don't know if this is more desirable, or whether it makes sense at all, just
throwing the idea around.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:48:
main_frame_url_(main_frame_url),
Actually, it looks like we have two URLs here: unsafe_resources[0].url and
main_frame_url. Are they always the same? Can we use only one of them?

Looks like SecurityInterstitialPage isn't documented well, so it's hard to tell
what the |url| parameter in  SecurityInterstitialPage constructor refers to, but
it seems to be the main page URL (based on what content::InterstitialPage says).

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:53:
unsafe_resources_[0].url, main_frame_url_,
Same here, would be nice to use only one of these.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:63: controller());
It could be slightly cleaner to create display_options if it's null, and then
call SafeBrowsingErrorUI constructor once.

Or a better approach is to pass |display_options| as const ref and let the
caller handle a missing display_options parameter. I think I like that better
since |display_options| is not an output parameter (the pointer implies that).
And then you can push the construction of sb_error_ui to the initializer list:

So something like:

BaseSafeBrowsingBlockingPage::BaseSafeBrowsingBlockingPage(
    BaseSafeBrowsingUIManager* ui_manager,
    ...
    const SBErrorDisplayOptions& display_options)
  : SecurityInterstitialPage(...),
    ui_manager_(ui_manager),
    main_frame_url_(main_frame_url),
    unsafe_resources_(unsafe_resources),
    sb_error_ui_(base::MakeUnique<SafeBrowsingErrorUI>(...),
    proceeded_(false) {
 ....
}

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:70:
navigation_entry_index_to_remove_ = -1;
nit: I wonder if navigation_entry_index_to_remove_ could be const int so that
it's also initialized in the parameter list? That way the constructor body would
be empty which I think makes for a slightly easier to read code (all parameters
are set once and never modified).

It looks like navigation_entry_index_to_remove_ is set to -1 in OnDontProceed
but I wonder if that's needed? Once we DontProceed, we the blocking page should
be destructed, right?

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:83:
resources.push_back(unsafe_resource);
nit:

const std::vector<UnsafeResource> resources{unsafe_resource};

You could also inline this below.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:84: auto
display_options = CreateDefaultDisplayOptions(resources);
If you decide to change display_options as a const ref, you can also inline
this. Otherwise it'll need to be changed to const auto& display_options = ...

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:175:
navigation_entry_index_to_remove_ = -1;
nit: As I mentioned above, I wonder if this is necessary? It may very well be, I
just don't know about the code that well to tell :)

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
File components/safe_browsing/base_safe_browsing_blocking_page.h (right):

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.h:1: // Copyright 2017
The Chromium Authors. All rights reserved.
nit: I wonder if this class could be renamed to SafeBrowsingBlockingPage and its
Chrome implementation could be ChromeSafeBrowsingBlockingPage? That way, the
WebView implementation would be AwSafeBrowsingBlockingPage. That seems more
consistent with the rest of the codebase.

Same goes for BaseSafeBrowsingUIManager.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.h:26: class
BaseSafeBrowsingBlockingPage
nit: Documentation

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
File components/safe_browsing/base_ui_manager.cc (right):

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ui_manager.cc:102: LOG(ERROR) << "This should not
be called";
Forgot to remove? :)

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ui_manager.cc:185: MalwarePatternType proto;
This doesn't seem to be used, remove?

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ui_manager.cc:204: std::vector<UnsafeResource>
resources;
nit: Maybe inline this?

OnBlockingPageDone(std::vector<UnsafeResource>{resource}, false, ...);
return;

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ui_manager.cc:222: // TODO(jialiul): BaseUIManager
currently don't send HitReport.
nit: The todo seemed a bit abstract, perhaps file a bug and point to it
TODO(crbug/1234567) style?

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ui_manager.cc:247:
"BaseSafeBrowsingUIManager::MaybeReportSafeBrowsingHit";
nit: Remove?

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
File components/safe_browsing/base_ui_manager.h (right):

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ui_manager.h:102: virtual const std::string
app_locale();
const method

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ui_manager.h:109: virtual const GURL
default_safe_page();
const method

[Jialiu Lin, 2017-01-11 00:00:55]

Description was changed from

==========
Componentize SafeBrowsingBlockingPage for WebView use

1. Create a BaseSafeBrowsingBlockingPage in components/safe_browsing,
  and move basic user interaction handling logic into this class
2. Implement BaseSafeBrowsingUIManager::DisplayBlockingPage to work
   with BaseSafeBrowsingBlockingPage
3. Make security_interstitials::MetricsHelper a non-abstract class,
   such that it can be instantiated by BaseSafeBrowsingBlockingPage
4. Adjust unit_tests and browser_tests

Overall, no logic change in 
chrome/browser/safe_browsing/SafeBrowsingBlockingPage class

BUG=666100
==========

to

==========
Componentize SafeBrowsingBlockingPage for WebView use

1. Create a BaseBlockingPage in components/safe_browsing, and move
   basic user interaction handling logic into this class
2. Implement BaseUIManager::DisplayBlockingPage to work with
   BaseBlockingPage
3. Make security_interstitials::MetricsHelper a non-abstract class,
   such that it can be instantiated by BaseBlockingPage
4. Adjust unit_tests and browser_tests
5. Rename BaseSafeBrowsingUIManager to BaseUIManager,
   BaseSafeBrowsingResourceThrottle to safe_browsing::ResourceThrottle

Overall, no logic change in 
chrome/browser/safe_browsing/SafeBrowsingBlockingPage class

BUG=666100
==========

[Jialiu Lin, 2017-01-11 00:07:22]

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

[commit-bot: I haz the power, 2017-01-11 00:08:10]

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-01-11 00:11:36]

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

[commit-bot: I haz the power, 2017-01-11 00:11:37]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Jialiu Lin, 2017-01-11 00:48:51]

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

[commit-bot: I haz the power, 2017-01-11 00:49:27]

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

[Jialiu Lin, 2017-01-11 00:50:07]

Thanks for your thorough review, vakh@ and meacer@! 
All your comments are addressed.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
File components/safe_browsing/base_safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:33:
g_unsafe_resource_map = LAZY_INSTANCE_INITIALIZER;
On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> You can make this leaky so that it's not destructed during shutdown.
> 
> base::LazyInstance<BaseSafeBrowsingBlockingPage::UnsafeResourceMap>::Leaky
>     g_unsafe_resource_map = LAZY_INSTANCE_INITIALIZER;

Good to know. Thanks!

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:45:
unsafe_resources[0].url,
On 2017/01/10 21:18:20, Mustafa Emre Acer wrote:
> nit: While you are at it, I wonder if it makes sense to refactor unsafe
> resources code so that it doesn't necessarily rely on the first item.
> 
> E.g. something like:
> 
> struct UnsafeResourceList {
>   GURL page_url;
>   vector<UnsafeResource> resources;
> }
> 
> I don't know if this is more desirable, or whether it makes sense at all, just
> throwing the idea around.

Good thinking. Let's come back to this idea later, since it is kind of
orthogonal to this refactoring.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:48:
main_frame_url_(main_frame_url),
On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> Actually, it looks like we have two URLs here: unsafe_resources[0].url and
> main_frame_url. Are they always the same? Can we use only one of them?
> 
> Looks like SecurityInterstitialPage isn't documented well, so it's hard to
tell
> what the |url| parameter in  SecurityInterstitialPage constructor refers to,
but
> it seems to be the main page URL (based on what content::InterstitialPage
says).

No, they can be different things. For example, if the SB blocking page is
triggered
by a bad sub-resources (e.g. an iframe, or a bad scrpt), the
unsafe_resources[0].url
is the url of this sub-resource, and the main_frame_url is the url of the top
level
page.

I added some comments to the SecurityInterstitialPage constructor to make it
clear
that it could be sub-resource URL.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:52: sb_error_ui_ =
base::MakeUnique<SafeBrowsingErrorUI>(
On 2017/01/10 20:44:26, vakh (Varun Khaneja) wrote:
> To avoid repetition, re-write as:
> 
> sb_error_ui_ = base::MakeUnique<SafeBrowsingErrorUI>(
>     unsafe_resources_[0].url, main_frame_url_,
>     GetInterstitialReason(unsafe_resources_),
>     display_options ? *display_options :
> CreateDefaultDisplayOptions(unsafe_resources),
>     ui_manager_->app_locale(), base::Time::NowFromSystemTime(),
>     controller());
Done. Thanks!

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:53:
unsafe_resources_[0].url, main_frame_url_,
On 2017/01/10 21:18:20, Mustafa Emre Acer wrote:
> Same here, would be nice to use only one of these.

Similar to the above, they could be different URLs.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:63: controller());
On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> It could be slightly cleaner to create display_options if it's null, and then
> call SafeBrowsingErrorUI constructor once.
> 
> Or a better approach is to pass |display_options| as const ref and let the
> caller handle a missing display_options parameter. I think I like that better
> since |display_options| is not an output parameter (the pointer implies that).
> And then you can push the construction of sb_error_ui to the initializer list:
> 
> So something like:
> 
> BaseSafeBrowsingBlockingPage::BaseSafeBrowsingBlockingPage(
>     BaseSafeBrowsingUIManager* ui_manager,
>     ...
>     const SBErrorDisplayOptions& display_options)
>   : SecurityInterstitialPage(...),
>     ui_manager_(ui_manager),
>     main_frame_url_(main_frame_url),
>     unsafe_resources_(unsafe_resources),
>     sb_error_ui_(base::MakeUnique<SafeBrowsingErrorUI>(...),
>     proceeded_(false) {
>  ....
> }
> 

Done.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:70:
navigation_entry_index_to_remove_ = -1;
On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> nit: I wonder if navigation_entry_index_to_remove_ could be const int so that
> it's also initialized in the parameter list? That way the constructor body
would
> be empty which I think makes for a slightly easier to read code (all
parameters
> are set once and never modified).
> 
> It looks like navigation_entry_index_to_remove_ is set to -1 in OnDontProceed
> but I wonder if that's needed? Once we DontProceed, we the blocking page
should
> be destructed, right?
You're right. Done.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:82:
std::vector<UnsafeResource> resources;
On 2017/01/10 20:44:26, vakh (Varun Khaneja) wrote:
> This should work and avoid the explicit call to push_back:
> 
> std::vector<UnsafeResource> resources {resource};
> 

Done.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:83:
resources.push_back(unsafe_resource);
On 2017/01/10 21:18:20, Mustafa Emre Acer wrote:
> nit:
> 
> const std::vector<UnsafeResource> resources{unsafe_resource};
> 
> You could also inline this below.
> 

Done.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:84: auto
display_options = CreateDefaultDisplayOptions(resources);
On 2017/01/10 21:18:20, Mustafa Emre Acer wrote:
> If you decide to change display_options as a const ref, you can also inline
> this. Otherwise it'll need to be changed to const auto& display_options = ...

Changed CreateDefaultDisplayOptions(..) to return a SBErrorDisplayOptions*, so
it can be called inline.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:97: DVLOG(1) <<
__func__ << " " << unsafe_resource.url.spec();
On 2017/01/10 20:44:26, vakh (Varun Khaneja) wrote:
> Remove DVLOG?

Done.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:131:
ui_manager_->OnBlockingPageDone(unsafe_resources_, true, web_contents(),
On 2017/01/10 20:44:26, vakh (Varun Khaneja) wrote:
> Add: /* proceed */

Done.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:150:
ui_manager_->OnBlockingPageDone(unsafe_resources_, false, web_contents(),
On 2017/01/10 20:44:26, vakh (Varun Khaneja) wrote:
> Add: /* proceed */

Done.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:175:
navigation_entry_index_to_remove_ = -1;
On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> nit: As I mentioned above, I wonder if this is necessary? It may very well be,
I
> just don't know about the code that well to tell :)

You're right. the final assignment of navigation_entry_index_to_remove_  is not
necessary.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:280: malware =
true;
On 2017/01/10 20:44:26, vakh (Varun Khaneja) wrote:
> You could shortcut the method here with: return
> SafeBrowsingErrorUI::SB_REASON_MALWARE;

You're right. I just copied it down without taking a closer look. Thanks!

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:307: GURL
request_url(unsafe_resources[0].url);
On 2017/01/10 20:44:26, vakh (Varun Khaneja) wrote:
> request_url is only being used in one place so we should not create that
> temporary, just use unsafe_resources[0].url directly.

Done.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:333: return
display_options;
On 2017/01/10 20:44:26, vakh (Varun Khaneja) wrote:
> directly return?
Sure. Done.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
File components/safe_browsing/base_safe_browsing_blocking_page.h (right):

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.h:1: // Copyright 2017
The Chromium Authors. All rights reserved.
On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> nit: I wonder if this class could be renamed to SafeBrowsingBlockingPage and
its
> Chrome implementation could be ChromeSafeBrowsingBlockingPage? That way, the
> WebView implementation would be AwSafeBrowsingBlockingPage. That seems more
> consistent with the rest of the codebase.
> 
> Same goes for BaseSafeBrowsingUIManager.

Since it is already in the safe_browsing namespace, I'll rename this class to
BaseBlockingPage and BaseSafeBrowsingUIManager to BaseUIManager.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.h:1: // Copyright 2017
The Chromium Authors. All rights reserved.
On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> nit: I wonder if this class could be renamed to SafeBrowsingBlockingPage and
its
> Chrome implementation could be ChromeSafeBrowsingBlockingPage? That way, the
> WebView implementation would be AwSafeBrowsingBlockingPage. That seems more
> consistent with the rest of the codebase.
> 
> Same goes for BaseSafeBrowsingUIManager.

Renamed to BaseBlockingPage and BaseUIManager accordingly. Thanks!

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.h:26: class
BaseSafeBrowsingBlockingPage
On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> nit: Documentation

Done.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.h:32: typedef
std::map<content::WebContents*, UnsafeResourceList> UnsafeResourceMap;
On 2017/01/10 20:44:26, vakh (Varun Khaneja) wrote:
> Do we need the map to be sorted? If not, we can use std::unordered_set

Done.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.h:39:
BaseSafeBrowsingUIManager* ui_manager,
On 2017/01/10 20:44:26, vakh (Varun Khaneja) wrote:
> Ideally this should also have been called BaseUIManager but let's do that
later.
> The redundancy has no value.

Sure, renamed BaseUIManager as well.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.h:115: bool
proceeded_;
On 2017/01/10 20:44:27, vakh (Varun Khaneja) wrote:
> Please add a comment explaining the purpose.

Done.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
File components/safe_browsing/base_ui_manager.cc (right):

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ui_manager.cc:102: LOG(ERROR) << "This should not
be called";
On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> Forgot to remove? :)

ye..s....I forgot...

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ui_manager.cc:185: MalwarePatternType proto;
On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> This doesn't seem to be used, remove?

Hmm.. I wonder what's the story of this line. I copied this part of code from
SafeBrowsingUIManager. Need to remove this line from SafeBrowsingUIManager too.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ui_manager.cc:204: std::vector<UnsafeResource>
resources;
On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> nit: Maybe inline this?
> 
> OnBlockingPageDone(std::vector<UnsafeResource>{resource}, false, ...);
> return;

Done.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ui_manager.cc:222: // TODO(jialiul): BaseUIManager
currently don't send HitReport.
On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> nit: The todo seemed a bit abstract, perhaps file a bug and point to it
> TODO(crbug/1234567) style?

I probably should make it a comment instead of TODO. It's up to its subclasses
to implement the reporting logic.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ui_manager.cc:247:
"BaseSafeBrowsingUIManager::MaybeReportSafeBrowsingHit";
On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> nit: Remove?

Done.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
File components/safe_browsing/base_ui_manager.h (right):

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ui_manager.h:102: virtual const std::string
app_locale();
On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> const method

Done.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_ui_manager.h:109: virtual const GURL
default_safe_page();
On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> const method

Done.

[Jialiu Lin, 2017-01-11 00:53:13]

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

[Jialiu Lin, 2017-01-11 00:53:13]

+mmenke@ for c/b/loader/* owner review Thanks!

[meacer, 2017-01-11 01:33:31]

Almost lgt and m, with more nits :)

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
File components/safe_browsing/base_safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:48:
main_frame_url_(main_frame_url),
On 2017/01/11 00:50:06, Jialiu Lin wrote:
> On 2017/01/10 21:18:21, Mustafa Emre Acer wrote:
> > Actually, it looks like we have two URLs here: unsafe_resources[0].url and
> > main_frame_url. Are they always the same? Can we use only one of them?
> > 
> > Looks like SecurityInterstitialPage isn't documented well, so it's hard to
> tell
> > what the |url| parameter in  SecurityInterstitialPage constructor refers to,
> but
> > it seems to be the main page URL (based on what content::InterstitialPage
> says).
> 
> No, they can be different things. For example, if the SB blocking page is
> triggered
> by a bad sub-resources (e.g. an iframe, or a bad scrpt), the
> unsafe_resources[0].url
> is the url of this sub-resource, and the main_frame_url is the url of the top
> level
> page.
> 
> I added some comments to the SecurityInterstitialPage constructor to make it
> clear
> that it could be sub-resource URL.

Sounds good, thanks for updating SecurityInterstitialPage.

https://codereview.chromium.org/2623733002/diff/20001/components/safe_browsin...
components/safe_browsing/base_safe_browsing_blocking_page.cc:53:
unsafe_resources_[0].url, main_frame_url_,
On 2017/01/11 00:50:06, Jialiu Lin wrote:
> On 2017/01/10 21:18:20, Mustafa Emre Acer wrote:
> > Same here, would be nice to use only one of these.
> 
> Similar to the above, they could be different URLs.

Acknowledged.

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

https://codereview.chromium.org/2623733002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:121: void
FinishThreatDetails(int64_t delay_ms,
nit: I know this is existing code, but delay_ms could be a base::TimeDelta
instead, and safe_browsing_blocking_page.cc can pass timedeltas to it.

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

https://codereview.chromium.org/2623733002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.cc:208: const std::string
SafeBrowsingUIManager::app_locale() const {
nit: Add DCHECK_CURRENTLY_ON(...) calls for this, history_service and
default_safe_page()?

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

https://codereview.chromium.org/2623733002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.h:41: // was found.
nit: was -> is

https://codereview.chromium.org/2623733002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.h:45: // not whitelisted.
nit: maybe say "Called when |resource| is classified as unsafe ..."?

https://codereview.chromium.org/2623733002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.h:94: // Add and remove observers. 
These methods must be invoked on the UI thread.
tiny nit: Single space after the period.

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
File components/safe_browsing/base_blocking_page.cc (right):

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.cc:41:
SafeBrowsingErrorUI::SBErrorDisplayOptions* display_options)
I'm still not fully convinced |display_options| should be a pointer instead of a
const ref :)

It seems like one of the tests is setting it to nullptr but otherwise it always
has a value. Perhaps that test can pass an empty
SafeBrowsingErrorUI::SBErrorDisplayOptions() to here? That way, this could be
|const SafeBrowsingErrorUI::SBErrorDisplayOptions& display_options| instead.

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.cc:154: int last_committed_index =
nit: const int

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.cc:253: // Severity ranking: malware
> UwS (harmful) > phishing.
nit: It would be better to put this comment in the header, or inside the method.

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
File components/safe_browsing/base_blocking_page.h (right):

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:39: static BaseBlockingPage*
CreateBlockingPage(
This seems to be called only from BaseBlockingPage::ShowBlockingPage, can it be
private? Or maybe you can call the constructor directly and remove this method?

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:47: // You can call this method
several times, if an interstitial is already
tiny nit: might want to reword this sentence a bit since it has two if
conditions:

"This can be called several times. If an interstitial is already showing and the
user decides to proceed, it will be discarded and a new one will displayed"

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:57: // InterstitialPageDelegate
method:
nit: methods -> methods?

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:80: // this base class. Overrided
by SafeBrowsingBlockingPage class.
nit: Overrided -> Overridden (though I think only keeping the first sentence is
sufficient)

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:86: // should be warned about. 
They are queued when displaying more than one
nit: Double spaces after period

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:101: BaseUIManager* ui_manager_;
In general fields shouldn't directly made available to subclasses
(https://google.github.io/styleguide/cppguide.html#Access_Control).

So you can move them to the private section and add protected getters for them.

https://codereview.chromium.org/2623733002/diff/60001/components/security_int...
File components/security_interstitials/content/security_interstitial_page.h
(right):

https://codereview.chromium.org/2623733002/diff/60001/components/security_int...
components/security_interstitials/content/security_interstitial_page.h:31: //
can be a main frame URL or subresource URL.
The comment might not be accurate for non-SafeBrowsing interstitials. For
example, there is no subresource URL for SSL interstitials. Not sure how to make
the comment apply for both types of interstitials. How about:

"|request_url| is the URL which triggered the interstitial page. For
SafeBrowsing interstitials, it can be a main frame or a subresource URL. For SSL
interstitials, it's always the subresource URL"

[Jialiu Lin, 2017-01-11 02:48:46]

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

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

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

[Jialiu Lin, 2017-01-11 02:53:16]

Thanks meacer@!
My latest patch should've addressed all your comments. :-)

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

https://codereview.chromium.org/2623733002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:121: void
FinishThreatDetails(int64_t delay_ms,
On 2017/01/11 01:33:30, Mustafa Emre Acer wrote:
> nit: I know this is existing code, but delay_ms could be a base::TimeDelta
> instead, and safe_browsing_blocking_page.cc can pass timedeltas to it.

Done.

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

https://codereview.chromium.org/2623733002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.cc:208: const std::string
SafeBrowsingUIManager::app_locale() const {
On 2017/01/11 01:33:30, Mustafa Emre Acer wrote:
> nit: Add DCHECK_CURRENTLY_ON(...) calls for this, history_service and
> default_safe_page()?

Done.

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

https://codereview.chromium.org/2623733002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.h:41: // was found.
On 2017/01/11 01:33:30, Mustafa Emre Acer wrote:
> nit: was -> is

Done.

https://codereview.chromium.org/2623733002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.h:45: // not whitelisted.
On 2017/01/11 01:33:30, Mustafa Emre Acer wrote:
> nit: maybe say "Called when |resource| is classified as unsafe ..."?

Done.

https://codereview.chromium.org/2623733002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/ui_manager.h:94: // Add and remove observers. 
These methods must be invoked on the UI thread.
On 2017/01/11 01:33:30, Mustafa Emre Acer wrote:
> tiny nit: Single space after the period.

Good eye! Done.

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
File components/safe_browsing/base_blocking_page.cc (right):

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.cc:41:
SafeBrowsingErrorUI::SBErrorDisplayOptions* display_options)
On 2017/01/11 01:33:30, Mustafa Emre Acer wrote:
> I'm still not fully convinced |display_options| should be a pointer instead of
a
> const ref :)
> 
> It seems like one of the tests is setting it to nullptr but otherwise it
always
> has a value. Perhaps that test can pass an empty
> SafeBrowsingErrorUI::SBErrorDisplayOptions() to here? That way, this could be
> |const SafeBrowsingErrorUI::SBErrorDisplayOptions& display_options| instead.

Yes, make sense, I'll also make that unit test pass in a default
SBErrorDisplayOptions
instead of nullptr.
Done.

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.cc:154: int last_committed_index =
On 2017/01/11 01:33:30, Mustafa Emre Acer wrote:
> nit: const int

Done.

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.cc:253: // Severity ranking: malware
> UwS (harmful) > phishing.
On 2017/01/11 01:33:31, Mustafa Emre Acer wrote:
> nit: It would be better to put this comment in the header, or inside the
method.

Done.

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
File components/safe_browsing/base_blocking_page.h (right):

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:39: static BaseBlockingPage*
CreateBlockingPage(
On 2017/01/11 01:33:31, Mustafa Emre Acer wrote:
> This seems to be called only from BaseBlockingPage::ShowBlockingPage, can it
be
> private? Or maybe you can call the constructor directly and remove this
method?

Removed this function and directly call constructor in ShowBlowckingPage(..)

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:47: // You can call this method
several times, if an interstitial is already
On 2017/01/11 01:33:31, Mustafa Emre Acer wrote:
> tiny nit: might want to reword this sentence a bit since it has two if
> conditions:
> 
> "This can be called several times. If an interstitial is already showing and
the
> user decides to proceed, it will be discarded and a new one will displayed"
> 

Done.

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:57: // InterstitialPageDelegate
method:
On 2017/01/11 01:33:31, Mustafa Emre Acer wrote:
> nit: methods -> methods?

Done.

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:80: // this base class. Overrided
by SafeBrowsingBlockingPage class.
On 2017/01/11 01:33:31, Mustafa Emre Acer wrote:
> nit: Overrided -> Overridden (though I think only keeping the first sentence
is
> sufficient)

Done.

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:86: // should be warned about. 
They are queued when displaying more than one
On 2017/01/11 01:33:31, Mustafa Emre Acer wrote:
> nit: Double spaces after period

Done.

https://codereview.chromium.org/2623733002/diff/60001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:101: BaseUIManager* ui_manager_;
On 2017/01/11 01:33:31, Mustafa Emre Acer wrote:
> In general fields shouldn't directly made available to subclasses
> (https://google.github.io/styleguide/cppguide.html#Access_Control).
> 
> So you can move them to the private section and add protected getters for
them.

Done.

https://codereview.chromium.org/2623733002/diff/60001/components/security_int...
File components/security_interstitials/content/security_interstitial_page.h
(right):

https://codereview.chromium.org/2623733002/diff/60001/components/security_int...
components/security_interstitials/content/security_interstitial_page.h:31: //
can be a main frame URL or subresource URL.
On 2017/01/11 01:33:31, Mustafa Emre Acer wrote:
> The comment might not be accurate for non-SafeBrowsing interstitials. For
> example, there is no subresource URL for SSL interstitials. Not sure how to
make
> the comment apply for both types of interstitials. How about:
> 
> "|request_url| is the URL which triggered the interstitial page. For
> SafeBrowsing interstitials, it can be a main frame or a subresource URL. For
SSL
> interstitials, it's always the subresource URL"

much better. Thanks!
(I think you meant "For SSL interstitials, it's always the main frame url.")

[commit-bot: I haz the power, 2017-01-11 04:01:19]

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

[commit-bot: I haz the power, 2017-01-11 04:01:20]

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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2017-01-11 16:28:09]

chrome/browser/loader/ LGTM.

[meacer, 2017-01-11 18:59:41]

Thanks! LGTM.

https://codereview.chromium.org/2623733002/diff/80001/components/safe_browsin...
File components/safe_browsing/base_blocking_page.cc (right):

https://codereview.chromium.org/2623733002/diff/80001/components/safe_browsin...
components/safe_browsing/base_blocking_page.cc:89: const UnsafeResourceList
resources{unsafe_resource};
nit: Inline this?

new BaseBlockingPage(
    ...,
    UnsafeResourceList{unsafe_resource},
    ...);

https://codereview.chromium.org/2623733002/diff/80001/components/safe_browsin...
File components/safe_browsing/base_blocking_page.h (right):

https://codereview.chromium.org/2623733002/diff/80001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:100: GURL main_frame_url_;
nit: Getters for these protected variables as well (like ui_manager)

[Jialiu Lin, 2017-01-11 20:11:40]

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

[commit-bot: I haz the power, 2017-01-11 20:12:12]

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

[Jialiu Lin, 2017-01-11 20:13:29]

Thanks!

https://codereview.chromium.org/2623733002/diff/80001/components/safe_browsin...
File components/safe_browsing/base_blocking_page.cc (right):

https://codereview.chromium.org/2623733002/diff/80001/components/safe_browsin...
components/safe_browsing/base_blocking_page.cc:89: const UnsafeResourceList
resources{unsafe_resource};
On 2017/01/11 18:59:40, Mustafa Emre Acer wrote:
> nit: Inline this?
> 
> new BaseBlockingPage(
>     ...,
>     UnsafeResourceList{unsafe_resource},
>     ...);

acknowledged. |resources| is used 3 times in this function, that's why I created
a temp variable for it.

https://codereview.chromium.org/2623733002/diff/80001/components/safe_browsin...
File components/safe_browsing/base_blocking_page.h (right):

https://codereview.chromium.org/2623733002/diff/80001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:100: GURL main_frame_url_;
On 2017/01/11 18:59:41, Mustafa Emre Acer wrote:
> nit: Getters for these protected variables as well (like ui_manager)

Done.

[Jialiu Lin, 2017-01-11 20:14:32]

@vakh, any additional comments?

[vakh (use Gerrit instead), 2017-01-11 20:23:09]

lgtm

[Jialiu Lin, 2017-01-11 20:26:27]

The CQ bit was unchecked by jialiul@chromium.org

[Jialiu Lin, 2017-01-11 20:26:36]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-01-11 20:26:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
ntfschr@chromium.org, meacer@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2623733002/#ps100001
(title: "address final comments")

[commit-bot: I haz the power, 2017-01-11 20:26:48]

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-01-11 20:34:52]

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

[commit-bot: I haz the power, 2017-01-11 20:34:53]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Jialiu Lin, 2017-01-11 22:13:19]

The CQ bit was checked by jialiul@chromium.org

[commit-bot: I haz the power, 2017-01-11 22:14:07]

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-01-12 00:41:36]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484172799393190,
"parent_rev": "db73904ff35571b578ef8d05127ac0cf659037ca", "commit_rev":
"3d6032edf2e884a270692a0fba98637bf525a52b"}

[commit-bot: I haz the power, 2017-01-12 00:42:32]

Description was changed from

==========
Componentize SafeBrowsingBlockingPage for WebView use

1. Create a BaseBlockingPage in components/safe_browsing, and move
   basic user interaction handling logic into this class
2. Implement BaseUIManager::DisplayBlockingPage to work with
   BaseBlockingPage
3. Make security_interstitials::MetricsHelper a non-abstract class,
   such that it can be instantiated by BaseBlockingPage
4. Adjust unit_tests and browser_tests
5. Rename BaseSafeBrowsingUIManager to BaseUIManager,
   BaseSafeBrowsingResourceThrottle to safe_browsing::ResourceThrottle

Overall, no logic change in 
chrome/browser/safe_browsing/SafeBrowsingBlockingPage class

BUG=666100
==========

to

==========
Componentize SafeBrowsingBlockingPage for WebView use

1. Create a BaseBlockingPage in components/safe_browsing, and move
   basic user interaction handling logic into this class
2. Implement BaseUIManager::DisplayBlockingPage to work with
   BaseBlockingPage
3. Make security_interstitials::MetricsHelper a non-abstract class,
   such that it can be instantiated by BaseBlockingPage
4. Adjust unit_tests and browser_tests
5. Rename BaseSafeBrowsingUIManager to BaseUIManager,
   BaseSafeBrowsingResourceThrottle to safe_browsing::ResourceThrottle

Overall, no logic change in 
chrome/browser/safe_browsing/SafeBrowsingBlockingPage class

BUG=666100

Review-Url: https://codereview.chromium.org/2623733002
Cr-Commit-Position: refs/heads/master@{#443087}
Committed:
https://chromium.googlesource.com/chromium/src/+/3d6032edf2e884a270692a0fba98...
==========

[commit-bot: I haz the power, 2017-01-12 00:42:34]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/3d6032edf2e884a270692a0fba98...