WebView: choose loud vs. quiet interstitial

WebView: choose loud vs. quiet interstitial

This CL lets WebView choose between a Loud vs. Quiet blocking page when
blocking malicious content.

This changes the constructor of BaseBlockingPage to accept an
ErrorUiType parameter indicating the correct SafeBrowsingErrorUI to
instantiate. It includes three states to cover interstitials that are
loud, giant, or small (to handle small and giant slightly differently in native code).

BaseSafeBrowsingErrorUI now has GetHTMLTemplateId(), which returns the 
proper resource ID for the blocking page's HTML template. This fixes a 
crash during the i18n stage when using the wrong HTML.

GetErrorUiType() has been plumbed out to native, allowing WebView 
native code to differentiate between the three interstitial states.

This adds the resources for the quiet interstitial, which corresponds 
to the following apk size increases:

 - SystemWebViewGoogle.apk's en-US.pak (6877 -> 7248) (+371)
 - SystemWebViewGoogle.apk's resources.pak (417015 -> 427494) (+10479)

BUG=718545
Review-Url: https://codereview.chromium.org/2898593002
Cr-Commit-Position: refs/heads/master@{#474858}
Committed: https://chromium.googlesource.com/chromium/src/+/96bd408ae32e5925624d5f4359687df707205de3

[Nate Fischer, 2017-05-20 04:47:18]

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

[commit-bot: I haz the power, 2017-05-20 04:47:30]

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-05-20 04:49:19]

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

[commit-bot: I haz the power, 2017-05-20 04:49:19]

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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Nate Fischer, 2017-05-20 04:54:49]

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

[commit-bot: I haz the power, 2017-05-20 04:54:59]

Dry run: CQ is trying da patch.

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

[Nate Fischer, 2017-05-20 04:55:53]

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

[Nate Fischer, 2017-05-20 04:55:54]

PTAL

jialiul@: for components/safe_browsing and components/security_interstitials
sgurun@: for android_webview

FYI felt@ and edwardjung@

I will update the CL description on Monday to include changes in apk size.

I had to upload with "--bypass-hooks" because of a DEPS issue I couldn't figure
out:

** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  components/safe_browsing/base_blocking_page.cc
      Illegal include: "components/grit/components_resources.h"
    Because of "-components" from components's include_rules.

Please let me know if someone has insight into how to fix this. Other components
include that grit file without issue, so this seems like a presubmit bug.

[commit-bot: I haz the power, 2017-05-20 04:57:09]

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

[commit-bot: I haz the power, 2017-05-20 04:57:09]

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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[felt, 2017-05-22 16:57:41]

Ideally the BaseBlockingPage shouldn't need to know the difference between
LOUD/QUIET/GIANT. I think the reason you did that was because of sb_error_ui_? I
think it should be possible to instantiate sb_error_ui_ in the child classes and
not in the base class, so then BaseBlockingPage doesn't need to know that
detail.

[felt, 2017-05-22 17:00:51]

On 2017/05/22 16:57:41, felt wrote:
> Ideally the BaseBlockingPage shouldn't need to know the difference between
> LOUD/QUIET/GIANT. I think the reason you did that was because of sb_error_ui_?
I
> think it should be possible to instantiate sb_error_ui_ in the child classes
and
> not in the base class, so then BaseBlockingPage doesn't need to know that
> detail.

And related to that we should avoid having GetHTMLTemplateId on the
BaseBlockingPage. (As a nice side effect this will fix your build deps problem
IIUC.)

[Jialiu Lin, 2017-05-22 17:06:00]

safe_browsing  and security_interstitial LGTM % nits.

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

https://codereview.chromium.org/2898593002/diff/20001/components/safe_browsin...
components/safe_browsing/base_blocking_page.cc:62: sb_error_ui_(error_type ==
ErrorUiType::LOUD
nit: construction list is supposed to be simple. How about moving the creation
of sb_error_ui_ into the body of constructor?

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

https://codereview.chromium.org/2898593002/diff/20001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:57: // parameters they require
period at the end.

https://codereview.chromium.org/2898593002/diff/20001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:153: ErrorUiType error_type_;
nit: can it be const?

[sgurun-gerrit only, 2017-05-22 21:05:30]

https://codereview.chromium.org/2898593002/diff/40001/android_webview/browser...
File android_webview/browser/aw_safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2898593002/diff/40001/android_webview/browser...
android_webview/browser/aw_safe_browsing_blocking_page.cc:68: ErrorUiType
errorType;
Have Java return the type rather than adding a new method for each type and
plumbing to java.

the enum is declared in Native. You can automatically generate the Java enum out
of it. See java_cpp_enum

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

https://codereview.chromium.org/2898593002/diff/40001/components/safe_browsin...
components/safe_browsing/base_blocking_page.cc:84: error_type_(error_type) {}
errorType sounds like we are dealing with an error here, while the goal is to
identify the type of UI.  
It is better to rename it to something that is more meaningful - such as
SafeBrowsingInterstitialType or SafeBrowsingInterstitialSize

[Nate Fischer, 2017-05-22 23:13:46]

Jialiu, your comments should be addressed.

Selim, I'll do the Java enum in the next patchset.

> > Ideally the BaseBlockingPage shouldn't need to know the difference between
> > LOUD/QUIET/GIANT. I think the reason you did that was because of
sb_error_ui_?
> > I think it should be possible to instantiate sb_error_ui_ in the child
classes and
> > not in the base class, so then BaseBlockingPage doesn't need to know that
> > detail.
>
> And related to that we should avoid having GetHTMLTemplateId on the
> BaseBlockingPage. (As a nice side effect this will fix your build deps problem
> IIUC.)

I originally tried this, but I had some trouble wrapping my head around all the
pointer ownership. Perhaps you could help me out here.

BaseSafeBrowsingErrorUI takes a ControllerClient* parameter, which it gets from
SecurityInterstitialPage::controller(). controller() can't be called until we're
already in BaseBlockingPage's constructor (once SecurityInterstitialPage is
created), which is after starting child class constructors.

I thought to save CreateControllerClient() to a stack variable, in order to
split it into a raw pointer and unique_ptr. But it's currently called in an
initialization list, so I don't think this is feasible. And it seems too weird
to require callers to pass both pointers for the same thing.

The remaining option is to leave things as-is, and allow BaseBlockingPage to
instantiate the SafeBrowsingErrorUI instance.

If I'm missing something, or you can think of a way to uncouple the classes,
please let me know. I'm also open to revisiting this in a future CL.

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

https://codereview.chromium.org/2898593002/diff/20001/components/safe_browsin...
components/safe_browsing/base_blocking_page.cc:62: sb_error_ui_(error_type ==
ErrorUiType::LOUD
On 2017/05/22 17:06:00, Jialiu Lin wrote:
> nit: construction list is supposed to be simple. How about moving the creation
> of sb_error_ui_ into the body of constructor?

Done.

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

https://codereview.chromium.org/2898593002/diff/20001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:57: // parameters they require
On 2017/05/22 17:06:00, Jialiu Lin wrote:
> period at the end.

Done.

https://codereview.chromium.org/2898593002/diff/20001/components/safe_browsin...
components/safe_browsing/base_blocking_page.h:153: ErrorUiType error_type_;
On 2017/05/22 17:06:00, Jialiu Lin wrote:
> nit: can it be const?

Acknowledged. Found a way to remove the member completely, which I think is the
better approach.

https://codereview.chromium.org/2898593002/diff/40001/android_webview/browser...
File android_webview/browser/aw_safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2898593002/diff/40001/android_webview/browser...
android_webview/browser/aw_safe_browsing_blocking_page.cc:68: ErrorUiType
errorType;
On 2017/05/22 21:05:29, sgurun wrote:
> Have Java return the type rather than adding a new method for each type and
> plumbing to java.
> 
> the enum is declared in Native. You can automatically generate the Java enum
out
> of it. See java_cpp_enum

Acknowledged. I'll work on this in the next patchset.

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

https://codereview.chromium.org/2898593002/diff/40001/components/safe_browsin...
components/safe_browsing/base_blocking_page.cc:84: error_type_(error_type) {}
On 2017/05/22 21:05:30, sgurun wrote:
> errorType sounds like we are dealing with an error here, while the goal is to
> identify the type of UI.  
> It is better to rename it to something that is more meaningful - such as
> SafeBrowsingInterstitialType or SafeBrowsingInterstitialSize

Acknowledged. The member is removed in the latest patchset, and the constructor
parameters have been renamed to errorUiType.

[Nate Fischer, 2017-05-22 23:48:05]

The CQ bit was checked by ntfschr@chromium.org

[Nate Fischer, 2017-05-22 23:48:05]

The patchset sent to the CQ was uploaded after l-g-t-m from jialiul@chromium.org
Link to the patchset: https://codereview.chromium.org/2898593002/#ps50018
(title: "Move GetHTMLTemplateId() to SafeBrowsingErrorUI, instantiate
sb_error_ui_ in constructor body, fix …")

[Nate Fischer, 2017-05-22 23:48:07]

The CQ bit was unchecked by ntfschr@chromium.org

[Nate Fischer, 2017-05-22 23:48:22]

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

[commit-bot: I haz the power, 2017-05-22 23:48:55]

Dry run: CQ is trying da patch.

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

[Nate Fischer, 2017-05-22 23:49:52]

Sorry, meant to click "dry run," not "commit"

[Nate Fischer, 2017-05-23 01:33:58]

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

[commit-bot: I haz the power, 2017-05-23 01:34:15]

Dry run: CQ is trying da patch.

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

[Nate Fischer, 2017-05-23 01:37:38]

ntfschr@chromium.org changed reviewers:
+ jochen@chromium.org

[Nate Fischer, 2017-05-23 01:37:39]

sgurun@, PTAL

jochen@, can you approve for the change to
//components/security_interstitials/DEPS?

[Nate Fischer, 2017-05-23 01:44:28]

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

[commit-bot: I haz the power, 2017-05-23 01:44:59]

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-05-23 02:20:38]

Thanks ntfschr! Could you add felt@ to reviewer list too?

[commit-bot: I haz the power, 2017-05-23 03:20:28]

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

[commit-bot: I haz the power, 2017-05-23 03:20:29]

Dry run: This issue passed the CQ dry run.

[Nate Fischer, 2017-05-23 03:20:32]

Description was changed from

==========
WebView: choose loud vs. quiet interstitial

This CL lets WebView choose between a Loud vs. Quiet blocking page when
blocking malicious content.

This changes the constructor of BaseBlockingPage to accept an
ErrorUiType parameter indicating the correct SafeBrowsingErrorUI to
instantiate. It includes three states to cover interstitials that are
loud, giant, or small (to handle small and giant slightly differently in
native code).

BaseBlockingPage now has GetHTMLTemplateId(), which returns the proper
resource ID for the blocking page's HTML template. This fixes a crash
during the i18n stage when using the wrong HTML.

ExtendsOutOfWindow() and CanShowInterstitial() are plumbed out to
native, allowing WebView native code to differentiate between the three
interstitial states.

BUG=718545
==========

to

==========
WebView: choose loud vs. quiet interstitial

This CL lets WebView choose between a Loud vs. Quiet blocking page when
blocking malicious content.

This changes the constructor of BaseBlockingPage to accept an
ErrorUiType parameter indicating the correct SafeBrowsingErrorUI to
instantiate. It includes three states to cover interstitials that are
loud, giant, or small (to handle small and giant slightly differently in
native code).

BaseBlockingPage now has GetHTMLTemplateId(), which returns the proper
resource ID for the blocking page's HTML template. This fixes a crash
during the i18n stage when using the wrong HTML.

ExtendsOutOfWindow() and CanShowInterstitial() are plumbed out to
native, allowing WebView native code to differentiate between the three
interstitial states.

BUG=718545
==========

[Nate Fischer, 2017-05-23 03:20:32]

ntfschr@chromium.org changed reviewers:
+ felt@chromium.org

[Nate Fischer, 2017-05-23 03:27:18]

On 2017/05/23 02:20:38, Jialiu Lin wrote:
> Thanks ntfschr! Could you add felt@ to reviewer list too?

Added. Felt, can you take a look?

[Nate Fischer, 2017-05-23 03:50:05]

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

[commit-bot: I haz the power, 2017-05-23 03:50:26]

Dry run: CQ is trying da patch.

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

[Nate Fischer, 2017-05-23 04:01:24]

Description was changed from

==========
WebView: choose loud vs. quiet interstitial

This CL lets WebView choose between a Loud vs. Quiet blocking page when
blocking malicious content.

This changes the constructor of BaseBlockingPage to accept an
ErrorUiType parameter indicating the correct SafeBrowsingErrorUI to
instantiate. It includes three states to cover interstitials that are
loud, giant, or small (to handle small and giant slightly differently in
native code).

BaseBlockingPage now has GetHTMLTemplateId(), which returns the proper
resource ID for the blocking page's HTML template. This fixes a crash
during the i18n stage when using the wrong HTML.

ExtendsOutOfWindow() and CanShowInterstitial() are plumbed out to
native, allowing WebView native code to differentiate between the three
interstitial states.

BUG=718545
==========

to

==========
WebView: choose loud vs. quiet interstitial

This CL lets WebView choose between a Loud vs. Quiet blocking page when
blocking malicious content.

This changes the constructor of BaseBlockingPage to accept an
ErrorUiType parameter indicating the correct SafeBrowsingErrorUI to
instantiate. It includes three states to cover interstitials that are
loud, giant, or small (to handle small and giant slightly differently in native
code).

BaseSafeBrowsingErrorUI now has GetHTMLTemplateId(), which returns the 
proper resource ID for the blocking page's HTML template. This fixes a 
crash during the i18n stage when using the wrong HTML.

GetErrorUiType() has been plumbed out to native, allowing WebView 
native code to differentiate between the three interstitial states.

This adds the resources for the quiet interstitial, which corresponds 
to the following apk size increases:

 - SystemWebViewGoogle.apk's en-US.pak (6877 -> 7248) (+371)
 - SystemWebViewGoogle.apk's resources.pak (417015 -> 427494) (+10479)

BUG=718545
==========

[commit-bot: I haz the power, 2017-05-23 06:30:07]

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

[commit-bot: I haz the power, 2017-05-23 06:30:08]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-05-23 11:12:06]

deps lgtm

please wait for others to approve the change

[sgurun-gerrit only, 2017-05-23 17:09:16]

lgtm after addressing these two comments.

https://codereview.chromium.org/2898593002/diff/120001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2898593002/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2974: if
(!canShowInterstitial()) return false;
I don't think this check is necessary. the caller should verify that by calling
canshowinterstitial. You can add a DCHECK though.

https://codereview.chromium.org/2898593002/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2975: if
(extendsOutOfWindow()) return false;
same as above.

[felt, 2017-05-23 17:10:11]

On 2017/05/23 17:09:16, sgurun wrote:
> lgtm after addressing these two comments.
> 
>
https://codereview.chromium.org/2898593002/diff/120001/android_webview/java/s...
> File android_webview/java/src/org/chromium/android_webview/AwContents.java
> (right):
> 
>
https://codereview.chromium.org/2898593002/diff/120001/android_webview/java/s...
> android_webview/java/src/org/chromium/android_webview/AwContents.java:2974: if
> (!canShowInterstitial()) return false;
> I don't think this check is necessary. the caller should verify that by
calling
> canshowinterstitial. You can add a DCHECK though.
> 
>
https://codereview.chromium.org/2898593002/diff/120001/android_webview/java/s...
> android_webview/java/src/org/chromium/android_webview/AwContents.java:2975: if
> (extendsOutOfWindow()) return false;
> same as above.

not lgtm
(sorry, I just want to make sure this doesn't land until I can approve -- I'll
review this morning)

[Nate Fischer, 2017-05-23 19:57:30]

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

[commit-bot: I haz the power, 2017-05-23 19:58:16]

Dry run: CQ is trying da patch.

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

[Nate Fischer, 2017-05-23 20:01:06]

Thanks, all! Comments should be addressed, I'll wait for Felt's review.

https://codereview.chromium.org/2898593002/diff/120001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2898593002/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2974: if
(!canShowInterstitial()) return false;
On 2017/05/23 17:09:15, sgurun wrote:
> I don't think this check is necessary. the caller should verify that by
calling
> canshowinterstitial. You can add a DCHECK though.

I don't think we can add DCHECKs in Java, so I just removed the conditional. Let
me know if you want the assert, a native DCHECK, or leave as-is without
anything.

https://codereview.chromium.org/2898593002/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2975: if
(extendsOutOfWindow()) return false;
On 2017/05/23 17:09:15, sgurun wrote:
> same as above. 

Done.

[sgurun-gerrit only, 2017-05-23 20:19:23]

https://codereview.chromium.org/2898593002/diff/120001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2898593002/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2974: if
(!canShowInterstitial()) return false;
On 2017/05/23 20:01:06, Nate Fischer wrote:
> On 2017/05/23 17:09:15, sgurun wrote:
> > I don't think this check is necessary. the caller should verify that by
> calling
> > canshowinterstitial. You can add a DCHECK though.
> 
> I don't think we can add DCHECKs in Java, so I just removed the conditional.
Let
> me know if you want the assert, a native DCHECK, or leave as-is without
> anything.

just drop, no need.

[Nate Fischer, 2017-05-23 20:21:19]

https://codereview.chromium.org/2898593002/diff/120001/android_webview/java/s...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2898593002/diff/120001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2974: if
(!canShowInterstitial()) return false;
On 2017/05/23 20:19:23, sgurun wrote:
> On 2017/05/23 20:01:06, Nate Fischer wrote:
> > On 2017/05/23 17:09:15, sgurun wrote:
> > > I don't think this check is necessary. the caller should verify that by
> > calling
> > > canshowinterstitial. You can add a DCHECK though.
> > 
> > I don't think we can add DCHECKs in Java, so I just removed the conditional.
> Let
> > me know if you want the assert, a native DCHECK, or leave as-is without
> > anything.
> 
> just drop, no need.

Ok, SGTM. That's what I did in PS#9

[commit-bot: I haz the power, 2017-05-23 21:39:24]

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

[commit-bot: I haz the power, 2017-05-23 21:39:25]

Dry run: This issue passed the CQ dry run.

[Nate Fischer, 2017-05-24 23:38:30]

felt@ friendly ping

I'm in no rush to land, I just wanted to make sure this didn't fall off your
radar (or that I'm not overlooking any comments you want me to address).

[felt, 2017-05-24 23:53:05]

> > > Ideally the BaseBlockingPage shouldn't need to know the difference between
> > > LOUD/QUIET/GIANT. I think the reason you did that was because of
> sb_error_ui_?
> > > I think it should be possible to instantiate sb_error_ui_ in the child
> classes and
> > > not in the base class, so then BaseBlockingPage doesn't need to know that
> > > detail.
> >
> > And related to that we should avoid having GetHTMLTemplateId on the
> > BaseBlockingPage. (As a nice side effect this will fix your build deps
problem
> > IIUC.)
> 
> I originally tried this, but I had some trouble wrapping my head around all
the
> pointer ownership. Perhaps you could help me out here.
> 
> BaseSafeBrowsingErrorUI takes a ControllerClient* parameter, which it gets
from
> SecurityInterstitialPage::controller(). controller() can't be called until
we're
> already in BaseBlockingPage's constructor (once SecurityInterstitialPage is
> created), which is after starting child class constructors.
> 
> I thought to save CreateControllerClient() to a stack variable, in order to
> split it into a raw pointer and unique_ptr. But it's currently called in an
> initialization list, so I don't think this is feasible. And it seems too weird
> to require callers to pass both pointers for the same thing.
> 
> The remaining option is to leave things as-is, and allow BaseBlockingPage to
> instantiate the SafeBrowsingErrorUI instance.
> 
> If I'm missing something, or you can think of a way to uncouple the classes,
> please let me know. I'm also open to revisiting this in a future CL.

I really really really don't want the base class to need to know what is going
on inside the child classes.

I am confused though. Won't the base class constructor run *before* the child
class constructors?

[sgurun-gerrit only, 2017-05-25 00:32:50]

On 2017/05/24 23:53:05, felt wrote:
> > > > Ideally the BaseBlockingPage shouldn't need to know the difference
between
> > > > LOUD/QUIET/GIANT. I think the reason you did that was because of
> > sb_error_ui_?
> > > > I think it should be possible to instantiate sb_error_ui_ in the child
> > classes and
> > > > not in the base class, so then BaseBlockingPage doesn't need to know
that
> > > > detail.
> > >
> > > And related to that we should avoid having GetHTMLTemplateId on the
> > > BaseBlockingPage. (As a nice side effect this will fix your build deps
> problem
> > > IIUC.)
> > 
> > I originally tried this, but I had some trouble wrapping my head around all
> the
> > pointer ownership. Perhaps you could help me out here.
> > 
> > BaseSafeBrowsingErrorUI takes a ControllerClient* parameter, which it gets
> from
> > SecurityInterstitialPage::controller(). controller() can't be called until
> we're
> > already in BaseBlockingPage's constructor (once SecurityInterstitialPage is
> > created), which is after starting child class constructors.
> > 
> > I thought to save CreateControllerClient() to a stack variable, in order to
> > split it into a raw pointer and unique_ptr. But it's currently called in an
> > initialization list, so I don't think this is feasible. And it seems too
weird
> > to require callers to pass both pointers for the same thing.
> > 
> > The remaining option is to leave things as-is, and allow BaseBlockingPage to
> > instantiate the SafeBrowsingErrorUI instance.
> > 
> > If I'm missing something, or you can think of a way to uncouple the classes,
> > please let me know. I'm also open to revisiting this in a future CL.
> 
> I really really really don't want the base class to need to know what is going
> on inside the child classes.
> 
> I am confused though. Won't the base class constructor run *before* the child
> class constructors?

Nate, felt@ is right. base class constructors run before the child class
constructor. 
Perhaps you can create a setter for setting a sb_error_ui_ in base_blocking_page
and then
call this setter in child class constructor to overwrite the existing
sb_error_ui?

[Nate Fischer, 2017-05-25 01:08:26]

On 2017/05/25 00:32:50, sgurun wrote:
> On 2017/05/24 23:53:05, felt wrote:
> > > > > Ideally the BaseBlockingPage shouldn't need to know the difference
> between
> > > > > LOUD/QUIET/GIANT. I think the reason you did that was because of
> > > sb_error_ui_?
> > > > > I think it should be possible to instantiate sb_error_ui_ in the child
> > > classes and
> > > > > not in the base class, so then BaseBlockingPage doesn't need to know
> that
> > > > > detail.
> > > >
> > > > And related to that we should avoid having GetHTMLTemplateId on the
> > > > BaseBlockingPage. (As a nice side effect this will fix your build deps
> > problem
> > > > IIUC.)
> > > 
> > > I originally tried this, but I had some trouble wrapping my head around
all
> > the
> > > pointer ownership. Perhaps you could help me out here.
> > > 
> > > BaseSafeBrowsingErrorUI takes a ControllerClient* parameter, which it gets
> > from
> > > SecurityInterstitialPage::controller(). controller() can't be called until
> > we're
> > > already in BaseBlockingPage's constructor (once SecurityInterstitialPage
is
> > > created), which is after starting child class constructors.
> > > 
> > > I thought to save CreateControllerClient() to a stack variable, in order
to
> > > split it into a raw pointer and unique_ptr. But it's currently called in
an
> > > initialization list, so I don't think this is feasible. And it seems too
> weird
> > > to require callers to pass both pointers for the same thing.
> > > 
> > > The remaining option is to leave things as-is, and allow BaseBlockingPage
to
> > > instantiate the SafeBrowsingErrorUI instance.
> > > 
> > > If I'm missing something, or you can think of a way to uncouple the
classes,
> > > please let me know. I'm also open to revisiting this in a future CL.
> > 
> > I really really really don't want the base class to need to know what is
going
> > on inside the child classes.
> > 
> > I am confused though. Won't the base class constructor run *before* the
child
> > class constructors?
> 
> Nate, felt@ is right. base class constructors run before the child class
> constructor. 
> Perhaps you can create a setter for setting a sb_error_ui_ in
base_blocking_page
> and then
> call this setter in child class constructor to overwrite the existing
> sb_error_ui?

I believe order of evaluation is: arguments to base class constructor -> base
class constructor -> rest of member initialization list -> constructor body.

sb_error_ui cannot be an argument to BaseBlockingPage. If sb_error_ui is an
argument to BaseBlockingPage, then it must be constructed before constructing
SecurityInterstitialPage. If we haven't constructed SecurityInterstitialPage,
then we can't call controller(). If we can't call controller(), then we can't
construct sb_error_ui.

My wording was unclear before. I hope my meaning is clear now.

Selim's approach is reasonable. We can set sb_error_ui to a LoudErrorUi in
BaseBlockingPage's member initialization list, and then
AwSafeBrowsingBlockingPage's constructor body will re-set it if we need a
QuietErrorUi instead. Is this agreeable?

[felt, 2017-05-25 13:15:12]

On 2017/05/25 01:08:26, Nate Fischer wrote:
> On 2017/05/25 00:32:50, sgurun wrote:
> > On 2017/05/24 23:53:05, felt wrote:
> > > > > > Ideally the BaseBlockingPage shouldn't need to know the difference
> > between
> > > > > > LOUD/QUIET/GIANT. I think the reason you did that was because of
> > > > sb_error_ui_?
> > > > > > I think it should be possible to instantiate sb_error_ui_ in the
child
> > > > classes and
> > > > > > not in the base class, so then BaseBlockingPage doesn't need to know
> > that
> > > > > > detail.
> > > > >
> > > > > And related to that we should avoid having GetHTMLTemplateId on the
> > > > > BaseBlockingPage. (As a nice side effect this will fix your build deps
> > > problem
> > > > > IIUC.)
> > > > 
> > > > I originally tried this, but I had some trouble wrapping my head around
> all
> > > the
> > > > pointer ownership. Perhaps you could help me out here.
> > > > 
> > > > BaseSafeBrowsingErrorUI takes a ControllerClient* parameter, which it
gets
> > > from
> > > > SecurityInterstitialPage::controller(). controller() can't be called
until
> > > we're
> > > > already in BaseBlockingPage's constructor (once SecurityInterstitialPage
> is
> > > > created), which is after starting child class constructors.
> > > > 
> > > > I thought to save CreateControllerClient() to a stack variable, in order
> to
> > > > split it into a raw pointer and unique_ptr. But it's currently called in
> an
> > > > initialization list, so I don't think this is feasible. And it seems too
> > weird
> > > > to require callers to pass both pointers for the same thing.
> > > > 
> > > > The remaining option is to leave things as-is, and allow
BaseBlockingPage
> to
> > > > instantiate the SafeBrowsingErrorUI instance.
> > > > 
> > > > If I'm missing something, or you can think of a way to uncouple the
> classes,
> > > > please let me know. I'm also open to revisiting this in a future CL.
> > > 
> > > I really really really don't want the base class to need to know what is
> going
> > > on inside the child classes.
> > > 
> > > I am confused though. Won't the base class constructor run *before* the
> child
> > > class constructors?
> > 
> > Nate, felt@ is right. base class constructors run before the child class
> > constructor. 
> > Perhaps you can create a setter for setting a sb_error_ui_ in
> base_blocking_page
> > and then
> > call this setter in child class constructor to overwrite the existing
> > sb_error_ui?
> 
> I believe order of evaluation is: arguments to base class constructor -> base
> class constructor -> rest of member initialization list -> constructor body.
> 
> sb_error_ui cannot be an argument to BaseBlockingPage. If sb_error_ui is an
> argument to BaseBlockingPage, then it must be constructed before constructing
> SecurityInterstitialPage. If we haven't constructed SecurityInterstitialPage,
> then we can't call controller(). If we can't call controller(), then we can't
> construct sb_error_ui.
> 
> My wording was unclear before. I hope my meaning is clear now.
> 
> Selim's approach is reasonable. We can set sb_error_ui to a LoudErrorUi in
> BaseBlockingPage's member initialization list, and then
> AwSafeBrowsingBlockingPage's constructor body will re-set it if we need a
> QuietErrorUi instead. Is this agreeable?

Selim's approach works for me.

Another option (which I was originally suggesting) is to not have sb_error_ui in
the base class at all. It would only exist in the subclasses and the base class
won't know about it at all. Wouldn't that also work? It doesn't look like the
base class actually needs it. I don't see how the order of evaluation would
affect it.

[Nate Fischer, 2017-05-25 19:37:31]

On 2017/05/25 13:15:12, felt wrote:
> On 2017/05/25 01:08:26, Nate Fischer wrote:
> > On 2017/05/25 00:32:50, sgurun wrote:
> > > On 2017/05/24 23:53:05, felt wrote:
> > > > > > > Ideally the BaseBlockingPage shouldn't need to know the difference
> > > between
> > > > > > > LOUD/QUIET/GIANT. I think the reason you did that was because of
> > > > > sb_error_ui_?
> > > > > > > I think it should be possible to instantiate sb_error_ui_ in the
> child
> > > > > classes and
> > > > > > > not in the base class, so then BaseBlockingPage doesn't need to
know
> > > that
> > > > > > > detail.
> > > > > >
> > > > > > And related to that we should avoid having GetHTMLTemplateId on the
> > > > > > BaseBlockingPage. (As a nice side effect this will fix your build
deps
> > > > problem
> > > > > > IIUC.)
> > > > > 
> > > > > I originally tried this, but I had some trouble wrapping my head
around
> > all
> > > > the
> > > > > pointer ownership. Perhaps you could help me out here.
> > > > > 
> > > > > BaseSafeBrowsingErrorUI takes a ControllerClient* parameter, which it
> gets
> > > > from
> > > > > SecurityInterstitialPage::controller(). controller() can't be called
> until
> > > > we're
> > > > > already in BaseBlockingPage's constructor (once
SecurityInterstitialPage
> > is
> > > > > created), which is after starting child class constructors.
> > > > > 
> > > > > I thought to save CreateControllerClient() to a stack variable, in
order
> > to
> > > > > split it into a raw pointer and unique_ptr. But it's currently called
in
> > an
> > > > > initialization list, so I don't think this is feasible. And it seems
too
> > > weird
> > > > > to require callers to pass both pointers for the same thing.
> > > > > 
> > > > > The remaining option is to leave things as-is, and allow
> BaseBlockingPage
> > to
> > > > > instantiate the SafeBrowsingErrorUI instance.
> > > > > 
> > > > > If I'm missing something, or you can think of a way to uncouple the
> > classes,
> > > > > please let me know. I'm also open to revisiting this in a future CL.
> > > > 
> > > > I really really really don't want the base class to need to know what is
> > going
> > > > on inside the child classes.
> > > > 
> > > > I am confused though. Won't the base class constructor run *before* the
> > child
> > > > class constructors?
> > > 
> > > Nate, felt@ is right. base class constructors run before the child class
> > > constructor. 
> > > Perhaps you can create a setter for setting a sb_error_ui_ in
> > base_blocking_page
> > > and then
> > > call this setter in child class constructor to overwrite the existing
> > > sb_error_ui?
> > 
> > I believe order of evaluation is: arguments to base class constructor ->
base
> > class constructor -> rest of member initialization list -> constructor body.
> > 
> > sb_error_ui cannot be an argument to BaseBlockingPage. If sb_error_ui is an
> > argument to BaseBlockingPage, then it must be constructed before
constructing
> > SecurityInterstitialPage. If we haven't constructed
SecurityInterstitialPage,
> > then we can't call controller(). If we can't call controller(), then we
can't
> > construct sb_error_ui.
> > 
> > My wording was unclear before. I hope my meaning is clear now.
> > 
> > Selim's approach is reasonable. We can set sb_error_ui to a LoudErrorUi in
> > BaseBlockingPage's member initialization list, and then
> > AwSafeBrowsingBlockingPage's constructor body will re-set it if we need a
> > QuietErrorUi instead. Is this agreeable?
> 
> Selim's approach works for me.

Ok, that's what I did for the most recent patchset. Let me know what you think.

> 
> Another option (which I was originally suggesting) is to not have sb_error_ui
in
> the base class at all. It would only exist in the subclasses and the base
class
> won't know about it at all. Wouldn't that also work? It doesn't look like the
> base class actually needs it. I don't see how the order of evaluation would
> affect it.

That would require BaseBlockingPage to be abstract, which is problematic because
it gets instantiated explicitly in ShowBlockingPage(). If we get rid of that
method, then we also have to make BaseUiManager abstract. I think this is
possible in principle, but I'm not sure how far back we would need to go (how
many classes would need to become abstract, and how many changes we need to make
to accommodate this), and I thought that a big refactoring would take away from
the point of this CL.

jialiul@, could you take a look again at components/safe_browsing/ and let me
know what you think?

[Nate Fischer, 2017-05-25 19:37:37]

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

[commit-bot: I haz the power, 2017-05-25 19:38: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-05-25 19:42:31]

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

[commit-bot: I haz the power, 2017-05-25 19:42:32]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[felt, 2017-05-25 19:48:20]

On 2017/05/25 19:37:31, Nate Fischer wrote:
> On 2017/05/25 13:15:12, felt wrote:
> > On 2017/05/25 01:08:26, Nate Fischer wrote:
> > > On 2017/05/25 00:32:50, sgurun wrote:
> > > > On 2017/05/24 23:53:05, felt wrote:
> > > > > > > > Ideally the BaseBlockingPage shouldn't need to know the
difference
> > > > between
> > > > > > > > LOUD/QUIET/GIANT. I think the reason you did that was because of
> > > > > > sb_error_ui_?
> > > > > > > > I think it should be possible to instantiate sb_error_ui_ in the
> > child
> > > > > > classes and
> > > > > > > > not in the base class, so then BaseBlockingPage doesn't need to
> know
> > > > that
> > > > > > > > detail.
> > > > > > >
> > > > > > > And related to that we should avoid having GetHTMLTemplateId on
the
> > > > > > > BaseBlockingPage. (As a nice side effect this will fix your build
> deps
> > > > > problem
> > > > > > > IIUC.)
> > > > > > 
> > > > > > I originally tried this, but I had some trouble wrapping my head
> around
> > > all
> > > > > the
> > > > > > pointer ownership. Perhaps you could help me out here.
> > > > > > 
> > > > > > BaseSafeBrowsingErrorUI takes a ControllerClient* parameter, which
it
> > gets
> > > > > from
> > > > > > SecurityInterstitialPage::controller(). controller() can't be called
> > until
> > > > > we're
> > > > > > already in BaseBlockingPage's constructor (once
> SecurityInterstitialPage
> > > is
> > > > > > created), which is after starting child class constructors.
> > > > > > 
> > > > > > I thought to save CreateControllerClient() to a stack variable, in
> order
> > > to
> > > > > > split it into a raw pointer and unique_ptr. But it's currently
called
> in
> > > an
> > > > > > initialization list, so I don't think this is feasible. And it seems
> too
> > > > weird
> > > > > > to require callers to pass both pointers for the same thing.
> > > > > > 
> > > > > > The remaining option is to leave things as-is, and allow
> > BaseBlockingPage
> > > to
> > > > > > instantiate the SafeBrowsingErrorUI instance.
> > > > > > 
> > > > > > If I'm missing something, or you can think of a way to uncouple the
> > > classes,
> > > > > > please let me know. I'm also open to revisiting this in a future CL.
> > > > > 
> > > > > I really really really don't want the base class to need to know what
is
> > > going
> > > > > on inside the child classes.
> > > > > 
> > > > > I am confused though. Won't the base class constructor run *before*
the
> > > child
> > > > > class constructors?
> > > > 
> > > > Nate, felt@ is right. base class constructors run before the child class
> > > > constructor. 
> > > > Perhaps you can create a setter for setting a sb_error_ui_ in
> > > base_blocking_page
> > > > and then
> > > > call this setter in child class constructor to overwrite the existing
> > > > sb_error_ui?
> > > 
> > > I believe order of evaluation is: arguments to base class constructor ->
> base
> > > class constructor -> rest of member initialization list -> constructor
body.
> > > 
> > > sb_error_ui cannot be an argument to BaseBlockingPage. If sb_error_ui is
an
> > > argument to BaseBlockingPage, then it must be constructed before
> constructing
> > > SecurityInterstitialPage. If we haven't constructed
> SecurityInterstitialPage,
> > > then we can't call controller(). If we can't call controller(), then we
> can't
> > > construct sb_error_ui.
> > > 
> > > My wording was unclear before. I hope my meaning is clear now.
> > > 
> > > Selim's approach is reasonable. We can set sb_error_ui to a LoudErrorUi in
> > > BaseBlockingPage's member initialization list, and then
> > > AwSafeBrowsingBlockingPage's constructor body will re-set it if we need a
> > > QuietErrorUi instead. Is this agreeable?
> > 
> > Selim's approach works for me.
> 
> Ok, that's what I did for the most recent patchset. Let me know what you
think.
> 
> > 
> > Another option (which I was originally suggesting) is to not have
sb_error_ui
> in
> > the base class at all. It would only exist in the subclasses and the base
> class
> > won't know about it at all. Wouldn't that also work? It doesn't look like
the
> > base class actually needs it. I don't see how the order of evaluation would
> > affect it.
> 
> That would require BaseBlockingPage to be abstract, which is problematic
because
> it gets instantiated explicitly in ShowBlockingPage(). If we get rid of that
> method, then we also have to make BaseUiManager abstract. I think this is
> possible in principle, but I'm not sure how far back we would need to go (how
> many classes would need to become abstract, and how many changes we need to
make
> to accommodate this), and I thought that a big refactoring would take away
from
> the point of this CL.
> 
> jialiul@, could you take a look again at components/safe_browsing/ and let me
> know what you think?

lgtm % the build failure. I can figure out what cleanup would be needed to turn
the BaseBlockingPage into either an interface or a set of helper classes, which
is what I think would be the right end state -- but shouldn't be a blocker here.

[Jialiu Lin, 2017-05-25 20:21:10]

BaseBlockingPage looks much better. Thanks!
LGTM

[Nate Fischer, 2017-05-25 20:48:26]

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

[commit-bot: I haz the power, 2017-05-25 20:49:08]

Dry run: CQ is trying da patch.

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

[Nate Fischer, 2017-05-25 20:50:35]

On 2017/05/25 20:21:10, Jialiu Lin wrote:
> BaseBlockingPage looks much better. Thanks!
> LGTM

Thanks, all! I'll land after dry run finishes unless anyone has final comments.

[Nate Fischer, 2017-05-25 22:47:37]

The CQ bit was unchecked by ntfschr@chromium.org

[Nate Fischer, 2017-05-25 22:47:48]

The CQ bit was checked by ntfschr@chromium.org

[Nate Fischer, 2017-05-25 22:47:48]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
sgurun@chromium.org, felt@chromium.org, jialiul@chromium.org
Link to the patchset: https://codereview.chromium.org/2898593002/#ps180001
(title: "Rebase, fix merge conflicts")

[commit-bot: I haz the power, 2017-05-25 22:48:53]

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-05-26 01:01:39]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495752468164900,
"parent_rev": "c5989b12b1bf78c6a04303e86c498a4ae9460a81", "commit_rev":
"96bd408ae32e5925624d5f4359687df707205de3"}

[commit-bot: I haz the power, 2017-05-26 01:01:51]

Description was changed from

==========
WebView: choose loud vs. quiet interstitial

This CL lets WebView choose between a Loud vs. Quiet blocking page when
blocking malicious content.

This changes the constructor of BaseBlockingPage to accept an
ErrorUiType parameter indicating the correct SafeBrowsingErrorUI to
instantiate. It includes three states to cover interstitials that are
loud, giant, or small (to handle small and giant slightly differently in native
code).

BaseSafeBrowsingErrorUI now has GetHTMLTemplateId(), which returns the 
proper resource ID for the blocking page's HTML template. This fixes a 
crash during the i18n stage when using the wrong HTML.

GetErrorUiType() has been plumbed out to native, allowing WebView 
native code to differentiate between the three interstitial states.

This adds the resources for the quiet interstitial, which corresponds 
to the following apk size increases:

 - SystemWebViewGoogle.apk's en-US.pak (6877 -> 7248) (+371)
 - SystemWebViewGoogle.apk's resources.pak (417015 -> 427494) (+10479)

BUG=718545
==========

to

==========
WebView: choose loud vs. quiet interstitial

This CL lets WebView choose between a Loud vs. Quiet blocking page when
blocking malicious content.

This changes the constructor of BaseBlockingPage to accept an
ErrorUiType parameter indicating the correct SafeBrowsingErrorUI to
instantiate. It includes three states to cover interstitials that are
loud, giant, or small (to handle small and giant slightly differently in native
code).

BaseSafeBrowsingErrorUI now has GetHTMLTemplateId(), which returns the 
proper resource ID for the blocking page's HTML template. This fixes a 
crash during the i18n stage when using the wrong HTML.

GetErrorUiType() has been plumbed out to native, allowing WebView 
native code to differentiate between the three interstitial states.

This adds the resources for the quiet interstitial, which corresponds 
to the following apk size increases:

 - SystemWebViewGoogle.apk's en-US.pak (6877 -> 7248) (+371)
 - SystemWebViewGoogle.apk's resources.pak (417015 -> 427494) (+10479)

BUG=718545

Review-Url: https://codereview.chromium.org/2898593002
Cr-Commit-Position: refs/heads/master@{#474858}
Committed:
https://chromium.googlesource.com/chromium/src/+/96bd408ae32e5925624d5f435968...
==========

[commit-bot: I haz the power, 2017-05-26 01:01:53]

Committed patchset #11 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/96bd408ae32e5925624d5f435968...