Launch date+time settings on BrowserThread::FILE

Launch date+time settings on BrowserThread::FILE

This avoids a crash in debug builds on Linux.

BUG=454893
TEST=Change IsErrorDueToBadClock() to always return true, launch a debug
build, visit a bad SSL site, and click "Update date and time". Observe
no crash.

Committed: https://crrev.com/ecf43a2596fefc1db2e3ae246dbd837e8020ec51
Cr-Commit-Position: refs/heads/master@{#321265}

[estark, 2015-03-18 18:39:10]

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

[estark, 2015-03-18 18:39:11]

Two questions:

1. Are there browser tests or unit tests for this code? I couldn't find any.

2. I put the entire |LaunchDateAndTimeSettings| function on the file thread
instead of just the small part of it that was crashing (|base::PathExists|)
because it did seem more correct to me that it would all happen on the file
thread. Specifically, other uses of |base::LaunchProcess|, such as
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/com...,
seem to happen on the file thread. Any objections to that?

[lgarron, 2015-03-18 20:07:19]

On 2015/03/18 at 18:39:11, estark wrote:
> Two questions:
> 
> 1. Are there browser tests or unit tests for this code? I couldn't find any.
> 
> 2. I put the entire |LaunchDateAndTimeSettings| function on the file thread
instead of just the small part of it that was crashing (|base::PathExists|)
because it did seem more correct to me that it would all happen on the file
thread. Specifically, other uses of |base::LaunchProcess|, such as
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/com...,
seem to happen on the file thread. Any objections to that?

I like it!

1. I touched this code in fall, and didn't really know whether we should be
testing anything. I don't think there's a test for this (SSLBlockingPage doesn't
seem to have its own tests, but there is a file called `ssl_browser_tests.cc`:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...)

2. Considering how much trouble we had with that function, I think keeping it
all in one process is good.

LGTM mod "I trust you're using the correct functions". Much more concise than I
expected. :-)

[felt, 2015-03-19 00:16:07]

On 2015/03/18 20:07:19, lgarron wrote:
> On 2015/03/18 at 18:39:11, estark wrote:
> > Two questions:
> > 
> > 1. Are there browser tests or unit tests for this code? I couldn't find any.

There are some tests under chrome/browser/ssl/ssl_browser_tests.cc. They are in
TERRIBLE shape. I've been planning to either unify the tests or make them more
similar to the tests in safe_browsing_blocking_page_test.cc, as a TODO for next
quarter. The testing coverage here makes me super sad.

> > 
> > 2. I put the entire |LaunchDateAndTimeSettings| function on the file thread
> instead of just the small part of it that was crashing (|base::PathExists|)
> because it did seem more correct to me that it would all happen on the file
> thread. Specifically, other uses of |base::LaunchProcess|, such as
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/com...,
> seem to happen on the file thread. Any objections to that?
> 
> I like it!
> 
> 1. I touched this code in fall, and didn't really know whether we should be
> testing anything. I don't think there's a test for this (SSLBlockingPage
doesn't
> seem to have its own tests, but there is a file called `ssl_browser_tests.cc`:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...)
> 
> 2. Considering how much trouble we had with that function, I think keeping it
> all in one process is good.
> 
> LGTM mod "I trust you're using the correct functions". Much more concise than
I
> expected. :-)

[felt, 2015-03-19 00:17:45]

lgtm

[estark, 2015-03-19 00:32:19]

Thanks felt and lgarron.

On 2015/03/19 00:16:07, felt wrote:
> On 2015/03/18 20:07:19, lgarron wrote:
> > On 2015/03/18 at 18:39:11, estark wrote:
> > > Two questions:
> > > 
> > > 1. Are there browser tests or unit tests for this code? I couldn't find
any.
> 
> There are some tests under chrome/browser/ssl/ssl_browser_tests.cc. They are
in
> TERRIBLE shape. I've been planning to either unify the tests or make them more
> similar to the tests in safe_browsing_blocking_page_test.cc, as a TODO for
next
> quarter. The testing coverage here makes me super sad.

Sorry, by "this code" I meant clock interstitials specifically rather than
SSLBlockingPage more generally. Filed a bug for it (crbug.com/468508) and dumped
some state there.

> 
> > > 
> > > 2. I put the entire |LaunchDateAndTimeSettings| function on the file
thread
> > instead of just the small part of it that was crashing (|base::PathExists|)
> > because it did seem more correct to me that it would all happen on the file
> > thread. Specifically, other uses of |base::LaunchProcess|, such as
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/com...,
> > seem to happen on the file thread. Any objections to that?
> > 
> > I like it!
> > 
> > 1. I touched this code in fall, and didn't really know whether we should be
> > testing anything. I don't think there's a test for this (SSLBlockingPage
> doesn't
> > seem to have its own tests, but there is a file called
`ssl_browser_tests.cc`:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...)
> > 
> > 2. Considering how much trouble we had with that function, I think keeping
it
> > all in one process is good.
> > 
> > LGTM mod "I trust you're using the correct functions". Much more concise
than
> I
> > expected. :-)

[estark, 2015-03-19 00:32:46]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2015-03-19 00:33:14]

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

[commit-bot: I haz the power, 2015-03-19 01:49:30]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-03-19 01:50:26]

Patchset 1 (id:??) landed as
https://crrev.com/ecf43a2596fefc1db2e3ae246dbd837e8020ec51
Cr-Commit-Position: refs/heads/master@{#321265}