Allow cert-popup for WebView guests.

Allow cert-popup for WebView guests.

The SSL cert-selection dialog is currently being suppressed
for requests originating within a WebView guest. This
appears to be on account of

1) ShowSSLClientCertificateSelector() not checking to see 
if the current WebContents is actually a guest without a 
WebContentsModalDialogManager, in which case it should 
attempt to use the embedder's WebContentsModalDialogManager,

and possibly also

2) The embedder WebContents in the inline login view will 
not have a WebContentsModalDialogManager since it is 
created via a views::WebView.

This CL remedies this by applying the correct check for the 
existence of an embedder WebContentsModalDialogManager, as 
well as making sure the inline flow provides a 
WebContentsModalDialogManager for the embedder WebContents.

Note: applications that want the ability to show the
certificate selector dialog,but are not hosted in a browser
tab, have the option to provide a 
WebContentsModalDialogManager for the embedder's 
WebContents.

BUG=499996

[wjmaclean, 2015-07-06 20:03:57]

wjmaclean@chromium.org changed reviewers:
+ rsleevi@chromium.org

[wjmaclean, 2015-07-06 20:06:43]

Here's a first draft of a CL to get WebView guests to show the SSL cert selector
dialog.

Did you want to give it a quick sanity check before starting a more formal
review process? I still need to test that this works properly with the inline
flow (bubble signin window may not display the cert selector properly without
some more information about allowed sizing.

Should the cert-selector perhaps be browser-modal? Or at least window-modal?

[Ryan Sleevi, 2015-07-07 12:27:19]

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

[Ryan Sleevi, 2015-07-07 12:27:19]

I don't know if I'm good to offer high or low-level review :)

I'd suggest David Benjamin is a good first sanity check, who also happens to be
an OWNER who can do some of the final LGs.

One response to your question below.

https://codereview.chromium.org/1207313003/diff/1/chrome/browser/ui/views/pro...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/1207313003/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1544: // bubble to be
browser-modal, and not bubble-modal.
I'm not sure what you mean by browser-modal here - but it was intentional that
it was tab-modal as part of the excision of browser-modal dialogs. That's why
the URLRequest surfaces up the renderer IDs, and we coalesce to a single prompt
across multiple tabs (or more aptly, we spawn a tab modal dialog for each tab
that is blocked on this auth flow, and when the user responds in any one tab, it
dismisses the other tabs dialogs)

[wjmaclean, 2015-07-07 12:46:57]

On 2015/07/07 12:27:19, Ryan Sleevi (slow through 7-15 wrote:
> I don't know if I'm good to offer high or low-level review :)
> 
> I'd suggest David Benjamin is a good first sanity check, who also happens to
be
> an OWNER who can do some of the final LGs.

Sure thing ... will add David next.

> One response to your question below.
> 
>
https://codereview.chromium.org/1207313003/diff/1/chrome/browser/ui/views/pro...
> File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):
> 
>
https://codereview.chromium.org/1207313003/diff/1/chrome/browser/ui/views/pro...
> chrome/browser/ui/views/profiles/profile_chooser_view.cc:1544: // bubble to be
> browser-modal, and not bubble-modal.
> I'm not sure what you mean by browser-modal here - but it was intentional that
> it was tab-modal as part of the excision of browser-modal dialogs. That's why
> the URLRequest surfaces up the renderer IDs, and we coalesce to a single
prompt
> across multiple tabs (or more aptly, we spawn a tab modal dialog for each tab
> that is blocked on this auth flow, and when the user responds in any one tab,
it
> dismisses the other tabs dialogs)

Ok, that's good to know. My main concern is whether or not the dialog has to be
constrained to be within the "tab" it represents, since for the inline login
case that's a (large-ish) popup (bubble), which is certainly narrower that the
certi dialog seems to like to be.

[wjmaclean, 2015-07-07 14:42:06]

Patchset #2 (id:20001) has been deleted

[wjmaclean, 2015-07-07 14:45:55]

My first CL got tripped up on the switch to WebContentsModalDialogManager ...
I've uploaded a revised version.

[davidben, 2015-07-07 16:40:22]

> Note: in future applications similar to the inline flow, 
> i.e. not hosted in a browser tab, it is the responsibility 
> of the embedder's owner to provide a 
> WebContentsModalDialogManager for the embedder's 
> WebContents.

Action-at-a-distance behavior like this is very confusing; while you do want
client certificate prompts, some other subsystem may follow suit and key on this
check, but you may not want those other prompts. Nor is it clear what is and
isn't alright to add as a dependency. Consider HTTP auth prompts (maybe we have
a proxy). You may well want those prompts too, but today they require the
WebContents have a password manager stapled to it. (In fact, the only reason we
didn't have an analogous crash that required the client auth check was because
extension background pages happened to not have password managers.) Or maybe
you're a little piece of web content embedded in some UI bubble and don't
actually want to throw up any prompts. Or maybe some prompt is only supposed to
show out of a "top-level" context, but this logic implicitly bumps up to the
parent WebContents in these webview cases.

Ideally, I think these kinds of things should surface through the
WebContentsDelegate and then individual embedders decide how to show UI.


All that said, since this is a regression from my fix, it doesn't make sense to
block it on that refactor (which I hope to work on as part of my project this
coming quarter). But could you tweak that part of the commit message slightly?
:-)

[davidben, 2015-07-07 16:41:23]

https://codereview.chromium.org/1207313003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/certificate_selector.h (right):

https://codereview.chromium.org/1207313003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector.h:38: static bool
CanShow(content::WebContents* web_contents);
Mind adding a TODO, so we remember to get rid of it later? I think ideally
nothing would call these functions except on WebContentses they own and know
satisfy whatever requirements are needed.

// TODO(davidben): Remove this when the certificate selector prompt is moved to
the WebContentsDelegate. https://crbug.com/456255.

https://codereview.chromium.org/1207313003/diff/40001/components/constrained_...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/1207313003/diff/40001/components/constrained_...
components/constrained_window/constrained_window_views.cc:139:
top_level_web_contents);
MSVC doesn't like implicit pointer-to-bool here. You'll need a != nullptr or !!
in front.

[wjmaclean, 2015-07-07 17:17:43]

I've addressed your comments ... let me know if I got the right gist in the
re-wording of that comment.

I also had to add WebContentsModalDialogHost/ManagerDelegate support to
ProfileChooserView to make this not crash.

*But* it looks kind of weird when the popup-bubble shows the certificate
selector on top of itself. Is there a better way to handle this? Is it OK to
move the dialog off the popup? (It would be obscuring some of the UI at that
point, but then so is the bubble.)

I don't see the cert dialog as being optional, though relatively few users will
ever see it.

A better option for the future might be to ask the signin team to detect when
the dialog needs to be shown, and transport the requesting WebContents into a
tab before showing the dialog. But that's another CL I suspect.

https://codereview.chromium.org/1207313003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/certificate_selector.h (right):

https://codereview.chromium.org/1207313003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector.h:38: static bool
CanShow(content::WebContents* web_contents);
On 2015/07/07 16:41:23, David Benjamin wrote:
> Mind adding a TODO, so we remember to get rid of it later? I think ideally
> nothing would call these functions except on WebContentses they own and know
> satisfy whatever requirements are needed.
> 
> // TODO(davidben): Remove this when the certificate selector prompt is moved
to
> the WebContentsDelegate. https://crbug.com/456255.

Done.

https://codereview.chromium.org/1207313003/diff/40001/components/constrained_...
File components/constrained_window/constrained_window_views.cc (right):

https://codereview.chromium.org/1207313003/diff/40001/components/constrained_...
components/constrained_window/constrained_window_views.cc:139:
top_level_web_contents);
On 2015/07/07 16:41:23, David Benjamin wrote:
> MSVC doesn't like implicit pointer-to-bool here. You'll need a != nullptr or
!!
> in front.

Done.

[wjmaclean, 2015-07-07 17:50:37]

On 2015/07/07 17:17:43, wjmaclean wrote:
> 
> *But* it looks kind of weird when the popup-bubble shows the certificate
> selector on top of itself. Is there a better way to handle this? Is it OK to
> move the dialog off the popup? (It would be obscuring some of the UI at that
> point, but then so is the bubble.)

Just to answer my own question here: if we move the selector off the bubble, it
doesn't seem to get input events.

[davidben, 2015-07-07 18:23:34]

On 2015/07/07 17:50:37, wjmaclean wrote:
> On 2015/07/07 17:17:43, wjmaclean wrote:
> > 
> > *But* it looks kind of weird when the popup-bubble shows the certificate
> > selector on top of itself. Is there a better way to handle this? Is it OK to
> > move the dialog off the popup? (It would be obscuring some of the UI at that
> > point, but then so is the bubble.)
> 
> Just to answer my own question here: if we move the selector off the bubble,
it
> doesn't seem to get input events.

I'm not familiar with what this popup pubble is. Could you add a screenshot?

This is, by the way, why I don't think this is the right approach and that it
should come out of a WebContentsDelegate hook. The signin team should be the
ones to determine how to show UI. Either with this prompt, a totally different
one, or whatever else. If we route around like this, we can't avoid weird
behaviors.

Another question. This fix only affects views, right? What about the profile
chooser on Mac?

[wjmaclean, 2015-07-07 18:38:39]

On 2015/07/07 18:23:34, David Benjamin wrote:
> On 2015/07/07 17:50:37, wjmaclean wrote:
> > On 2015/07/07 17:17:43, wjmaclean wrote:
> > > 
> > > *But* it looks kind of weird when the popup-bubble shows the certificate
> > > selector on top of itself. Is there a better way to handle this? Is it OK
to
> > > move the dialog off the popup? (It would be obscuring some of the UI at
that
> > > point, but then so is the bubble.)
> > 
> > Just to answer my own question here: if we move the selector off the bubble,
> it
> > doesn't seem to get input events.
> 
> I'm not familiar with what this popup pubble is. Could you add a screenshot?

It's just the popup signin dialog you get when you click/touch in the top-right
corner of the screen, when not logged in already (you get the "signin to chrome"
button, then a bubble where you can enter your e-mail/password). This "inline"
login page is based on a browser-tag WebView, which is based on GuestView.

> This is, by the way, why I don't think this is the right approach and that it
> should come out of a WebContentsDelegate hook.

I'm not arguing it shouldn't. But when will that be ready? This is meant to be a
fix for enterprise clients who cannot login at present on account of cert
selection issues. :-)

> The signin team should be the
> ones to determine how to show UI. Either with this prompt, a totally different
> one, or whatever else. If we route around like this, we can't avoid weird
> behaviors.

Again, I will be getting the signin team involved before this lands, and they
will get to decide how to show the UI.  At this stage, this CL is all about
understanding what works, and what doesn't.

> 
> Another question. This fix only affects views, right? What about the profile
> chooser on Mac?

Not sure yet, haven't looked at it, though so far as I know it uses the same
ProfileChooserView class, which is views-based..

[wjmaclean, 2015-07-09 15:08:52]

davidben@ - Just to move this work along, would you be supportive of my
splitting off the ssl_client_certificate_selector.cc and landing that
separately, so we can try to address the concerns in
https://code.google.com/p/chromium/issues/detail?id=499996? This would allow a
work-around where we can allow login for enterprise clients (using client certs)
in a tab using chrome://chrome-signin/?source=0, and once that's in place we'd
be in a position to fix the signin-bubble flow with less urgency. This would
also potentially make it easier to move forward with work on
https://code.google.com/p/chromium/issues/detail?id=291417

Does that seem ok?

[davidben, 2015-07-13 19:34:28]

On 2015/07/07 18:38:39, wjmaclean wrote:
> On 2015/07/07 18:23:34, David Benjamin wrote:
> > On 2015/07/07 17:50:37, wjmaclean wrote:
> > > On 2015/07/07 17:17:43, wjmaclean wrote:
> > > > 
> > > > *But* it looks kind of weird when the popup-bubble shows the certificate
> > > > selector on top of itself. Is there a better way to handle this? Is it
OK
> to
> > > > move the dialog off the popup? (It would be obscuring some of the UI at
> that
> > > > point, but then so is the bubble.)
> > > 
> > > Just to answer my own question here: if we move the selector off the
bubble,
> > it
> > > doesn't seem to get input events.
> > 
> > I'm not familiar with what this popup pubble is. Could you add a screenshot?
> 
> It's just the popup signin dialog you get when you click/touch in the
top-right
> corner of the screen, when not logged in already (you get the "signin to
chrome"
> button, then a bubble where you can enter your e-mail/password). This "inline"
> login page is based on a browser-tag WebView, which is based on GuestView.
> 
> > This is, by the way, why I don't think this is the right approach and that
it
> > should come out of a WebContentsDelegate hook.
> 
> I'm not arguing it shouldn't. But when will that be ready? This is meant to be
a
> fix for enterprise clients who cannot login at present on account of cert
> selection issues. :-)
> 
> > The signin team should be the
> > ones to determine how to show UI. Either with this prompt, a totally
different
> > one, or whatever else. If we route around like this, we can't avoid weird
> > behaviors.
> 
> Again, I will be getting the signin team involved before this lands, and they
> will get to decide how to show the UI.  At this stage, this CL is all about
> understanding what works, and what doesn't.

The problem is they can't. That's why I want the WebContentsDelegate approach
long-term. Short-term, sure, something like this is fine. But you have to life
with whatever the UI looks like for the most part. That's I asked for a
screenshot. Could you attach one, so we're on the same page w.r.t. what we're
talking about here?


> > Another question. This fix only affects views, right? What about the profile
> > chooser on Mac?
> 
> Not sure yet, haven't looked at it, though so far as I know it uses the same
> ProfileChooserView class, which is views-based..

Could you please check this? Mac certainly isn't views-based for the dialog. I
also see chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm so that
suggests it's not using it there either.


> davidben@ - Just to move this work along, would you be supportive of my
> splitting off the ssl_client_certificate_selector.cc and landing that
> separately, so we can try to address the concerns in
> https://code.google.com/p/chromium/issues/detail?id=499996? This would allow a
> work-around where we can allow login for enterprise clients (using client
certs)
> in a tab using chrome://chrome-signin/?source=0, and once that's in place we'd
> be in a position to fix the signin-bubble flow with less urgency. This would
> also potentially make it easier to move forward with work on
> https://code.google.com/p/chromium/issues/detail?id=291417

I don't seem to have access to the second bug. Could you CC me?

[wjmaclean, 2015-07-13 20:20:01]

On 2015/07/13 19:34:28, David Benjamin wrote:
> On 2015/07/07 18:38:39, wjmaclean wrote:
> > On 2015/07/07 18:23:34, David Benjamin wrote:
> > > On 2015/07/07 17:50:37, wjmaclean wrote:
> > > > On 2015/07/07 17:17:43, wjmaclean wrote:
> > > > > 
> > > > > *But* it looks kind of weird when the popup-bubble shows the
certificate
> > > > > selector on top of itself. Is there a better way to handle this? Is it
> OK
> > to
> > > > > move the dialog off the popup? (It would be obscuring some of the UI
at
> > that
> > > > > point, but then so is the bubble.)
> > > > 
> > > > Just to answer my own question here: if we move the selector off the
> bubble,
> > > it
> > > > doesn't seem to get input events.
> > > 
> > > I'm not familiar with what this popup pubble is. Could you add a
screenshot?
> > 
> > It's just the popup signin dialog you get when you click/touch in the
> top-right
> > corner of the screen, when not logged in already (you get the "signin to
> chrome"
> > button, then a bubble where you can enter your e-mail/password). This
"inline"
> > login page is based on a browser-tag WebView, which is based on GuestView.
> > 
> > > This is, by the way, why I don't think this is the right approach and that
> it
> > > should come out of a WebContentsDelegate hook.
> > 
> > I'm not arguing it shouldn't. But when will that be ready? This is meant to
be
> a
> > fix for enterprise clients who cannot login at present on account of cert
> > selection issues. :-)
> > 
> > > The signin team should be the
> > > ones to determine how to show UI. Either with this prompt, a totally
> different
> > > one, or whatever else. If we route around like this, we can't avoid weird
> > > behaviors.
> > 
> > Again, I will be getting the signin team involved before this lands, and
they
> > will get to decide how to show the UI.  At this stage, this CL is all about
> > understanding what works, and what doesn't.
> 
> The problem is they can't. That's why I want the WebContentsDelegate approach
> long-term. Short-term, sure, something like this is fine. But you have to life
> with whatever the UI looks like for the most part. That's I asked for a
> screenshot. Could you attach one, so we're on the same page w.r.t. what we're
> talking about here?
> 
> 
> > > Another question. This fix only affects views, right? What about the
profile
> > > chooser on Mac?
> > 
> > Not sure yet, haven't looked at it, though so far as I know it uses the same
> > ProfileChooserView class, which is views-based..
> 
> Could you please check this? Mac certainly isn't views-based for the dialog. I
> also see chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm so
that
> suggests it's not using it there either.
> 
> 
> > davidben@ - Just to move this work along, would you be supportive of my
> > splitting off the ssl_client_certificate_selector.cc and landing that
> > separately, so we can try to address the concerns in
> > https://code.google.com/p/chromium/issues/detail?id=499996? This would allow
a
> > work-around where we can allow login for enterprise clients (using client
> certs)
> > in a tab using chrome://chrome-signin/?source=0, and once that's in place
we'd
> > be in a position to fix the signin-bubble flow with less urgency. This would
> > also potentially make it easier to move forward with work on
> > https://code.google.com/p/chromium/issues/detail?id=291417
> 
> I don't seem to have access to the second bug. Could you CC me?

I've CC'd you on the bug, and sent screenshots via e-mail (since I couldn't
attach them to this issue.

I'll check out how the Mac code works next.

[wjmaclean, 2015-07-14 18:27:29]

I closed this in favour of https://codereview.chromium.org/1234073002/