Refactor security interstitials, add SecurityInterstitialPage.

Refactor security interstitials, add SecurityInterstitialPage.

This patch adds a new SecurityInterstitialPage class and derives SSLBlockingPage
and SafeBrowsingBlockingPage from it. In the future, a captive portal
interstitial will also derive from this new class. In particular,
SecurityInterstitial does:

- Unify HTML generating code.
- Differentiate between different types of security interstitials for tests.
- Contain common code for showing and creating the interstitial page.

BUG=349737
Committed: https://crrev.com/6e48c989d1bb431c9922a36e94390d49b09d78b7
Cr-Commit-Position: refs/heads/master@{#304970}

[meacer, 2014-10-02 17:54:01]

meacer@chromium.org changed reviewers:
+ felt@chromium.org, mattm@chromium.org

[meacer, 2014-10-02 17:54:01]

felt, mattm, can you please take a look? Thanks.

[felt, 2014-10-02 19:39:03]

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
chrome/browser/interstitials/security_interstitial_page.cc:55:
base::i18n::WrapStringWithLTRFormatting(&host);
^ isn't this wrapping causing the bug with URLs on the SSL page in RTL? I was
thinking we should delete it

[meacer, 2014-10-02 19:42:48]

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
chrome/browser/interstitials/security_interstitial_page.cc:55:
base::i18n::WrapStringWithLTRFormatting(&host);
On 2014/10/02 19:39:03, felt wrote:
> ^ isn't this wrapping causing the bug with URLs on the SSL page in RTL? I was
> thinking we should delete it

I'm going to do that in a separate CL. It's not clear to me yet whether SB or
SSL version is the correct one given the other RTL mark visibility bug.

[felt, 2014-10-06 20:43:26]

lgtm.

[meacer, 2014-10-06 23:06:40]

meacer@chromium.org changed reviewers:
+ noelutz@chromium.org

[meacer, 2014-10-06 23:06:40]

Looks like Matt might be ooo this week. Noé, could you please take a look?

[mattm, 2014-10-14 08:39:19]

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
chrome/browser/interstitials/security_interstitial_page.cc:55:
base::i18n::WrapStringWithLTRFormatting(&host);
On 2014/10/02 19:42:48, Mustafa Emre Acer wrote:
> On 2014/10/02 19:39:03, felt wrote:
> > ^ isn't this wrapping causing the bug with URLs on the SSL page in RTL? I
was
> > thinking we should delete it
> 
> I'm going to do that in a separate CL. It's not clear to me yet whether SB or
> SSL version is the correct one given the other RTL mark visibility bug.

Seems like it would be better to fix that first and then do the merge. Otherwise
you might regress whichever one is doing it correctly currently.

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
chrome/browser/interstitials/security_interstitial_page.h:27: };
this seems like a bit of a layering violation

[meacer, 2014-10-20 22:43:15]

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
chrome/browser/interstitials/security_interstitial_page.cc:55:
base::i18n::WrapStringWithLTRFormatting(&host);
On 2014/10/14 08:39:19, mattm wrote:
> On 2014/10/02 19:42:48, Mustafa Emre Acer wrote:
> > On 2014/10/02 19:39:03, felt wrote:
> > > ^ isn't this wrapping causing the bug with URLs on the SSL page in RTL? I
> was
> > > thinking we should delete it
> > 
> > I'm going to do that in a separate CL. It's not clear to me yet whether SB
or
> > SSL version is the correct one given the other RTL mark visibility bug.
> 
> Seems like it would be better to fix that first and then do the merge.
Otherwise
> you might regress whichever one is doing it correctly currently.

This part is blocked on https://codereview.chromium.org/643963004/ Once that's
fixed, I'll get back to this.

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
chrome/browser/interstitials/security_interstitial_page.h:27: };
On 2014/10/14 08:39:19, mattm wrote:
> this seems like a bit of a layering violation

How do you feel about moving both SSL and SafeBrowsing interstitials under this
directory?

[mmenke, 2014-11-10 16:42:57]

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

[mmenke, 2014-11-10 16:42:58]

Two quick comments, inspired by looking at your followup CL.

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
chrome/browser/interstitials/security_interstitial_page.cc:61:
PopulateLoadTimeData(&load_time_data);
"LoadTime" is overloaded (See, for example, LoadTimingInfo), and "Data" is a
pretty empty term.  I would not have guessed it meant "static strings for this
specific SecurityInteristitialPage subclass".  Suggest LoadTimeData->Strings or
InterstitialStrings or PageSpecificStrings or somesuch.

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
chrome/browser/interstitials/security_interstitial_page.h:43: virtual void
PopulateLoadTimeData(
Should document this.

[meacer, 2014-11-18 23:47:57]

Reviewers: WDYT about SecurityInterstitialPage::Type? If you think it's a
layering violation, I'll move SafeBrowsingBlockingPage and SSLBlockingPage under
browser/interstitials but that's going to be a large change, so I'd like to
confirm you are OK with this before I move forward.

[mattm, 2014-11-19 00:36:39]

On 2014/11/18 23:47:57, Mustafa Emre Acer wrote:
> Reviewers: WDYT about SecurityInterstitialPage::Type? If you think it's a
> layering violation, I'll move SafeBrowsingBlockingPage and SSLBlockingPage
under
> browser/interstitials but that's going to be a large change, so I'd like to
> confirm you are OK with this before I move forward.


Sorry for not replying sooner.

Looking at this again, I guess since it's only used for testing it's not a huge
issue. An alternative would be to return a string or void* or something, that
each class could define it independently of the parent. Maybe something like the
"const void* kType = &kType" type of thing that is often used for
SupportsUserData keys. Then you could do something like:
ASSERT_EQ(SafeBrowsingBlockingPage::kType, page->GetTypeForTesting());

[mmenke, 2014-11-19 16:59:41]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[meacer, 2014-11-19 21:30:00]

Removed SecurityInterstitialPage::Type. PTAL.

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
chrome/browser/interstitials/security_interstitial_page.cc:55:
base::i18n::WrapStringWithLTRFormatting(&host);
On 2014/10/20 22:43:15, Mustafa Emre Acer wrote:
> On 2014/10/14 08:39:19, mattm wrote:
> > On 2014/10/02 19:42:48, Mustafa Emre Acer wrote:
> > > On 2014/10/02 19:39:03, felt wrote:
> > > > ^ isn't this wrapping causing the bug with URLs on the SSL page in RTL?
I
> > was
> > > > thinking we should delete it
> > > 
> > > I'm going to do that in a separate CL. It's not clear to me yet whether SB
> or
> > > SSL version is the correct one given the other RTL mark visibility bug.
> > 
> > Seems like it would be better to fix that first and then do the merge.
> Otherwise
> > you might regress whichever one is doing it correctly currently.
> 
> This part is blocked on https://codereview.chromium.org/643963004/ Once that's
> fixed, I'll get back to this.

Done, since crrev/643963004 is closed.

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
chrome/browser/interstitials/security_interstitial_page.cc:61:
PopulateLoadTimeData(&load_time_data);
On 2014/11/10 16:42:58, mmenke wrote:
> "LoadTime" is overloaded (See, for example, LoadTimingInfo), and "Data" is a
> pretty empty term.  I would not have guessed it meant "static strings for this
> specific SecurityInteristitialPage subclass".  Suggest LoadTimeData->Strings
or
> InterstitialStrings or PageSpecificStrings or somesuch.

Done.

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/622683006/diff/1/chrome/browser/interstitials...
chrome/browser/interstitials/security_interstitial_page.h:43: virtual void
PopulateLoadTimeData(
On 2014/11/10 16:42:58, mmenke wrote:
> Should document this.

Done.

[mattm, 2014-11-20 00:46:12]

lgtm

[felt, 2014-11-20 00:48:43]

On 2014/11/20 00:46:12, mattm wrote:
> lgtm

changes to chrome/browser/ssl/ still lgtm

[meacer, 2014-11-20 01:23:27]

meacer@chromium.org changed reviewers:
+ thestig@chromium.org

[meacer, 2014-11-20 01:23:28]

I fixed a merging error in ssl_blocking_page.cc. felt, can you please take
another look at patchset #3?

Lei: Can you please take a look at chrome/browser/interstitials?

palmer, rsleevi: FYI as I added you as OWNERs to chrome/browser/interstitials.

[felt, 2014-11-20 01:35:10]

On 2014/11/20 01:23:28, Mustafa Emre Acer wrote:
> I fixed a merging error in ssl_blocking_page.cc. felt, can you please take
> another look at patchset #3?
> 
> Lei: Can you please take a look at chrome/browser/interstitials?
> 
> palmer, rsleevi: FYI as I added you as OWNERs to chrome/browser/interstitials.

Looks fine still

[Lei Zhang, 2014-11-20 01:35:12]

Mostly nits, but there's some funny logic. See first comment.

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.cc:47: if
(!create_view_)
Expanding this out, it'll be:

if (create_view_ == false) {
  create_view_ = false;
}

which doesn't make sense to me.

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
nit: no "(c)" going forward

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:10: #include
"grit/browser_resources.h"
This should be in the .cc file as chrome/grit/browser_resources.h

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:26: virtual
~SecurityInterstitialPage() {}
bots may complain if the empty body isn't in the .cc file.

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:35: void
DontCreateViewForTesting();
Maybe PreventCreatingViewForTesting?

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:46: virtual
std::string GetHTMLContents() override;
no need for virtual if you are using override now.

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:49: base::string16
GetFormattedHostName();
const method?

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:58: // Once shown,
interstitial_page takes ownership of this
interstitial_page -> |interstitial_page_| ?

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:63: };
DISALLOW_COPY_AND_ASSIGN() unless you have some reason not to?

[meacer, 2014-11-20 01:58:53]

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.cc:47: if
(!create_view_)
On 2014/11/20 01:35:06, Lei Zhang wrote:
> Expanding this out, it'll be:
> 
> if (create_view_ == false) {
>   create_view_ = false;
> }
> 
> which doesn't make sense to me.

SecurityInterstitialPage is a delegate of |interstitial_page| and not a
descendant of it, so the |create_view_|s are different than each other. Perhaps
the naming of this class is being confusing, since it's not actually an
InterstitialPage but an InterstitialPageDelegate.

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
On 2014/11/20 01:35:12, Lei Zhang wrote:
> nit: no "(c)" going forward

Done.

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:10: #include
"grit/browser_resources.h"
On 2014/11/20 01:35:06, Lei Zhang wrote:
> This should be in the .cc file as chrome/grit/browser_resources.h

Done.

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:26: virtual
~SecurityInterstitialPage() {}
On 2014/11/20 01:35:12, Lei Zhang wrote:
> bots may complain if the empty body isn't in the .cc file.

Done.

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:35: void
DontCreateViewForTesting();
On 2014/11/20 01:35:06, Lei Zhang wrote:
> Maybe PreventCreatingViewForTesting?

This name is from InterstitialPage, I wanted to keep it consistent.

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:46: virtual
std::string GetHTMLContents() override;
On 2014/11/20 01:35:06, Lei Zhang wrote:
> no need for virtual if you are using override now.

Done.

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:49: base::string16
GetFormattedHostName();
On 2014/11/20 01:35:06, Lei Zhang wrote:
> const method?

Done.

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:58: // Once shown,
interstitial_page takes ownership of this
On 2014/11/20 01:35:12, Lei Zhang wrote:
> interstitial_page -> |interstitial_page_| ?

Done.

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.h:63: };
On 2014/11/20 01:35:06, Lei Zhang wrote:
> DISALLOW_COPY_AND_ASSIGN() unless you have some reason not to?

Done.

[Lei Zhang, 2014-11-20 02:05:27]

lgtm

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/622683006/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_page.cc:47: if
(!create_view_)
On 2014/11/20 01:58:52, Mustafa Emre Acer wrote:
> On 2014/11/20 01:35:06, Lei Zhang wrote:
> > Expanding this out, it'll be:
> > 
> > if (create_view_ == false) {
> >   create_view_ = false;
> > }
> > 
> > which doesn't make sense to me.
> 
> SecurityInterstitialPage is a delegate of |interstitial_page| and not a
> descendant of it, so the |create_view_|s are different than each other.
Perhaps
> the naming of this class is being confusing, since it's not actually an
> InterstitialPage but an InterstitialPageDelegate.

Hmm, I see this is essentially moved over from
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc, so I'll assume it's
correct.

[meacer, 2014-11-20 02:08:12]

Thanks all.

[meacer, 2014-11-20 02:08:18]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2014-11-20 02:09:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/622683006/60001

[commit-bot: I haz the power, 2014-11-20 04:18:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-20 04:19:30]

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