avi@, over to you for M72 fix :)

I'm not sure how to move sheets between windows. I'd have to prevent tearing off windows while they have native sheets on them. No worse than what we had.

#2: That seems entirely appropriate to me.

How do we move the sheet to the new window in step 2 of the repro?! 🤔

Soooo.... On my ToT MacViews Chromium, we don't use SSLCertificateViewerMacViews in chrome/browser/ui/views/certificate_viewer_mac_views.mm. We use SSLCertificateViewerCocoa in chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h|mm. That's... interesting.

chrome/browser/ui/views/ssl_client_certificate_selector.cc has:

void ShowSSLClientCertificateSelector(
    content::WebContents* contents,
    net::SSLCertRequestInfo* cert_request_info,
    net::ClientCertIdentityList client_certs,
    std::unique_ptr<content::ClientCertificateDelegate> delegate) {
#if defined(OS_MACOSX)
  return ShowSSLClientCertificateSelectorCocoa(contents, cert_request_info,
                                               std::move(client_certs),
                                               std::move(delegate));
#else   // defined(OS_MACOSX)
  //...

What *is* SSLCertificateViewerMacViews? Do we even use it?

It appears unused - it's only created in CertificateAnchorWidgetDelegate which itself is never referenced. I bet you can delete it.

So I almost did, but Trent seems to disagree; see the CL at https://chromium-review.googlesource.com/c/chromium/src/+/1262597.

We need to either upgrade the Cocoa constrained window sheet controller to be smarter, even though it's only used for this cert window, or build new infrastructure to host native sheets on Views windows and delete all the code for the Cocoa constrained window sheet controller.

On https://chromium-review.googlesource.com/c/chromium/src/+/1262597 Trent says (and rightly so):

I didn't have time to boot up a mac to play around, but here's what I think we should do.

Delete all these files:
        "cocoa/single_web_contents_dialog_manager_cocoa.h",
        "cocoa/single_web_contents_dialog_manager_cocoa.mm",
        "cocoa/certificate_viewer_mac_cocoa.h",
        "cocoa/certificate_viewer_mac_cocoa.mm",
        "cocoa/constrained_window/constrained_window_mac.h",
        "cocoa/constrained_window/constrained_window_mac.mm",
        "cocoa/constrained_window/constrained_window_sheet.h",
        "cocoa/constrained_window/constrained_window_sheet_controller.h",
        "cocoa/constrained_window/constrained_window_sheet_controller.mm",
        "cocoa/constrained_window/constrained_window_sheet_info.h",
        "cocoa/constrained_window/constrained_window_sheet_info.mm",
(in fact probably delete all of cocoa/constrained_window/*)

There was a time in the past where the Native Cocoa Certificate *viewer* was fully fully functional without those files listed above in a mac_views_browser build.

Then migrate

      "cocoa/ssl_client_certificate_selector_cocoa.h",
      "cocoa/ssl_client_certificate_selector_cocoa.mm",
into something that looks like certificate_viewer_mac_views.mm + certificate_viewer_mac.mm (i.e. remove all the cocoa/constrained_window dependencies)


(maybe certificate_viewer_mac_views.mm can even support either the viewer or the selector with a bit of tweaking)

If we just delete certificate_viewer_mac_views.mm then we are relying on the ~1868 lines of code in cocoa/constrained_window just to support these two dialogs, and creating compatibility problems with the other tab modal dialog plumbing.

He's right; we shouldn't keep the machinery around just for those dialogs.

> or build new infrastructure to host native sheets on Views windows

I *think* this infrastructure is all, or mostly, already built :)

That is, the 63 lines in certificate_viewer_mac_views.mm should be all that's necessary. (along with the machinery in  constrained_window::ShowWebModalDialogWithOverlayViews(), which only certificate_viewer_mac_views.mm currently invokes).

OK, I was super confused in comments 5 and 6. Comment 5 was about the cert viewer, and comment 6 was about the client cert selector. Different dialogs.

So forget comment 6. Back to comment 5:

"""
Soooo.... On my ToT MacViews Chromium, we don't use SSLCertificateViewerMacViews in chrome/browser/ui/views/certificate_viewer_mac_views.mm. We use SSLCertificateViewerCocoa in chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h|mm. That's... interesting.
"""

That's because ShowCertificateViewer() in chrome/browser/certificate_viewer.h has only one implementation, the Cocoa one, in chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm.

There used to be the ShowCertificateViewer() implementation in chrome/browser/ui/views/certificate_viewer_mac_views.mm, but that was removed as part of Polychrome (https://chromium.googlesource.com/chromium/src/+/de30ba8e626978965312af8c42f3cb89bb808519%5E%21/#F7). That's what made the MacViews cert viewer code be dead.

This bug seems to be about all ConstrainedWindowMac dialogs, which happen to be just two: the cert viewer, and the client cert selector. I get to address both.

First the cert viewer. If I revert the change so that certificate_viewer_mac_views is now live and certificate_viewer_mac_cocoa is dead, we fail in a slightly different way. The dialog is closed when we tear away the tab (fine) but we still have a blue dot and the page is locked (very bad).

Second the cert selector. Is there an easy sample page to trigger it? We likely have to go down the same code route as the cert viewer to migrate it to views. Then we can kill all the cocoa code like Trent suggests in comment 9.

OK.

CL #1: Switch to the Views version of the cert viewer.
CL #2: Fix the brokenness when tearing off the cert viewer's tab.
CL #3: Build a Views version of the client cert selector.
CL #4: Remove all the constrained window sheet stuff in comment 9.

CL #1: https://chromium-review.googlesource.com/c/chromium/src/+/1277578
CL #2: It's... a window layer issue!

Deleted: Screen Shot 2018-10-12 at 5.41.32 PM.png 298 KB

	Screen Shot 2018-10-12 at 5.41.32 PM.png 298 KB View Download

It.... not! a window layer issue.

We're hiding and showing the sheet based on the wrong window's visibility. It's attached to the correct window, but showing/hiding itself based on the wrong window.

Deleted: window.mov 748 KB

	window.mov 748 KB View Download

Question: how is the sheet appearing/disappearing?

Not -[NSWindow setAlphaValue:]. lldb doesn't see us hit it.

Not -[NSWindow orderOut:]

What's bothering me is that Quartz Debug sees the sheet. As I switch between the browser window, the cert window appears and disappears, but QD sees no change; it's always listed as alpha 1.00 and completely visible.

Hoooooly moly.

Chromium is halted in the debugger. It's showing a spinning wheel, but the cert window is still doing the appearing/disappearing act.

We're confusing the WindowServer.

Oooh - I think we never implemented the ~HostChanged() plumbing for the cert viewer that lets it switch parent windows, since we assumed we would be keeping the "tabs can't be dragged off" behaviour of the Cocoa browser.. Likely it's using the overlay Widget of the old NSWindow.

In fact... it may, actually, be impossible to change the parent of an NSSheet. Of course, we parent it to the overlay window - maybe we _can_ change its parent..

Here's more evidence of that. Chromium is still halted in the debugger, but when you select "Show All Windows" from the dock icon, the hide-and-peek cert window shows up as attached to the wrong window.

Yet it still tracks the location of the correct window.

Whee.

Deleted: Screen Shot 2018-10-14 at 6.42.05 PM.png 2.8 MB

	Screen Shot 2018-10-14 at 6.42.05 PM.png 2.8 MB View Download

Yah, I think the issue is in

NativeWebContentsModalDialogManagerViews::HostChanged()

NativeWebContentsModalDialogManagerViewsMac doesn't override it, and the Cocoa browser would never call it, so it slipped under the radar.

The implementation in NativeWebContentsModalDialogManagerViews will call views::Widget::ReparentNativeView which will do.. weird stuff. It's not aware of relationships between windows -- only views. I think it will actually tear out the -[NSWindow contentView] of one window and add it as a child of another o_O. So yeah - defs weird.

Ah, thank you.

I saw that it was called but didn't realize what was involved. I'll give it a try in a rewrite.

The cert dialog is shown with a views::Widget as the parent (the "overlay window"). That widget is returned from constrained_window::ShowWebModalDialogWithOverlayViews(), which internally calls views::DialogDelegate::CreateDialogWidget() which makes the Widget with:

Widget* DialogDelegate::CreateDialogWidget(WidgetDelegate* delegate,
                                           gfx::NativeWindow context,
                                           gfx::NativeView parent) {
  views::Widget* widget = new views::Widget;
  views::Widget::InitParams params =
      GetDialogWidgetInitParams(delegate, context, parent, gfx::Rect());
  widget->Init(params);
  return widget;
}

Is the parent widget of a widget permanent? I can't find anything in widget.h that mentions "parent" except for ReparentNativeView() which is clearly not what we want. I could get the NSWindow of the overlay window Widget, and manually reparent it via AppKit but I can't imagine that Views would take that meddling in its affairs lightly.

Any thoughts?

I think Views should be fine with it.. It does it with aura::Window for precisely this.

And in fact ccameron's work recently added a `SetParent()` method invoked from NativeWidgetPrivate::ReparentNativeView(). The weird NSView manipulation is happening in BridgedNativeWidgetImpl::ReparentNativeView(). SadTabs used to rely on this NSView hierarchy manipulation. Maybe interstitials still do :/.

Maybe there's a case we can detect there. E.g. only TYPE_CONTROL Widgets should ever be torn out of the view hierarchy of their owning Widget. For others, we can probably skip over the addSubview call. (there might be more to it though).

Totally nope.

You can move the window from one parent to another, but then showing it things tend to explode.

[6145:1295:1014/204457.768921:FATAL:bridged_native_widget_impl.mm(1191)] Check failed: visible_bridged_children == CountBridgedWindows([window_ childWindows]) (0 vs. 1)
5   libviews.dylib                      0x0000000143033b82 views::BridgedNativeWidgetImpl::NotifyVisibilityChangeDown() + 2722
6   libviews.dylib                      0x000000014303307f views::BridgedNativeWidgetImpl::OnVisibilityChanged() + 415
7   libviews.dylib                      0x00000001432695f8 -[ViewsNSWindowDelegate onWindowOrderChanged:] + 40
8   libviews.dylib                      0x000000014304116b -[NativeWidgetMacNSWindow orderWindow:relativeTo:] + 123
9   AppKit                              0x00007fff891d3fc6 -[NSWindow makeKeyAndOrderFront:] + 106
10  libviews.dylib                      0x0000000143030ef0 views::BridgedNativeWidgetImpl::SetVisibilityState(views_bridge_mac::mojom::WindowVisibilityState) + 1120
11  libviews.dylib                      0x0000000143219e9a views::NativeWidgetMac::Show(ui::WindowShowState, gfx::Rect const&) + 730

Sigh.

Let's land https://chromium-review.googlesource.com/c/chromium/src/+/1277578 and I'll keep banging at this. I honestly am kinda lost with the gfx::NativeThings vs Widgets vs Views, vs all the internals. Why does Widget take a gfx::NativeView as a parent? This really makes no sense to me...

Trent,

You're one of the few people knowledgeable enough to write NativeWebContentsModalDialogManagerViewsMac::HostChanged(). Can you take a look at it? I'm super confused about how to reparent the Widget.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2de299c3687beea1fa182bf4faa43f1ec33da8f6

commit 2de299c3687beea1fa182bf4faa43f1ec33da8f6
Author: Avi Drissman <avi@chromium.org>
Date: Mon Oct 15 17:36:34 2018

Switch to the MacViews version of the cert viewer.

There was a Cocoa and MacViews version with a common superclass;
this removes the Cocoa version and folds the superclass into the
MacViews subclass which is the only one that remains.

This also reverts 027233cd43df4596af6b3ba501a15090f1284c05 as
that hack was only needed while we linked against the 10.10 SDK
and we no longer do so.

BUG=891699

Change-Id: Iefdd30268f1e0b52ee1ecd71bdb252e5b8b5c631
Reviewed-on: https://chromium-review.googlesource.com/c/1277578
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599663}
[modify] https://crrev.com/2de299c3687beea1fa182bf4faa43f1ec33da8f6/chrome/browser/ui/BUILD.gn
[delete] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/chrome/browser/ui/certificate_viewer_mac.h
[delete] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/chrome/browser/ui/certificate_viewer_mac.mm
[delete] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h
[delete] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm
[rename] https://crrev.com/2de299c3687beea1fa182bf4faa43f1ec33da8f6/chrome/browser/ui/views/certificate_viewer_mac_browsertest.mm
[modify] https://crrev.com/2de299c3687beea1fa182bf4faa43f1ec33da8f6/chrome/browser/ui/views/certificate_viewer_mac_views.mm
[modify] https://crrev.com/2de299c3687beea1fa182bf4faa43f1ec33da8f6/chrome/test/BUILD.gn

I have to do some more mucking around in this general region for RemoteMacViews. Feel free to throw my way.

ccameron: Re comment 17, the remaining steps are:

CL #2: Fix the brokenness when tearing off the cert viewer's tab.
CL #3: Build a Views version of the client cert selector.
CL #4: Remove all the constrained window sheet stuff in comment 9.

I can do #3 and #4, and then toss this to you to take #2.

Actually, let's split off #3 and #4 into a new bug;  bug 895514 .

To summarize:
- Repro: Open a window with many tabs. In one tab, show a site cert. Tear off that tab; the cert window should properly be reparented. Reattach that tab; the cert window should properly be reparented.
- Cause (comment 26): We need a NativeWebContentsModalDialogManagerViewsMac::HostChanged(); currently we fall back to NativeWebContentsModalDialogManagerViews::HostChanged() which does the wrong thing and ends up generating windows that terribly confuse the WindowServer (comment 24).

Lemme know if you have any questions.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dba0af4a7ae22e65a1e89022a588d0aded2a783c

commit dba0af4a7ae22e65a1e89022a588d0aded2a783c
Author: Avi Drissman <avi@chromium.org>
Date: Tue Oct 16 15:27:37 2018

Properly release the certificate sheet when the owning tab is closed.

BUG=891699

Change-Id: Ib0d473673ba6f06e9d46096380df517ebeb3ae4f
Reviewed-on: https://chromium-review.googlesource.com/c/1282363
Reviewed-by: Leonard Grey <lgrey@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599992}
[modify] https://crrev.com/dba0af4a7ae22e65a1e89022a588d0aded2a783c/chrome/browser/ui/views/certificate_viewer_mac_views.mm

Since Fix is landed, Closing this bug as a part of UI mass triage.If you feel this issue should still be addressed, feel free to reopen it or to file a new issue.

I don't think this is done if I read comment 34 right.

This is indeed not yet fixed.