Componentize the bad clock blocking page

Componentize the bad clock blocking page

This breaks the former BadClockBlockingPage into three parts:

1. security_interstitials::BadClockUI handles strings and commands.

2. security_interstitials::ControllerClient abstracts the functionality
   provided by the embedder to act on commands. ChromeControllerClient
   implements this functionality for Chrome. BadClockUI has a
   ControllerClient to ask to do its bidding.

3. BadClockBlockingPage implements InterstitialPageDelegate and has a
   BadClockUI, which it asks to handle strings, commands, and everything
   else not platform-specific.

(BadClockBlockingPage has a BadClockUI has a ControllerClient.)

Other embedders will:
- make their own analogs to the BadClockBlockingPage
- implement their own PlatformControllerClient that inherits from
  ControllerClient
- share the BadClockUI class

I also added security_interstitials::common_string_utils to hold string-
manipulation methods that I expect to reuse in the future for other
classes that are componentized.

BUG=488673
Committed: https://crrev.com/146e920a4cf5fda4c30bc348e16f81bfea5e3990
Cr-Commit-Position: refs/heads/master@{#362897}

[felt, 2015-11-28 08:53:02]

Description was changed from

==========
Componentize the bad clock blocking page

******** NOT READY FOR REVIEW YET **********

This breaks the former BadClockBlockingPage into three parts:

1. security_interstitials::BadClockUI handles strings and commands.

2. security_interstitials::ControllerClient abstracts the functionality
   provided by the embedder to act on commands. BadClockUI has a
   ControllerClient to do its bidding. ChromeControllerClient implements
   this functionality for Chrome.

3. BadClockBlockingPage implements InterstitialPageDelegate and has a
   BadClockUI, which it asks to handle strings, commands, and everything
   else not platform-specific.

(BadClockBlockingPage has a BadClockUI has a ControllerClient.)

Other embedders will:
- make their own analogs to the BadClockBlockingPage
- implement their own PlatformControllerClient that inherits from
  ControllerClient
- share the BadClockUI class

I also added security_interstitials::common_string_utils to hold string-
manipulation methods that I expect to reuse in the future for other
classes that are componentized.

BUG=488673
==========

to

==========
Componentize the bad clock blocking page

******** NOT READY FOR REVIEW YET **********

This breaks the former BadClockBlockingPage into three parts:

1. security_interstitials::BadClockUI handles strings and commands.

2. security_interstitials::ControllerClient abstracts the functionality
   provided by the embedder to act on commands. ChromeControllerClient
   implements this functionality for Chrome. BadClockUI has a
   ControllerClient to ask to do its bidding. 

3. BadClockBlockingPage implements InterstitialPageDelegate and has a
   BadClockUI, which it asks to handle strings, commands, and everything
   else not platform-specific.

(BadClockBlockingPage has a BadClockUI has a ControllerClient.)

Other embedders will:
- make their own analogs to the BadClockBlockingPage
- implement their own PlatformControllerClient that inherits from
  ControllerClient
- share the BadClockUI class

I also added security_interstitials::common_string_utils to hold string-
manipulation methods that I expect to reuse in the future for other
classes that are componentized.

BUG=488673
==========

[felt, 2015-11-28 19:21:28]

Patchset #4 (id:60001) has been deleted

[felt, 2015-11-28 19:21:38]

Patchset #3 (id:40001) has been deleted

[felt, 2015-11-28 19:21:47]

Patchset #2 (id:20001) has been deleted

[felt, 2015-11-28 19:21:54]

Patchset #1 (id:1) has been deleted

[felt, 2015-11-28 19:59:02]

Patchset #1 (id:80001) has been deleted

[felt, 2015-11-28 21:38:24]

Description was changed from

==========
Componentize the bad clock blocking page

******** NOT READY FOR REVIEW YET **********

This breaks the former BadClockBlockingPage into three parts:

1. security_interstitials::BadClockUI handles strings and commands.

2. security_interstitials::ControllerClient abstracts the functionality
   provided by the embedder to act on commands. ChromeControllerClient
   implements this functionality for Chrome. BadClockUI has a
   ControllerClient to ask to do its bidding. 

3. BadClockBlockingPage implements InterstitialPageDelegate and has a
   BadClockUI, which it asks to handle strings, commands, and everything
   else not platform-specific.

(BadClockBlockingPage has a BadClockUI has a ControllerClient.)

Other embedders will:
- make their own analogs to the BadClockBlockingPage
- implement their own PlatformControllerClient that inherits from
  ControllerClient
- share the BadClockUI class

I also added security_interstitials::common_string_utils to hold string-
manipulation methods that I expect to reuse in the future for other
classes that are componentized.

BUG=488673
==========

to

==========
Componentize the bad clock blocking page

This breaks the former BadClockBlockingPage into three parts:

1. security_interstitials::BadClockUI handles strings and commands.

2. security_interstitials::ControllerClient abstracts the functionality
   provided by the embedder to act on commands. ChromeControllerClient
   implements this functionality for Chrome. BadClockUI has a
   ControllerClient to ask to do its bidding. 

3. BadClockBlockingPage implements InterstitialPageDelegate and has a
   BadClockUI, which it asks to handle strings, commands, and everything
   else not platform-specific.

(BadClockBlockingPage has a BadClockUI has a ControllerClient.)

Other embedders will:
- make their own analogs to the BadClockBlockingPage
- implement their own PlatformControllerClient that inherits from
  ControllerClient
- share the BadClockUI class

I also added security_interstitials::common_string_utils to hold string-
manipulation methods that I expect to reuse in the future for other
classes that are componentized.

BUG=488673
==========

[felt, 2015-11-28 21:56:01]

felt@chromium.org changed reviewers:
+ blundell@chromium.org, estark@chromium.org

[felt, 2015-11-28 21:57:01]

estark, blundell, PTAL? In particular, I'd appreciate high-level thoughts on the
structure.

[blundell, 2015-11-30 16:49:21]

What's your vision for SecurityInterstitialPage? Are you envisioning that
continuing to live in //chrome? I had been envisioning all the interstitial
pages being componentized in shared core code and the client(s) being the
content::InterstitialPageDelegate(s). I'm not close enough to the code to know
what advantages/disadvantages that has wrt this approach.

[felt, 2015-11-30 17:26:52]

On 2015/11/30 16:49:21, blundell wrote:
> What's your vision for SecurityInterstitialPage? Are you envisioning that
> continuing to live in //chrome? I had been envisioning all the interstitial
> pages being componentized in shared core code and the client(s) being the
> content::InterstitialPageDelegate(s). I'm not close enough to the code to know
> what advantages/disadvantages that has wrt this approach.

I originally thought that WebView used interstitial pages, so my plan to start
with was to put the UI code in /core and the interstitial code in /content.
However, it turns out that WebView cannot use interstitials.

Given that, I instead thought that I would move all of the UI code into /core
along with a client that abstracts the necessary functionality of the UI: going
back, clicking checkboxes, launching date/time settings, etc. And then keep the
details relating specifically to interstitials in /chrome.

WDYT?

[felt, 2015-11-30 17:28:02]

On 2015/11/30 17:26:52, felt wrote:
> On 2015/11/30 16:49:21, blundell wrote:
> > What's your vision for SecurityInterstitialPage? Are you envisioning that
> > continuing to live in //chrome? I had been envisioning all the interstitial
> > pages being componentized in shared core code and the client(s) being the
> > content::InterstitialPageDelegate(s). I'm not close enough to the code to
know
> > what advantages/disadvantages that has wrt this approach.
> 
> I originally thought that WebView used interstitial pages, so my plan to start
> with was to put the UI code in /core and the interstitial code in /content.
> However, it turns out that WebView cannot use interstitials.
> 
> Given that, I instead thought that I would move all of the UI code into /core
> along with a client that abstracts the necessary functionality of the UI:
going
> back, clicking checkboxes, launching date/time settings, etc. And then keep
the
> details relating specifically to interstitials in /chrome.
> 
> WDYT?

(And in terms of what happens to SecurityInterstitialPage, most of its
UI-related functionality will move into the ChromeControllerClient. The parts
that are specific to InterstitialPageDelegates will remain in
SecurityInterstitialPage, in Chrome.)

[blundell, 2015-12-01 16:35:52]

This looks fine to me at a high level given the constraint you described. I'm a
little concerned that it might be leaving the embedder pretty fat (i.e. more
code to duplicate into //ios/chrome than would be ideal), but we can always see
what that looks like when the time comes.

I'll leave the detailed review to Emily.

[felt, 2015-12-01 17:22:56]

On 2015/12/01 16:35:52, blundell wrote:
> This looks fine to me at a high level given the constraint you described. I'm
a
> little concerned that it might be leaving the embedder pretty fat (i.e. more
> code to duplicate into //ios/chrome than would be ideal), but we can always
see
> what that looks like when the time comes.
> 
> I'll leave the detailed review to Emily.

How does iOS display interstitial pages without using the content/ layer? What
does that look like? Maybe my calculations will change if I have a better
understanding of that.

[blundell, 2015-12-01 17:39:40]

On 2015/12/01 17:22:56, felt wrote:
> On 2015/12/01 16:35:52, blundell wrote:
> > This looks fine to me at a high level given the constraint you described.
I'm
> a
> > little concerned that it might be leaving the embedder pretty fat (i.e. more
> > code to duplicate into //ios/chrome than would be ideal), but we can always
> see
> > what that looks like when the time comes.
> > 
> > I'll leave the detailed review to Emily.
> 
> How does iOS display interstitial pages without using the content/ layer? What
> does that look like? Maybe my calculations will change if I have a better
> understanding of that.

iOS has a parallel to //content's interstitial infrastructure; it's in
//ios/web/public/interstitials. It works more-or-less the same but probably a
good bit simplified.

We'll definitely have to duplicate the code that talks to the concrete
interstitial infrastructure. Ideally that code would be as thin and as simple as
possible since it will have to live in two places. I'm happy to go forward with
this; I think it will be much easier to get a sense of whether there's code that
should be on the other side of the boundary when I'm actually bringing up the
iOS embedder parts.

[felt, 2015-12-01 18:13:30]

On 2015/12/01 17:39:40, blundell wrote:
> On 2015/12/01 17:22:56, felt wrote:
> > On 2015/12/01 16:35:52, blundell wrote:
> > > This looks fine to me at a high level given the constraint you described.
> I'm
> > a
> > > little concerned that it might be leaving the embedder pretty fat (i.e.
more
> > > code to duplicate into //ios/chrome than would be ideal), but we can
always
> > see
> > > what that looks like when the time comes.
> > > 
> > > I'll leave the detailed review to Emily.
> > 
> > How does iOS display interstitial pages without using the content/ layer?
What
> > does that look like? Maybe my calculations will change if I have a better
> > understanding of that.
> 
> iOS has a parallel to //content's interstitial infrastructure; it's in
> //ios/web/public/interstitials. It works more-or-less the same but probably a
> good bit simplified.
> 
> We'll definitely have to duplicate the code that talks to the concrete
> interstitial infrastructure. Ideally that code would be as thin and as simple
as
> possible since it will have to live in two places. I'm happy to go forward
with
> this; I think it will be much easier to get a sense of whether there's code
that
> should be on the other side of the boundary when I'm actually bringing up the
> iOS embedder parts.

OK, let's go forward with this and then move more common parts over if necessary
later.

FWIW, the code that's going to remain in the Interstitial Pages is going to be
very simple.

[estark, 2015-12-01 18:40:17]

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/interst...
File chrome/browser/interstitials/chrome_controller_client.h (right):

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/interst...
chrome/browser/interstitials/chrome_controller_client.h:11: #include
"content/public/browser/web_contents.h"
can these be left as forward-declarations?

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/ssl/bad...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.cc:63: controller_(new
ChromeControllerClient(web_contents, nullptr)) {
What is this nullptr? This constructor appears to take only the single argument
unless I am very confused.

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.cc:77:
scoped_ptr<security_interstitials::MetricsHelper> metrics_helper(
Can this just be

scoped_ptr<security_interstitials::MetricsHelper> metrics_helper(new
ChromeMetricsHelper(...));

and remove line 74-75?

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/ssl/bad...
File chrome/browser/ssl/bad_clock_blocking_page.h (right):

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.h:67: const base::Time
time_triggered_;
Did you mean to drop the comment on this?

https://codereview.chromium.org/1481213003/diff/160001/components/security_in...
File components/security_interstitials/core/controller_client.h (right):

https://codereview.chromium.org/1481213003/diff/160001/components/security_in...
components/security_interstitials/core/controller_client.h:12: #include
"components/security_interstitials/core/metrics_helper.h"
why can't this be forward-declared?

[felt, 2015-12-01 19:13:45]

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/interst...
File chrome/browser/interstitials/chrome_controller_client.h (right):

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/interst...
chrome/browser/interstitials/chrome_controller_client.h:11: #include
"content/public/browser/web_contents.h"
On 2015/12/01 18:40:17, estark wrote:
> can these be left as forward-declarations?

Gah, I was just working in google3 where forward-declarations aren't allowed,
and I got the style guides mixed up. Will fix here and elsewhere.

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/ssl/bad...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.cc:63: controller_(new
ChromeControllerClient(web_contents, nullptr)) {
On 2015/12/01 18:40:17, estark wrote:
> What is this nullptr? This constructor appears to take only the single
argument
> unless I am very confused.

Vestiges of an old patchset, fixed.

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.cc:77:
scoped_ptr<security_interstitials::MetricsHelper> metrics_helper(
On 2015/12/01 18:40:17, estark wrote:
> Can this just be
> 
> scoped_ptr<security_interstitials::MetricsHelper> metrics_helper(new
> ChromeMetricsHelper(...));
> 
> and remove line 74-75?

I need to be able to call
chrome_metrics_helper->StartRecordingCaptivePortalMetrics, which is only
available on the ChromeMetricsHelper and not on the MetricsHelper.

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/ssl/bad...
File chrome/browser/ssl/bad_clock_blocking_page.h (right):

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.h:67: const base::Time
time_triggered_;
On 2015/12/01 18:40:17, estark wrote:
> Did you mean to drop the comment on this?

I didn't think it added anything. It's named time_triggered_ and the explanation
is that it's the time it was triggered.

https://codereview.chromium.org/1481213003/diff/160001/components/security_in...
File components/security_interstitials/core/controller_client.h (right):

https://codereview.chromium.org/1481213003/diff/160001/components/security_in...
components/security_interstitials/core/controller_client.h:12: #include
"components/security_interstitials/core/metrics_helper.h"
On 2015/12/01 18:40:17, estark wrote:
> why can't this be forward-declared?

Done.

[estark, 2015-12-01 21:41:51]

lgtm % removing the unnecessary #includes in favor of forward-declarations, and
the comment on bad_clock_blocking_page.cc:80

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/ssl/bad...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.cc:77:
scoped_ptr<security_interstitials::MetricsHelper> metrics_helper(
On 2015/12/01 19:13:44, felt wrote:
> On 2015/12/01 18:40:17, estark wrote:
> > Can this just be
> > 
> > scoped_ptr<security_interstitials::MetricsHelper> metrics_helper(new
> > ChromeMetricsHelper(...));
> > 
> > and remove line 74-75?
> 
> I need to be able to call
> chrome_metrics_helper->StartRecordingCaptivePortalMetrics, which is only
> available on the ChromeMetricsHelper and not on the MetricsHelper.

Oh, oops, I missed that

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/ssl/bad...
File chrome/browser/ssl/bad_clock_blocking_page.h (right):

https://codereview.chromium.org/1481213003/diff/160001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.h:67: const base::Time
time_triggered_;
On 2015/12/01 19:13:45, felt wrote:
> On 2015/12/01 18:40:17, estark wrote:
> > Did you mean to drop the comment on this?
> 
> I didn't think it added anything. It's named time_triggered_ and the
explanation
> is that it's the time it was triggered.

ok, makes sense, just wanted to make sure it wasn't an accident

https://codereview.chromium.org/1481213003/diff/180001/chrome/browser/interst...
File chrome/browser/interstitials/chrome_controller_client.h (right):

https://codereview.chromium.org/1481213003/diff/180001/chrome/browser/interst...
chrome/browser/interstitials/chrome_controller_client.h:10: #include
"content/public/browser/interstitial_page.h"
remove

https://codereview.chromium.org/1481213003/diff/180001/chrome/browser/interst...
chrome/browser/interstitials/chrome_controller_client.h:17: #include
"content/public/browser/web_contents.h"
woah, looks like you did something funny here :)

https://codereview.chromium.org/1481213003/diff/180001/chrome/browser/ssl/bad...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/1481213003/diff/180001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.cc:80: controller_.reset(new
ChromeControllerClient(web_contents));
I don't think you need this since you construct it on line 63

https://codereview.chromium.org/1481213003/diff/180001/chrome/browser/ssl/bad...
File chrome/browser/ssl/bad_clock_blocking_page.h (right):

https://codereview.chromium.org/1481213003/diff/180001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.h:14: #include
"components/security_interstitials/core/bad_clock_ui.h"
forward declare?

https://codereview.chromium.org/1481213003/diff/180001/components/security_in...
File components/security_interstitials/core/controller_client.h (right):

https://codereview.chromium.org/1481213003/diff/180001/components/security_in...
components/security_interstitials/core/controller_client.h:12: #include
"components/security_interstitials/core/metrics_helper.h"
unnecessary, forward-declared below

[felt, 2015-12-01 22:59:24]

https://codereview.chromium.org/1481213003/diff/180001/chrome/browser/interst...
File chrome/browser/interstitials/chrome_controller_client.h (right):

https://codereview.chromium.org/1481213003/diff/180001/chrome/browser/interst...
chrome/browser/interstitials/chrome_controller_client.h:10: #include
"content/public/browser/interstitial_page.h"
On 2015/12/01 21:41:51, estark wrote:
> remove

Done.

https://codereview.chromium.org/1481213003/diff/180001/chrome/browser/interst...
chrome/browser/interstitials/chrome_controller_client.h:17: #include
"content/public/browser/web_contents.h"
On 2015/12/01 21:41:51, estark wrote:
> woah, looks like you did something funny here :)

i have... absolutely no idea what happened here

https://codereview.chromium.org/1481213003/diff/180001/chrome/browser/ssl/bad...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/1481213003/diff/180001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.cc:80: controller_.reset(new
ChromeControllerClient(web_contents));
On 2015/12/01 21:41:51, estark wrote:
> I don't think you need this since you construct it on line 63

Done.

https://codereview.chromium.org/1481213003/diff/180001/chrome/browser/ssl/bad...
File chrome/browser/ssl/bad_clock_blocking_page.h (right):

https://codereview.chromium.org/1481213003/diff/180001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.h:14: #include
"components/security_interstitials/core/bad_clock_ui.h"
On 2015/12/01 21:41:51, estark wrote:
> forward declare?

Done.

https://codereview.chromium.org/1481213003/diff/180001/components/security_in...
File components/security_interstitials/core/controller_client.h (right):

https://codereview.chromium.org/1481213003/diff/180001/components/security_in...
components/security_interstitials/core/controller_client.h:12: #include
"components/security_interstitials/core/metrics_helper.h"
On 2015/12/01 21:41:51, estark wrote:
> unnecessary, forward-declared below

Done.

[felt, 2015-12-02 00:52:41]

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

[felt, 2015-12-02 00:52:41]

To approve new DEPS...
pkasting: +components/url_formatter/
mattm: +net/ssl

And mattm, please also take a look at the trivial change in for
c/b/safe_browsing/.

[Peter Kasting, 2015-12-02 01:14:40]

Dependency LGTM, but I have other questions.

https://codereview.chromium.org/1481213003/diff/200001/chrome/browser/interst...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/1481213003/diff/200001/chrome/browser/interst...
chrome/browser/interstitials/security_interstitial_page.cc:22: #include
"components/url_formatter/url_formatter.h"
Seems like this can be removed now?

https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
File components/security_interstitials/core/common_string_util.cc (right):

https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
components/security_interstitials/core/common_string_util.cc:23:
base::i18n::WrapStringWithLTRFormatting(&host);
Is this really correct?  We force the host to LTR even when it contains RTL
chars?  I'm concerned that this may be wrong for e.g. an Arabic subdomain +
gTLD.

https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
File components/security_interstitials/core/common_string_util.h (right):

https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
components/security_interstitials/core/common_string_util.h:21: base::string16
GetFormattedHostName(const GURL& gurl,
We should also be using this in
CaptivePortalBlockingPage::PopulateInterstitialStrings().

Should we also be using this in LocalizedError::GetStrings()?

[felt, 2015-12-02 03:30:38]

https://codereview.chromium.org/1481213003/diff/200001/chrome/browser/interst...
File chrome/browser/interstitials/security_interstitial_page.cc (right):

https://codereview.chromium.org/1481213003/diff/200001/chrome/browser/interst...
chrome/browser/interstitials/security_interstitial_page.cc:22: #include
"components/url_formatter/url_formatter.h"
On 2015/12/02 01:14:40, Peter Kasting wrote:
> Seems like this can be removed now?

Done.

https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
File components/security_interstitials/core/common_string_util.cc (right):

https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
components/security_interstitials/core/common_string_util.cc:23:
base::i18n::WrapStringWithLTRFormatting(&host);
On 2015/12/02 01:14:40, Peter Kasting wrote:
> Is this really correct?  We force the host to LTR even when it contains RTL
> chars?  I'm concerned that this may be wrong for e.g. an Arabic subdomain +
> gTLD.

This is the existing code, just moved. My understanding is that meacer@ tested
this. However, I have sent him an e-mail to ask him to check.

https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
File components/security_interstitials/core/common_string_util.h (right):

https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
components/security_interstitials/core/common_string_util.h:21: base::string16
GetFormattedHostName(const GURL& gurl,
On 2015/12/02 01:14:40, Peter Kasting wrote:
> We should also be using this in
> CaptivePortalBlockingPage::PopulateInterstitialStrings().

Yes, I'm going to go through each of the BlockingPages one at a time and convert
them in the same manner (using all of the methods here).

> Should we also be using this in LocalizedError::GetStrings()?

That's a good idea. I will see in a follow-up CL how much code could be shared
with LocalizedError.

[Peter Kasting, 2015-12-02 05:28:14]

OK, LGTM on this

[meacer, 2015-12-02 18:20:22]

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

[meacer, 2015-12-02 18:20:23]

https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
File components/security_interstitials/core/common_string_util.cc (right):

https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
components/security_interstitials/core/common_string_util.cc:23:
base::i18n::WrapStringWithLTRFormatting(&host);
On 2015/12/02 03:30:38, felt wrote:
> On 2015/12/02 01:14:40, Peter Kasting wrote:
> > Is this really correct?  We force the host to LTR even when it contains RTL
> > chars?  I'm concerned that this may be wrong for e.g. an Arabic subdomain +
> > gTLD.
> 
> This is the existing code, just moved. My understanding is that meacer@ tested
> this. However, I have sent him an e-mail to ask him to check.

I carried this over from previous code too
(https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti...),
can't fully confirm it's correctness. I filed crbug.com/451213 back then to
investigate this exact thing. Maybe a good time to check that?

[Peter Kasting, 2015-12-02 20:16:02]

pkasting@chromium.org changed reviewers:
+ mgiuca@chromium.org

[Peter Kasting, 2015-12-02 20:16:49]

On 2015/12/02 18:20:23, Mustafa Emre Acer wrote:
>
https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
> File components/security_interstitials/core/common_string_util.cc (right):
> 
>
https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
> components/security_interstitials/core/common_string_util.cc:23:
> base::i18n::WrapStringWithLTRFormatting(&host);
> On 2015/12/02 03:30:38, felt wrote:
> > On 2015/12/02 01:14:40, Peter Kasting wrote:
> > > Is this really correct?  We force the host to LTR even when it contains
RTL
> > > chars?  I'm concerned that this may be wrong for e.g. an Arabic subdomain
+
> > > gTLD.
> > 
> > This is the existing code, just moved. My understanding is that meacer@
tested
> > this. However, I have sent him an e-mail to ask him to check.
> 
> I carried this over from previous code too
>
(https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti...),
> can't fully confirm it's correctness. I filed crbug.com/451213 back then to
> investigate this exact thing. Maybe a good time to check that?

Added mgiuca as a reviewer based on his work with RTL hostnames in the omnibox. 
Matt, you want to look at the code in question and/or the bug linked just above
and add any comments you have?

[felt, 2015-12-02 20:18:31]

On 2015/12/02 20:16:49, Peter Kasting wrote:
> On 2015/12/02 18:20:23, Mustafa Emre Acer wrote:
> >
>
https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
> > File components/security_interstitials/core/common_string_util.cc (right):
> > 
> >
>
https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
> > components/security_interstitials/core/common_string_util.cc:23:
> > base::i18n::WrapStringWithLTRFormatting(&host);
> > On 2015/12/02 03:30:38, felt wrote:
> > > On 2015/12/02 01:14:40, Peter Kasting wrote:
> > > > Is this really correct?  We force the host to LTR even when it contains
> RTL
> > > > chars?  I'm concerned that this may be wrong for e.g. an Arabic
subdomain
> +
> > > > gTLD.
> > > 
> > > This is the existing code, just moved. My understanding is that meacer@
> tested
> > > this. However, I have sent him an e-mail to ask him to check.
> > 
> > I carried this over from previous code too
> >
>
(https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti...),
> > can't fully confirm it's correctness. I filed crbug.com/451213 back then to
> > investigate this exact thing. Maybe a good time to check that?
> 
> Added mgiuca as a reviewer based on his work with RTL hostnames in the
omnibox. 
> Matt, you want to look at the code in question and/or the bug linked just
above
> and add any comments you have?

FWIW this should be a straight refactoring CL. Even if there is a bug in the RTL
code I would not want to couple the fix with the refactoring. Could we please
move the discussion to the bug instead? crbug.com/451213

[Peter Kasting, 2015-12-02 20:19:50]

On 2015/12/02 20:18:31, felt wrote:
> On 2015/12/02 20:16:49, Peter Kasting wrote:
> > On 2015/12/02 18:20:23, Mustafa Emre Acer wrote:
> > >
> >
>
https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
> > > File components/security_interstitials/core/common_string_util.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
> > > components/security_interstitials/core/common_string_util.cc:23:
> > > base::i18n::WrapStringWithLTRFormatting(&host);
> > > On 2015/12/02 03:30:38, felt wrote:
> > > > On 2015/12/02 01:14:40, Peter Kasting wrote:
> > > > > Is this really correct?  We force the host to LTR even when it
contains
> > RTL
> > > > > chars?  I'm concerned that this may be wrong for e.g. an Arabic
> subdomain
> > +
> > > > > gTLD.
> > > > 
> > > > This is the existing code, just moved. My understanding is that meacer@
> > tested
> > > > this. However, I have sent him an e-mail to ask him to check.
> > > 
> > > I carried this over from previous code too
> > >
> >
>
(https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti...),
> > > can't fully confirm it's correctness. I filed crbug.com/451213 back then
to
> > > investigate this exact thing. Maybe a good time to check that?
> > 
> > Added mgiuca as a reviewer based on his work with RTL hostnames in the
> omnibox. 
> > Matt, you want to look at the code in question and/or the bug linked just
> above
> > and add any comments you have?
> 
> FWIW this should be a straight refactoring CL. Even if there is a bug in the
RTL
> code I would not want to couple the fix with the refactoring. Could we please
> move the discussion to the bug instead? crbug.com/451213

Right, I agree that not fixing in this CL is fine, I just CCed Matt here instead
of strictly on the bug because that bug doesn't seem (to me) to give as much
context as the code/discussion here.

[Matt Giuca, 2015-12-02 23:59:41]

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

[Matt Giuca, 2015-12-02 23:59:42]

Removing myself as reviewer (as discussed, I think this is out-of-scope for this
CL, will continue discussion on http://crbug.com/451213.).

https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
File components/security_interstitials/core/common_string_util.cc (right):

https://codereview.chromium.org/1481213003/diff/200001/components/security_in...
components/security_interstitials/core/common_string_util.cc:23:
base::i18n::WrapStringWithLTRFormatting(&host);
On 2015/12/02 18:20:23, Mustafa Emre Acer wrote:
> On 2015/12/02 03:30:38, felt wrote:
> > On 2015/12/02 01:14:40, Peter Kasting wrote:
> > > Is this really correct?  We force the host to LTR even when it contains
RTL
> > > chars?  I'm concerned that this may be wrong for e.g. an Arabic subdomain
+
> > > gTLD.
> > 
> > This is the existing code, just moved. My understanding is that meacer@
tested
> > this. However, I have sent him an e-mail to ask him to check.
> 
> I carried this over from previous code too
>
(https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti...),
> can't fully confirm it's correctness. I filed crbug.com/451213 back then to
> investigate this exact thing. Maybe a good time to check that?

Thanks for looping me in. I need to go back and do some more research on the
master bug at some point. I think this is more or less the same as
crbug.com/351639, and the answer is, we don't really know how to display it at
this point.

The current code here seems *generally* acceptable in the interim (it is on the
way to my proposed solution, which is to always display URLs in LTR no matter
the context or contents). BUT it may not match the current Omnibox
implementation (which IIRC renders in RTL if the language is RTL). I think if
you want to match the Omnibox, you would delete these two lines of code. But I
haven't tested.

Is there a known website with RTL syntax with an SSL error. Perhaps we can add
it to badssl.com so we can test this case?

(I don't want to block this particular CL which is just a refactor. Let's
continue the discussion on http://crbug.com/451213.)

[felt, 2015-12-03 00:48:13]

mattm, could you please take a look at the net/ssl DEPS addition and the tiny
change to c/b/safr_browsing?

[mattm, 2015-12-03 00:51:51]

lgtm

[felt, 2015-12-03 00:56:52]

The CQ bit was checked by felt@chromium.org

[felt, 2015-12-03 00:56:52]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org
Link to the patchset: https://codereview.chromium.org/1481213003/#ps220001
(title: "Change for pkasting")

[commit-bot: I haz the power, 2015-12-03 00:57:43]

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

[commit-bot: I haz the power, 2015-12-03 03:49:11]

Description was changed from

==========
Componentize the bad clock blocking page

This breaks the former BadClockBlockingPage into three parts:

1. security_interstitials::BadClockUI handles strings and commands.

2. security_interstitials::ControllerClient abstracts the functionality
   provided by the embedder to act on commands. ChromeControllerClient
   implements this functionality for Chrome. BadClockUI has a
   ControllerClient to ask to do its bidding. 

3. BadClockBlockingPage implements InterstitialPageDelegate and has a
   BadClockUI, which it asks to handle strings, commands, and everything
   else not platform-specific.

(BadClockBlockingPage has a BadClockUI has a ControllerClient.)

Other embedders will:
- make their own analogs to the BadClockBlockingPage
- implement their own PlatformControllerClient that inherits from
  ControllerClient
- share the BadClockUI class

I also added security_interstitials::common_string_utils to hold string-
manipulation methods that I expect to reuse in the future for other
classes that are componentized.

BUG=488673
==========

to

==========
Componentize the bad clock blocking page

This breaks the former BadClockBlockingPage into three parts:

1. security_interstitials::BadClockUI handles strings and commands.

2. security_interstitials::ControllerClient abstracts the functionality
   provided by the embedder to act on commands. ChromeControllerClient
   implements this functionality for Chrome. BadClockUI has a
   ControllerClient to ask to do its bidding. 

3. BadClockBlockingPage implements InterstitialPageDelegate and has a
   BadClockUI, which it asks to handle strings, commands, and everything
   else not platform-specific.

(BadClockBlockingPage has a BadClockUI has a ControllerClient.)

Other embedders will:
- make their own analogs to the BadClockBlockingPage
- implement their own PlatformControllerClient that inherits from
  ControllerClient
- share the BadClockUI class

I also added security_interstitials::common_string_utils to hold string-
manipulation methods that I expect to reuse in the future for other
classes that are componentized.

BUG=488673
==========

[commit-bot: I haz the power, 2015-12-03 03:49:12]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2015-12-03 03:49:48]

Description was changed from

==========
Componentize the bad clock blocking page

This breaks the former BadClockBlockingPage into three parts:

1. security_interstitials::BadClockUI handles strings and commands.

2. security_interstitials::ControllerClient abstracts the functionality
   provided by the embedder to act on commands. ChromeControllerClient
   implements this functionality for Chrome. BadClockUI has a
   ControllerClient to ask to do its bidding. 

3. BadClockBlockingPage implements InterstitialPageDelegate and has a
   BadClockUI, which it asks to handle strings, commands, and everything
   else not platform-specific.

(BadClockBlockingPage has a BadClockUI has a ControllerClient.)

Other embedders will:
- make their own analogs to the BadClockBlockingPage
- implement their own PlatformControllerClient that inherits from
  ControllerClient
- share the BadClockUI class

I also added security_interstitials::common_string_utils to hold string-
manipulation methods that I expect to reuse in the future for other
classes that are componentized.

BUG=488673
==========

to

==========
Componentize the bad clock blocking page

This breaks the former BadClockBlockingPage into three parts:

1. security_interstitials::BadClockUI handles strings and commands.

2. security_interstitials::ControllerClient abstracts the functionality
   provided by the embedder to act on commands. ChromeControllerClient
   implements this functionality for Chrome. BadClockUI has a
   ControllerClient to ask to do its bidding.

3. BadClockBlockingPage implements InterstitialPageDelegate and has a
   BadClockUI, which it asks to handle strings, commands, and everything
   else not platform-specific.

(BadClockBlockingPage has a BadClockUI has a ControllerClient.)

Other embedders will:
- make their own analogs to the BadClockBlockingPage
- implement their own PlatformControllerClient that inherits from
  ControllerClient
- share the BadClockUI class

I also added security_interstitials::common_string_utils to hold string-
manipulation methods that I expect to reuse in the future for other
classes that are componentized.

BUG=488673

Committed: https://crrev.com/146e920a4cf5fda4c30bc348e16f81bfea5e3990
Cr-Commit-Position: refs/heads/master@{#362897}
==========

[commit-bot: I haz the power, 2015-12-03 03:49:49]

Patchset 7 (id:??) landed as
https://crrev.com/146e920a4cf5fda4c30bc348e16f81bfea5e3990
Cr-Commit-Position: refs/heads/master@{#362897}