MacViews: Remove constrained window dependencies for certificate viewer.

chrome/browser/ui/cocoa/certificate_viewer_mac.mm

Index: chrome/browser/ui/cocoa/certificate_viewer_mac.mm
diff --git a/chrome/browser/ui/cocoa/certificate_viewer_mac.mm b/chrome/browser/ui/cocoa/certificate_viewer_mac.mm
index 1adb0f166bd8687c67f8d0aa8155bed3b1d21f63..5af9dee539cfc02676aa7950fdd5e8eb3e50d1c1 100644
--- a/chrome/browser/ui/cocoa/certificate_viewer_mac.mm
+++ b/chrome/browser/ui/cocoa/certificate_viewer_mac.mm
@@ -4,78 +4,18 @@
 
 #import "chrome/browser/ui/cocoa/certificate_viewer_mac.h"
 
-#include <Security/Security.h>
-#include <SecurityInterface/SFCertificatePanel.h>
-#include <vector>
-
 #include "base/mac/foundation_util.h"
 #include "base/mac/scoped_cftyperef.h"
-#include "base/macros.h"
-#include "chrome/browser/certificate_viewer.h"
-#import "chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h"
-#import "chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet.h"
-#import "chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet_controller.h"
+#include "content/public/browser/web_contents.h"
 #include "net/cert/x509_certificate.h"
 #include "net/cert/x509_util_mac.h"
-#import "ui/base/cocoa/window_size_constants.h"
-
-class SSLCertificateViewerCocoaBridge;
-
-@interface SFCertificatePanel (SystemPrivate)
-// A system-private interface that dismisses a panel whose sheet was started by
-// -beginSheetForWindow:
-//        modalDelegate:
-//       didEndSelector:
-//          contextInfo:
-//         certificates:
-//            showGroup:
-// as though the user clicked the button identified by returnCode. Verified
-// present in 10.8.
-- (void)_dismissWithCode:(NSInteger)code;
-@end
-
-@interface SSLCertificateViewerCocoa ()
-- (void)onConstrainedWindowClosed;
-@end
-
-class SSLCertificateViewerCocoaBridge : public ConstrainedWindowMacDelegate {
- public:
-  explicit SSLCertificateViewerCocoaBridge(SSLCertificateViewerCocoa *
-                                           controller)
-      : controller_(controller) {
-  }
-
-  virtual ~SSLCertificateViewerCocoaBridge() {}
 
-  // ConstrainedWindowMacDelegate implementation:
-  void OnConstrainedWindowClosed(ConstrainedWindowMac* window) override {
-    // |onConstrainedWindowClosed| will delete the sheet which might be still
-    // in use higher up the call stack. Wait for the next cycle of the event
-    // loop to call this function.
-    [controller_ performSelector:@selector(onConstrainedWindowClosed)
-                      withObject:nil
-                      afterDelay:0];
-  }
-
- private:
-  SSLCertificateViewerCocoa* controller_;  // weak
-
-  DISALLOW_COPY_AND_ASSIGN(SSLCertificateViewerCocoaBridge);
-};
-
-void ShowCertificateViewer(content::WebContents* web_contents,
-                           gfx::NativeWindow parent,
-                           net::X509Certificate* cert) {
-  // SSLCertificateViewerCocoa will manage its own lifetime and will release
-  // itself when the dialog is closed.
-  // See -[SSLCertificateViewerCocoa onConstrainedWindowClosed].
-  SSLCertificateViewerCocoa* viewer =
-      [[SSLCertificateViewerCocoa alloc] initWithCertificate:cert];
-  [viewer displayForWebContents:web_contents];
+@implementation SSLCertificateViewerMac {
+  // The corresponding list of certificates.
+  base::scoped_nsobject<NSArray> certificates_;
+  base::scoped_nsobject<SFCertificatePanel> panel_;
 }
 
-@implementation SSLCertificateViewerCocoa
-
 - (id)initWithCertificate:(net::X509Certificate*)certificate {
   if ((self = [super init])) {
     base::ScopedCFTypeRef<CFArrayRef> cert_chain(
@@ -89,8 +29,7 @@ void ShowCertificateViewer(content::WebContents* web_contents,
 - (void)sheetDidEnd:(NSWindow*)parent
          returnCode:(NSInteger)returnCode
             context:(void*)context {
-  if (!closePending_)
-    constrainedWindow_->CloseWebContentsModalDialog();
+  NOTREACHED();  // Subclasses must implement this.
 }
 
 - (void)displayForWebContents:(content::WebContents*)webContents {
@@ -133,17 +72,15 @@ void ShowCertificateViewer(content::WebContents* web_contents,
 
   panel_.reset([[SFCertificatePanel alloc] init]);
   [panel_ setPolicies:(id) policies.get()];
-
-  constrainedWindow_ =
-      CreateAndShowWebModalDialogMac(observer_.get(), webContents, self);
 }
 
-- (NSWindow*)overlayWindow {
-  return overlayWindow_;
+- (void)releaseSheetWindow {
+  panel_.reset();
 }
 
+// ConstrainedWindowSheet protocol implementation.
+
 - (void)showSheetForWindow:(NSWindow*)window {
-  overlayWindow_.reset([window retain]);
   [panel_ beginSheetForWindow:window
                 modalDelegate:self
                didEndSelector:@selector(sheetDidEnd:
@@ -155,26 +92,23 @@ void ShowCertificateViewer(content::WebContents* web_contents,
 }
 
 - (void)closeSheetWithAnimation:(BOOL)withAnimation {
-  closePending_ = YES;
-  overlayWindow_.reset();
   // Closing the sheet using -[NSApp endSheet:] doesn't work so use the private
   // method.
   [panel_ _dismissWithCode:NSFileHandlingPanelCancelButton];
+  certificates_.reset();
+
+  // NOTE: Subclasses must release the panel themselves. This allows the Cocoa
+  // build to use |SSLCertificateViewerCocoaBridge::OnConstrainedWindowClosed|,

[tapted, 2016/05/25 06:52:24]

this doesn't really explain why we can't do panel_.reset() -- panel_ is
refcounted -- it should be retained by things that really need it. But if
there's something *here* that wants to access panel_ and have it be non-null,
then that would trip it up -- is that what happens?

[Patti Lor, 2016/05/31 06:00:33]

SingleWebContentsDialogManagerCocoa::Close() needs it for a call to dialog()
which calls the sheetWindow method on SSLCertificateViewerMacCocoa, which
returns panel_. Since that doesn't add to the ref count, releasing the panel
immediately in closeCertificateSheet makes it nil by the time it's called, so it
doesn't think the dialog is closed which makes Chrome hang when you try to quit.
If you add a print statement for "if (!dialog())" at about
single_web_contents_dialog_manager_cocoa.mm:64 you can see this happening
(running browser_tests SSLCertificateViewerCocoaTest.* will continually try to
call Close() without success which was causing the test timeouts; quitting
Chrome anytime after opening the certificate viewer will hang until killed).

[tapted, 2016/06/01 01:12:47]

oh - whoops. I haven't poked at this bit yet. Needs some thought. The current
approach is probably fine, but I think resetting panel_ here makes it easier to
reason about -- single_web_contents_dialog_manager_cocoa.mm might just be
relying on it instead of something else to infer the show state

[Patti Lor, 2016/06/06 06:52:45]

We could make single_web_contents_dialog_manager_cocoa.mm keep a reference to
the sheet instead of using [sheet_ sheetWindow]? It's a pretty simple change to
make, will upload this as a patchset separate to the one with changes for your
other comments. (Basically everything in the
single_web_contents_dialog_manager_cocoa.* files).

+  // which ensures the sheet is no longer used by waiting for the next event
+  // loop cycle to release it.
 }
 
 - (void)hideSheet {
-  NSWindow* sheetWindow = [overlayWindow_ attachedSheet];
-  [sheetWindow setAlphaValue:0.0];
-
-  oldResizesSubviews_ = [[sheetWindow contentView] autoresizesSubviews];
-  [[sheetWindow contentView] setAutoresizesSubviews:NO];
+  NOTREACHED();  // Subclasses must implement this.
 }
 
 - (void)unhideSheet {
-  NSWindow* sheetWindow = [overlayWindow_ attachedSheet];
-
-  [[sheetWindow contentView] setAutoresizesSubviews:oldResizesSubviews_];
-  [[overlayWindow_ attachedSheet] setAlphaValue:1.0];
+  NOTREACHED();  // Subclasses must implement this.
 }
 
 - (void)pulseSheet {
@@ -182,7 +116,7 @@ void ShowCertificateViewer(content::WebContents* web_contents,
 }
 
 - (void)makeSheetKeyAndOrderFront {
-  [[overlayWindow_ attachedSheet] makeKeyAndOrderFront:nil];
+  NOTREACHED();  // Subclasses must implement this.
 }
 
 - (void)updateSheetPosition {
@@ -197,10 +131,4 @@ void ShowCertificateViewer(content::WebContents* web_contents,
   return panel_;
 }
 
-- (void)onConstrainedWindowClosed {
-  panel_.reset();
-  constrainedWindow_.reset();
-  [self release];
-}
-
 @end