MacViews: Remove constrained window dependencies for certificate viewer.

chrome/browser/ui/cocoa/certificate_viewer_mac.mm

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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(
         certificate->CreateOSCertChainForCert());
     NSArray* certificates = base::mac::CFToNSCast(cert_chain.get());
     certificates_.reset([certificates retain]);
   }
   return self;
 }
 
 - (void)sheetDidEnd:(NSWindow*)parent
          returnCode:(NSInteger)returnCode
             context:(void*)context {
-  if (!closePending_)
-    constrainedWindow_->CloseWebContentsModalDialog();
+  NOTREACHED();  // Subclasses must implement this.
 }
 
 - (void)displayForWebContents:(content::WebContents*)webContents {
   // Explicitly disable revocation checking, regardless of user preferences
   // or system settings. The behaviour of SFCertificatePanel is to call
   // SecTrustEvaluate on the certificate(s) supplied, effectively
   // duplicating the behaviour of net::X509Certificate::Verify(). However,
   // this call stalls the UI if revocation checking is enabled in the
   // Keychain preferences or if the cert may be an EV cert. By disabling
   // revocation checking, the stall is limited to the time taken for path
@@ skipping 22 matching lines @@
   CFRelease(basic_policy);
 
   status = net::x509_util::CreateRevocationPolicies(false, false, policies);
   if (status != noErr) {
     NOTREACHED();
     return;
   }
 
   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:
                                          returnCode:
                                             context:)
                   contextInfo:NULL
                  certificates:certificates_
                     showGroup:YES];
 }
 
 - (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 {
   // NOOP
 }
 
 - (void)makeSheetKeyAndOrderFront {
-  [[overlayWindow_ attachedSheet] makeKeyAndOrderFront:nil];
+  NOTREACHED();  // Subclasses must implement this.
 }
 
 - (void)updateSheetPosition {
   // NOOP
 }
 
 - (void)resizeWithNewSize:(NSSize)preferredSize {
   // NOOP
 }
 
 - (NSWindow*)sheetWindow {
   return panel_;
 }
 
-- (void)onConstrainedWindowClosed {
-  panel_.reset();
-  constrainedWindow_.reset();
-  [self release];
-}
-
 @end