Wrap SSL error handler configuration with a lazy instance

Wrap SSL error handler configuration with a lazy instance

This is in preparation for the SSL error assistant which will
initalize a list of captive portal certs on first use.

BUG=640835
Review-Url: https://codereview.chromium.org/2610183004
Cr-Commit-Position: refs/heads/master@{#442439}
Committed: https://chromium.googlesource.com/chromium/src/+/352dc2ea1f8f043345e4eea1b298ff7292bb882c

[meacer, 2017-01-05 23:06:43]

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

[commit-bot: I haz the power, 2017-01-05 23:07:37]

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

[meacer, 2017-01-05 23:55:52]

Description was changed from

==========
Wrap SSL error handler configuration with a lazy instance

BUG=640835
==========

to

==========
Wrap SSL error handler configuration with a lazy instance

This is in preparation for the SSL error assistant which will
initalize a list of captive portal certs on first use.

BUG=640835
==========

[meacer, 2017-01-05 23:56:05]

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

[meacer, 2017-01-05 23:56:06]

estark: PTAL?

[meacer, 2017-01-06 00:05:45]

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

[commit-bot: I haz the power, 2017-01-06 00:06:31]

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

[commit-bot: I haz the power, 2017-01-06 01:03:42]

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

[commit-bot: I haz the power, 2017-01-06 01:03:42]

Dry run: This issue passed the CQ dry run.

[estark, 2017-01-08 17:01:51]

lgtm w/ tiny nits

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler.cc (left):

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:40: class NetworkTimeTracker;
doh. thanks for cleaning up my mess :)

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler.cc (right):

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:156: void
SetInterstitialDelayForTest(const base::TimeDelta& delay);
tiny nit: I think the suffix is technically supposed to be ForTest*ing*
(here and below)
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:157: void
SetTimerStartedCallback(SSLErrorHandler::TimerStartedCallback* callback);
tiny nit: set_timer_started_callback() for consistency with
timer_started_callback()

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:166:
SSLErrorHandler::TimerStartedCallback* timer_started_callback_ = nullptr;
Not particular to this CL, but it's a little weird to have a base::Callback*
isn't it? I feel like it's usually not a pointer. Dunno if it matters.

[meacer, 2017-01-09 20:47:59]

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

[meacer, 2017-01-09 20:48:25]

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

[meacer, 2017-01-09 20:48:26]

mmenke: Can you PTAL at captive_portal_browsertest.cc? Thanks.

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler.cc (left):

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:40: class NetworkTimeTracker;
On 2017/01/08 17:01:51, estark wrote:
> doh. thanks for cleaning up my mess :)

Np :)

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler.cc (right):

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:156: void
SetInterstitialDelayForTest(const base::TimeDelta& delay);
On 2017/01/08 17:01:51, estark wrote:
> tiny nit: I think the suffix is technically supposed to be ForTest*ing*
> (here and below)
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done, removed ForTest* suffixes from ConfigSingleton, changed them to ForTesting
in SSLErrorHandler.

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:157: void
SetTimerStartedCallback(SSLErrorHandler::TimerStartedCallback* callback);
On 2017/01/08 17:01:51, estark wrote:
> tiny nit: set_timer_started_callback() for consistency with
> timer_started_callback()

Hmm, I feel like I'm missing something. Why would this be named differently than
the other setters? Or did you mean set_timer_started_callback_for_testing? (but
even then, the style would be different)

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:166:
SSLErrorHandler::TimerStartedCallback* timer_started_callback_ = nullptr;
On 2017/01/08 17:01:51, estark wrote:
> Not particular to this CL, but it's a little weird to have a base::Callback*
> isn't it? I feel like it's usually not a pointer. Dunno if it matters.

The original idea was so that it could be left as null as opposed to keeping it
by value or ref (since timer_started_callback was a global variable and we
didn't want complex initializers)

[commit-bot: I haz the power, 2017-01-09 20:49:03]

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

[mmenke, 2017-01-09 21:02:34]

captive_portal_browsertest.cc LGTM.

[commit-bot: I haz the power, 2017-01-09 21:47:44]

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

[commit-bot: I haz the power, 2017-01-09 21:47:45]

Dry run: This issue passed the CQ dry run.

[estark, 2017-01-09 23:13:41]

still lgtm

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler.cc (right):

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:157: void
SetTimerStartedCallback(SSLErrorHandler::TimerStartedCallback* callback);
On 2017/01/09 20:48:26, Mustafa Emre Acer wrote:
> On 2017/01/08 17:01:51, estark wrote:
> > tiny nit: set_timer_started_callback() for consistency with
> > timer_started_callback()
> 
> Hmm, I feel like I'm missing something. Why would this be named differently
than
> the other setters? Or did you mean set_timer_started_callback_for_testing?
(but
> even then, the style would be different)

Oh, I had missed that this setter was for testing only. My reasoning was:
1. my_data_member() and set_my_data_member() are allowed naming conventions for
getters/setters...
2. but if the getter is in that style, then the setter should be too...
3. unless the setter is for testing only, in which case it should have the
ForTesting suffix which means that it has to be SetMyDataMemberForTesting().

So I concluded that this *not* testing-only setter should be
set_my_data_member() while the other testing-only setters should be
SetMyDataMemberForTesting().

I guess they are all testing-only so they should all be SetBlahForTesting?

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:166:
SSLErrorHandler::TimerStartedCallback* timer_started_callback_ = nullptr;
On 2017/01/09 20:48:26, Mustafa Emre Acer wrote:
> On 2017/01/08 17:01:51, estark wrote:
> > Not particular to this CL, but it's a little weird to have a base::Callback*
> > isn't it? I feel like it's usually not a pointer. Dunno if it matters.
> 
> The original idea was so that it could be left as null as opposed to keeping
it
> by value or ref (since timer_started_callback was a global variable and we
> didn't want complex initializers)

Acknowledged.

[meacer, 2017-01-09 23:38:06]

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler.cc (right):

https://codereview.chromium.org/2610183004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:157: void
SetTimerStartedCallback(SSLErrorHandler::TimerStartedCallback* callback);
On 2017/01/09 23:13:41, estark wrote:
> On 2017/01/09 20:48:26, Mustafa Emre Acer wrote:
> > On 2017/01/08 17:01:51, estark wrote:
> > > tiny nit: set_timer_started_callback() for consistency with
> > > timer_started_callback()
> > 
> > Hmm, I feel like I'm missing something. Why would this be named differently
> than
> > the other setters? Or did you mean set_timer_started_callback_for_testing?
> (but
> > even then, the style would be different)
> 
> Oh, I had missed that this setter was for testing only. My reasoning was:
> 1. my_data_member() and set_my_data_member() are allowed naming conventions
for
> getters/setters...
> 2. but if the getter is in that style, then the setter should be too...
> 3. unless the setter is for testing only, in which case it should have the
> ForTesting suffix which means that it has to be SetMyDataMemberForTesting().

I see. I'm by no means an expert on these, but I seem to remember
set_data_member_for_testing() is also fine and that the decision between
SetMyDataMemberForTesting and set_data_member_for_testing is made on whether
it's a simple setter or not, no? (at least, there seem to be examples of it:
https://cs.chromium.org/search/?q=set_%5Ba-zA-Z_%5D%2Bfor_testing+chrome/&sq=...)


> 
> So I concluded that this *not* testing-only setter should be
> set_my_data_member() while the other testing-only setters should be
> SetMyDataMemberForTesting().
> 
> I guess they are all testing-only so they should all be SetBlahForTesting?

For now they are all test setters, but the new setter for the captive portal
list will be used from production code as well (when there is a new component
updater component). But renamed them in the meanwhile.

[meacer, 2017-01-09 23:38:19]

The CQ bit was checked by meacer@chromium.org

[meacer, 2017-01-09 23:38:20]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2610183004/#ps60001
(title: "Add ForTesting to config setter methods")

[commit-bot: I haz the power, 2017-01-09 23:39:49]

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

[commit-bot: I haz the power, 2017-01-10 01:27:46]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484005099698590,
"parent_rev": "cbd6bc1fdfee8c3948818dc6d5dff1da645e8c1f", "commit_rev":
"352dc2ea1f8f043345e4eea1b298ff7292bb882c"}

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

Description was changed from

==========
Wrap SSL error handler configuration with a lazy instance

This is in preparation for the SSL error assistant which will
initalize a list of captive portal certs on first use.

BUG=640835
==========

to

==========
Wrap SSL error handler configuration with a lazy instance

This is in preparation for the SSL error assistant which will
initalize a list of captive portal certs on first use.

BUG=640835

Review-Url: https://codereview.chromium.org/2610183004
Cr-Commit-Position: refs/heads/master@{#442439}
Committed:
https://chromium.googlesource.com/chromium/src/+/352dc2ea1f8f043345e4eea1b298...
==========

[commit-bot: I haz the power, 2017-01-10 01:28:12]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/352dc2ea1f8f043345e4eea1b298...