Make certificate viewer non-modal using BaseShellDialog to fix aura painting

Make certificate viewer non-modal using BaseShellDialog to fix aura painting

Use BaseShellDialogImpl to spawn the certificate dialog in the
background on win aura.

Having Windows pump a nested message loop as it does while the dialog
is up isn't sufficient because then our tasks that would normally get
run by base::MessageLoop don't get executed. These include e.g.
GpuProcessHost::RouteOnUIThread which is necessary for other windows
to paint.

Because the callers don't really care about being modal, but don't
have a great place to store the dialog viewer object, use a static
method to Show, and have the dialog viewer object own itself as there's
no interaction afterwards with the parent anyway.

R=sky@chromium.org, rsleevi@chromium.org
BUG=306295

[scottmg, 2013-11-05 22:02:54]

sky: code
rsleevi: +net DEPS addition

[sky, 2013-11-05 22:34:02]

Why do we need to this change? I get that it caused painting problems, but why?
Make there be some other lurking painting issues?

https://codereview.chromium.org/61093002/diff/1/ui/shell_dialogs/certificate_...
File ui/shell_dialogs/certificate_viewer_dialog_win.h (right):

https://codereview.chromium.org/61093002/diff/1/ui/shell_dialogs/certificate_...
ui/shell_dialogs/certificate_viewer_dialog_win.h:26: class SHELL_DIALOGS_EXPORT
CertificateViewerDialogWin
Document why this runs on a background thread.

https://codereview.chromium.org/61093002/diff/1/ui/shell_dialogs/certificate_...
ui/shell_dialogs/certificate_viewer_dialog_win.h:32: static void Show(HWND
owning_window, net::X509Certificate* certificate);
Can this take a const net::X509C& ?

https://codereview.chromium.org/61093002/diff/1/ui/shell_dialogs/certificate_...
ui/shell_dialogs/certificate_viewer_dialog_win.h:51: PCCERT_CONTEXT cert_list;
Does this need to be freed? And is this tied to the life of the cerificate
passed in?

[scottmg, 2013-11-05 23:40:32]

https://codereview.chromium.org/61093002/diff/1/ui/shell_dialogs/certificate_...
File ui/shell_dialogs/certificate_viewer_dialog_win.h (right):

https://codereview.chromium.org/61093002/diff/1/ui/shell_dialogs/certificate_...
ui/shell_dialogs/certificate_viewer_dialog_win.h:26: class SHELL_DIALOGS_EXPORT
CertificateViewerDialogWin
On 2013/11/05 22:34:03, sky wrote:
> Document why this runs on a background thread.

Done, and updated CL description.

https://codereview.chromium.org/61093002/diff/1/ui/shell_dialogs/certificate_...
ui/shell_dialogs/certificate_viewer_dialog_win.h:32: static void Show(HWND
owning_window, net::X509Certificate* certificate);
On 2013/11/05 22:34:03, sky wrote:
> Can this take a const net::X509C& ?

Done.

https://codereview.chromium.org/61093002/diff/1/ui/shell_dialogs/certificate_...
ui/shell_dialogs/certificate_viewer_dialog_win.h:51: PCCERT_CONTEXT cert_list;
On 2013/11/05 22:34:03, sky wrote:
> Does this need to be freed? And is this tied to the life of the cerificate
> passed in?

Yes, it does. It's freed in CertificateViewerCompleted which is back on the main
thread.

I don't believe it's tied to the lifetime of the cert that's passed in (my
understanding is that it copies the data into the new one), but maybe Ryan can
comment on that.

[sky, 2013-11-05 23:51:57]

How come the cert viewer blocks painting, but other places that run nested
message loops (like drag and drop or dragging a window) doesn't block painting?

[scottmg, 2013-11-06 00:16:02]

On 2013/11/05 23:51:57, sky wrote:
> How come the cert viewer blocks painting, but other places that run nested
> message loops (like drag and drop or dragging a window) doesn't block
painting?

Aha! After looking into DoDragDrop, adding
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...
this before CryptUIDlgViewCertificate is sufficient. CL here:
https://codereview.chromium.org/59613009

The warning there is a bit scary though. I'm not sure exactly what it means or
whether we meet those requirements.

It also made me wonder how it works on current Stable... and the answer is it's
totally broken. If you bring up a cert dialog and then minimize/restore the
window it happens to paint ok (because windows re-presents the GDI surface), but
if you try to drag the other window, things get very broken.

[sky, 2013-11-06 00:20:20]

I thought it might be something like that.

On Tue, Nov 5, 2013 at 4:16 PM,  <scottmg@chromium.org> wrote:
> On 2013/11/05 23:51:57, sky wrote:
>>
>> How come the cert viewer blocks painting, but other places that run nested
>> message loops (like drag and drop or dragging a window) doesn't block
>
> painting?
>
> Aha! After looking into DoDragDrop, adding
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...
> this before CryptUIDlgViewCertificate is sufficient. CL here:
> https://codereview.chromium.org/59613009
>
> The warning there is a bit scary though. I'm not sure exactly what it means
> or
> whether we meet those requirements.
>
> It also made me wonder how it works on current Stable... and the answer is
> it's
> totally broken. If you bring up a cert dialog and then minimize/restore the
> window it happens to paint ok (because windows re-presents the GDI surface),
> but
> if you try to drag the other window, things get very broken.
>
>
> https://codereview.chromium.org/61093002/

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

[scottmg, 2013-11-06 00:25:27]

On 2013/11/06 00:20:20, sky wrote:
> I thought it might be something like that.

Thanks for the prodding. :) Closing.

> 
> On Tue, Nov 5, 2013 at 4:16 PM,  <mailto:scottmg@chromium.org> wrote:
> > On 2013/11/05 23:51:57, sky wrote:
> >>
> >> How come the cert viewer blocks painting, but other places that run nested
> >> message loops (like drag and drop or dragging a window) doesn't block
> >
> > painting?
> >
> > Aha! After looking into DoDragDrop, adding
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...
> > this before CryptUIDlgViewCertificate is sufficient. CL here:
> > https://codereview.chromium.org/59613009
> >
> > The warning there is a bit scary though. I'm not sure exactly what it means
> > or
> > whether we meet those requirements.
> >
> > It also made me wonder how it works on current Stable... and the answer is
> > it's
> > totally broken. If you bring up a cert dialog and then minimize/restore the
> > window it happens to paint ok (because windows re-presents the GDI surface),
> > but
> > if you try to drag the other window, things get very broken.
> >
> >
> > https://codereview.chromium.org/61093002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ryan Sleevi, 2013-11-06 22:47:50]

https://codereview.chromium.org/61093002/diff/1/ui/shell_dialogs/certificate_...
File ui/shell_dialogs/certificate_viewer_dialog_win.h (right):

https://codereview.chromium.org/61093002/diff/1/ui/shell_dialogs/certificate_...
ui/shell_dialogs/certificate_viewer_dialog_win.h:51: PCCERT_CONTEXT cert_list;
On 2013/11/05 23:40:33, scottmg wrote:
> On 2013/11/05 22:34:03, sky wrote:
> > Does this need to be freed? And is this tied to the life of the cerificate
> > passed in?
> 
> Yes, it does. It's freed in CertificateViewerCompleted which is back on the
main
> thread.
> 
> I don't believe it's tied to the lifetime of the cert that's passed in (my
> understanding is that it copies the data into the new one), but maybe Ryan can
> comment on that.

Correct, it copies into a new one, since the dialog may mutate the object.