I'm trying to start an implementation for this, and I want to understand the lifetime of this new class.

If I understand correctly, SSLErrorControllerClient replaces the ChromeControllerClient used by SSLBlockingPage in its parent constructor call to SecurityInterstitialPage(). https://cs.chromium.org/chromium/src/chrome/browser/ssl/ssl_blocking_page.cc?l=181&rcl=f39c062ad8adeb6c7c97440ef1d626f954d138f7

The current inheritance chain is:
- ControllerClient
  - SecurityInterstitialControllerClient (implements e.g. GoBack)
    - ChromeControllerClient (implements e.g. LaunchDateAndTimeSettings)

Your description introduces:

- ControllerClient
  - SSLErrorControllerClient
    - SSLErrorControllerContentClient
      - owns a SecurityInterstitialControllerClient

I see some issues there:
1) SSLBlockingPage (and similar for other interstitials) needs to know whether to construct ChromeControllerClient or SSLErrorControllerContentClient.
2) SecurityInterstitialPage currently takes an SecurityInterstitialControllerClient, not a general ControllerClient. It currently assumes it can call `set_interstitial_page()`, which does not exist on ControllerClient.
3) SSLErrorControllerContentClient won't implement LaunchDateAndTimeSettings() if it owns a regular SecurityInterstitialControllerClient.

1) has a simple strawman: modify the constructors of interstitials to take a boolean for whether we're using committed interstitials or not, and construct accordingly.
We could instead pass a ControllerClient to the constructors, but then we'd still have to construct a ChromeControllerClient inside //chrome in case ControllerClient is a SSLErrorControllerContentClient (see #3).

2) The options I can think of are:
- Introduce a virtual class that both SecurityInterstitialControllerClient and SSLErrorControllerClient inherit from.
- Add set_interstitial_page() and interstitial_page() (the two public methods of SecurityInterstitialControllerClient) to ControllerClient.

Do you have a preference, or another idea?

3) We can handle this by making the SecurityInterstitialControllerClient that SSLErrorControllerContentClient owns an instance of ChromeControllerClient.

> It will have its own GoBack implementation that simply does a back navigation. SSLErrorControllerClient will implement Proceed() by calling a protected virtual method to store a certificate exception and then call Reload().

Looks like GoBack() can just copy https://cs.chromium.org/chromium/src/components/security_interstitials/content/security_interstitial_controller_client.cc?l=53&rcl=adb61db19020ed8ecee5e91b1a0ea4c924ae2988
Do you know if there's an existing part of the code that works like what you described?

Thanks for the nice write-up! After reading through this, I'm wondering if we can simplify this by not introducing a new class but just cleaning up ChromeControllerClient to do what we want.

First of all, it looks like ChromeControllerClient is actually only used for SSL errors, so I think it is actually the SSLErrorControllerClient that I wanted us to make. So we can probably just rename it to SSLErrorControllerClient and move it into chrome/browser/ssl rather than introduce a new class.

Then we can give its constructor a `bool has_committed` argument that the blocking pages pass in when they create their controller clients, depending on whether the committed interstitials code path is in use.

Then SSLErrorControllerClient (nee ChromeControllerClient) can override Proceed and GoBack, doing the new behavior when has_committed is true and calling the superclass methods when has_committed is false.

WDYT? Does that solve these issues?

> Do you know if there's an existing part of the code that works like what you described?

Not sure what you mean by this -- do you mean an existing part of the code that works like the new Proceed() that I described?

> WDYT? Does that solve these issues?

That sounds pretty good. I'll see if I can make that work.

> Not sure what you mean by this -- do you mean an existing part of the code that works like the new Proceed() that I described?

Yes, that. :-)
I forgot to mention that I was talking about Proceed(). 😣

Some thoughts on this:

1)
I'm working on updating the functionality of ChromeControllerClient without moving or renaming it yet. That can be done in a separate CL without affecting functionality.

2)
Adding and removing the `is_committed` flags for all the blocking page classes is a bunch of code churn, and requires either:
- SSLErrorNavigationThrottle passing an `is_committed` flag.
- Modifying SSLErrorHandler so that it assumes that a blocking_page_ready_callback means we're using committed interstitials.

How would you feel about just reading the Chrome flag directly in ChromeControllerClient?
(In addition to SSLErrorNavigationThrottle.)

> SSLErrorControllerContentClient will implement the protected virtual method to store the certificate exception (using the GetHostStateDelegate() method of the BrowserContext).

It looks to me like this requires at least one more design decision.

In order to call SSLHostStateDelegate::AllowCert(), we need to to have the host, cert, and cert status of the interstitial. We can get the latter if we have the SSLInfo.

The old interstitial code path does this by holding on the data in an SSLManager callback, using content::SSLErrorHandler [1][2].

After looking into a few ways for ChromeControllerClient/SSLErrorControllerContentClient to get this info, it seems to me that the the simply passing the request URL and SSL info into its constructor is at least as good as any other approach. I'll be trying that, but happy to hear other thoughts – especially if I missed some way that ChromeControllerClient/SSLErrorControllerContentClient has access to the info already.

[1] https://cs.chromium.org/chromium/src/content/browser/ssl/ssl_manager.cc?l=342
[2] https://cs.chromium.org/chromium/src/content/browser/ssl/ssl_manager.cc?type=cs&sq=package:chromium&l=72

Passing it into the constructor SGTM.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e578ebc0b1d2525f52d98b8bf76d92688357300

commit 1e578ebc0b1d2525f52d98b8bf76d92688357300
Author: Lucas Garron <lgarron@chromium.org>
Date: Wed Nov 15 08:48:49 2017

Implement ChromeControllerClient methods for committed interstitials.

Bug:  752376 
Change-Id: I2c1f53a009bef76737e16ed7796217c24f9001b4
Reviewed-on: https://chromium-review.googlesource.com/750237
Commit-Queue: Lucas Garron <lgarron@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516644}
[modify] https://crrev.com/1e578ebc0b1d2525f52d98b8bf76d92688357300/chrome/browser/interstitials/chrome_controller_client.cc
[modify] https://crrev.com/1e578ebc0b1d2525f52d98b8bf76d92688357300/chrome/browser/interstitials/chrome_controller_client.h
[modify] https://crrev.com/1e578ebc0b1d2525f52d98b8bf76d92688357300/chrome/browser/ssl/bad_clock_blocking_page.cc
[modify] https://crrev.com/1e578ebc0b1d2525f52d98b8bf76d92688357300/chrome/browser/ssl/captive_portal_blocking_page.cc
[modify] https://crrev.com/1e578ebc0b1d2525f52d98b8bf76d92688357300/chrome/browser/ssl/mitm_software_blocking_page.cc
[modify] https://crrev.com/1e578ebc0b1d2525f52d98b8bf76d92688357300/chrome/browser/ssl/ssl_blocking_page.cc
[modify] https://crrev.com/1e578ebc0b1d2525f52d98b8bf76d92688357300/chrome/browser/ssl/ssl_browser_tests.cc