Cancel client auth requests when not promptable.

Cancel client auth requests when not promptable.

Currently they hang (holding the cache lock) or crash. This plumbs
through a dedicated delegate interface. If the delegate is destroyed with
no notification, the request is aborted. This is distinct from
affirmatively continuing with no certificat (what you get from pressing
cancel). This is extremely bizarre UI, but this CL does not attempt to
address the existing UI being odd.

This fixes the following:
- Closing a tab with a client auth prompt acts as if you affirmatively selected
  to continue without a cert.
- A SharedWorker requesting client auth hangs the request.
- Hitting client auth in an extension background page crashes.

BUG=417092, 410967
Committed: https://crrev.com/3b8455ae75609ee9e91e3af240882785d9942359
Cr-Commit-Position: refs/heads/master@{#320117}

[davidben, 2015-02-06 20:45:07]

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

[davidben, 2015-02-06 20:45:08]

I'm still hoping to add a test for the extension case, but let's get the ball
rolling. Have fun. :-)

[davidben, 2015-02-06 22:39:38]

And there's an extension test now. I think this is ready for review.

https://codereview.chromium.org/859213006/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/xhr_apitest.cc (right):

https://codereview.chromium.org/859213006/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/xhr_apitest.cc:20:
ASSERT_TRUE(RunExtensionTestWithArg("xhr", url.c_str())) << message_;
It's a bit of a nuisance that this test (and the one below) doesn't actually
assert that the extension actually hit the server. Any ideas on how best to
intercept the net stack here while still forwarding to the real request? I just
verified that commenting out the fix still crashed.

[mmenke, 2015-02-10 17:19:11]

Haven't looked through the whole thing, just the most exciting bits, for some
value of "exciting".

https://codereview.chromium.org/859213006/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/xhr_apitest.cc (right):

https://codereview.chromium.org/859213006/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/xhr_apitest.cc:20:
ASSERT_TRUE(RunExtensionTestWithArg("xhr", url.c_str())) << message_;
On 2015/02/06 22:39:38, David Benjamin wrote:
> It's a bit of a nuisance that this test (and the one below) doesn't actually
> assert that the extension actually hit the server. Any ideas on how best to
> intercept the net stack here while still forwarding to the real request? I
just
> verified that commenting out the fix still crashed.

Can we check if the displayed page has ERR_SSL_CLIENT_AUTH_CERT_NEEDED
displayed?  Not lovely, but a number of tests do that.

There may be some way to hook the URLRequest to get its final status, I'm not
sure, though (A ResourceThrottle?  Not sure they get the final status code
unless they do something in their destructor...Which is hideous.  Watching the
ErrorCodesForMainFrame3 histogram also seems really ugly).  A
WebContents::Observer?  They get network error codes, don't they?

https://codereview.chromium.org/859213006/diff/240001/content/browser/loader/...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/859213006/diff/240001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:327:
TestWebContents::Create(browser_context_.get(), site_instance.get()));
I'm confused.  Why are these needed?  Aren't we just using the default
ContentBrowserClient, which will cancel any cert requests it sees, anyways?

https://codereview.chromium.org/859213006/diff/240001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:342: rfh->GetRoutingID(),
Think you messed up the order here

https://codereview.chromium.org/859213006/diff/240001/content/browser/shared_...
File content/browser/shared_worker/worker_browsertest.cc (right):

https://codereview.chromium.org/859213006/diff/240001/content/browser/shared_...
content/browser/shared_worker/worker_browsertest.cc:137: // accordingly.
In this CL, you're actually doing that moving, no?  I have another comment
asking you about it.

https://codereview.chromium.org/859213006/diff/240001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_client_auth_handler.cc (right):

https://codereview.chromium.org/859213006/diff/240001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:25:
ClientCertificateDelegateImpl(
explicit

https://codereview.chromium.org/859213006/diff/240001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:63: if (!web_contents) {
Is there a reason for moving this logic here?  Just wondering if there's a
reason an embedder would want to do anything different in this case.

https://codereview.chromium.org/859213006/diff/240001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:149:
delegate->ContinueWithCertificate(cert);
nit:  Gere, and below, can we just merge these two lines?

https://codereview.chromium.org/859213006/diff/240001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:175:
scoped_refptr<net::X509Certificate>()));
optional:  Worth creating the ClientCertificateDelegateImpl here, and passing it
to the UI thread if needed?  Means the callbacks only go through one location. 
Do make the delegate's threaded-ness kinda weird, though.

[davidben, 2015-02-10 20:28:50]

https://codereview.chromium.org/859213006/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/xhr_apitest.cc (right):

https://codereview.chromium.org/859213006/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/xhr_apitest.cc:20:
ASSERT_TRUE(RunExtensionTestWithArg("xhr", url.c_str())) << message_;
On 2015/02/10 17:19:10, mmenke wrote:
> On 2015/02/06 22:39:38, David Benjamin wrote:
> > It's a bit of a nuisance that this test (and the one below) doesn't actually
> > assert that the extension actually hit the server. Any ideas on how best to
> > intercept the net stack here while still forwarding to the real request? I
> just
> > verified that commenting out the fix still crashed.
> 
> Can we check if the displayed page has ERR_SSL_CLIENT_AUTH_CERT_NEEDED
> displayed?  Not lovely, but a number of tests do that.

It's an XHR made by the background page of an extension, so there's not much to
be displayed. :-) Also the extension itself doesn't get the error code because
of same-origin.

> There may be some way to hook the URLRequest to get its final status, I'm not
> sure, though (A ResourceThrottle?  Not sure they get the final status code
> unless they do something in their destructor...Which is hideous.  Watching the
> ErrorCodesForMainFrame3 histogram also seems really ugly).  A
> WebContents::Observer?  They get network error codes, don't they?

I don't think WCO exposes any useful hooks here. This isn't a navigation but a
network request from a top-level. I'm not sure there's a terribly easy way to
inject a ResourceThrottle either. It'd need to be something that you can attach
extra bits after initialization, rather than pass in a dependency like something
reasonable, since this a browser test and the normal browser assembly has
already started.

https://codereview.chromium.org/859213006/diff/240001/content/browser/loader/...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/859213006/diff/240001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:327:
TestWebContents::Create(browser_context_.get(), site_instance.get()));
On 2015/02/10 17:19:10, mmenke wrote:
> I'm confused.  Why are these needed?  Aren't we just using the default
> ContentBrowserClient, which will cancel any cert requests it sees, anyways?

There's a handful of tests that install a custom one, so I need there to be an
actual WebContents on it. See SelectCertificateBrowserClient up top.

https://codereview.chromium.org/859213006/diff/240001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:342: rfh->GetRoutingID(),
On 2015/02/10 17:19:10, mmenke wrote:
> Think you messed up the order here

I'm passing in RFH ID, and not RVH ID. Getting the RVH ID for completeness is
probably a good plan though...

https://codereview.chromium.org/859213006/diff/240001/content/browser/shared_...
File content/browser/shared_worker/worker_browsertest.cc (right):

https://codereview.chromium.org/859213006/diff/240001/content/browser/shared_...
content/browser/shared_worker/worker_browsertest.cc:137: // accordingly.
On 2015/02/10 17:19:10, mmenke wrote:
> In this CL, you're actually doing that moving, no?  I have another comment
> asking you about it.

This CL's about client auth, but this is HTTP auth. These tests still pass on
accident for HTTP auth. Revised the comment to make it clearer.

https://codereview.chromium.org/859213006/diff/240001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_client_auth_handler.cc (right):

https://codereview.chromium.org/859213006/diff/240001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:25:
ClientCertificateDelegateImpl(
On 2015/02/10 17:19:11, mmenke wrote:
> explicit

Done.

https://codereview.chromium.org/859213006/diff/240001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:63: if (!web_contents) {
On 2015/02/10 17:19:10, mmenke wrote:
> Is there a reason for moving this logic here?  Just wondering if there's a
> reason an embedder would want to do anything different in this case.

Partly so this can be tested within content. Partly so the API surface can be
WebContents, which is cleaner (and gets us closer to moving it to
WebContentsDelegate). Partly because I don't think there's any reason an
embedder would want to change it, yeah. I feel how a SharedWorker interacts with
network prompts like this (which WebContents we map it to, etc) is a web
platform decision, and so it's part of content.

https://codereview.chromium.org/859213006/diff/240001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:149:
delegate->ContinueWithCertificate(cert);
On 2015/02/10 17:19:10, mmenke wrote:
> nit:  Gere, and below, can we just merge these two lines?

Done.

https://codereview.chromium.org/859213006/diff/240001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:175:
scoped_refptr<net::X509Certificate>()));
On 2015/02/10 17:19:10, mmenke wrote:
> optional:  Worth creating the ClientCertificateDelegateImpl here, and passing
it
> to the UI thread if needed?  Means the callbacks only go through one location.

> Do make the delegate's threaded-ness kinda weird, though.

So, I do think the WebContents check should come before the empty
ClientCertStore check rather than after---I think ClientCertStore should be
something only the embedder knows about to begin with. I'm a little worried
about this breaking sites that request no client auth and expect things to
silently proceed, so I've added a TODO and will do it as a follow-up.

[mmenke, 2015-02-10 20:39:20]

https://codereview.chromium.org/859213006/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/xhr_apitest.cc (right):

https://codereview.chromium.org/859213006/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/xhr_apitest.cc:20:
ASSERT_TRUE(RunExtensionTestWithArg("xhr", url.c_str())) << message_;
On 2015/02/10 20:28:49, David Benjamin wrote:
> On 2015/02/10 17:19:10, mmenke wrote:
> > On 2015/02/06 22:39:38, David Benjamin wrote:
> > > It's a bit of a nuisance that this test (and the one below) doesn't
actually
> > > assert that the extension actually hit the server. Any ideas on how best
to
> > > intercept the net stack here while still forwarding to the real request? I
> > just
> > > verified that commenting out the fix still crashed.
> > 
> > Can we check if the displayed page has ERR_SSL_CLIENT_AUTH_CERT_NEEDED
> > displayed?  Not lovely, but a number of tests do that.
> 
> It's an XHR made by the background page of an extension, so there's not much
to
> be displayed. :-) Also the extension itself doesn't get the error code because
> of same-origin.

Oh...Erm...right, was forgetting this wasn't a navigation.

> > There may be some way to hook the URLRequest to get its final status, I'm
not
> > sure, though (A ResourceThrottle?  Not sure they get the final status code
> > unless they do something in their destructor...Which is hideous.  Watching
the
> > ErrorCodesForMainFrame3 histogram also seems really ugly).  A
> > WebContents::Observer?  They get network error codes, don't they?
> 
> I don't think WCO exposes any useful hooks here. This isn't a navigation but a
> network request from a top-level. I'm not sure there's a terribly easy way to
> inject a ResourceThrottle either. It'd need to be something that you can
attach
> extra bits after initialization, rather than pass in a dependency like
something
> reasonable, since this a browser test and the normal browser assembly has
> already started.

The CrossSite tests inject ResourceThrottles by using an RDHDelegate.  That same
hook can also be used to hook up anything else you want to a URLRequest
(UserData that does something during destruction?  You could just check that the
URLRequest is created for the right URL and then destroyed by using a UserData
object, for instance).  Anyhow, not sure it's worth it.

https://codereview.chromium.org/859213006/diff/240001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_client_auth_handler.cc (right):

https://codereview.chromium.org/859213006/diff/240001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:175:
scoped_refptr<net::X509Certificate>()));
On 2015/02/10 20:28:49, David Benjamin wrote:
> On 2015/02/10 17:19:10, mmenke wrote:
> > optional:  Worth creating the ClientCertificateDelegateImpl here, and
passing
> it
> > to the UI thread if needed?  Means the callbacks only go through one
location.
> 
> > Do make the delegate's threaded-ness kinda weird, though.
> 
> So, I do think the WebContents check should come before the empty
> ClientCertStore check rather than after---I think ClientCertStore should be
> something only the embedder knows about to begin with. I'm a little worried
> about this breaking sites that request no client auth and expect things to
> silently proceed, so I've added a TODO and will do it as a follow-up.

I'm not sure you understood me.  I was suggesting creating the
ClientCertificateDelegateImpl in DidGetClientCerts, and using it instead of
directly calling PostTask.

That having been said, I agree that the order of the checks (And Android's
behavior in general here) seems really odd.

[davidben, 2015-02-10 22:20:52]

https://codereview.chromium.org/859213006/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/xhr_apitest.cc (right):

https://codereview.chromium.org/859213006/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/xhr_apitest.cc:20:
ASSERT_TRUE(RunExtensionTestWithArg("xhr", url.c_str())) << message_;
On 2015/02/10 20:39:20, mmenke wrote:
> On 2015/02/10 20:28:49, David Benjamin wrote:
> > On 2015/02/10 17:19:10, mmenke wrote:
> > > On 2015/02/06 22:39:38, David Benjamin wrote:
> > > > It's a bit of a nuisance that this test (and the one below) doesn't
> actually
> > > > assert that the extension actually hit the server. Any ideas on how best
> to
> > > > intercept the net stack here while still forwarding to the real request?
I
> > > just
> > > > verified that commenting out the fix still crashed.
> > > 
> > > Can we check if the displayed page has ERR_SSL_CLIENT_AUTH_CERT_NEEDED
> > > displayed?  Not lovely, but a number of tests do that.
> > 
> > It's an XHR made by the background page of an extension, so there's not much
> to
> > be displayed. :-) Also the extension itself doesn't get the error code
because
> > of same-origin.
> 
> Oh...Erm...right, was forgetting this wasn't a navigation.
> 
> > > There may be some way to hook the URLRequest to get its final status, I'm
> not
> > > sure, though (A ResourceThrottle?  Not sure they get the final status code
> > > unless they do something in their destructor...Which is hideous.  Watching
> the
> > > ErrorCodesForMainFrame3 histogram also seems really ugly).  A
> > > WebContents::Observer?  They get network error codes, don't they?
> > 
> > I don't think WCO exposes any useful hooks here. This isn't a navigation but
a
> > network request from a top-level. I'm not sure there's a terribly easy way
to
> > inject a ResourceThrottle either. It'd need to be something that you can
> attach
> > extra bits after initialization, rather than pass in a dependency like
> something
> > reasonable, since this a browser test and the normal browser assembly has
> > already started.
> 
> The CrossSite tests inject ResourceThrottles by using an RDHDelegate.  That
same
> hook can also be used to hook up anything else you want to a URLRequest
> (UserData that does something during destruction?  You could just check that
the
> URLRequest is created for the right URL and then destroyed by using a UserData
> object, for instance).  Anyhow, not sure it's worth it.

Mmm. I'm a little worried about these tests swapping out random delegates at the
level of RDHDelegate and stuff. I don't know anything about extensions and
they'll probably get upset if, say, their code in ChromeNetworkDelegate got
pulled out.

https://codereview.chromium.org/859213006/diff/240001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_client_auth_handler.cc (right):

https://codereview.chromium.org/859213006/diff/240001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:175:
scoped_refptr<net::X509Certificate>()));
On 2015/02/10 20:39:20, mmenke wrote:
> On 2015/02/10 20:28:49, David Benjamin wrote:
> > On 2015/02/10 17:19:10, mmenke wrote:
> > > optional:  Worth creating the ClientCertificateDelegateImpl here, and
> passing
> > it
> > > to the UI thread if needed?  Means the callbacks only go through one
> location.
> > 
> > > Do make the delegate's threaded-ness kinda weird, though.
> > 
> > So, I do think the WebContents check should come before the empty
> > ClientCertStore check rather than after---I think ClientCertStore should be
> > something only the embedder knows about to begin with. I'm a little worried
> > about this breaking sites that request no client auth and expect things to
> > silently proceed, so I've added a TODO and will do it as a follow-up.
> 
> I'm not sure you understood me.  I was suggesting creating the
> ClientCertificateDelegateImpl in DidGetClientCerts, and using it instead of
> directly calling PostTask.

Ah. I'm not sure it's that much clearer; I think I'd prefer that object not live
on two threads.

> That having been said, I agree that the order of the checks (And Android's
> behavior in general here) seems really odd.

[mmenke, 2015-02-11 17:09:16]

I'd like two more tests, and then I'm happy (Though plan to do another skim over
the code).

https://codereview.chromium.org/859213006/diff/240001/content/browser/loader/...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/859213006/diff/240001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:327:
TestWebContents::Create(browser_context_.get(), site_instance.get()));
On 2015/02/10 20:28:49, David Benjamin wrote:
> On 2015/02/10 17:19:10, mmenke wrote:
> > I'm confused.  Why are these needed?  Aren't we just using the default
> > ContentBrowserClient, which will cancel any cert requests it sees, anyways?
> 
> There's a handful of tests that install a custom one, so I need there to be an
> actual WebContents on it. See SelectCertificateBrowserClient up top.

Ah, somehow I was missing SelectCertificateBrowserClient.  Should we have a
tests in this file where SelectClientCertificate cancels the request, and a
request where there's no WebContents?  Yes, we're adding browser tests for those
cases, but it seems like since they're both loader behavior (And the code is all
in content/), we should have tests for both cases here, too.  Also always nice
to have unit tests, as they're easier to debug.

[davidben, 2015-02-12 22:55:15]

https://codereview.chromium.org/859213006/diff/240001/content/browser/loader/...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/859213006/diff/240001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:327:
TestWebContents::Create(browser_context_.get(), site_instance.get()));
On 2015/02/11 17:09:16, mmenke wrote:
> On 2015/02/10 20:28:49, David Benjamin wrote:
> > On 2015/02/10 17:19:10, mmenke wrote:
> > > I'm confused.  Why are these needed?  Aren't we just using the default
> > > ContentBrowserClient, which will cancel any cert requests it sees,
anyways?
> > 
> > There's a handful of tests that install a custom one, so I need there to be
an
> > actual WebContents on it. See SelectCertificateBrowserClient up top.
> 
> Ah, somehow I was missing SelectCertificateBrowserClient.  Should we have a
> tests in this file where SelectClientCertificate cancels the request, and a
> request where there's no WebContents?  Yes, we're adding browser tests for
those
> cases, but it seems like since they're both loader behavior (And the code is
all
> in content/), we should have tests for both cases here, too.  Also always nice
> to have unit tests, as they're easier to debug.

Done.

[mmenke, 2015-02-13 19:35:53]

I've reviewed the new tests, the loader/ changes, the ssl_client_auth_handler
changes, and skimmed over some of the rest...

I don't suppose there's someone who's actually familiar with how the UI thread
stuff works who can review all the SSL.*observer changes?

https://codereview.chromium.org/859213006/diff/280001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_client_auth_observer.cc (right):

https://codereview.chromium.org/859213006/diff/280001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_client_auth_observer.cc:59: // Stop listening right away
so we don't get our own notification.
I don't think there is a notification here.  Should there be?

https://codereview.chromium.org/859213006/diff/280001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_client_auth_observer.h (right):

https://codereview.chromium.org/859213006/diff/280001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_client_auth_observer.h:24: class SSLClientAuthObserver :
public content::NotificationObserver {
While you're here, mind documenting this class?  The fact that it's not a
general observer of SSLClientAuthNotifications, but rather perfoms specific
actions in response to them, is not at all obvious.

https://codereview.chromium.org/859213006/diff/280001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/xhr/background.js (right):

https://codereview.chromium.org/859213006/diff/280001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/xhr/background.js:10:
chrome.test.notifyPass();
The tests that use this both expect a failure, right?  Can we make this xhrfail
or something, and assert on that?

https://codereview.chromium.org/859213006/diff/280001/content/browser/loader/...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/859213006/diff/280001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:90: class
CancelingClientCertStore : public net::ClientCertStore {
"CancelingClientCertStore" seems a bit confusing...It doesn't cancel the cert
lookup, or call loader->CancelRequest().  Instead it deletes the loader.  Maybe
"LoaderDestroyingCertStore"?

https://codereview.chromium.org/859213006/diff/280001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:128: 
// net::URLRequestTestJob:

https://codereview.chromium.org/859213006/diff/280001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:420:
base::RunLoop().RunUntilIdle();
Think this is worth a comment

https://codereview.chromium.org/859213006/diff/280001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:583: new
CancelingClientCertStore(&loader_));
Can we just keep a raw pointer to the loader ourselves, and pass ownership here?

[davidben, 2015-02-13 22:04:11]

https://codereview.chromium.org/859213006/diff/280001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_client_auth_observer.cc (right):

https://codereview.chromium.org/859213006/diff/280001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_client_auth_observer.cc:59: // Stop listening right away
so we don't get our own notification.
On 2015/02/13 19:35:53, mmenke wrote:
> I don't think there is a notification here.  Should there be?

There shouldn't be. SSLClientAuthObserver is a (somewhat wonky) way to close all
dialogs when one is resolved. I've revised the comment. Stopping observing is I
think still reasonable becaues the delegate has now been called.

https://codereview.chromium.org/859213006/diff/280001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_client_auth_observer.h (right):

https://codereview.chromium.org/859213006/diff/280001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_client_auth_observer.h:24: class SSLClientAuthObserver :
public content::NotificationObserver {
On 2015/02/13 19:35:53, mmenke wrote:
> While you're here, mind documenting this class?  The fact that it's not a
> general observer of SSLClientAuthNotifications, but rather perfoms specific
> actions in response to them, is not at all obvious.

Done.

https://codereview.chromium.org/859213006/diff/280001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/xhr/background.js (right):

https://codereview.chromium.org/859213006/diff/280001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/xhr/background.js:10:
chrome.test.notifyPass();
On 2015/02/13 19:35:53, mmenke wrote:
> The tests that use this both expect a failure, right?  Can we make this
xhrfail
> or something, and assert on that?

Done.

https://codereview.chromium.org/859213006/diff/280001/content/browser/loader/...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/859213006/diff/280001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:90: class
CancelingClientCertStore : public net::ClientCertStore {
On 2015/02/13 19:35:53, mmenke wrote:
> "CancelingClientCertStore" seems a bit confusing...It doesn't cancel the cert
> lookup, or call loader->CancelRequest().  Instead it deletes the loader. 
Maybe
> "LoaderDestroyingCertStore"?

Done.

https://codereview.chromium.org/859213006/diff/280001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:128: 
On 2015/02/13 19:35:53, mmenke wrote:
> // net::URLRequestTestJob:

Done.

https://codereview.chromium.org/859213006/diff/280001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:420:
base::RunLoop().RunUntilIdle();
On 2015/02/13 19:35:53, mmenke wrote:
> Think this is worth a comment

Added /a/ comment. I'm not actually sure what the cleanup tasks in question are
though. It otherwise crashes and I saw that some other RVHTestEnabler uses did
that.

https://codereview.chromium.org/859213006/diff/280001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:583: new
CancelingClientCertStore(&loader_));
On 2015/02/13 19:35:53, mmenke wrote:
> Can we just keep a raw pointer to the loader ourselves, and pass ownership
here?

Hrm. We could, although I dunno if I'd be able to assert on the loader having
been destroyed. If, for some reason, RunUntilIdle stops working, we'll be able
to notice by that EXPECT_FALSE failing.

[davidben, 2015-02-13 22:05:57]

davidben@chromium.org changed reviewers:
+ mattm@chromium.org, pneubeck@chromium.org

[davidben, 2015-02-13 22:05:59]

> I don't suppose there's someone who's actually familiar with how the UI thread
> stuff works who can review all the SSL.*observer changes?

+mattm who hopefully is more familiar with the bits in //chrome.

Also +pneubeck who previously had opinions on self-deleting in
SSLClientAuthHandler. I did the static thing, so hopefully this is less
objectionable? :-P

[mmenke, 2015-02-13 22:17:33]

On 2015/02/13 22:05:59, David Benjamin wrote:
> > I don't suppose there's someone who's actually familiar with how the UI
thread
> > stuff works who can review all the SSL.*observer changes?
> 
> +mattm who hopefully is more familiar with the bits in //chrome.
> 
> Also +pneubeck who previously had opinions on self-deleting in
> SSLClientAuthHandler. I did the static thing, so hopefully this is less
> objectionable? :-P

I'm happy with net/, loader/, content/browser/ssl/,
chrome/browser/chrome_content_browser_client, chrome/browser/extensions/,
chrome/test/data/extensions/api_test/.  Want to take another look at the setup
of the worker_browsertests again...Only things I haven't reviewed are the other
ssl/ directories, I believe, but want to double-check that before officially
signing off.

[mattm, 2015-02-14 00:32:58]

https://codereview.chromium.org/859213006/diff/320001/chrome/browser/ui/andro...
File chrome/browser/ui/android/ssl_client_certificate_request.cc (right):

https://codereview.chromium.org/859213006/diff/320001/chrome/browser/ui/andro...
chrome/browser/ui/android/ssl_client_certificate_request.cc:53:
&content::ClientCertificateDelegate::ContinueWithCertificate,
Is there a reason these aren't CancelCertificateSelection?

https://codereview.chromium.org/859213006/diff/320001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm (right):

https://codereview.chromium.org/859213006/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:207:
observer_->CancelCertificateSelection();
Does sheetDidEnd not have a unique returnCode for when it is closed rather than
canceled?

[pneubeck (no reviews), 2015-02-14 10:53:02]

I didn't review a lot. Just a few comments while passing by.
SSLClientAuthHandler looks fine.

https://codereview.chromium.org/859213006/diff/320001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_client_auth_handler.cc (right):

https://codereview.chromium.org/859213006/diff/320001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:155: return;
optional nit:

if (handler)
  handler-> ...

would probably more discourage to use handler after the function call.
(here and in ContinueWithCertificate)

https://codereview.chromium.org/859213006/diff/320001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_client_auth_handler.h (right):

https://codereview.chromium.org/859213006/diff/320001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.h:38: };
nit:

private:
  DISALLOW_ASSIGN(...);

to through a compile-error instead of silently doing a no-op in case of:
  Delegate* a = ...;
  Delegate *b = ...;
  *a = *b;

https://codereview.chromium.org/859213006/diff/320001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.h:78: // does not outlive
SSLClientAuthHandler.
This documented lifetime is... surprising? Did you mean '... does outlive
SSLClientAuthHandler'?

https://codereview.chromium.org/859213006/diff/320001/content/public/browser/...
File content/public/browser/client_certificate_delegate.h (right):

https://codereview.chromium.org/859213006/diff/320001/content/public/browser/...
content/public/browser/client_certificate_delegate.h:30: };
nit:

private:
  DISALLOW_ASSIGN(...);

to through a compile-error instead of silently doing a no-op in case of:
  Delegate* a = ...;
  Delegate *b = ...;
  *a = *b;

[mmenke, 2015-02-17 19:50:14]

https://codereview.chromium.org/859213006/diff/280001/content/browser/loader/...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/859213006/diff/280001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:420:
base::RunLoop().RunUntilIdle();
On 2015/02/13 22:04:11, David Benjamin wrote:
> On 2015/02/13 19:35:53, mmenke wrote:
> > Think this is worth a comment
> 
> Added /a/ comment. I'm not actually sure what the cleanup tasks in question
are
> though. It otherwise crashes and I saw that some other RVHTestEnabler uses did
> that.

That's fine.  I don't think we really care what it does.

https://codereview.chromium.org/859213006/diff/280001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:583: new
CancelingClientCertStore(&loader_));
On 2015/02/13 22:04:11, David Benjamin wrote:
> On 2015/02/13 19:35:53, mmenke wrote:
> > Can we just keep a raw pointer to the loader ourselves, and pass ownership
> here?
> 
> Hrm. We could, although I dunno if I'd be able to assert on the loader having
> been destroyed. If, for some reason, RunUntilIdle stops working, we'll be able
> to notice by that EXPECT_FALSE failing.

Ahh...right, good point.

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
File android_webview/native/aw_contents_client_bridge.cc (right):

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
android_webview/native/aw_contents_client_bridge.cc:61: }
BUG:  This should go in the destructor.

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
android_webview/native/aw_contents_client_bridge.cc:249:
ignore_result(guard.Release());
Need to include base/macros.h for ignore_result.  Header isn't using it, since
it doesn't use DISALLOW_COPY_AND_ASSIGN (Erroneously, presumably)

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
android_webview/native/aw_contents_client_bridge.cc:391:
delegate->ContinueWithCertificate(nullptr);
You should probably have someone with an opinion on such things review when we
continue and when we cancel.  I really don't have sufficient context to review
these decisions.

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
File android_webview/native/aw_contents_client_bridge_unittest.cc (right):

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
android_webview/native/aw_contents_client_bridge_unittest.cc:60:
TestClientCertificateDelegate(AwContentsClientBridgeTest* test)
explicit

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
android_webview/native/aw_contents_client_bridge_unittest.cc:74:
DISALLOW_COPY_AND_ASSIGN(TestClientCertificateDelegate);
nit:  Blank line before DISALLOW_...

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
android_webview/native/aw_contents_client_bridge_unittest.cc:97: }
I don't think the saved code is worth the ugliness of having a circular
dependency.  Fine however you choose to unwrangle it (Make this a static method
of TestClientCertificateDelegate that takes the test fixture, make the delegate
take a callback, give the Delegate a RunLoop and then pull the chain from that,
whatever).

https://codereview.chromium.org/859213006/diff/320001/chrome/browser/ui/andro...
File chrome/browser/ui/android/ssl_client_certificate_request.cc (right):

https://codereview.chromium.org/859213006/diff/320001/chrome/browser/ui/andro...
chrome/browser/ui/android/ssl_client_certificate_request.cc:149:
base::Unretained(delegate.get()), scoped_refptr<net::X509Certificate>()));
Hrm...This is more than a little ugly.  We're depending on this being destroyed
before the delegate and calling Run() synchronously when we call return.  We
depend on destruction order a lot, which is fine, but this dependency seems less
obvious than most.  Not a change of how this code works, though, so I guess it's
fine.

https://codereview.chromium.org/859213006/diff/320001/content/browser/shared_...
File content/browser/shared_worker/worker_browsertest.cc (right):

https://codereview.chromium.org/859213006/diff/320001/content/browser/shared_...
content/browser/shared_worker/worker_browsertest.cc:49: int
select_certificate_count_;
DISALLOW_COPY_AND_ASSIGN?  And include the relevant header.

https://codereview.chromium.org/859213006/diff/320001/content/test/data/worke...
File content/test/data/workers/worker_tls_client_auth.html (right):

https://codereview.chromium.org/859213006/diff/320001/content/test/data/worke...
content/test/data/workers/worker_tls_client_auth.html:10: var url =
decodeURIComponent(/url=([^;]*)/.exec(location.search)[1]);
The tests really use ";" and not "&" to separate arguments?

https://codereview.chromium.org/859213006/diff/320001/content/test/data/worke...
content/test/data/workers/worker_tls_client_auth.html:16:
worker.postMessage("tls-client-auth " + url);
Could we have some comments here... "tls-client-auth" doesn't seem to appear in
worker_common.js, but searching for "tls-client-auth" and "worker" doesn't
produce anything, so I have no idea what this does.

https://codereview.chromium.org/859213006/diff/320001/content/test/data/worke...
content/test/data/workers/worker_tls_client_auth.html:21: <div
id=statusPanel></div>
Per HTML style guide, names should be all lower case
(https://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml?showone=C...)

[davidben, 2015-02-18 22:31:45]

Oookay. Hopefully I got all the comments there.

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
File android_webview/native/aw_contents_client_bridge.cc (right):

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
android_webview/native/aw_contents_client_bridge.cc:61: }
On 2015/02/17 19:50:14, mmenke wrote:
> BUG:  This should go in the destructor.

*facepalm* Done.

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
android_webview/native/aw_contents_client_bridge.cc:249:
ignore_result(guard.Release());
On 2015/02/17 19:50:14, mmenke wrote:
> Need to include base/macros.h for ignore_result.  Header isn't using it, since
> it doesn't use DISALLOW_COPY_AND_ASSIGN (Erroneously, presumably)

Done.

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
android_webview/native/aw_contents_client_bridge.cc:391:
delegate->ContinueWithCertificate(nullptr);
On 2015/02/17 19:50:14, mmenke wrote:
> You should probably have someone with an opinion on such things review when we
> continue and when we cancel.  I really don't have sufficient context to review
> these decisions.

Yeah, this is partly why I left it alone for the non-Chrome uses.
(ContinueWithCertificate(nullptr) the no-op option.)

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
File android_webview/native/aw_contents_client_bridge_unittest.cc (right):

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
android_webview/native/aw_contents_client_bridge_unittest.cc:60:
TestClientCertificateDelegate(AwContentsClientBridgeTest* test)
On 2015/02/17 19:50:14, mmenke wrote:
> explicit

Done.

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
android_webview/native/aw_contents_client_bridge_unittest.cc:74:
DISALLOW_COPY_AND_ASSIGN(TestClientCertificateDelegate);
On 2015/02/17 19:50:14, mmenke wrote:
> nit:  Blank line before DISALLOW_...

Done.

https://codereview.chromium.org/859213006/diff/320001/android_webview/native/...
android_webview/native/aw_contents_client_bridge_unittest.cc:97: }
On 2015/02/17 19:50:14, mmenke wrote:
> I don't think the saved code is worth the ugliness of having a circular
> dependency.  Fine however you choose to unwrangle it (Make this a static
method
> of TestClientCertificateDelegate that takes the test fixture, make the
delegate
> take a callback, give the Delegate a RunLoop and then pull the chain from
that,
> whatever).

Switched it to use make_scoped_ptr which I just discovered existed.

https://codereview.chromium.org/859213006/diff/320001/chrome/browser/ui/andro...
File chrome/browser/ui/android/ssl_client_certificate_request.cc (right):

https://codereview.chromium.org/859213006/diff/320001/chrome/browser/ui/andro...
chrome/browser/ui/android/ssl_client_certificate_request.cc:53:
&content::ClientCertificateDelegate::ContinueWithCertificate,
On 2015/02/14 00:32:58, mattm wrote:
> Is there a reason these aren't CancelCertificateSelection?

Mostly I figured I'd leave the ones I wasn't sure about alone so I don't
accidentally change import behavior. I agree CancelCertificateSelection is
correct here and flipped it to that.

https://codereview.chromium.org/859213006/diff/320001/chrome/browser/ui/andro...
chrome/browser/ui/android/ssl_client_certificate_request.cc:149:
base::Unretained(delegate.get()), scoped_refptr<net::X509Certificate>()));
On 2015/02/17 19:50:14, mmenke wrote:
> Hrm...This is more than a little ugly.  We're depending on this being
destroyed
> before the delegate and calling Run() synchronously when we call return.  We
> depend on destruction order a lot, which is fine, but this dependency seems
less
> obvious than most.  Not a change of how this code works, though, so I guess
it's
> fine.

Acknowledged.

https://codereview.chromium.org/859213006/diff/320001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm (right):

https://codereview.chromium.org/859213006/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:207:
observer_->CancelCertificateSelection();
On 2015/02/14 00:32:58, mattm wrote:
> Does sheetDidEnd not have a unique returnCode for when it is closed rather
than
> canceled?

Looks like it gives the same as cancel. Sane dialogs would try those the same,
but this isn't a sane dialog. :-) Possibly we should have an explicit "no
certificate" entry in there. Though that would mean not using the native one.

I've added a comment to clarify.

https://codereview.chromium.org/859213006/diff/320001/content/browser/shared_...
File content/browser/shared_worker/worker_browsertest.cc (right):

https://codereview.chromium.org/859213006/diff/320001/content/browser/shared_...
content/browser/shared_worker/worker_browsertest.cc:49: int
select_certificate_count_;
On 2015/02/17 19:50:14, mmenke wrote:
> DISALLOW_COPY_AND_ASSIGN?  And include the relevant header.

Done.

https://codereview.chromium.org/859213006/diff/320001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_client_auth_handler.cc (right):

https://codereview.chromium.org/859213006/diff/320001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:155: return;
On 2015/02/14 10:53:02, pneubeck wrote:
> optional nit:
> 
> if (handler)
>   handler-> ...
> 
> would probably more discourage to use handler after the function call.
> (here and in ContinueWithCertificate)

Done.

https://codereview.chromium.org/859213006/diff/320001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_client_auth_handler.h (right):

https://codereview.chromium.org/859213006/diff/320001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.h:38: };
On 2015/02/14 10:53:02, pneubeck wrote:
> nit:
> 
> private:
>   DISALLOW_ASSIGN(...);
> 
> to through a compile-error instead of silently doing a no-op in case of:
>   Delegate* a = ...;
>   Delegate *b = ...;
>   *a = *b;

Done.

https://codereview.chromium.org/859213006/diff/320001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.h:78: // does not outlive
SSLClientAuthHandler.
On 2015/02/14 10:53:02, pneubeck wrote:
> This documented lifetime is... surprising? Did you mean '... does outlive
> SSLClientAuthHandler'?

Yeah, typo. Fixed and moved to the ctor since this guarantee is part of public
API.

https://codereview.chromium.org/859213006/diff/320001/content/public/browser/...
File content/public/browser/client_certificate_delegate.h (right):

https://codereview.chromium.org/859213006/diff/320001/content/public/browser/...
content/public/browser/client_certificate_delegate.h:30: };
On 2015/02/14 10:53:02, pneubeck wrote:
> nit:
> 
> private:
>   DISALLOW_ASSIGN(...);
> 
> to through a compile-error instead of silently doing a no-op in case of:
>   Delegate* a = ...;
>   Delegate *b = ...;
>   *a = *b;

Done.

https://codereview.chromium.org/859213006/diff/320001/content/test/data/worke...
File content/test/data/workers/worker_tls_client_auth.html (right):

https://codereview.chromium.org/859213006/diff/320001/content/test/data/worke...
content/test/data/workers/worker_tls_client_auth.html:10: var url =
decodeURIComponent(/url=([^;]*)/.exec(location.search)[1]);
On 2015/02/17 19:50:14, mmenke wrote:
> The tests really use ";" and not "&" to separate arguments?

Switched it to & since that's a bit more standard. (FWIW, I think CGI scripts
normally accept both.)

Someday we'll have this thing, but Chrome doesn't have an implementation yet:
https://url.spec.whatwg.org/#interface-urlsearchparams

Actually, it looks like that thing as spec'd only splits on &, so let's stick
with that.

https://codereview.chromium.org/859213006/diff/320001/content/test/data/worke...
content/test/data/workers/worker_tls_client_auth.html:16:
worker.postMessage("tls-client-auth " + url);
On 2015/02/17 19:50:14, mmenke wrote:
> Could we have some comments here... "tls-client-auth" doesn't seem to appear
in
> worker_common.js, but searching for "tls-client-auth" and "worker" doesn't
> produce anything, so I have no idea what this does.

Hrm? It's in worker_common.js. I added it.

https://codereview.chromium.org/859213006/diff/320001/content/test/data/worke...

https://codereview.chromium.org/859213006/diff/320001/content/test/data/worke...
content/test/data/workers/worker_tls_client_auth.html:21: <div
id=statusPanel></div>
On 2015/02/17 19:50:14, mmenke wrote:
> Per HTML style guide, names should be all lower case
>
(https://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml?showone=C...)

I copied it from a different file. Unfortunately the JS code in that test
assumes it's capital. (Probably copied over from Blink.)

https://code.google.com/p/chromium/codesearch#chromium/src/content/test/data/...

[mmenke, 2015-02-19 21:24:35]

LGTM.

https://codereview.chromium.org/859213006/diff/320001/content/test/data/worke...
File content/test/data/workers/worker_tls_client_auth.html (right):

https://codereview.chromium.org/859213006/diff/320001/content/test/data/worke...
content/test/data/workers/worker_tls_client_auth.html:16:
worker.postMessage("tls-client-auth " + url);
On 2015/02/18 22:31:45, David Benjamin wrote:
> On 2015/02/17 19:50:14, mmenke wrote:
> > Could we have some comments here... "tls-client-auth" doesn't seem to appear
> in
> > worker_common.js, but searching for "tls-client-auth" and "worker" doesn't
> > produce anything, so I have no idea what this does.
> 
> Hrm? It's in worker_common.js. I added it.
> 
>
https://codereview.chromium.org/859213006/diff/320001/content/test/data/worke...

Oops...Forgot to remove that comment once I found it.

[mattm, 2015-02-19 22:41:08]

I'm not sure about the android thing still. Seems like there should be some case
that ends with ContinueWithCertificate(nullptr)?

[davidben, 2015-02-19 23:02:26]

New patchsets have been uploaded after l-g-t-m from mmenke@chromium.org

[davidben, 2015-02-19 23:53:06]

On 2015/02/19 22:41:08, mattm wrote:
> I'm not sure about the android thing still. Seems like there should be some
case
> that ends with ContinueWithCertificate(nullptr)?

Oof. Good point. Also touched up WebView behavior.

We so badly need to tests on this stuff. :-(

[mattm, 2015-02-20 02:19:51]

lgtm!

On 2015/02/19 23:53:06, David Benjamin wrote:
> On 2015/02/19 22:41:08, mattm wrote:
> > I'm not sure about the android thing still. Seems like there should be some
> case
> > that ends with ContinueWithCertificate(nullptr)?
> 
> Oof. Good point. Also touched up WebView behavior.
> 
> We so badly need to tests on this stuff. :-(

Yeah :(

[davidben, 2015-02-20 19:31:58]

New patchsets have been uploaded after l-g-t-m from mattm@chromium.org

[davidben, 2015-02-20 19:50:30]

davidben@chromium.org changed reviewers:
+ damienv@chromium.org, sgurun@chromium.org, sky@chromium.org

[davidben, 2015-02-20 19:50:32]

Adding a bunch of OWNERS. Note: I do NOT intend to land this until after branch
point. Although it fixes a crash, the crash isn't new and this is a huge CL.

sgurun: android_webview
sky: chrome/ (I don't believe I have OWNERS coverage on any of it)
damienv: chromecast/

[lcwu1, 2015-02-20 20:11:35]

lcwu@chromium.org changed reviewers:
+ dougsteed@chromium.org, gunsch@chromium.org
- damienv@chromium.org

[sky, 2015-02-20 21:32:30]

I'm not a good owner for the ssl, extensions or cocoa directories. Please get a
more local owner.

https://codereview.chromium.org/859213006/diff/420001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/859213006/diff/420001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1857:
delegate->CancelCertificateSelection();
Since the delegate is going to be deleted in the return why does it need an
explicit cancel?

https://codereview.chromium.org/859213006/diff/420001/chrome/browser/ui/views...
File chrome/browser/ui/views/ssl_client_certificate_selector.cc (right):

https://codereview.chromium.org/859213006/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/ssl_client_certificate_selector.cc:108:
delegate->CancelCertificateSelection();
Same comment about cancel here.

[gunsch, 2015-02-20 21:41:29]

chromecast lgtm

[sgurun-gerrit only, 2015-02-21 01:54:07]

On 2015/02/20 21:41:29, gunsch wrote:
> chromecast lgtm

aw lgtm

[davidben, 2015-02-24 22:38:40]

FYI to prior reviewers: the delegate changed a bit in the latest patch set.

https://codereview.chromium.org/859213006/diff/420001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/859213006/diff/420001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1857:
delegate->CancelCertificateSelection();
On 2015/02/20 21:32:29, sky wrote:
> Since the delegate is going to be deleted in the return why does it need an
> explicit cancel?

Switched the delegate to trigger cancel in the destructor. That's cleaner, yeah.

[davidben, 2015-02-24 22:49:15]

davidben@chromium.org changed reviewers:
+ asargent@chromium.org, rsleevi@chromium.org, yfriedman@chromium.org

[davidben, 2015-02-24 22:49:17]

> I'm not a good owner for the ssl, extensions or cocoa directories. Please get
a
> more local owner.

Sure. Sorry, I didn't notice most of these have local OWNERS.

+asargent for chrome/browser/extensions OWNERS
+rsleevi for chrome/browser/ssl OWNERS
+yfriedman for chrome/browser/ui/android OWNERS
+thakis for chrome/browser/ui/cocoa OWNERS
sky for chrome/browser/ui/views OWNERS and remaining bits of chrome/ scattered
around.

(Yay giant CLs that have to plumb things through. :-/ I'm not sure there's a
sensible way to split it up.)

[sky, 2015-02-25 00:26:53]

On Tue, Feb 24, 2015 at 2:49 PM,  <davidben@chromium.org> wrote:
>> I'm not a good owner for the ssl, extensions or cocoa directories. Please
>> get
>
> a
>>
>> more local owner.
>
>
> Sure. Sorry, I didn't notice most of these have local OWNERS.
>
> +asargent for chrome/browser/extensions OWNERS
> +rsleevi for chrome/browser/ssl OWNERS
> +yfriedman for chrome/browser/ui/android OWNERS
> +thakis for chrome/browser/ui/cocoa OWNERS
> sky for chrome/browser/ui/views OWNERS and remaining bits of chrome/
> scattered
> around.

Sorry, I'm not up for trying to figure out that set. Could you
explicitly list those files.

  -Scott
>
> (Yay giant CLs that have to plumb things through. :-/ I'm not sure there's a
> sensible way to split it up.)
>
> https://codereview.chromium.org/859213006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[davidben, 2015-02-25 00:36:05]

On 2015/02/25 00:26:53, sky wrote:
> Sorry, I'm not up for trying to figure out that set. Could you
> explicitly list those files.

chrome/browser/chrome_content_browser_client.h
chrome/browser/chrome_content_browser_client.cc
chrome/browser/ui/views/ssl_client_certificate_selector.h
chrome/browser/ui/views/ssl_client_certificate_selector.cc
chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc

[Ryan Sleevi, 2015-02-25 06:31:36]

LGTM % nits

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_client_auth_requestor_mock.cc (right):

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_client_auth_requestor_mock.cc:14: class
MockClientCertificateDelegate
Pedantry: This seems more a Fake than a Mock.

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.h (right):

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.h:46: BOOL
certificateSelected_;
I found this a little confusing, and was a cause of concern.

That is, I expected it to only be true if a certificate was positively selected,
but it's also true if the user clicks cancel (e.g. declining to select), because
then we affirmatively return that the user chose _no_ certificate.

It's only false if the sheet ends abnormally.

I'm bad at naming, and suspect you'll have a better expression for this, but
perhaps userResponded or something to indicate it's a user action (vs indirect
through the closing of the tab)?

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm
(right):

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:34:
~TestClientCertificateDelegate() {
newline

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:34:
~TestClientCertificateDelegate() {
override?

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/views...
File chrome/browser/ui/views/ssl_client_certificate_selector.cc (right):

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/views...
chrome/browser/ui/views/ssl_client_certificate_selector.cc:33: DVLOG(1) <<
__FUNCTION__;
Any idea why all this debug spam is here? Can you nuke it in a follow-up CL

https://codereview.chromium.org/859213006/diff/500001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/xhr_fail/background.js (right):

https://codereview.chromium.org/859213006/diff/500001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/xhr_fail/background.js:10:
chrome.test.notifyPass();
If the test server breaks, this test will continue to succeed.

Is there anything we can do to isolate ourselves from this? Perhaps ensuring we
also make a successful connection to a known good testserver in the same test?
I'm not sure...

[Yaron, 2015-02-25 14:54:49]

chrome/browser/ui/android lgtm

[sky, 2015-02-25 15:57:27]

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/views...
File chrome/browser/ui/views/ssl_client_certificate_selector.cc (right):

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/views...
chrome/browser/ui/views/ssl_client_certificate_selector.cc:33: DVLOG(1) <<
__FUNCTION__;
On 2015/02/25 06:31:36, Ryan Sleevi wrote:
> Any idea why all this debug spam is here? Can you nuke it in a follow-up CL

If you don't like this spam, why are we adding more on 82?

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/views...
chrome/browser/ui/views/ssl_client_certificate_selector.cc:83: StopObserving();
Is there a reason we need StopObserving every where and it can't be done only in
the constructor?

[davidben, 2015-02-26 18:09:41]

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_client_auth_requestor_mock.cc (right):

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_client_auth_requestor_mock.cc:14: class
MockClientCertificateDelegate
On 2015/02/25 06:31:36, Ryan Sleevi wrote:
> Pedantry: This seems more a Fake than a Mock.

Done.

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.h (right):

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.h:46: BOOL
certificateSelected_;
On 2015/02/25 06:31:36, Ryan Sleevi wrote:
> I found this a little confusing, and was a cause of concern.
> 
> That is, I expected it to only be true if a certificate was positively
selected,
> but it's also true if the user clicks cancel (e.g. declining to select),
because
> then we affirmatively return that the user chose _no_ certificate.
> 
> It's only false if the sheet ends abnormally.
> 
> I'm bad at naming, and suspect you'll have a better expression for this, but
> perhaps userResponded or something to indicate it's a user action (vs indirect
> through the closing of the tab)?

userResponded_ SGTM. Renamed to that and switched up the code to actually match
that names.

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm
(right):

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:34:
~TestClientCertificateDelegate() {
On 2015/02/25 06:31:36, Ryan Sleevi wrote:
> newline

Done.

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:34:
~TestClientCertificateDelegate() {
On 2015/02/25 06:31:36, Ryan Sleevi wrote:
> override?

Done.

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/views...
File chrome/browser/ui/views/ssl_client_certificate_selector.cc (right):

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/views...
chrome/browser/ui/views/ssl_client_certificate_selector.cc:33: DVLOG(1) <<
__FUNCTION__;
On 2015/02/25 15:57:27, sky wrote:
> On 2015/02/25 06:31:36, Ryan Sleevi wrote:
> > Any idea why all this debug spam is here? Can you nuke it in a follow-up CL
> 
> If you don't like this spam, why are we adding more on 82?

Copy-pasta mostly. I've removed it from the newly added function and will remove
the rest in a follow-up.

https://codereview.chromium.org/859213006/diff/500001/chrome/browser/ui/views...
chrome/browser/ui/views/ssl_client_certificate_selector.cc:83: StopObserving();
On 2015/02/25 15:57:27, sky wrote:
> Is there a reason we need StopObserving every where and it can't be done only
in
> the constructor?

Constructor wouldn't work since this is Stop, not Start. But that confused me
too. The two completion hooks hooks already StopObserving(), so there's really
no need to do it here, except possibly in Accept[*]. I've removed it from the
new method and will remove it from Cancel() in the follow-up as the DVLOG spew.

Also added a comment that Close() sorely needs.[**]

[*] That code also has other issues anyway. https://crbug.com/460215

[**] Yeah, this UI is awful and making the Cancel button take an action is
screwy. We should have an explicit entry for 'Send no certificate' that you hit
OK for maybe. I'm intentionally not trying to resolve that half here and just
trying to get the plumbing needed for the crash and close-tab cases. Having an
explicit "no certificate" option would still need this same plumbing.

https://codereview.chromium.org/859213006/diff/500001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/xhr_fail/background.js (right):

https://codereview.chromium.org/859213006/diff/500001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/xhr_fail/background.js:10:
chrome.test.notifyPass();
On 2015/02/25 06:31:36, Ryan Sleevi wrote:
> If the test server breaks, this test will continue to succeed.
> 
> Is there anything we can do to isolate ourselves from this? Perhaps ensuring
we
> also make a successful connection to a known good testserver in the same test?
> I'm not sure...

I was going to write why this is kind of a pain, and then I had a brilliant
idea! We could have the extension code watch its own request via
chrome.webRequest.

[asargent_no_longer_on_chrome, 2015-03-03 00:10:30]

https://codereview.chromium.org/859213006/diff/560001/chrome/browser/extensio...
File chrome/browser/extensions/background_xhr_apitest.cc (right):

https://codereview.chromium.org/859213006/diff/560001/chrome/browser/extensio...
chrome/browser/extensions/background_xhr_apitest.cc:33:
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, BackgroundXhrTlsClientAuth) {
Usually ExtensionApiTest tests are for testing extension API implementations -
the more common thing to inherit from here would be ExtensionBrowserTest
instead. But I guess that would mean you'd have to roll your own logic to call
ui_test_utils::NavigateToURL to open a tab to the "test_tls_client_auth.html"
page, and wait for a result from the test framework. Hmm.. I guess this is a
sign that the RunExtensionSubTest functionality should perhaps be hoisted up to
ExtensionBrowserTest or something. So feel free to leave as is. I guess just
keep in mind that as you end up using the ExtensionTestMessageListener thing for
passing a parameter from C++ to javascript, you might consider communicating
success/failure using that instead of using the ExtensionApiTest test framework.

https://codereview.chromium.org/859213006/diff/560001/chrome/browser/extensio...
File chrome/browser/extensions/extension_apitest.h (right):

https://codereview.chromium.org/859213006/diff/560001/chrome/browser/extensio...
chrome/browser/extensions/extension_apitest.h:112: const char* custom_arg);
A better to do this instead of adding a new method override to this class is
with an ExtensionTestMessageListener that you set up inside your
BackgroundXhrTlsClientAuth test. (See
extensions/test/extension_test_message_listener.h )


E.g.

ExtensionTestMessageListener listener(true);

...

EXPECT_TRUE(listener.WaitUntilSatisfied());
listener.Reply( < some message here that you would have put in custom_arg>);


From your javascript code, you can get access to this data by doing:


chrome.test.sendMessage("whatever", function(response) {
  // response is whatever string we sent via listener.Reply in C++
  // do something with it
});

[davidben, 2015-03-05 20:01:52]

https://codereview.chromium.org/859213006/diff/560001/chrome/browser/extensio...
File chrome/browser/extensions/background_xhr_apitest.cc (right):

https://codereview.chromium.org/859213006/diff/560001/chrome/browser/extensio...
chrome/browser/extensions/background_xhr_apitest.cc:33:
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, BackgroundXhrTlsClientAuth) {
On 2015/03/03 00:10:30, Antony Sargent wrote:
> Usually ExtensionApiTest tests are for testing extension API implementations -
> the more common thing to inherit from here would be ExtensionBrowserTest
> instead. But I guess that would mean you'd have to roll your own logic to call
> ui_test_utils::NavigateToURL to open a tab to the "test_tls_client_auth.html"
> page, and wait for a result from the test framework. Hmm.. I guess this is a
> sign that the RunExtensionSubTest functionality should perhaps be hoisted up
to
> ExtensionBrowserTest or something. So feel free to leave as is. I guess just
> keep in mind that as you end up using the ExtensionTestMessageListener thing
for
> passing a parameter from C++ to javascript, you might consider communicating
> success/failure using that instead of using the ExtensionApiTest test
framework.
> 

Done.

https://codereview.chromium.org/859213006/diff/560001/chrome/browser/extensio...
File chrome/browser/extensions/extension_apitest.h (right):

https://codereview.chromium.org/859213006/diff/560001/chrome/browser/extensio...
chrome/browser/extensions/extension_apitest.h:112: const char* custom_arg);
On 2015/03/03 00:10:30, Antony Sargent wrote:
> A better to do this instead of adding a new method override to this class is
> with an ExtensionTestMessageListener that you set up inside your
> BackgroundXhrTlsClientAuth test. (See
> extensions/test/extension_test_message_listener.h )
> 
> 
> E.g.
> 
> ExtensionTestMessageListener listener(true);
> 
> ...
> 
> EXPECT_TRUE(listener.WaitUntilSatisfied());
> listener.Reply( < some message here that you would have put in custom_arg>);
> 
> 
> From your javascript code, you can get access to this data by doing:
> 
> 
> chrome.test.sendMessage("whatever", function(response) {
>   // response is whatever string we sent via listener.Reply in C++
>   // do something with it
> });

Done. (I just snuck the custom arg through the URL since that seemed easier.
Saves a round-trip.)

[asargent_no_longer_on_chrome, 2015-03-05 23:56:10]

lgtm

[davidben, 2015-03-06 21:50:42]

Okay. Hopefully that's the last of the churn. Sorry about all the email spam.
I'd missed that the tests were unhappy on Android---turns out Android can't deal
with swapping out ContentBrowserClient. And the test server doesn't let you
spawn more than one of them on Android. Who knew? I'm routing through
ShellContentBrowserClient now, which is kinda nasty, but there's some precedence
with ShellResourceDispatcherHostDelegate I suppose.

I think I'm just missing an OWNERS review from sky now (same set of files).
mmenke, since you were the main reviewer, if you want to give it a second look
over, that might also make sense.

[mmenke, 2015-03-09 15:22:38]

On 2015/03/06 21:50:42, David Benjamin wrote:
> Okay. Hopefully that's the last of the churn. Sorry about all the email spam.
> I'd missed that the tests were unhappy on Android---turns out Android can't
deal
> with swapping out ContentBrowserClient. And the test server doesn't let you
> spawn more than one of them on Android. Who knew? I'm routing through
> ShellContentBrowserClient now, which is kinda nasty, but there's some
precedence
> with ShellResourceDispatcherHostDelegate I suppose.
> 
> I think I'm just missing an OWNERS review from sky now (same set of files).
> mmenke, since you were the main reviewer, if you want to give it a second look
> over, that might also make sense.

Which files should I be looking at?

[davidben, 2015-03-09 15:37:13]

On 2015/03/09 15:22:38, mmenke wrote:
> > I think I'm just missing an OWNERS review from sky now (same set of files).
> > mmenke, since you were the main reviewer, if you want to give it a second
look
> > over, that might also make sense.
> 
> Which files should I be looking at?

Patch set 24 to patch set 25 is when the delegate switched to using the
destructor rather than CancelCertificateSelection. It changed the interface, so
you'll probably want to look at the inter-patch-set diff for all files which
changed as a result. content/browser/ssl/ssl_client_auth_handler.cc,
chrome/browser/chrome_content_browser_client.cc, and
chrome/browser/ssl/ssl_client_auth_observer.cc are the interesting ones.

Patch set 32 to patch set 33 is the reworked SharedWorker test to appease
Android. That's content/browser/shared_worker/worker_browsertest.cc and
content/shell/browser/shell_content_browser_client.{h,cc}.

(chrome/test/data/extensions/background_xhr/ and
chrome/browser/extensions/background_xhr_browsertest.cc have also been reworked
since your review, but asargent has looked at the current iteration. Notably
they now assert that the request was made and have the expected result, which
was something you had commented on.)

[mmenke, 2015-03-09 18:30:27]

LGTM

https://codereview.chromium.org/859213006/diff/680001/content/browser/shared_...
File content/browser/shared_worker/worker_browsertest.cc (right):

https://codereview.chromium.org/859213006/diff/680001/content/browser/shared_...
content/browser/shared_worker/worker_browsertest.cc:42:
base::Callback<void()>());
base::Closure?  Or could just use a WeakPtrFactory.

https://codereview.chromium.org/859213006/diff/680001/content/browser/shared_...
content/browser/shared_worker/worker_browsertest.cc:163: #endif
Should this be disabled via the same mechanism as all the other SharedWorker
tests?  Seems weird to disable this one way, and everything else another.

https://codereview.chromium.org/859213006/diff/680001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_client_auth_handler.cc (right):

https://codereview.chromium.org/859213006/diff/680001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:50: bool called_;
Can we name this something clearer?  continue_called_? 
continued_with_certificate_[that_was_possibly_a_null_cert_with_is_kinda_weird_]?

https://codereview.chromium.org/859213006/diff/680001/content/public/browser/...
File content/public/browser/client_certificate_delegate.h (right):

https://codereview.chromium.org/859213006/diff/680001/content/public/browser/...
content/public/browser/client_certificate_delegate.h:23: // certificate request
is aborted.
optional:  Maybe "is" -> "will be"?

https://codereview.chromium.org/859213006/diff/680001/content/public/browser/...
content/public/browser/client_certificate_delegate.h:23: // certificate request
is aborted.
optional:  Maybe this should be part of the class description?

https://codereview.chromium.org/859213006/diff/680001/content/shell/browser/s...
File content/shell/browser/shell_content_browser_client.h (right):

https://codereview.chromium.org/859213006/diff/680001/content/shell/browser/s...
content/shell/browser/shell_content_browser_client.h:103: base::Callback<void()>
select_client_certificate_callback) {
base::Closure?

https://codereview.chromium.org/859213006/diff/680001/content/shell/browser/s...
content/shell/browser/shell_content_browser_client.h:119: base::Callback<void()>
select_client_certificate_callback_;
base::Closure?

[davidben, 2015-03-10 02:07:58]

https://codereview.chromium.org/859213006/diff/680001/content/browser/shared_...
File content/browser/shared_worker/worker_browsertest.cc (right):

https://codereview.chromium.org/859213006/diff/680001/content/browser/shared_...
content/browser/shared_worker/worker_browsertest.cc:42:
base::Callback<void()>());
On 2015/03/09 18:30:27, mmenke wrote:
> base::Closure?  Or could just use a WeakPtrFactory.

Done. (I think I prefer base::Closure. Setting it to the null callback is the
inverse of the SetUp operation.)

https://codereview.chromium.org/859213006/diff/680001/content/browser/shared_...
content/browser/shared_worker/worker_browsertest.cc:163: #endif
On 2015/03/09 18:30:27, mmenke wrote:
> Should this be disabled via the same mechanism as all the other SharedWorker
> tests?  Seems weird to disable this one way, and everything else another.

So, I really don't like this out-of-band thing. :-) It wasn't clear the tests
were disabled. I just chatted with yusufo@ who said Chrome for Android has no
set rules and inline is probably better. My plan is to switch the remainder to
inline in a follow-up.

I switched it to a SupportsFooBar() check. That's less verbose when applied in
bulk than a bunch of MAYBE lines. Also added a TODO to transplant the others
over.

https://codereview.chromium.org/859213006/diff/680001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_client_auth_handler.cc (right):

https://codereview.chromium.org/859213006/diff/680001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.cc:50: bool called_;
On 2015/03/09 18:30:27, mmenke wrote:
> Can we name this something clearer?  continue_called_? 
>
continued_with_certificate_[that_was_possibly_a_null_cert_with_is_kinda_weird_]?

Done. (Went with continue_called_)

https://codereview.chromium.org/859213006/diff/680001/content/public/browser/...
File content/public/browser/client_certificate_delegate.h (right):

https://codereview.chromium.org/859213006/diff/680001/content/public/browser/...
content/public/browser/client_certificate_delegate.h:23: // certificate request
is aborted.
On 2015/03/09 18:30:27, mmenke wrote:
> optional:  Maybe this should be part of the class description?

Done.

https://codereview.chromium.org/859213006/diff/680001/content/public/browser/...
content/public/browser/client_certificate_delegate.h:23: // certificate request
is aborted.
On 2015/03/09 18:30:27, mmenke wrote:
> optional:  Maybe "is" -> "will be"?

Done.

https://codereview.chromium.org/859213006/diff/680001/content/shell/browser/s...
File content/shell/browser/shell_content_browser_client.h (right):

https://codereview.chromium.org/859213006/diff/680001/content/shell/browser/s...
content/shell/browser/shell_content_browser_client.h:103: base::Callback<void()>
select_client_certificate_callback) {
On 2015/03/09 18:30:27, mmenke wrote:
> base::Closure?

Done.

https://codereview.chromium.org/859213006/diff/680001/content/shell/browser/s...
content/shell/browser/shell_content_browser_client.h:119: base::Callback<void()>
select_client_certificate_callback_;
On 2015/03/09 18:30:27, mmenke wrote:
> base::Closure?

Done.

[mmenke, 2015-03-10 02:10:55]

https://codereview.chromium.org/859213006/diff/680001/content/browser/shared_...
File content/browser/shared_worker/worker_browsertest.cc (right):

https://codereview.chromium.org/859213006/diff/680001/content/browser/shared_...
content/browser/shared_worker/worker_browsertest.cc:163: #endif
On 2015/03/10 02:07:57, David Benjamin wrote:
> On 2015/03/09 18:30:27, mmenke wrote:
> > Should this be disabled via the same mechanism as all the other SharedWorker
> > tests?  Seems weird to disable this one way, and everything else another.
> 
> So, I really don't like this out-of-band thing. :-) It wasn't clear the tests
> were disabled. I just chatted with yusufo@ who said Chrome for Android has no
> set rules and inline is probably better. My plan is to switch the remainder to
> inline in a follow-up.
> 
> I switched it to a SupportsFooBar() check. That's less verbose when applied in
> bulk than a bunch of MAYBE lines. Also added a TODO to transplant the others
> over.

SGTM.

[davidben, 2015-03-11 00:52:03]

davidben@chromium.org changed reviewers:
+ thakis@chromium.org

[davidben, 2015-03-11 00:52:05]

sky: PTAL at
chrome/browser/ui/views/ssl_client_certificate_selector.cc
chrome/browser/ui/views/ssl_client_certificate_selector.h
chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc

thakis: PTAL at (apparently I failed to add you the first time around)
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.h
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm

Thanks! (Ugh, this CL... how I wish we could just remove all client auth
support...)

[Nico, 2015-03-11 13:45:13]

c/b/u/cocoa lgtm

(It's usually a good idea to split up CLs like this into several CLs. Change the
parameter type in one CL, then one CL per actual UI change or something)

[sky, 2015-03-11 15:56:30]

LGTM

[davidben, 2015-03-11 16:46:25]

On 2015/03/11 13:45:13, Nico (traveling) wrote:
> (It's usually a good idea to split up CLs like this into several CLs. Change
the
> parameter type in one CL, then one CL per actual UI change or something)

Yeah, it occurred to me partway through that that would have been a reasonable
split. Though the first CL would still have touched every file. I'll do that
next time.

[davidben, 2015-03-11 16:46:34]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-03-11 16:46:35]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org,
sgurun@chromium.org, mattm@chromium.org, rsleevi@chromium.org,
yfriedman@chromium.org, asargent@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/859213006/#ps700001
(title: "mmenke comments, webview test")

[commit-bot: I haz the power, 2015-03-11 16:46:54]

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

[commit-bot: I haz the power, 2015-03-11 17:08:47]

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

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

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[davidben, 2015-03-11 17:14:25]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-03-11 17:14:25]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
sky@chromium.org, rsleevi@chromium.org, gunsch@chromium.org,
yfriedman@chromium.org, asargent@chromium.org, mattm@chromium.org,
thakis@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/859213006/#ps720001
(title: "worker_common.js was missing a license header (also a rebase)")

[commit-bot: I haz the power, 2015-03-11 17:14:44]

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

[commit-bot: I haz the power, 2015-03-11 19:43:17]

Committed patchset #37 (id:720001)

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

Patchset 37 (id:??) landed as
https://crrev.com/3b8455ae75609ee9e91e3af240882785d9942359
Cr-Commit-Position: refs/heads/master@{#320117}