Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1779383002:
MacViews: Remove constrained window dependencies for certificate viewer. (Closed)
|
Created:
4 years, 9 months ago by Patti Lor Modified:
4 years, 3 months ago CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org, Elly Fong-Jones, Robert Sesek Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Remove constrained window dependencies for certificate viewer.
Move constrained_window Mac dependencies to a Cocoa build only. To make the
MacViews browser's certificate viewer work, refactor out common code taken from
the original certificate viewer and subclass into SSLCertificateViewerCocoa and
SSLCertificateViewerMacViews. The MacViews certificate viewer will be created
via a new constrained_window method ShowWebModalDialogWithOverlayViews().
Similarly, NativeWebContentsModalDialogManagerViews is subclassed into a
MacViews specific NativeWebContentsModalDialogManagerViewsMac, which handles
hiding/showing the certificate viewer.
Note that the certificate viewer is not the same as the Views one because OSX
has its own, which is why there is special code for it needed here.
BUG=587239
# Long include line in constrained_window_views.cc, wrapped in an ifdef.
NOPRESUBMIT=true
Committed: https://crrev.com/8f36f7b892825b3117d2b1d2cbd60b05ce6da4ba
Cr-Commit-Position: refs/heads/master@{#414971}
Patch Set 1 #Patch Set 2 : Position correctly (kind of) & block on tab only. #
Total comments: 70
Patch Set 3 : Refactor to use views:: NativeWebContentsModalDialogManagerViews. #Patch Set 4 : #Patch Set 5 : Cleanup. #Patch Set 6 : Fix positioning after resize from top, don't allow interaction with tab WebContents while certifica… #
Total comments: 104
Patch Set 7 : Prevent NativeWeb...Mac from being used for all dialogs, address review comments. #Patch Set 8 : Fix non-mac builds. #Patch Set 9 : Rebase & address review comments. #
Total comments: 41
Patch Set 10 : Address review comments. #Patch Set 11 : Fix gn and other build targets. #Patch Set 12 : Restore overlayWindow method for SSLCertificateViewerCocoa, fix test, other nits. #
Total comments: 28
Patch Set 13 : Review comments. #Patch Set 14 : Fix SSLCertificateViewerCocoaTest.* #Patch Set 15 : Update comment. #
Total comments: 14
Patch Set 16 : Move out ConstrainedWindowSheet to Cocoa-only, other review comments. #
Total comments: 43
Patch Set 17 : Address review comments. #Patch Set 18 : Keep a reference to the sheet window in single_web_contents_dialog_manager_cocoa.mm. #
Total comments: 2
Patch Set 19 : Rebase. #
Total comments: 14
Patch Set 20 : Rebase & address review comments. #
Total comments: 3
Patch Set 21 : Move web_contents_modal_dialog_manager_views* into components/constrained_window. #Patch Set 22 : Rebase on top of constrained window refactor CL. #Patch Set 23 : Revert constrained_window.gypi, remove unused web_contents_modal_dialog_manager_views.cc. #Patch Set 24 : Rebase on master after dependent CL landed. #Patch Set 25 : Revert random change that was made accidentally? #
Total comments: 21
Patch Set 26 : Move certificate_viewer_mac.* to chrome/browser/ui and certificate_viewer_mac_views.mm to chrome/br… #
Total comments: 24
Patch Set 27 : rsesek@ review - merge displayForWebContents & initWithCertificate, etc. #
Total comments: 10
Patch Set 28 : msw@ & rsesek@ review, move sheetDidEnd::: into separate SSLCertificateViewerMac declaration. #Messages
Total messages: 95 (58 generated)
patricialor@chromium.org changed reviewers: + tapted@chromium.org
Hi Trent, could you take a look at this initial implementation? It's still a WIP, I'm currently stuck on a few issues which happen when the certificate viewer is opened: 1. The whole browser disappears when switching away from current space and back (can still be retrieved from the dock). 2. Crashes on minimise. 3. Crashes entering fullscreen. 4. The certificate viewer is not aligned consistently with the Cocoa build. The Cocoa browser aligns it with the bottom of the grey browser UI, covering any info bars that might be open, but my implementation does not (it uses the same y-origin as the WebContents, see SingleWebContentsDialogManagerViewsMac::UpdateSheetPosition). I think 1 & 2 are because I bypassed the BridgedNativeWidget when adding a child view to the parent (for example, on minimising, it assumes all its children will be a BridgedNativeWidget - see bridged_native_widget.mm:1027). Is using a BridgedNativeWidget instead of what I'm currently doing on web_contents_modal_dialog_manager_views_mac.mm:70 the right approach to fixing this?
OK - I think the main trick is to call constrained_window::CreateWebModalDialogViews(..) for the overlay window. Hopefully the rest will follow https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... File chrome/browser/mac/certificate_viewer_base.h (right): https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. I think we want to leave this as chrome/browser/ui/cocoa/certificate_viewer_mac.h for now. It should definitely be under a .../ui/... path. That will keep the diff more digestible for review. We might need to move it to satisfy DEPS though - you might be able to play around with `git cl upload --similarity` to keep a similar diff https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.h:6: no blank line https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.h:11: #import <SecurityInterface/SFCertificatePanel.h> can you just forward-declare @class SFCertificatePanel; and move the two Security/ includes to the .mm? https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.h:14: #include "base/memory/scoped_ptr.h" unused? https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.h:16: #include "content/public/browser/web_contents.h" forward declare https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.h:17: remove extra blank line https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.h:36: @interface SSLCertificateViewerMacBase : NSObject<ConstrainedWindowSheet> { I'd call this just SSLCertificateViewerMac - then we have SSLCertificateViewerMacCocoa, SSLCertificateViewerMacViews. Also Needs a class comment. All the methods in .h files should be commented to per go/objcguide?cl=head#Declaration_Comments https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... File chrome/browser/mac/certificate_viewer_base.mm (right): https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.mm:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.mm:5: #include "chrome/browser/mac/certificate_viewer_base.h" import https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.mm:71: // ConstrainedWindowSheet: For objc the current approach is something like // ConstrainedWindowSheet protocol implementation. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.mm:80: contextInfo:NULL rollback indenting change https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.mm:88: // Closing the sheet using -[NSApp endSheet:] doesn't work so use the re-wrap https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.h:5: #ifndef CHROME_BROWSER_UI_COCOA_CERTIFICATE_VIEWER_MAC_H_ rename to certificate_viewer_mac_cocoa.h https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.h:6: remove blank line https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.h:16: remove blank line https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.h:18: @class SFCertificatePanel; unused https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:8: #include <SecurityInterface/SFCertificatePanel.h> These includes shouldn't be needed https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:11: #include "base/mac/scoped_cftyperef.h" nor this https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:12: #include "base/macros.h" or this - but there should probable be a base/logging.h https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:17: #include "net/cert/x509_util_mac.h" the net/cert includes probably aren't needed either - the forward declared should be enough https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:18: #import "ui/base/cocoa/window_size_constants.h" this seems unused too, (but that seems to have always been so) https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac.h (left): https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. keep this file :) and the .mm https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/mac/certificate_viewer_views_mac.h (right): https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.h:5: #ifndef CHROME_BROWSER_UI_VIEWS_MAC_CERTIFICATE_VIEWER_VIEWS_MAC_H_ Let's keep it in chrome/browser/ui/cocoa/ for now so they're all together, and call it certificate_viewer_mac_views.h/cc https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.h:6: remove blank line https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.h:10: #import "chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet.h" This protocol is inherited, so not needed here. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.h:13: remove blank line https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.h:14: @interface SSLCertificateViewerViewsMac : needs a comments https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.h:15: SSLCertificateViewerMacBase<ConstrainedWindowSheet> { remove <ConstrainedWindowSheet> https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/mac/certificate_viewer_views_mac.mm (right): https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.mm:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.mm:11: no blank line https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.mm:20: new SingleWebContentsDialogManagerViewsMac(self, webContents)); SingleWebContentsDialogManagerViewsMac isn't the right class -- that's for parenting views dialogs off a Cocoa window. The right SingleWebContentsDialogManager is in a .cc file only. I think we want something like anchor_widget_ = constrained_window::CreateWebModalDialogViews(new CertificateAnchorWidgetDelegate, webContents); Where you declare a // A fully transparent, borderless web-modal dialog used to display the OS-provided // window-modal sheet that displays certificate information. class CertificateAnchorWidgetDelegate : public views::WidgetDelegateView { public: Certificate..() private: DISALLOW... } And I'm actually not sure if you need *anything* in CertificateAnchorWidgetDelegate, in which case you could just pass a `new WidgetDelegateView`. More likely there's some stuff around size/transparency/borders/activation that needs to be adjusted https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.mm:22: [[NSNotificationCenter defaultCenter] Hopefully these bits won't be needed -- calling constrained_window::CreateWebModalDialogViews should set this up automatically https://codereview.chromium.org/1779383002/diff/20001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (left): https://codereview.chromium.org/1779383002/diff/20001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:1491: 'browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.h', header should move as well https://codereview.chromium.org/1779383002/diff/20001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1779383002/diff/20001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:2570: 'browser/ui/views/mac/web_contents_modal_dialog_manager_views_mac.h', I'm pretty sure we use this to parent toolkit-views dialogs in the Cocoa browser, so it can probably stay where it is
Hi Trent, can you please take another look? If you undo the changes in http://crrev.com/1614663002 the MacViews code will work (as per http://crbug.com/602846). See the patch below: diff --git a/ui/views/cocoa/bridged_native_widget.mm b/ui/views/cocoa/bridged_native_widget.mm index 6fd2594..882b7a9 100644 --- a/ui/views/cocoa/bridged_native_widget.mm +++ b/ui/views/cocoa/bridged_native_widget.mm @@ -339,7 +339,6 @@ BridgedNativeWidget::BridgedNativeWidget(NativeWidgetMac* parent) } BridgedNativeWidget::~BridgedNativeWidget() { - bool close_window = false; if ([window_ delegate]) { // If the delegate is still set on a modal dialog, it means it was not // closed via [NSApplication endSheet:]. This is probably OK if the widget @@ -361,8 +360,6 @@ BridgedNativeWidget::~BridgedNativeWidget() { // Note that if the window has children it can't be closed until the // children are gone, but removing child windows calls into AppKit for the // parent window, so the delegate must be cleared first. - [window_ setDelegate:nil]; - close_window = true; } RemoveOrDestroyChildren(); @@ -371,10 +368,7 @@ BridgedNativeWidget::~BridgedNativeWidget() { SetRootView(nullptr); DestroyCompositor(); - if (close_window) { - OnWindowWillClose(); - [window_ close]; - } + [window_ close]; } void BridgedNativeWidget::Init(base::scoped_nsobject<NSWindow> window, diff --git a/ui/views/cocoa/native_widget_mac_nswindow.mm b/ui/views/cocoa/native_widget_mac_nswindow.mm index b93ee6f..6ff1e68 100644 --- a/ui/views/cocoa/native_widget_mac_nswindow.mm +++ b/ui/views/cocoa/native_widget_mac_nswindow.mm @@ -86,15 +86,7 @@ // Dialogs and bubbles shouldn't take large shadows away from their parent. views::Widget* widget = [self viewsWidget]; - return widget->CanActivate() && - !views::NativeWidgetMac::GetBridgeForNativeWindow(self)->parent(); -} - -// Lets the traffic light buttons on the parent window keep their active state. -- (BOOL)_sharesParentKeyState { - // Follow -canBecomeMainWindow unless the window provides its own buttons. - return ([self styleMask] & NSClosableWindowMask) == 0 && - ![self canBecomeMainWindow]; + return widget->CanActivate() && !widget->IsDialogBox(); } // Override sendEvent to allow key events to be forwarded to a toolkit-views https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... File chrome/browser/mac/certificate_viewer_base.h (right): https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/03/21 02:47:12, tapted wrote: > I think we want to leave this as > chrome/browser/ui/cocoa/certificate_viewer_mac.h for now. It should definitely > be under a .../ui/... path. That will keep the diff more digestible for review. > We might need to move it to satisfy DEPS though - you might be able to play > around with `git cl upload --similarity` to keep a similar diff Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.h:6: On 2016/03/21 02:47:12, tapted wrote: > no blank line Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.h:11: #import <SecurityInterface/SFCertificatePanel.h> On 2016/03/21 02:47:12, tapted wrote: > can you just forward-declare > > @class SFCertificatePanel; and move the two Security/ includes to the .mm? certificate_viewer.mac.mm complains it can't find instance method _dismissWithCode: (interface declaration later in this file). https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.h:14: #include "base/memory/scoped_ptr.h" On 2016/03/21 02:47:12, tapted wrote: > unused? Done, have moved this to certificate_viewer_views_mac.h. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.h:16: #include "content/public/browser/web_contents.h" On 2016/03/21 02:47:12, tapted wrote: > forward declare Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.h:17: On 2016/03/21 02:47:12, tapted wrote: > remove extra blank line Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.h:36: @interface SSLCertificateViewerMacBase : NSObject<ConstrainedWindowSheet> { On 2016/03/21 02:47:12, tapted wrote: > I'd call this just SSLCertificateViewerMac - then we have > SSLCertificateViewerMacCocoa, SSLCertificateViewerMacViews. Also Needs a class > comment. All the methods in .h files should be commented to per > go/objcguide?cl=head#Declaration_Comments Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... File chrome/browser/mac/certificate_viewer_base.mm (right): https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.mm:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/03/21 02:47:12, tapted wrote: > nit: no (c) Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.mm:5: #include "chrome/browser/mac/certificate_viewer_base.h" On 2016/03/21 02:47:12, tapted wrote: > import Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.mm:71: // ConstrainedWindowSheet: On 2016/03/21 02:47:12, tapted wrote: > For objc the current approach is something like > > // ConstrainedWindowSheet protocol implementation. Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.mm:80: contextInfo:NULL On 2016/03/21 02:47:12, tapted wrote: > rollback indenting change Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/mac/cert... chrome/browser/mac/certificate_viewer_base.mm:88: // Closing the sheet using -[NSApp endSheet:] doesn't work so use the On 2016/03/21 02:47:12, tapted wrote: > re-wrap Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.h:5: #ifndef CHROME_BROWSER_UI_COCOA_CERTIFICATE_VIEWER_MAC_H_ On 2016/03/21 02:47:12, tapted wrote: > rename to certificate_viewer_mac_cocoa.h Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.h:6: On 2016/03/21 02:47:12, tapted wrote: > remove blank line Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.h:16: On 2016/03/21 02:47:12, tapted wrote: > remove blank line Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.h:18: @class SFCertificatePanel; On 2016/03/21 02:47:12, tapted wrote: > unused Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:8: #include <SecurityInterface/SFCertificatePanel.h> On 2016/03/21 02:47:13, tapted wrote: > These includes shouldn't be needed Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:11: #include "base/mac/scoped_cftyperef.h" On 2016/03/21 02:47:13, tapted wrote: > nor this Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:12: #include "base/macros.h" On 2016/03/21 02:47:12, tapted wrote: > or this - but there should probable be a base/logging.h Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:17: #include "net/cert/x509_util_mac.h" On 2016/03/21 02:47:13, tapted wrote: > the net/cert includes probably aren't needed either - the forward declared > should be enough Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:18: #import "ui/base/cocoa/window_size_constants.h" On 2016/03/21 02:47:13, tapted wrote: > this seems unused too, (but that seems to have always been so) Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac.h (left): https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/03/21 02:47:12, tapted wrote: > keep this file :) and the .mm Done - sorry, I think this was an accident D: https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/mac/certificate_viewer_views_mac.h (right): https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/03/21 02:47:13, tapted wrote: > nit: no (c) Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.h:5: #ifndef CHROME_BROWSER_UI_VIEWS_MAC_CERTIFICATE_VIEWER_VIEWS_MAC_H_ On 2016/03/21 02:47:13, tapted wrote: > Let's keep it in chrome/browser/ui/cocoa/ for now so they're all together, and > call it > > certificate_viewer_mac_views.h/cc Done, but with .mm not .cc, is that what you meant? https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.h:6: On 2016/03/21 02:47:13, tapted wrote: > remove blank line Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.h:10: #import "chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet.h" On 2016/03/21 02:47:13, tapted wrote: > This protocol is inherited, so not needed here. Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.h:13: On 2016/03/21 02:47:13, tapted wrote: > remove blank line Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.h:14: @interface SSLCertificateViewerViewsMac : On 2016/03/21 02:47:13, tapted wrote: > needs a comments Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.h:15: SSLCertificateViewerMacBase<ConstrainedWindowSheet> { On 2016/03/21 02:47:13, tapted wrote: > remove <ConstrainedWindowSheet> Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/mac/certificate_viewer_views_mac.mm (right): https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.mm:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/03/21 02:47:13, tapted wrote: > nit: no (c) Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.mm:11: On 2016/03/21 02:47:13, tapted wrote: > no blank line Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.mm:20: new SingleWebContentsDialogManagerViewsMac(self, webContents)); On 2016/03/21 02:47:13, tapted wrote: > SingleWebContentsDialogManagerViewsMac isn't the right class -- that's for > parenting views dialogs off a Cocoa window. The right > SingleWebContentsDialogManager is in a .cc file only. I think we want something > like > > anchor_widget_ = constrained_window::CreateWebModalDialogViews(new > CertificateAnchorWidgetDelegate, webContents); > > Where you declare a > > // A fully transparent, borderless web-modal dialog used to display the > OS-provided > // window-modal sheet that displays certificate information. > class CertificateAnchorWidgetDelegate : public views::WidgetDelegateView { > public: > Certificate..() > private: > DISALLOW... > } > > > And I'm actually not sure if you need *anything* in > CertificateAnchorWidgetDelegate, in which case you could just pass a `new > WidgetDelegateView`. > > More likely there's some stuff around size/transparency/borders/activation that > needs to be adjusted Done - switched to using the dialog created by the views constrained_window stuff. This requires that the modal type of the delegate be ui::MODAL_TYPE_CHILD, so have gone ahead with your CertificateAnchorWidgetDelegate idea. https://codereview.chromium.org/1779383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/mac/certificate_viewer_views_mac.mm:22: [[NSNotificationCenter defaultCenter] On 2016/03/21 02:47:13, tapted wrote: > Hopefully these bits won't be needed -- calling > constrained_window::CreateWebModalDialogViews should set this up automatically Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (left): https://codereview.chromium.org/1779383002/diff/20001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:1491: 'browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.h', On 2016/03/21 02:47:13, tapted wrote: > header should move as well Done. https://codereview.chromium.org/1779383002/diff/20001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1779383002/diff/20001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:2570: 'browser/ui/views/mac/web_contents_modal_dialog_manager_views_mac.h', On 2016/03/21 02:47:13, tapted wrote: > I'm pretty sure we use this to parent toolkit-views dialogs in the Cocoa > browser, so it can probably stay where it is Done.
Nice! this looks like the right approach. Main bits from the comments below: - sizing of the overlay window doesn't seem to match the tab properly - we should split out the bits that create web_contents_modal_dialog_manager_views.h and moves NativeWebContentsModalDialogManagerViews in there so we can run a mechanical change past OWNERS https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: keep the old copyright https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:9: #import <Security/Security.h> I think Security.h is actually a C header, so it should be #included. But also I don't think we need it in the header -- we just need SFCertificatePanel.h and Cocoa.h https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:12: #include <memory> nit: this can be removed since unique_ptr is gone https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:14: #include "base/mac/scoped_nsobject.h" nit: import https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:20: namespace net { nit: blank line before https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:37: /** Base class for the certificate viewer generated by OSX for Chrome Mac. this comment style never took off in Chrome for some reason - we should just put // on each line- same below, and in other files https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:42: @protected These should stay @private. If the derived class needs them, we typically add a @property(nonatomic, readonly) https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:69: #endif // CHROME_BROWSER_MAC_CERTIFICATE_VIEWER_MAC_H_ nit: ensure comment matches #ifdef https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:10: #include <memory.h> no .h https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:13: #import "chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h" can this be forward declared as well? https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:18: @interface SSLCertificateViewerCocoa : needs a comment https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:19: SSLCertificateViewerMac<ConstrainedWindowSheet> { should be able to omit <ConstrainedWindowSheet> since it's inherited. then the constrained_window_sheet.h #import probably isn't needed https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:23: // A copy of the sheet's frame used to restore on show. nit: full stop after `frame`. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. (c) 2012 -> 2016? Otherwise, can you play with `git cl upload --similarity to get this to appear as a git-move from certificate_viewer_mac.mm https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:8: #include "base/mac/foundation_util.h" nit: import https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:15: explicit SSLCertificateViewerCocoaBridge(SSLCertificateViewerCocoa * no space between SSLCertificateViewerCocoa and "*" - also the formatting is weird - `git cl format`? https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:24: // |onConstrainedWindowClosed| will delete the sheet which might be still nit: still -> still be https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:33: SSLCertificateViewerCocoa* controller_; // weak nit: // Weak. Owns this. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_views.h (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.h:5: #ifndef CHROME_BROWSER_UI_VIEWS_MAC_CERTIFICATE_VIEWER_VIEWS_MAC_H_ this need to match the path https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.h:17: /** Certificate viewer class for MacViews which handles displaying and closing nit: update comment style. more below https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.h:22: std::unique_ptr<views::Widget> dialog_; perhaps call this overlayWindow_? https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.h:23: views::WidgetDelegateView* delegate_; is this member needed? - looks like it could be a temporary in displayForWebContents https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.h:32: - (void)displayForWebContents:(content::WebContents*)webContents; ObjC overrides don't need to be redeclared https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:10: #include "ui/views/widget/widget_delegate.cc" .cc ? :) https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:12: using namespace web_modal; remove? it's uncommon to use `using namespace` except in tests. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:18: CertificateAnchorWidgetDelegate(content::WebContents* webContents) {} the |webContents| argument can be removed https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:19: ui::ModalType GetModalType() const override { return ui::MODAL_TYPE_CHILD; } nit: // WidgetDelegate: https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:45: int windowWidth = [webContents->GetTopLevelNativeWindow() frame].size.width; nit GGFloat windowWidth = NSWidth([webContents->GetTopLevelNativeWindow() frame]); https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:47: dialog_->SetBounds(gfx::Rect(tabViewSize.x(), is OnPositionRequiresUpdate() called during construction? Maybe this logic only needs to be there https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:1: // Copyright 2016 The Chromium Authors. All rights reserved. call this file native_web_contents_modal_dialog_manager_views_mac.mm? https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:7: #include <Cocoa/Cocoa.h> nit: import https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:11: class NativeWebContentsModalDialogManagerViewsMac nit: needs a comment https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:22: gfx::Rect originalBounds = widget->GetWindowBoundsInScreen(); perhaps just `bounds` since we change it later https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:23: NativeWebContentsModalDialogManagerViews::OnPositionRequiresUpdate(); does this need to be called at all? https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:25: // Reset the dialog size to the area under the browser UI to avoid Cocoa I don't think this is working right -- I can interact with the web page while the dialog is showing. Ideally overlayWindow_ intercepts all the clicks, but that doesn't seem to happen. The other weird thing (and I think this is a separate bug, which exists on ChromeOS too) is that interaction with the DevTools half of the webcontents isn't blocked - it should be. (but with a proper overlay window sizing to the tab contents exactly this shouldn't affect Mac). But, still, it's probably a bug on ChromeOS. Doesn't affect Windows since the certificate viewer is window-modal. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:35: NSWindow* dialogWindow = widget->GetNativeWindow(); dialogWindow -> dialog_window (camelCase only between @{implementation,interface}/@end). More below https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:36: if (![dialogWindow attachedSheet]) { comment about this? (when does it come up?) https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:60: void ChangeVisibilityAndMouseEvents(gfx::NativeWindow window, bool show) { perhaps call this void SetOverlayVisible(gfx::NativeWindow overlay, bool visible) https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:62: BOOL mouseEvents = show ? NO : YES; mouseEvents -> ignore_events https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:64: // Don't allow interaction with the tab underneath the certificate viewer. tab -> overlayWindow? https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:67: [[window attachedSheet] setAlphaValue:alpha]; does this always hide the sheet? I thought we'd need code for that https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:71: }; DISALLOW_COPY_AND_ASSIGN(..) https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/native_web_modal_manager.cc (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/native_web_modal_manager.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. `(c) 2012` -> `2016` https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:8: #include <set> move to header https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:10: #include "base/macros.h" move to header https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:14: #include "components/web_modal/web_contents_modal_dialog_host.h" make sure nothing in the header includes repeats here https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:144: // WebContentsModalDialogHostObserver: nit: this comment isn't needed https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/web_contents_modal_dialog_manager_views.h (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. since this CL is large, it's probably worth splitting out this and native_web_modal_manager.cc into a separate change: "Move NativeWebContentsModalDialogManagerViews into its own header". The diff isn't cooperating at all :/ and OWNERS will appreciate reviewing the purely mechanical stuff separately https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.h:5: #ifndef CHROME_BROWSER_UI_VIEWS_WEB_CONTENTS_MODAL_DIALOG_MANAGER_VIEWS_H_ Hum - this file should get called native_web_contents_modal_dialog_manager_views.h. The .cc should match, but that will remove any `diff` ability. But since that's so broken anyway, I think we should just do it now. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.h:10: #include "ui/views/widget/widget.h" this can probably be forward-declared https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.h:13: class NativeWebContentsModalDialogManagerViews nit: needs a comment https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.h:24: // Sets up this object to manage the dialog_. Registers for closing events nit: dialog_ -> |dialog_|
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1779383002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1779383002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
Hi Trent, have rebased this off the CL that splits NativeWebContentsModal...Views into a header/body file (http://crrev.com/1966583002). The diff is still not great (e.g. for components/web_modal/web_contents_modal_dialog_views.cc, I can't find the old file it links up to in codesearch at all :/). I've also added a separate method called ShowParentedWebModalDialogViews as you suggested, but since there are still a few layers between that and the final creation of NativeWebContentsModalDialogManagerViewsMac, I had to edit some of those methods as well. So there's a web_contents_modal_dialog_views.cc file that is included when mac_views_browser = 0. Let me know if you can think of a better way to do this! Test CL: https://codereview.chromium.org/1965703002/ The gyp stuff isn't set up properly for component_unittests, so it dies with linker errors, but I'll leave that for now assuming we can figure out a better way to set this up. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/05/04 03:29:23, tapted wrote: > nit: keep the old copyright Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:9: #import <Security/Security.h> On 2016/05/04 03:29:23, tapted wrote: > I think Security.h is actually a C header, so it should be #included. But also I > don't think we need it in the header -- we just need SFCertificatePanel.h and > Cocoa.h Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:12: #include <memory> On 2016/05/04 03:29:23, tapted wrote: > nit: this can be removed since unique_ptr is gone Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:14: #include "base/mac/scoped_nsobject.h" On 2016/05/04 03:29:23, tapted wrote: > nit: import Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:20: namespace net { On 2016/05/04 03:29:23, tapted wrote: > nit: blank line before Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:37: /** Base class for the certificate viewer generated by OSX for Chrome Mac. On 2016/05/04 03:29:23, tapted wrote: > this comment style never took off in Chrome for some reason - we should just put > // on each line- same below, and in other files Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:42: @protected On 2016/05/04 03:29:23, tapted wrote: > These should stay @private. If the derived class needs them, we typically add a > @property(nonatomic, readonly) Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:69: #endif // CHROME_BROWSER_MAC_CERTIFICATE_VIEWER_MAC_H_ On 2016/05/04 03:29:23, tapted wrote: > nit: ensure comment matches #ifdef Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:10: #include <memory.h> On 2016/05/04 03:29:24, tapted wrote: > no .h Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:13: #import "chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.h" On 2016/05/04 03:29:24, tapted wrote: > can this be forward declared as well? Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:18: @interface SSLCertificateViewerCocoa : On 2016/05/04 03:29:24, tapted wrote: > needs a comment Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:19: SSLCertificateViewerMac<ConstrainedWindowSheet> { On 2016/05/04 03:29:24, tapted wrote: > should be able to omit <ConstrainedWindowSheet> since it's inherited. then the > constrained_window_sheet.h #import probably isn't needed Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:23: // A copy of the sheet's frame used to restore on show. On 2016/05/04 03:29:24, tapted wrote: > nit: full stop after `frame`. Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/05/04 03:29:24, tapted wrote: > (c) 2012 -> 2016? Otherwise, can you play with `git cl upload --similarity to > get this to appear as a git-move from certificate_viewer_mac.mm Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:8: #include "base/mac/foundation_util.h" On 2016/05/04 03:29:24, tapted wrote: > nit: import Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:15: explicit SSLCertificateViewerCocoaBridge(SSLCertificateViewerCocoa * On 2016/05/04 03:29:24, tapted wrote: > no space between SSLCertificateViewerCocoa and "*" - also the formatting is > weird - `git cl format`? Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:24: // |onConstrainedWindowClosed| will delete the sheet which might be still On 2016/05/04 03:29:24, tapted wrote: > nit: still -> still be Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:33: SSLCertificateViewerCocoa* controller_; // weak On 2016/05/04 03:29:24, tapted wrote: > nit: // Weak. Owns this. Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_views.h (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.h:5: #ifndef CHROME_BROWSER_UI_VIEWS_MAC_CERTIFICATE_VIEWER_VIEWS_MAC_H_ On 2016/05/04 03:29:24, tapted wrote: > this need to match the path Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.h:17: /** Certificate viewer class for MacViews which handles displaying and closing On 2016/05/04 03:29:24, tapted wrote: > nit: update comment style. more below Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.h:22: std::unique_ptr<views::Widget> dialog_; On 2016/05/04 03:29:24, tapted wrote: > perhaps call this overlayWindow_? Done, have also moved the overlayWindow_ variable in certificate_viewer_mac.* to certificate_viewer_mac_cocoa.*. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.h:23: views::WidgetDelegateView* delegate_; On 2016/05/04 03:29:24, tapted wrote: > is this member needed? - looks like it could be a temporary in > displayForWebContents Done, removed! https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.h:32: - (void)displayForWebContents:(content::WebContents*)webContents; On 2016/05/04 03:29:24, tapted wrote: > ObjC overrides don't need to be redeclared Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:10: #include "ui/views/widget/widget_delegate.cc" On 2016/05/04 03:29:24, tapted wrote: > .cc ? :) Done - oops! https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:12: using namespace web_modal; On 2016/05/04 03:29:24, tapted wrote: > remove? it's uncommon to use `using namespace` except in tests. Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:18: CertificateAnchorWidgetDelegate(content::WebContents* webContents) {} On 2016/05/04 03:29:24, tapted wrote: > the |webContents| argument can be removed Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:19: ui::ModalType GetModalType() const override { return ui::MODAL_TYPE_CHILD; } On 2016/05/04 03:29:25, tapted wrote: > nit: // WidgetDelegate: Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:45: int windowWidth = [webContents->GetTopLevelNativeWindow() frame].size.width; On 2016/05/04 03:29:24, tapted wrote: > nit > > GGFloat windowWidth = NSWidth([webContents->GetTopLevelNativeWindow() frame]); Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:47: dialog_->SetBounds(gfx::Rect(tabViewSize.x(), On 2016/05/04 03:29:24, tapted wrote: > is OnPositionRequiresUpdate() called during construction? Maybe this logic only > needs to be there Done - it isn't, but have added a call to it after showing the widget which fixes this I think. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/05/04 03:29:25, tapted wrote: > call this file native_web_contents_modal_dialog_manager_views_mac.mm? Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:7: #include <Cocoa/Cocoa.h> On 2016/05/04 03:29:25, tapted wrote: > nit: import Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:11: class NativeWebContentsModalDialogManagerViewsMac On 2016/05/04 03:29:25, tapted wrote: > nit: needs a comment Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:22: gfx::Rect originalBounds = widget->GetWindowBoundsInScreen(); On 2016/05/04 03:29:25, tapted wrote: > perhaps just `bounds` since we change it later Removed it entirely. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:23: NativeWebContentsModalDialogManagerViews::OnPositionRequiresUpdate(); On 2016/05/04 03:29:25, tapted wrote: > does this need to be called at all? Yup, the views version of ConstrainedWindow sets the origin of the 1x1 views dialog the sheet is attached to to the middle-top of the webcontents. Then when we resize we need to set the origin to the top-left corner. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:25: // Reset the dialog size to the area under the browser UI to avoid Cocoa On 2016/05/04 03:29:25, tapted wrote: > I don't think this is working right -- I can interact with the web page while > the dialog is showing. Ideally overlayWindow_ intercepts all the clicks, but > that doesn't seem to happen. > > The other weird thing (and I think this is a separate bug, which exists on > ChromeOS too) is that interaction with the DevTools half of the webcontents > isn't blocked - it should be. (but with a proper overlay window sizing to the > tab contents exactly this shouldn't affect Mac). But, still, it's probably a bug > on ChromeOS. Doesn't affect Windows since the certificate viewer is > window-modal. Was this after resizing the browser? I realised the size of the overlay window is only set during construction so maybe if you resize to a larger window the overlay isn't blocking the right area any more. This should be fixed now though so maybe try again? Otherwise you might have to show me. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:35: NSWindow* dialogWindow = widget->GetNativeWindow(); On 2016/05/04 03:29:25, tapted wrote: > dialogWindow -> dialog_window (camelCase only between > @{implementation,interface}/@end). More below Done. Oops, sorry - thanks! https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:36: if (![dialogWindow attachedSheet]) { On 2016/05/04 03:29:25, tapted wrote: > comment about this? (when does it come up?) Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:60: void ChangeVisibilityAndMouseEvents(gfx::NativeWindow window, bool show) { On 2016/05/04 03:29:25, tapted wrote: > perhaps call this > > void SetOverlayVisible(gfx::NativeWindow overlay, bool visible) The overlay should be invisible the whole time, so I've changed this to SetSheetVisible(). Do you think that's a better choice? https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:62: BOOL mouseEvents = show ? NO : YES; On 2016/05/04 03:29:25, tapted wrote: > mouseEvents -> ignore_events Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:64: // Don't allow interaction with the tab underneath the certificate viewer. On 2016/05/04 03:29:25, tapted wrote: > tab -> overlayWindow? Maybe WebContents? The line below is setting the overlayWindow to accept/ignore mouse clicks depending on whether the certificate viewer is opened/closed respectively. I changed it to "tab underneath the overlay" for now. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:67: [[window attachedSheet] setAlphaValue:alpha]; On 2016/05/04 03:29:25, tapted wrote: > does this always hide the sheet? I thought we'd need code for that What do you mean by 'always'? Are there cases other than switching to a different tab and minimising (those are the only two cases I've tested for)? As far as I can tell it should work for all the cases NativeWebContentsModalDialogManagerViews dialogs work for. If you meant the overlay, it'd still be open when hidden as well, just invisible and ignoring mouse clicks (same as the sheet). https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/web_contents_modal_dialog_manager_views_mac_views.mm:71: }; On 2016/05/04 03:29:25, tapted wrote: > DISALLOW_COPY_AND_ASSIGN(..) Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/native_web_modal_manager.cc (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/native_web_modal_manager.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/05/04 03:29:25, tapted wrote: > `(c) 2012` -> `2016` This file is now gone because of a slight refactoring to prevent NativeWebContentsModalDialogManagerViewsMac being used for dialogs other than ones hosting sheets (as mentioned offline). https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:8: #include <set> On 2016/05/04 03:29:25, tapted wrote: > move to header Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:10: #include "base/macros.h" On 2016/05/04 03:29:25, tapted wrote: > move to header Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:14: #include "components/web_modal/web_contents_modal_dialog_host.h" On 2016/05/04 03:29:25, tapted wrote: > make sure nothing in the header includes repeats here Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc:144: // WebContentsModalDialogHostObserver: Really? I've updated it to web_modal::ModalDialogHostObserver in this file and the header file too. Or did you mean remove the comment from the .cc? https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/web_contents_modal_dialog_manager_views.h (right): https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/05/04 03:29:25, tapted wrote: > since this CL is large, it's probably worth splitting out this and > native_web_modal_manager.cc into a separate change: "Move > NativeWebContentsModalDialogManagerViews into its own header". The diff isn't > cooperating at all :/ and OWNERS will appreciate reviewing the purely mechanical > stuff separately Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.h:5: #ifndef CHROME_BROWSER_UI_VIEWS_WEB_CONTENTS_MODAL_DIALOG_MANAGER_VIEWS_H_ On 2016/05/04 03:29:25, tapted wrote: > Hum - this file should get called > native_web_contents_modal_dialog_manager_views.h. The .cc should match, but that > will remove any `diff` ability. But since that's so broken anyway, I think we > should just do it now. Done, updated the ifdefs as well. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.h:10: #include "ui/views/widget/widget.h" On 2016/05/04 03:29:26, tapted wrote: > this can probably be forward-declared Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.h:13: class NativeWebContentsModalDialogManagerViews On 2016/05/04 03:29:25, tapted wrote: > nit: needs a comment Done. https://codereview.chromium.org/1779383002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/web_contents_modal_dialog_manager_views.h:24: // Sets up this object to manage the dialog_. Registers for closing events On 2016/05/04 03:29:25, tapted wrote: > nit: dialog_ -> |dialog_| Done.
https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:38: @interface SSLCertificateViewerMac : NSObject<ConstrainedWindowSheet> { I wonder.. is there anything still preventing moving the <ConstrainedWindowSheet> onto the Cocoa-only subclass? That would let us remove some of the empty methods.. https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:39: // The corresponding list of certificates. @private And actually, the New Way is to move all these to the @implementation instead. Maybe we should do that. If you need access to one of these in a subclass you'll need an accessor method https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (left): https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:89: - (void)sheetDidEnd:(NSWindow*)parent I think we should keep this and make it NOTREACHED - put a prototype in the header. certificate_viewer_mac_views.h doesn't need to redeclare it then, and it makes more sense that the @selector call below has something it can see. https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:88: NOTIMPLEMENTED(); Can these be NOTREACHED() instead? E.g. NOTREACHED(); // Subclasses must override this. https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h (right): https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:30: - (NSWindow*)overlayWindow; can this be removed? - I'm not sure anything calls it... https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm (right): https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:5: #include "chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.h" This should be in the #include block below since we're declaring a new class (presubmit allows it since it thinks you're doing mac-specific stuff for the class declared in this header, which isn't quite right). https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:15: // Class for parenting a Mac Cocoa sheet on a views tab modal dialogs off of a anonymous namespace { before this https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:15: // Class for parenting a Mac Cocoa sheet on a views tab modal dialogs off of a dialogs -> dialog https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:16: // views browser, e.g. for tab-modal Cocoa sheets. Perhaps also mention something like "Since cocoa sheets are modal to the parent window, the sheet is instead parented to an invisible overlay window, which is tab-modal. https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:88: } // namespace https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.h (right): https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.h:65: virtual void ShowWidget(views::Widget* widget); comment for these. E.g. like // By default just calls widget->Show() or Hide(), but allows a derived class to override to hide a different way (e.g. if hiding would tear down attached dialogs too early). https://codereview.chromium.org/1779383002/diff/160001/chrome/chrome_browser_... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1779383002/diff/160001/chrome/chrome_browser_... chrome/chrome_browser_ui.gypi:3168: '../components/web_modal/web_contents_modal_dialog_views.cc' there should be a variable somewhere that this can go instead, so we don't break .gn https://codereview.chromium.org/1779383002/diff/160001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/1779383002/diff/160001/components/constrained... components/constrained_window/constrained_window_views.cc:107: bool parented) { ooh naming this is tough. Maybe `grandchild`? or `use_hidden_overlay` parented is bad since "all" dialogs are parented. Change throughout (but let's discuss a name) https://codereview.chromium.org/1779383002/diff/160001/components/constrained... components/constrained_window/constrained_window_views.cc:116: manager->ShowModalDialog(widget->GetNativeWindow(), parented); I think it's fine to duplicate the lines above, so that we don't need to pass `parented` to ShowModalDialog -- instead have ShowModalDialog and ShowParentedModalDialog. (i.e. remove this helper funciton) https://codereview.chromium.org/1779383002/diff/160001/components/constrained... File components/constrained_window/constrained_window_views.h (right): https://codereview.chromium.org/1779383002/diff/160001/components/constrained... components/constrained_window/constrained_window_views.h:50: // Like ShowWebModalDialogViews, but uses a Mac-specific class for creating put this in #if defined(OS_MACOSX) and I'd go with // Like ShowWebModalDialogViews, but used to show a native dialog "sheet" on Mac. Sheets are always modal to their parent window. To make them tab-modal, this will parent the sheet off an invisible window that is tab-modal. https://codereview.chromium.org/1779383002/diff/160001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.cc (right): https://codereview.chromium.org/1779383002/diff/160001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.cc:42: mgr.reset(CreateParentedNativeWebModalManager(dialog, this)); avoid this on !mac https://codereview.chromium.org/1779383002/diff/160001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.h (right): https://codereview.chromium.org/1779383002/diff/160001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.h:36: static SingleWebContentsDialogManager* CreateParentedNativeWebModalManager( #if defined(OS_MACOSX) for this There should be a comment too, like // Creates a manager for a single dialog parented to an invisible overlay window. https://codereview.chromium.org/1779383002/diff/160001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.h:42: void ShowModalDialog(gfx::NativeWindow dialog, bool parented = false); I think we should overload this instead, so we can hide it on not-Mac https://codereview.chromium.org/1779383002/diff/160001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_views.cc (right): https://codereview.chromium.org/1779383002/diff/160001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_views.cc:13: NOTIMPLEMENTED(); can this be NOTREACHED()? And even though I've said elsewhere that we should avoid compiling bits on Mac, we will still want this to be linked in for the Cocoa browser on Mac (but calling it should be an error).
PTAL, thanks! The trybot presubmit is failing for a reason unrelated to this CL I think. :/ https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:38: @interface SSLCertificateViewerMac : NSObject<ConstrainedWindowSheet> { On 2016/05/13 06:08:46, tapted wrote: > I wonder.. is there anything still preventing moving the > <ConstrainedWindowSheet> onto the Cocoa-only subclass? That would let us remove > some of the empty methods.. Both the MacViews and Cocoa subclasses rely on methods declared in ConstrainedWindowSheet; MacViews needs showSheetForWindow and closeSheetWithAnimation. https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:39: // The corresponding list of certificates. On 2016/05/13 06:08:46, tapted wrote: > @private > > And actually, the New Way is to move all these to the @implementation instead. > Maybe we should do that. > > If you need access to one of these in a subclass you'll need an accessor method Done, I've added a @property for closePending and oldResizesSubviews, and a reset method for panel_ (named deleteSheetWindow to be consistent with ConstrainedWindowSheet.h's naming). https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (left): https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:89: - (void)sheetDidEnd:(NSWindow*)parent On 2016/05/13 06:08:46, tapted wrote: > I think we should keep this and make it NOTREACHED - put a prototype in the > header. certificate_viewer_mac_views.h doesn't need to redeclare it then, and it > makes more sense that the @selector call below has something it can see. Done. https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:88: NOTIMPLEMENTED(); On 2016/05/13 06:08:46, tapted wrote: > Can these be NOTREACHED() instead? E.g. > > NOTREACHED(); // Subclasses must override this. Done. https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h (right): https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:30: - (NSWindow*)overlayWindow; On 2016/05/13 06:08:46, tapted wrote: > can this be removed? - I'm not sure anything calls it... I've left it in because it's called once in a test in certificate_viewer_mac_browsertest.mm. Alternatively we could change it so that the test accesses overlayWindow_ variable instead? https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm (right): https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:5: #include "chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.h" On 2016/05/13 06:08:46, tapted wrote: > This should be in the #include block below since we're declaring a new class > (presubmit allows it since it thinks you're doing mac-specific stuff for the > class declared in this header, which isn't quite right). Done. https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:15: // Class for parenting a Mac Cocoa sheet on a views tab modal dialogs off of a On 2016/05/13 06:08:46, tapted wrote: > anonymous > > namespace { > > before this Done. https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:15: // Class for parenting a Mac Cocoa sheet on a views tab modal dialogs off of a On 2016/05/13 06:08:46, tapted wrote: > dialogs -> dialog Done. https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:16: // views browser, e.g. for tab-modal Cocoa sheets. On 2016/05/13 06:08:46, tapted wrote: > Perhaps also mention something like > > "Since cocoa sheets are modal to the parent window, the sheet is instead > parented to an invisible overlay window, which is tab-modal. Done. https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:88: On 2016/05/13 06:08:46, tapted wrote: > } // namespace Done. https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.h (right): https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.h:65: virtual void ShowWidget(views::Widget* widget); On 2016/05/13 06:08:46, tapted wrote: > comment for these. > > E.g. like > > // By default just calls widget->Show() or Hide(), but allows a derived class to > override to hide a different way (e.g. if hiding would tear down attached > dialogs too early). Done. https://codereview.chromium.org/1779383002/diff/160001/chrome/chrome_browser_... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1779383002/diff/160001/chrome/chrome_browser_... chrome/chrome_browser_ui.gypi:3168: '../components/web_modal/web_contents_modal_dialog_views.cc' On 2016/05/13 06:08:46, tapted wrote: > there should be a variable somewhere that this can go instead, so we don't break > .gn Deleted! This has been moved to web_modal.gypi. https://codereview.chromium.org/1779383002/diff/160001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/1779383002/diff/160001/components/constrained... components/constrained_window/constrained_window_views.cc:107: bool parented) { On 2016/05/13 06:08:46, tapted wrote: > ooh naming this is tough. Maybe `grandchild`? or `use_hidden_overlay` > > parented is bad since "all" dialogs are parented. Change throughout (but let's > discuss a name) Done, changed as discussed offline. https://codereview.chromium.org/1779383002/diff/160001/components/constrained... components/constrained_window/constrained_window_views.cc:116: manager->ShowModalDialog(widget->GetNativeWindow(), parented); On 2016/05/13 06:08:46, tapted wrote: > I think it's fine to duplicate the lines above, so that we don't need to pass > `parented` to ShowModalDialog -- instead have ShowModalDialog and > ShowParentedModalDialog. (i.e. remove this helper funciton) Done. https://codereview.chromium.org/1779383002/diff/160001/components/constrained... File components/constrained_window/constrained_window_views.h (right): https://codereview.chromium.org/1779383002/diff/160001/components/constrained... components/constrained_window/constrained_window_views.h:50: // Like ShowWebModalDialogViews, but uses a Mac-specific class for creating On 2016/05/13 06:08:46, tapted wrote: > put this in #if defined(OS_MACOSX) > > and I'd go with > > // Like ShowWebModalDialogViews, but used to show a native dialog "sheet" on > Mac. Sheets are always modal to their parent window. To make them tab-modal, > this will parent the sheet off an invisible window that is tab-modal. Done. https://codereview.chromium.org/1779383002/diff/160001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.cc (right): https://codereview.chromium.org/1779383002/diff/160001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.cc:42: mgr.reset(CreateParentedNativeWebModalManager(dialog, this)); On 2016/05/13 06:08:46, tapted wrote: > avoid this on !mac Done. https://codereview.chromium.org/1779383002/diff/160001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.h (right): https://codereview.chromium.org/1779383002/diff/160001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.h:36: static SingleWebContentsDialogManager* CreateParentedNativeWebModalManager( On 2016/05/13 06:08:47, tapted wrote: > #if defined(OS_MACOSX) for this > > There should be a comment too, like > > // Creates a manager for a single dialog parented to an invisible overlay > window. Done. https://codereview.chromium.org/1779383002/diff/160001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.h:42: void ShowModalDialog(gfx::NativeWindow dialog, bool parented = false); On 2016/05/13 06:08:46, tapted wrote: > I think we should overload this instead, so we can hide it on not-Mac Done. https://codereview.chromium.org/1779383002/diff/160001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_views.cc (right): https://codereview.chromium.org/1779383002/diff/160001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_views.cc:13: NOTIMPLEMENTED(); On 2016/05/13 06:08:47, tapted wrote: > can this be NOTREACHED()? > > And even though I've said elsewhere that we should avoid compiling bits on Mac, > we will still want this to be linked in for the Cocoa browser on Mac (but > calling it should be an error). Done.
https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:38: @interface SSLCertificateViewerMac : NSObject<ConstrainedWindowSheet> { On 2016/05/18 04:48:37, Patti Lor wrote: > On 2016/05/13 06:08:46, tapted wrote: > > I wonder.. is there anything still preventing moving the > > <ConstrainedWindowSheet> onto the Cocoa-only subclass? That would let us > remove > > some of the empty methods.. > > Both the MacViews and Cocoa subclasses rely on methods declared in > ConstrainedWindowSheet; MacViews needs showSheetForWindow and > closeSheetWithAnimation. Ah!, so I think we can just rename those methods. Then only SSLCertificateViewerCocoa needs to implement the ConstrainedWindowSheet protocol [SSLCertificateViewerCocoa already overrides showSheetForWindow and closeSheetWithAniamtion anyway), and I think we can remove the protocol from the mac_views_browser build entirely. https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h (right): https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:30: - (NSWindow*)overlayWindow; On 2016/05/18 04:48:37, Patti Lor wrote: > On 2016/05/13 06:08:46, tapted wrote: > > can this be removed? - I'm not sure anything calls it... > > I've left it in because it's called once in a test in > certificate_viewer_mac_browsertest.mm. Alternatively we could change it so that > the test accesses overlayWindow_ variable instead? Acknowledged. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:51: // Sets the certificate viewer sheet to null. nit: Releases the native SFCertificatePanel window sheet. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:52: - (void)deleteSheetWindow; nit: releaseSheetWindow (since there's no guarantee there aren't more references that prevent it from actually being deleted) https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:18: BOOL oldResizesSubviews_; Can this move into certificate_viewer_mac_cocoa.mm ? https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:34: - (void)sheetDidEnd:(NSWindow*)parent { this should be the full prototype - (void)sheetDidEnd:(NSWindow*)parent returnCode:(NSInteger)returnCode context:(void*)context; subclasses don't need to redeclare it, just implement it https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:86: - (void)showSheetForWindow:(NSWindow*)window { Rename this method to showCertificateSheet https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:97: - (void)closeSheetWithAnimation:(BOOL)withAnimation { Rename this method to closeCertificateSheet https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:101: [panel_ _dismissWithCode:NSFileHandlingPanelCancelButton]; try a panel_.reset() after this? https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm (right): https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:9: #include "chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h" So... I think we don't want this change. That way, we can re-use these tests on a mac_views_browser build, which we should be doing. I think all these other headers are in both builds, so the only thing preventing us running this test in both cocoa and views builds is the stuff that manipulates SSLCertificateViewerCocoa directly. to run the same tests, you might need to go through WebContentsModalDialogManager::TestApi by reaching into WebContentsModalDialogManager::child_dialogs_ via the test API and grab the gfx::NativeWindow. I think this is a good follow-up though. Can you file a bug for this, and add a TODO in this file. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:61: SSLCertificateViewerCocoa* viewer = e.g. TODO(..): Display the certificate viewer in a cross-toolkit manner and retrieve |sheetWindow| via the WebContentsModalDialogManager::TestApi. See http://crbug.com/XXX https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:67: NSWindow* sheetWindow = [[viewer overlayWindow] attachedSheet]; TODO(..) Remove the overlayWindow accessor from SSLCertificateViewerMac when the overlay is obtained via WebContentsModalDialogManager::TestApi instead. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm (right): https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:96: [self deleteSheetWindow]; Does certificate_viewer_mac_views.mm need to call this too? - see later comment But also this might not be needed at all -- the `[self release]` below should eventually clean it up, but it *might* still be needed in order to "nerf" the panel. But if *that's* needed, I think the panel_.reset() should instead happen at the end of closeCertificateSheet https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_views.h (right): https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. I think we don't need this header -- can it all be moved into the .mm ? https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm (right): https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:31: [self closeSheetWithAnimation:YES]; Does this need to check [self closePending] and return early? (I think it probably doesn't, since there's no ConstrainedWindowController, so nothing else calling closeSheetWithAnimation , in which case we should move closePending into certificate_viewer_mac_cocoa.mm) https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:49: SSLCertificateViewerViewsMac* viewer = So I think this allocation is currently leaked. Either way, it needs a comment about where the allocation is balanced (similar to the cocoa version of this method), because it's not being allocated into a scoped_nsobject. The cocoa dialog is self-releasing in `- (void)onConstrainedWindowClosed` We might need something similar. Alternatively, I think that perhaps CertificateAnchorWidgetDelegate can instead own a scoped_nsobject<SSLCertificateViewerViewsMac>, the widget owns the delegate, so everything is tied to overlayWindow->Close() (overlayWindow_->Close(); should get a comment like // Asynchronously releases |self|.)
On 2016/05/18 04:48:38, Patti Lor wrote: > PTAL, thanks! The trybot presubmit is failing for a reason unrelated to this CL > I think. :/ Ah, I figured out this bit too. Those errors are because your local git config has its upstream branch pointing to https://codereview.chromium.org/1946443002#ps20001, which has some LOG(INFO) statements . You need to either rebase on master, `git cl issue 0` on master, or prevent master from reporting its CLs to rietveld as upstream branches.
Thanks, PTAL. Have CC'd you on the bug for a follow up CL to fix certificate viewer tests. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:51: // Sets the certificate viewer sheet to null. On 2016/05/19 01:53:25, tapted wrote: > nit: Releases the native SFCertificatePanel window sheet. Done. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:52: - (void)deleteSheetWindow; On 2016/05/19 01:53:25, tapted wrote: > nit: releaseSheetWindow > > (since there's no guarantee there aren't more references that prevent it from > actually being deleted) Done. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:18: BOOL oldResizesSubviews_; On 2016/05/19 01:53:25, tapted wrote: > Can this move into certificate_viewer_mac_cocoa.mm ? Done. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:34: - (void)sheetDidEnd:(NSWindow*)parent { On 2016/05/19 01:53:25, tapted wrote: > this should be the full prototype > > - (void)sheetDidEnd:(NSWindow*)parent > returnCode:(NSInteger)returnCode > context:(void*)context; > > subclasses don't need to redeclare it, just implement it Done. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:86: - (void)showSheetForWindow:(NSWindow*)window { On 2016/05/19 01:53:25, tapted wrote: > Rename this method to showCertificateSheet This is a method declared in the ConstrainedWindowSheet protocol (same with closeSheetWithAnimation), so I don't think we can rename it. If you think it's clearer we could make a wrapper for it? https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:97: - (void)closeSheetWithAnimation:(BOOL)withAnimation { On 2016/05/19 01:53:25, tapted wrote: > Rename this method to closeCertificateSheet See comment on showSheetForWindow. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:101: [panel_ _dismissWithCode:NSFileHandlingPanelCancelButton]; On 2016/05/19 01:53:25, tapted wrote: > try a panel_.reset() after this? As discussed offline this doesn't work for the Cocoa build, have added a comment here as to why. Both subclasses now call releaseSheetWindow in separate places. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm (right): https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:9: #include "chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h" On 2016/05/19 01:53:25, tapted wrote: > So... I think we don't want this change. That way, we can re-use these tests on > a mac_views_browser build, which we should be doing. I think all these other > headers are in both builds, so the only thing preventing us running this test in > both cocoa and views builds is the stuff that manipulates > SSLCertificateViewerCocoa directly. > > to run the same tests, you might need to go through > WebContentsModalDialogManager::TestApi by reaching into > WebContentsModalDialogManager::child_dialogs_ via the test API and grab the > gfx::NativeWindow. > > I think this is a good follow-up though. Can you file a bug for this, and add a > TODO in this file. Done. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:61: SSLCertificateViewerCocoa* viewer = On 2016/05/19 01:53:25, tapted wrote: > e.g. TODO(..): Display the certificate viewer in a cross-toolkit manner and > retrieve |sheetWindow| via the WebContentsModalDialogManager::TestApi. See > http://crbug.com/XXX Done. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:67: NSWindow* sheetWindow = [[viewer overlayWindow] attachedSheet]; On 2016/05/19 01:53:25, tapted wrote: > TODO(..) Remove the overlayWindow accessor from SSLCertificateViewerMac when the > overlay is obtained via WebContentsModalDialogManager::TestApi instead. Done. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm (right): https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:96: [self deleteSheetWindow]; On 2016/05/19 01:53:25, tapted wrote: > Does certificate_viewer_mac_views.mm need to call this too? - see later comment > > But also this might not be needed at all -- the `[self release]` below should > eventually clean it up, but it *might* still be needed in order to "nerf" the > panel. But if *that's* needed, I think the panel_.reset() should instead happen > at the end of closeCertificateSheet Have added a call to this for the MacViews version too, thanks for the help offline with this! (See previous comment about why I haven't added it to closeSheetWithAnimation in the superclass.) Also, while looking at this I noticed that despite the comments, this method is never actually called because observer_ is never created. Have fixed this now. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_views.h (right): https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/05/19 01:53:25, tapted wrote: > I think we don't need this header -- can it all be moved into the .mm ? Done. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm (right): https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:31: [self closeSheetWithAnimation:YES]; On 2016/05/19 01:53:25, tapted wrote: > Does this need to check [self closePending] and return early? > > (I think it probably doesn't, since there's no ConstrainedWindowController, so > nothing else calling closeSheetWithAnimation , in which case we should move > closePending into certificate_viewer_mac_cocoa.mm) Done, have moved closePending to the Cocoa version. https://codereview.chromium.org/1779383002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:49: SSLCertificateViewerViewsMac* viewer = On 2016/05/19 01:53:25, tapted wrote: > So I think this allocation is currently leaked. Either way, it needs a comment > about where the allocation is balanced (similar to the cocoa version of this > method), because it's not being allocated into a scoped_nsobject. > > The cocoa dialog is self-releasing in `- (void)onConstrainedWindowClosed` > > We might need something similar. Alternatively, I think that perhaps > CertificateAnchorWidgetDelegate can instead own a > scoped_nsobject<SSLCertificateViewerViewsMac>, the widget owns the delegate, so > everything is tied to overlayWindow->Close() > > (overlayWindow_->Close(); should get a comment like // Asynchronously releases > |self|.) Done, have taken the second approach you suggested!
I think we can still rename - showSheetForWindow/closeSheetWithAnimation but the secret to do it is in a thread on an older patchset https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:38: @interface SSLCertificateViewerMac : NSObject<ConstrainedWindowSheet> { On 2016/05/19 01:53:25, tapted wrote: > On 2016/05/18 04:48:37, Patti Lor wrote: > > On 2016/05/13 06:08:46, tapted wrote: > > > I wonder.. is there anything still preventing moving the > > > <ConstrainedWindowSheet> onto the Cocoa-only subclass? That would let us > > remove > > > some of the empty methods.. > > > > Both the MacViews and Cocoa subclasses rely on methods declared in > > ConstrainedWindowSheet; MacViews needs showSheetForWindow and > > closeSheetWithAnimation. > > Ah!, so I think we can just rename those methods. Then only > SSLCertificateViewerCocoa needs to implement the ConstrainedWindowSheet protocol > [SSLCertificateViewerCocoa already overrides showSheetForWindow and > closeSheetWithAniamtion anyway), and I think we can remove the protocol from the > mac_views_browser build entirely. Did you see this comment from an older patchset? It's the bit that should let you rename showSheetForWindow/closeSheetWithAnimation https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:44: // Closes the certificate viewer Cocoa sheet. mention here that subclasses must implement this https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:101: // build to use |SSLCertificateViewerCocoaBridge::OnConstrainedWindowClosed|, 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? https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h (right): https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:10: #include <memory> no longer needed https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:15: class SSLCertificateViewerCocoaBridge; these are unused now https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm (right): https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:23: - (void)sheetDidEnd:(NSWindow*)parent doesn't need to be redeclared https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:33: CertificateAnchorWidgetDelegate( nit: explicit.. but! The ownership is still a little funny. I think we should delete `displayForWebContent`. Then ShowCertificateViewer(..) should just be { // CertificateAnchorWidgetDelegate is owned by the Widget it creates. new CertificateAnchorWidgetDelegate(web_contents, cert); } CertificateAnchorWidgetDelegate::CertificateAnchorWidgetDelegate(..) : certificate_viewer_([[.. alloc] initWithCertificate:cert]) { [certificate_viewer_ displayForWebContents:web_conents]; overlayWindow_ = ShowWebModalDialogWithOverlayViews(this, web_contents); [certificate_viewer_ showSheetforWindow:overlayWindow_->GetNativeWindow()]; }
Thanks Trent, PTAL! Let me know if what I said about the panel_.reset thing makes sense. https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.h:44: // Closes the certificate viewer Cocoa sheet. On 2016/05/25 06:52:24, tapted wrote: > mention here that subclasses must implement this Done. https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:101: // build to use |SSLCertificateViewerCocoaBridge::OnConstrainedWindowClosed|, On 2016/05/25 06:52:24, tapted wrote: > 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? 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). https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h (right): https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:10: #include <memory> On 2016/05/25 06:52:24, tapted wrote: > no longer needed Done. https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.h:15: class SSLCertificateViewerCocoaBridge; On 2016/05/25 06:52:24, tapted wrote: > these are unused now Done. https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm (right): https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:23: - (void)sheetDidEnd:(NSWindow*)parent On 2016/05/25 06:52:24, tapted wrote: > doesn't need to be redeclared Done. https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:33: CertificateAnchorWidgetDelegate( On 2016/05/25 06:52:24, tapted wrote: > nit: explicit.. but! The ownership is still a little funny. > > I think we should delete `displayForWebContent`. Then ShowCertificateViewer(..) > should just be > > { > // CertificateAnchorWidgetDelegate is owned by the Widget it creates. > new CertificateAnchorWidgetDelegate(web_contents, cert); > } > > CertificateAnchorWidgetDelegate::CertificateAnchorWidgetDelegate(..) > : certificate_viewer_([[.. alloc] initWithCertificate:cert]) { > [certificate_viewer_ displayForWebContents:web_conents]; > overlayWindow_ = ShowWebModalDialogWithOverlayViews(this, web_contents); > [certificate_viewer_ showSheetforWindow:overlayWindow_->GetNativeWindow()]; > } Done.
https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:8: #import "base/mac/scoped_cftyperef.h" nit: include (cf is CoreFoundation, which is pure C, whereas Foundation is Objective C) https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:97: - (void)closeCertificateSheet:(BOOL)withAnimation { |withAnimation| isn't used, so can be removed from the prototype, now that we're decoupled from the protocol. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm (right): https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:11: #import "chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet.h" nit: remove (not needed since the corresponding header includes it) https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:14: class SSLCertificateViewerCocoaBridge : public ConstrainedWindowMacDelegate { wrap this in an anonymous namespace { https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:20: virtual ~SSLCertificateViewerCocoaBridge() {} nit: can this be removed? the compiler-provided default should be fine https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:22: // ConstrainedWindowMacDelegate implementation: nit: // ConstrainedWindowMacDelegate: (the ... implementation: style is just for ObjC these days) https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:37: } // namespace https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:49: - (void)sheetDidEnd:(NSWindow*)parent nit: this should come before // ConstrainedWindowSheet protocol implementation. and preceded by // SSLCertificateViewerMac overrides: https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:56: - (void)displayForWebContents:(content::WebContents*)webContents { So.. since displayForWebContents and onConstrainedWindowClosed are newly introduced "private" interfaces, we should declare those in a private category. This should come before the anonymous namespace that holds SSLCertificateViewerCocoaBridge, and it looks like @interface SSLCertificateViewerCocoa () - (void)displayForWebContents:(content::WebContents*)webContents; - (void)onConstrainedWindowClosed; @end https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:64: - (NSWindow*)overlayWindow { nit: this should be defined first, since it's in the public interface, followed by the private category (displayForWebContents,onConstrainedwindowClosed), then overrides, then protocol impl. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm (right): https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:17: nit: remove vertical whitespace https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:18: - (void)setOverlayWindow:(views::Widget*)overlay; nit: overlay -> overlayWindow https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:26: explicit CertificateAnchorWidgetDelegate(content::WebContents* web_contents, nit: explicit no longer needed https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:48: views::Widget* overlayWindow_; since the interface is only in the .mm file, it's fine to declare this above https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:68: new CertificateAnchorWidgetDelegate(web_contents, cert); ownership comment here, like // Shows a new Widget, which owns the delegate. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm (right): https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:61: [dialog_window setAlphaValue:0.0]; Should this already be the case? (can this line be removed?) https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:67: if (![dialog_window attachedSheet]) { When is this line hit? (comment? or perhaps we can remove) https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:72: // Avoid views::Widget::Show(), as a call to orderOut: on a NSWindow with an nit: ::Show() -> ::Hide() https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.h (right): https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.h:63: web_modal::SingleWebContentsDialogManagerDelegate* native_delegate(); nit: this can be defined inline -- just `return native_delegate_;` here https://codereview.chromium.org/1779383002/diff/300001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/1779383002/diff/300001/components/constrained... components/constrained_window/constrained_window_views.cc:151: views::Widget* ShowWebModalDialogWithOverlayViews( This might be a long shot, but can this entire method be moved to native_web_contents_modal_dialog_manager_views_mac.mm ? If we can do that, we probably don't need to add web_contents_modal_dialog_views.cc at all https://codereview.chromium.org/1779383002/diff/300001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.cc (right): https://codereview.chromium.org/1779383002/diff/300001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.cc:47: std::unique_ptr<SingleWebContentsDialogManager> mgr( Is the compiler still happy if we move these lines into ShowWebModalDialogWithOverlayViews() ? (i.e. call dialog_manager = CreateNativeWebModalOverlayManager(widget->GetNativeWindow(), manager); manager->ShowDialogWithManager(widget->GetNativeWindow(), std::move(dialog_manager))
https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:101: // build to use |SSLCertificateViewerCocoaBridge::OnConstrainedWindowClosed|, On 2016/05/31 06:00:33, Patti Lor wrote: > On 2016/05/25 06:52:24, tapted wrote: > > 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? > > 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). 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
Patchset #18 (id:340001) has been deleted
Patchset #19 (id:380001) has been deleted
Thanks for all the nits! D: See patch set 19 (vs 17) for the change I suggested making to single_web_contents... to allow the panel_.reset() at the end of closeCertificateSheet. Also, I keep getting timeouts running browser_tests, which I thought would have been fixed by the panel.reset() / releaseSheetWindow stuff, but I guess not? Instruments says it's poll_dispatch (from the Heaviest Stack Trace tab), which seems unrelated. Any ideas? https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:101: // build to use |SSLCertificateViewerCocoaBridge::OnConstrainedWindowClosed|, On 2016/06/01 01:12:47, tapted wrote: > On 2016/05/31 06:00:33, Patti Lor wrote: > > On 2016/05/25 06:52:24, tapted wrote: > > > 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? > > > > 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). > > 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 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). https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:8: #import "base/mac/scoped_cftyperef.h" On 2016/05/31 07:44:47, tapted wrote: > nit: include (cf is CoreFoundation, which is pure C, whereas Foundation is > Objective C) Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:97: - (void)closeCertificateSheet:(BOOL)withAnimation { On 2016/05/31 07:44:47, tapted wrote: > |withAnimation| isn't used, so can be removed from the prototype, now that we're > decoupled from the protocol. Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm (right): https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:11: #import "chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet.h" On 2016/05/31 07:44:47, tapted wrote: > nit: remove (not needed since the corresponding header includes it) Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:14: class SSLCertificateViewerCocoaBridge : public ConstrainedWindowMacDelegate { On 2016/05/31 07:44:47, tapted wrote: > wrap this in an anonymous > > namespace { Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:20: virtual ~SSLCertificateViewerCocoaBridge() {} On 2016/05/31 07:44:47, tapted wrote: > nit: can this be removed? the compiler-provided default should be fine Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:22: // ConstrainedWindowMacDelegate implementation: On 2016/05/31 07:44:47, tapted wrote: > nit: // ConstrainedWindowMacDelegate: > > (the ... implementation: style is just for ObjC these days) Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:37: On 2016/05/31 07:44:47, tapted wrote: > } // namespace Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:49: - (void)sheetDidEnd:(NSWindow*)parent On 2016/05/31 07:44:47, tapted wrote: > nit: this should come before // ConstrainedWindowSheet protocol implementation. > and preceded by > > // SSLCertificateViewerMac overrides: Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:56: - (void)displayForWebContents:(content::WebContents*)webContents { On 2016/05/31 07:44:48, tapted wrote: > So.. since displayForWebContents and onConstrainedWindowClosed are newly > introduced "private" interfaces, we should declare those in a private category. > This should come before the anonymous namespace that holds > SSLCertificateViewerCocoaBridge, and it looks like > > @interface SSLCertificateViewerCocoa () > - (void)displayForWebContents:(content::WebContents*)webContents; > - (void)onConstrainedWindowClosed; > @end Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:64: - (NSWindow*)overlayWindow { On 2016/05/31 07:44:47, tapted wrote: > nit: this should be defined first, since it's in the public interface, followed > by the private category (displayForWebContents,onConstrainedwindowClosed), then > overrides, then protocol impl. Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm (right): https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:17: On 2016/05/31 07:44:48, tapted wrote: > nit: remove vertical whitespace Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:18: - (void)setOverlayWindow:(views::Widget*)overlay; On 2016/05/31 07:44:48, tapted wrote: > nit: overlay -> overlayWindow Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:26: explicit CertificateAnchorWidgetDelegate(content::WebContents* web_contents, On 2016/05/31 07:44:48, tapted wrote: > nit: explicit no longer needed Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:48: views::Widget* overlayWindow_; On 2016/05/31 07:44:48, tapted wrote: > since the interface is only in the .mm file, it's fine to declare this above Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:68: new CertificateAnchorWidgetDelegate(web_contents, cert); On 2016/05/31 07:44:48, tapted wrote: > ownership comment here, like > > // Shows a new Widget, which owns the delegate. Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm (right): https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:61: [dialog_window setAlphaValue:0.0]; On 2016/05/31 07:44:48, tapted wrote: > Should this already be the case? (can this line be removed?) Asking for the widget visibility / NSWindow alphaValue here both report that the overlay window is visible, but from manually opening the certificate viewer and looking at it, this line doesn't seem to make a difference either way. I thought I'd put it in to be safe. Do you know what's going on with it? If we keep it it should probably be moved to run before the if statement above as well.. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:67: if (![dialog_window attachedSheet]) { On 2016/05/31 07:44:48, tapted wrote: > When is this line hit? (comment? or perhaps we can remove) Done, I had previously thought this would be hit when the sheet got dismissed but I realised sheetDidEnd would take care of that :O https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:72: // Avoid views::Widget::Show(), as a call to orderOut: on a NSWindow with an On 2016/05/31 07:44:48, tapted wrote: > nit: ::Show() -> ::Hide() Done. https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.h (right): https://codereview.chromium.org/1779383002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/native_web_contents_modal_dialog_manager_views.h:63: web_modal::SingleWebContentsDialogManagerDelegate* native_delegate(); On 2016/05/31 07:44:48, tapted wrote: > nit: this can be defined inline -- just `return native_delegate_;` here Done. https://codereview.chromium.org/1779383002/diff/300001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/1779383002/diff/300001/components/constrained... components/constrained_window/constrained_window_views.cc:151: views::Widget* ShowWebModalDialogWithOverlayViews( On 2016/05/31 07:44:48, tapted wrote: > This might be a long shot, but can this entire method be moved to > native_web_contents_modal_dialog_manager_views_mac.mm ? > > If we can do that, we probably don't need to add > web_contents_modal_dialog_views.cc at all This seems to work? I don't understand it though - I thought Cocoa would complain about not having a method definition for this? Btw, I had to remove the DCHECK to get this to work. https://codereview.chromium.org/1779383002/diff/300001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.cc (right): https://codereview.chromium.org/1779383002/diff/300001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.cc:47: std::unique_ptr<SingleWebContentsDialogManager> mgr( On 2016/05/31 07:44:48, tapted wrote: > Is the compiler still happy if we move these lines into > ShowWebModalDialogWithOverlayViews() ? (i.e. call > > dialog_manager = CreateNativeWebModalOverlayManager(widget->GetNativeWindow(), > manager); > manager->ShowDialogWithManager(widget->GetNativeWindow(), > std::move(dialog_manager)) > Done.
This is pretty close! Let's discuss the panel.reset() / releaseSheetWindow thing at your desk https://codereview.chromium.org/1779383002/diff/300001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/1779383002/diff/300001/components/constrained... components/constrained_window/constrained_window_views.cc:151: views::Widget* ShowWebModalDialogWithOverlayViews( On 2016/06/06 06:52:46, Patti Lor wrote: > On 2016/05/31 07:44:48, tapted wrote: > > This might be a long shot, but can this entire method be moved to > > native_web_contents_modal_dialog_manager_views_mac.mm ? > > > > If we can do that, we probably don't need to add > > web_contents_modal_dialog_views.cc at all > > This seems to work? I don't understand it though - I thought Cocoa would > complain about not having a method definition for this? > > Btw, I had to remove the DCHECK to get this to work. Yeah, it's "fine" to provide the function prototype on all builds. If a particular build doesn't invoke the function, then the linker doesn't care if no definition is provided. https://codereview.chromium.org/1779383002/diff/360001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm (right): https://codereview.chromium.org/1779383002/diff/360001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:52: - (void)sheetDidEnd:(NSWindow*)parent nit: // SSLCertificateViewerMac overrides: https://codereview.chromium.org/1779383002/diff/360001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm:69: // Shows a new widget, which owns the delegate. nit: widget -> Widget https://codereview.chromium.org/1779383002/diff/400001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm (right): https://codereview.chromium.org/1779383002/diff/400001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:121: WebContentsModalDialogManager::CreateNativeWebModalOverlayManager( I think we can just replace this with `new NativeWebContentsModalDialogManagerViewsMac..` and avoid changing things under web_modal https://codereview.chromium.org/1779383002/diff/400001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm (right): https://codereview.chromium.org/1779383002/diff/400001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm:90: return dialog_; yeah - let's try to avoid this change https://codereview.chromium.org/1779383002/diff/400001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/1779383002/diff/400001/components/constrained... components/constrained_window/constrained_window_views.cc:146: manager->ShowModalDialog(widget->GetNativeWindow()); revert this change - since we're not changing anything else in this file https://codereview.chromium.org/1779383002/diff/400001/components/constrained... File components/constrained_window/constrained_window_views.h (right): https://codereview.chromium.org/1779383002/diff/400001/components/constrained... components/constrained_window/constrained_window_views.h:53: // this will parent the sheet off an invisible window that is tab-modal. this.. -> this provides an invisible tab-modal overlay window managed by WebContentsModalDialogManager, which can host a dialog sheet. https://codereview.chromium.org/1779383002/diff/400001/components/web_modal/B... File components/web_modal/BUILD.gn (right): https://codereview.chromium.org/1779383002/diff/400001/components/web_modal/B... components/web_modal/BUILD.gn:5: import("//build/config/ui.gni") what was this needed for? https://codereview.chromium.org/1779383002/diff/400001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.h (right): https://codereview.chromium.org/1779383002/diff/400001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.h:43: static SingleWebContentsDialogManager* CreateNativeWebModalOverlayManager( we can remove this once it's inlined in ShowModalDialogWithOverlayViews https://codereview.chromium.org/1779383002/diff/400001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.h:48: void ShowModalDialogWithOverlay(gfx::NativeWindow dialog); remove - no longer exists
PTAL, thank you! https://codereview.chromium.org/1779383002/diff/400001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm (right): https://codereview.chromium.org/1779383002/diff/400001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:121: WebContentsModalDialogManager::CreateNativeWebModalOverlayManager( On 2016/06/07 06:10:02, tapted wrote: > I think we can just replace this with `new > NativeWebContentsModalDialogManagerViewsMac..` and avoid changing things under > web_modal Done. https://codereview.chromium.org/1779383002/diff/400001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm (right): https://codereview.chromium.org/1779383002/diff/400001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/single_web_contents_dialog_manager_cocoa.mm:90: return dialog_; On 2016/06/07 06:10:02, tapted wrote: > yeah - let's try to avoid this change Done. https://codereview.chromium.org/1779383002/diff/400001/components/constrained... File components/constrained_window/constrained_window_views.cc (right): https://codereview.chromium.org/1779383002/diff/400001/components/constrained... components/constrained_window/constrained_window_views.cc:146: manager->ShowModalDialog(widget->GetNativeWindow()); On 2016/06/07 06:10:02, tapted wrote: > revert this change - since we're not changing anything else in this file Done. https://codereview.chromium.org/1779383002/diff/400001/components/constrained... File components/constrained_window/constrained_window_views.h (right): https://codereview.chromium.org/1779383002/diff/400001/components/constrained... components/constrained_window/constrained_window_views.h:53: // this will parent the sheet off an invisible window that is tab-modal. On 2016/06/07 06:10:02, tapted wrote: > this.. -> this provides an invisible tab-modal overlay window managed by > WebContentsModalDialogManager, which can host a dialog sheet. Done. https://codereview.chromium.org/1779383002/diff/400001/components/web_modal/B... File components/web_modal/BUILD.gn (right): https://codereview.chromium.org/1779383002/diff/400001/components/web_modal/B... components/web_modal/BUILD.gn:5: import("//build/config/ui.gni") On 2016/06/07 06:10:03, tapted wrote: > what was this needed for? Oops, reverted this whole file. https://codereview.chromium.org/1779383002/diff/400001/components/web_modal/w... File components/web_modal/web_contents_modal_dialog_manager.h (right): https://codereview.chromium.org/1779383002/diff/400001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.h:43: static SingleWebContentsDialogManager* CreateNativeWebModalOverlayManager( On 2016/06/07 06:10:03, tapted wrote: > we can remove this once it's inlined in ShowModalDialogWithOverlayViews Done. https://codereview.chromium.org/1779383002/diff/400001/components/web_modal/w... components/web_modal/web_contents_modal_dialog_manager.h:48: void ShowModalDialogWithOverlay(gfx::NativeWindow dialog); On 2016/06/07 06:10:03, tapted wrote: > remove - no longer exists Done.
tapted@chromium.org changed reviewers: + msw@chromium.org
lgtm, but +msw for OWNERS and thoughts on the comment below. https://codereview.chromium.org/1779383002/diff/420001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm (right): https://codereview.chromium.org/1779383002/diff/420001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:98: views::Widget* ShowWebModalDialogWithOverlayViews( so, having this here (under chrome/browser/ui, rather than components/constrained_window) is the main thing I'm unsure about. But it matches what's done by chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc with its definition of WebContentsModalDialogManager::CreateNativeWebModalManager(). To fix this properly, I *think* the approach should be to move this, and NativeWebContentsModalDialogManagerViews (and maybe some other stuff) into components/constrained_window . But there might be a good reason why that hasn't been done already. Of course components/constrained_window/DEPS would need to update, but I think it would be a matter of merging it with components/web_modal/DEPS and some simple refactoring (e.g. chrome/browser/platform_util.h should go somewhere since components/* can't have a chrome dependency). Another, simpler, option might be to move the declaration of ShowWebModalDialogWithOverlayViews out of constrained_window and into a new header file (e.g. this file's header). Then just include that directly in chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm. That sorta hides the issue, but it might be preferred by OWNERS. Also I think it's "nice" to keep: ShowWebModalDialogWithOverlayViews() and ShowWebModalDialogViews() in the same header file.
https://codereview.chromium.org/1779383002/diff/420001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm (right): https://codereview.chromium.org/1779383002/diff/420001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:98: views::Widget* ShowWebModalDialogWithOverlayViews( On 2016/06/09 04:40:00, tapted wrote: > so, having this here (under chrome/browser/ui, rather than > components/constrained_window) is the main thing I'm unsure about. But it > matches what's done by > chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc with its > definition of WebContentsModalDialogManager::CreateNativeWebModalManager(). > > To fix this properly, I *think* the approach should be to move this, and > NativeWebContentsModalDialogManagerViews (and maybe some other stuff) into > components/constrained_window . But there might be a good reason why that hasn't > been done already. Of course components/constrained_window/DEPS would need to > update, but I think it would be a matter of merging it with > components/web_modal/DEPS and some simple refactoring (e.g. > chrome/browser/platform_util.h should go somewhere since components/* can't have > a chrome dependency). > > Another, simpler, option might be to move the declaration of > ShowWebModalDialogWithOverlayViews out of constrained_window and into a new > header file (e.g. this file's header). Then just include that directly in > chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm. That sorta hides the > issue, but it might be preferred by OWNERS. Also I think it's "nice" to keep: > > ShowWebModalDialogWithOverlayViews() > and > ShowWebModalDialogViews() > > in the same header file. I agree, this is odd; these managers/impls all belong in the component directories. Afaict, platform_util::GetParent is the only chrome dep blocking us from moving NativeWebContentsModalDialogManagerViews (and then the rest). Since the calls to platform_util::GetParent are in "#if defined(USE_AURA)" blocks, we should just be able to replace them with widget->GetNativeWindow()->parent(). Would you mind doing that here, or in a precursor to this CL?
Patchset #21 (id:440001) has been deleted
Description was changed from ========== MacViews: Remove constrained window dependencies for certificate viewer. Move constrained_window Mac dependencies to a Cocoa build only. To make the MacViews browser's certificate viewer work, refactor out common code taken from the original certificate viewer and subclass into SSLCertificateViewerCocoa and SSLCertificateViewerMacViews. SingleWebContentsDialogManagerViewsMac has also been added to handle Cocoa dialogs parented off a views browser. BUG=587239 ========== to ========== MacViews: Remove constrained window dependencies for certificate viewer. Move constrained_window Mac dependencies to a Cocoa build only. To make the MacViews browser's certificate viewer work, refactor out common code taken from the original certificate viewer and subclass into SSLCertificateViewerCocoa and SSLCertificateViewerMacViews. The MacViews certificate viewer will be created via a new constrained_window method ShowWebModalDialogWithOverlayViews(). Additionally, move native_web_contents_modal_dialog_manager_views* into the components/constrained_window folder. BUG=587239 ==========
Patchset #21 (id:460001) has been deleted
Patchset #21 (id:480001) has been deleted
Patchset #21 (id:500001) has been deleted
Patchset #21 (id:520001) has been deleted
Patchset #21 (id:540001) has been deleted
Patchset #22 (id:570018) has been deleted
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== MacViews: Remove constrained window dependencies for certificate viewer. Move constrained_window Mac dependencies to a Cocoa build only. To make the MacViews browser's certificate viewer work, refactor out common code taken from the original certificate viewer and subclass into SSLCertificateViewerCocoa and SSLCertificateViewerMacViews. The MacViews certificate viewer will be created via a new constrained_window method ShowWebModalDialogWithOverlayViews(). Additionally, move native_web_contents_modal_dialog_manager_views* into the components/constrained_window folder. BUG=587239 ========== to ========== MacViews: Remove constrained window dependencies for certificate viewer. Move constrained_window Mac dependencies to a Cocoa build only. To make the MacViews browser's certificate viewer work, refactor out common code taken from the original certificate viewer and subclass into SSLCertificateViewerCocoa and SSLCertificateViewerMacViews. The MacViews certificate viewer will be created via a new constrained_window method ShowWebModalDialogWithOverlayViews(). Similarly, NativeWebContentsModalDialogManagerViews is subclassed into a MacViews specific NativeWebContentsModalDialogManagerViewsMac, which handles hiding/showing the certificate viewer. Note that the certificate viewer is not the same as the Views one because OSX has its own, which is why there is special code for it needed here. BUG=587239 ==========
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Hi msw@, PTAL - this is the Mac certificate viewer CL from ages ago that was waiting on https://codereview.chromium.org/2087643003/ to land. The presubmit is failing because of a #include line that is too long wrapped inside a #ifdef (OS_MACOSX) - not sure what to do about that. Thanks very much! https://codereview.chromium.org/1779383002/diff/420001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm (right): https://codereview.chromium.org/1779383002/diff/420001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/native_web_contents_modal_dialog_manager_views_mac.mm:98: views::Widget* ShowWebModalDialogWithOverlayViews( On 2016/06/09 17:36:30, msw wrote: > On 2016/06/09 04:40:00, tapted wrote: > > so, having this here (under chrome/browser/ui, rather than > > components/constrained_window) is the main thing I'm unsure about. But it > > matches what's done by > > chrome/browser/ui/views/web_contents_modal_dialog_manager_views.cc with its > > definition of WebContentsModalDialogManager::CreateNativeWebModalManager(). > > > > To fix this properly, I *think* the approach should be to move this, and > > NativeWebContentsModalDialogManagerViews (and maybe some other stuff) into > > components/constrained_window . But there might be a good reason why that > hasn't > > been done already. Of course components/constrained_window/DEPS would need to > > update, but I think it would be a matter of merging it with > > components/web_modal/DEPS and some simple refactoring (e.g. > > chrome/browser/platform_util.h should go somewhere since components/* can't > have > > a chrome dependency). > > > > Another, simpler, option might be to move the declaration of > > ShowWebModalDialogWithOverlayViews out of constrained_window and into a new > > header file (e.g. this file's header). Then just include that directly in > > chrome/browser/ui/cocoa/certificate_viewer_mac_views.mm. That sorta hides the > > issue, but it might be preferred by OWNERS. Also I think it's "nice" to keep: > > > > ShowWebModalDialogWithOverlayViews() > > and > > ShowWebModalDialogViews() > > > > in the same header file. > > I agree, this is odd; these managers/impls all belong in the component > directories. Afaict, platform_util::GetParent is the only chrome dep blocking us > from moving NativeWebContentsModalDialogManagerViews (and then the rest). Since > the calls to platform_util::GetParent are in "#if defined(USE_AURA)" blocks, we > should just be able to replace them with widget->GetNativeWindow()->parent(). > Would you mind doing that here, or in a precursor to this CL? Rebased on top of https://codereview.chromium.org/2087643003/, which both of you have LGTMed :)
I only have minor nits/comments, mostly because I'm not a great cocoa reviewer. I know Trent approved this CL before the rebase, but could you ping another Cocoa reviewer (maybe ellyjones or rsesek?) to take a fresh look; or wait until Trent is back? I can really only rubber stamp lgtm this with my sub-par Mac/Cocoa review skills. https://codereview.chromium.org/1779383002/diff/650001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/650001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:33: NOTREACHED(); // Subclasses must implement this. q: could you make this pure virtual instead? https://codereview.chromium.org/1779383002/diff/650001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:78: - (void)releaseSheetWindow { nit: reorder to match decl https://codereview.chromium.org/1779383002/diff/650001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:82: - (NSWindow*)certificatePanel { nit: reorder to match decl https://codereview.chromium.org/1779383002/diff/650001/chrome/chrome_browser_... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1779383002/diff/650001/chrome/chrome_browser_... chrome/chrome_browser_ui.gypi:2587: 'browser/ui/cocoa/certificate_viewer_mac_views.mm', should this be moved to browser/ui/views? https://codereview.chromium.org/1779383002/diff/650001/components/constrained... File components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.h (right): https://codereview.chromium.org/1779383002/diff/650001/components/constrained... components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.h:23: : NativeWebContentsModalDialogManagerViews(dialog, native_delegate) {} nit: this should be defined in the .mm file, right? https://codereview.chromium.org/1779383002/diff/650001/components/constrained... File components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.mm (right): https://codereview.chromium.org/1779383002/diff/650001/components/constrained... components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.mm:29: // area under the chrome UI. The origin of the dialog then also needs to be nit: capitalize Chrome https://codereview.chromium.org/1779383002/diff/650001/components/constrained... components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.mm:33: NSWidth([web_contents->GetTopLevelNativeWindow() frame]); q: could this just use the web_contents->GetContainerBounds() width? https://codereview.chromium.org/1779383002/diff/650001/components/constrained... components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.mm:69: void NativeWebContentsModalDialogManagerViewsMac::SetSheetVisible( Could this just be a file-local anon-namespace helper? (ie. not a class member?)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... File chrome/browser/ui/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: no (c) in new files, and it's 2016 https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:19: @interface SFCertificatePanel (SystemPrivate) This should be in the .mm file if nobody else needs to use it. https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:32: // Base class for the certificate viewer generated by OSX for Chrome Mac. nit: "macOS" instead of "OSX" now https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:39: - (id)initWithCertificate:(net::X509Certificate*)certificate; instancetype for new code is preferred https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:48: - (void)displayForWebContents:(content::WebContents*)webContents; I'm a little confused by the API here, between the difference of dispalyForWebContents: and showCertificateSheet:. https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:57: - (void)releaseSheetWindow; Why not just have the sheet released when closed? https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... File chrome/browser/ui/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.mm:22: base::ScopedCFTypeRef<CFArrayRef> cert_chain( naming: certChain https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.mm:59: SecPolicyRef basic_policy = NULL; naming: basicPolicy https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.mm:75: [panel_ setPolicies:(id)policies.get()]; CFToNSCast again? https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/view... File chrome/browser/ui/views/certificate_viewer_mac_views.mm (right): https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/view... chrome/browser/ui/views/certificate_viewer_mac_views.mm:17: @private Skip @private since this is in a .mm (like you did in the other files).
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #27 (id:690001) has been deleted
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Thanks for the reviews msw@ and rsesek@, PTAL! To rsesek@ specifically: Could I also get your opinion on the file move described in https://codereview.chromium.org/1779383002/diff/650001/chrome/chrome_browser_... ? Thanks again! https://codereview.chromium.org/1779383002/diff/650001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/650001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:33: NOTREACHED(); // Subclasses must implement this. On 2016/08/22 23:01:18, msw wrote: > q: could you make this pure virtual instead? There isn't a good way to enforce this in Objective C - I believe it is possible with protocols which are like Java interfaces, but abstract classes aren't a thing (can't have classes with only some methods implemented). https://codereview.chromium.org/1779383002/diff/650001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:78: - (void)releaseSheetWindow { On 2016/08/22 23:01:18, msw wrote: > nit: reorder to match decl Done. https://codereview.chromium.org/1779383002/diff/650001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac.mm:82: - (NSWindow*)certificatePanel { On 2016/08/22 23:01:18, msw wrote: > nit: reorder to match decl Done. https://codereview.chromium.org/1779383002/diff/650001/chrome/chrome_browser_... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1779383002/diff/650001/chrome/chrome_browser_... chrome/chrome_browser_ui.gypi:2587: 'browser/ui/cocoa/certificate_viewer_mac_views.mm', On 2016/08/22 23:01:18, msw wrote: > should this be moved to browser/ui/views? Oh, that's a good point. I'll let rsesek@ make the final decision on this, but I think I agree with you. It's weird though, I feel like if that's the case, where should certificate_viewer_mac.{h,mm} go? Since it's not strictly for a Cocoa build. Have made the move for now, along with certificate_viewer_mac.{h,mm} into chrome/browser/ui/). https://codereview.chromium.org/1779383002/diff/650001/components/constrained... File components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.h (right): https://codereview.chromium.org/1779383002/diff/650001/components/constrained... components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.h:23: : NativeWebContentsModalDialogManagerViews(dialog, native_delegate) {} On 2016/08/22 23:01:19, msw wrote: > nit: this should be defined in the .mm file, right? Done. https://codereview.chromium.org/1779383002/diff/650001/components/constrained... File components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.mm (right): https://codereview.chromium.org/1779383002/diff/650001/components/constrained... components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.mm:29: // area under the chrome UI. The origin of the dialog then also needs to be On 2016/08/22 23:01:19, msw wrote: > nit: capitalize Chrome Done. https://codereview.chromium.org/1779383002/diff/650001/components/constrained... components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.mm:33: NSWidth([web_contents->GetTopLevelNativeWindow() frame]); On 2016/08/22 23:01:19, msw wrote: > q: could this just use the web_contents->GetContainerBounds() width? The width of the WebContents isn't what we want since it doesn't include the width of the DevTools panel if it is docked. The certificate viewer ends up opening horizontally centred on the WebContents and not the horizontal centre of the whole window, plus you're still able to interact with the DevTools panel while the certificate viewer is opened since it doesn't cover that part of the screen. https://codereview.chromium.org/1779383002/diff/650001/components/constrained... components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.mm:69: void NativeWebContentsModalDialogManagerViewsMac::SetSheetVisible( On 2016/08/22 23:01:19, msw wrote: > Could this just be a file-local anon-namespace helper? (ie. not a class member?) Done. https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... File chrome/browser/ui/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2016/08/24 18:42:40, Robert Sesek wrote: > nit: no (c) in new files, and it's 2016 These files are actually from chrome/browser/ui/cocoa/certificate_viewer_mac.{h,mm}, the diff isn't showing up properly I think because there were so many changes + file location moved (diff is still there for patch set 25 if you want to have a look). https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:19: @interface SFCertificatePanel (SystemPrivate) On 2016/08/24 18:42:41, Robert Sesek wrote: > This should be in the .mm file if nobody else needs to use it. Done. https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:32: // Base class for the certificate viewer generated by OSX for Chrome Mac. On 2016/08/24 18:42:41, Robert Sesek wrote: > nit: "macOS" instead of "OSX" now Done. https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:39: - (id)initWithCertificate:(net::X509Certificate*)certificate; On 2016/08/24 18:42:41, Robert Sesek wrote: > instancetype for new code is preferred Done. https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:48: - (void)displayForWebContents:(content::WebContents*)webContents; On 2016/08/24 18:42:41, Robert Sesek wrote: > I'm a little confused by the API here, between the difference of > dispalyForWebContents: and showCertificateSheet:. Yeah, it could probably be named better. showCertificateSheet is the one that actually shows anything (displayForWebContents just creates an appropriate SFCertificatePanel for the given webContents). I ended up merging the two methods because they were all called directly one after the other. New name is initWithCertificate:andWebContents:. https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:57: - (void)releaseSheetWindow; On 2016/08/24 18:42:41, Robert Sesek wrote: > Why not just have the sheet released when closed? Cocoa and MacViews need to call this at different times - you could do this for MacViews, but the Cocoa build's certificate viewer is closed inside SingleWebContentsDialogManagerCocoa::Close(), which expects |panel_| to not be null (see line 64 in single_web_contents_dialog_manager_cocoa.mm). Calling it in closeCertificateSheet makes Close() constantly call WillClose() until you kill Chrome manually. See https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... for a bit more info on this. https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... File chrome/browser/ui/certificate_viewer_mac.mm (right): https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.mm:22: base::ScopedCFTypeRef<CFArrayRef> cert_chain( On 2016/08/24 18:42:41, Robert Sesek wrote: > naming: certChain Done. https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.mm:59: SecPolicyRef basic_policy = NULL; On 2016/08/24 18:42:41, Robert Sesek wrote: > naming: basicPolicy Done. https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.mm:75: [panel_ setPolicies:(id)policies.get()]; On 2016/08/24 18:42:41, Robert Sesek wrote: > CFToNSCast again? Done. https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/view... File chrome/browser/ui/views/certificate_viewer_mac_views.mm (right): https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/view... chrome/browser/ui/views/certificate_viewer_mac_views.mm:17: @private On 2016/08/24 18:42:41, Robert Sesek wrote: > Skip @private since this is in a .mm (like you did in the other files). Done.
https://codereview.chromium.org/1779383002/diff/650001/components/constrained... File components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.mm (right): https://codereview.chromium.org/1779383002/diff/650001/components/constrained... components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.mm:33: NSWidth([web_contents->GetTopLevelNativeWindow() frame]); On 2016/08/25 08:25:12, Patti Lor wrote: > On 2016/08/22 23:01:19, msw wrote: > > q: could this just use the web_contents->GetContainerBounds() width? > > The width of the WebContents isn't what we want since it doesn't include the > width of the DevTools panel if it is docked. The certificate viewer ends up > opening horizontally centred on the WebContents and not the horizontal centre of > the whole window, plus you're still able to interact with the DevTools panel > while the certificate viewer is opened since it doesn't cover that part of the > screen. Please add a short comment to that effect. (include DevTools bounds)
https://codereview.chromium.org/1779383002/diff/710001/chrome/chrome_browser_... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1779383002/diff/710001/chrome/chrome_browser_... chrome/chrome_browser_ui.gypi:501: 'browser/ui/webui/offline/offline_internals_ui.cc', Make sure you sync and rebase this file properly; it looks like some tangential changes crept in?
https://codereview.chromium.org/1779383002/diff/650001/chrome/chrome_browser_... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1779383002/diff/650001/chrome/chrome_browser_... chrome/chrome_browser_ui.gypi:2587: 'browser/ui/cocoa/certificate_viewer_mac_views.mm', On 2016/08/25 08:25:12, Patti Lor wrote: > On 2016/08/22 23:01:18, msw wrote: > > should this be moved to browser/ui/views? > > Oh, that's a good point. I'll let rsesek@ make the final decision on this, but I > think I agree with you. It's weird though, I feel like if that's the case, where > should certificate_viewer_mac.{h,mm} go? Since it's not strictly for a Cocoa > build. > > Have made the move for now, along with certificate_viewer_mac.{h,mm} into > chrome/browser/ui/). Hm, I don't really have a strong opinion. I'd defer to Trent, but he's out. Maybe ask Elly? https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... File chrome/browser/ui/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2016/08/25 08:25:12, Patti Lor wrote: > On 2016/08/24 18:42:40, Robert Sesek wrote: > > nit: no (c) in new files, and it's 2016 > > These files are actually from > chrome/browser/ui/cocoa/certificate_viewer_mac.{h,mm}, the diff isn't showing up > properly I think because there were so many changes + file location moved (diff > is still there for patch set 25 if you want to have a look). I'd still probably update it, but not required. https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:57: - (void)releaseSheetWindow; On 2016/08/25 08:25:12, Patti Lor wrote: > On 2016/08/24 18:42:41, Robert Sesek wrote: > > Why not just have the sheet released when closed? > > Cocoa and MacViews need to call this at different times - you could do this for > MacViews, but the Cocoa build's certificate viewer is closed inside > SingleWebContentsDialogManagerCocoa::Close(), which expects |panel_| to not be > null (see line 64 in single_web_contents_dialog_manager_cocoa.mm). Calling it in > closeCertificateSheet makes Close() constantly call WillClose() until you kill > Chrome manually. > > See > https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... > for a bit more info on this. OK, I think more of this should be documented in the header (see other comment). https://codereview.chromium.org/1779383002/diff/710001/chrome/browser/ui/cert... File chrome/browser/ui/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/710001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:29: // Closes the certificate viewer Cocoa sheet. Subclasses must implement this. I'd maybe move this into a |@interface SSLCertificateViewerMac (Subclassing)| or |@interface SSLCertificateViewerMac (Protected)| to separate out the public interface from that of the implementer's. https://codereview.chromium.org/1779383002/diff/710001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:37: // Closes the certificate viewer sheet. Note that this doesn't release the window. https://codereview.chromium.org/1779383002/diff/710001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:40: // Releases the native SFCertificatePanel window sheet. When is the window window created? (Document at the method in which it is). https://codereview.chromium.org/1779383002/diff/710001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm (right): https://codereview.chromium.org/1779383002/diff/710001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:58: self = [super initWithCertificate:certificate forWebContents:webContents]; if ((self = ...)) { // other initialization }
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
patricialor@chromium.org changed reviewers: + ellyjones@chromium.org
Hi rsesek@, please take another look, thanks! +ellyjones@ - Could you take a quick look at https://codereview.chromium.org/1779383002/diff/650001/chrome/chrome_browser_... and see what you think? Basically the question is where to put the files containing SSLCertificateViewerMac and its two subclasses (one for MacViews, one for Cocoa). Thanks! https://codereview.chromium.org/1779383002/diff/650001/components/constrained... File components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.mm (right): https://codereview.chromium.org/1779383002/diff/650001/components/constrained... components/constrained_window/native_web_contents_modal_dialog_manager_views_mac.mm:33: NSWidth([web_contents->GetTopLevelNativeWindow() frame]); On 2016/08/25 15:30:47, msw wrote: > On 2016/08/25 08:25:12, Patti Lor wrote: > > On 2016/08/22 23:01:19, msw wrote: > > > q: could this just use the web_contents->GetContainerBounds() width? > > > > The width of the WebContents isn't what we want since it doesn't include the > > width of the DevTools panel if it is docked. The certificate viewer ends up > > opening horizontally centred on the WebContents and not the horizontal centre > of > > the whole window, plus you're still able to interact with the DevTools panel > > while the certificate viewer is opened since it doesn't cover that part of the > > screen. > > Please add a short comment to that effect. (include DevTools bounds) Done. https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... File chrome/browser/ui/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2016/08/25 21:32:38, Robert Sesek wrote: > On 2016/08/25 08:25:12, Patti Lor wrote: > > On 2016/08/24 18:42:40, Robert Sesek wrote: > > > nit: no (c) in new files, and it's 2016 > > > > These files are actually from > > chrome/browser/ui/cocoa/certificate_viewer_mac.{h,mm}, the diff isn't showing > up > > properly I think because there were so many changes + file location moved > (diff > > is still there for patch set 25 if you want to have a look). > > I'd still probably update it, but not required. Acknowledged. https://codereview.chromium.org/1779383002/diff/670001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:57: - (void)releaseSheetWindow; On 2016/08/25 21:32:39, Robert Sesek wrote: > On 2016/08/25 08:25:12, Patti Lor wrote: > > On 2016/08/24 18:42:41, Robert Sesek wrote: > > > Why not just have the sheet released when closed? > > > > Cocoa and MacViews need to call this at different times - you could do this > for > > MacViews, but the Cocoa build's certificate viewer is closed inside > > SingleWebContentsDialogManagerCocoa::Close(), which expects |panel_| to not be > > null (see line 64 in single_web_contents_dialog_manager_cocoa.mm). Calling it > in > > closeCertificateSheet makes Close() constantly call WillClose() until you kill > > Chrome manually. > > > > See > > > https://codereview.chromium.org/1779383002/diff/280001/chrome/browser/ui/coco... > > for a bit more info on this. > > OK, I think more of this should be documented in the header (see other comment). Acknowledged. https://codereview.chromium.org/1779383002/diff/710001/chrome/browser/ui/cert... File chrome/browser/ui/certificate_viewer_mac.h (right): https://codereview.chromium.org/1779383002/diff/710001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:29: // Closes the certificate viewer Cocoa sheet. Subclasses must implement this. On 2016/08/25 21:32:39, Robert Sesek wrote: > I'd maybe move this into a |@interface SSLCertificateViewerMac (Subclassing)| or > |@interface SSLCertificateViewerMac (Protected)| to separate out the public > interface from that of the implementer's. Done. https://codereview.chromium.org/1779383002/diff/710001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:37: // Closes the certificate viewer sheet. On 2016/08/25 21:32:39, Robert Sesek wrote: > Note that this doesn't release the window. Done. https://codereview.chromium.org/1779383002/diff/710001/chrome/browser/ui/cert... chrome/browser/ui/certificate_viewer_mac.h:40: // Releases the native SFCertificatePanel window sheet. On 2016/08/25 21:32:39, Robert Sesek wrote: > When is the window window created? (Document at the method in which it is). Done. https://codereview.chromium.org/1779383002/diff/710001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm (right): https://codereview.chromium.org/1779383002/diff/710001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/certificate_viewer_mac_cocoa.mm:58: self = [super initWithCertificate:certificate forWebContents:webContents]; On 2016/08/25 21:32:39, Robert Sesek wrote: > if ((self = ...)) { > // other initialization > } Done. https://codereview.chromium.org/1779383002/diff/710001/chrome/chrome_browser_... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1779383002/diff/710001/chrome/chrome_browser_... chrome/chrome_browser_ui.gypi:501: 'browser/ui/webui/offline/offline_internals_ui.cc', On 2016/08/25 15:35:22, msw wrote: > Make sure you sync and rebase this file properly; it looks like some tangential > changes crept in? No worries, it isn't from a bad rebase. The reordering is from the tools/git/move_source_file.py script which I used to move the certificate_viewer_* files. It also re-sorts all the gyp files containing entries for the touched files into the correct order, so I just left all the other changes in :O
LGTM
https://codereview.chromium.org/1779383002/diff/650001/chrome/chrome_browser_... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1779383002/diff/650001/chrome/chrome_browser_... chrome/chrome_browser_ui.gypi:2587: 'browser/ui/cocoa/certificate_viewer_mac_views.mm', On 2016/08/25 21:32:38, Robert Sesek wrote: > On 2016/08/25 08:25:12, Patti Lor wrote: > > On 2016/08/22 23:01:18, msw wrote: > > > should this be moved to browser/ui/views? > > > > Oh, that's a good point. I'll let rsesek@ make the final decision on this, but > I > > think I agree with you. It's weird though, I feel like if that's the case, > where > > should certificate_viewer_mac.{h,mm} go? Since it's not strictly for a Cocoa > > build. > > > > Have made the move for now, along with certificate_viewer_mac.{h,mm} into > > chrome/browser/ui/). > > Hm, I don't really have a strong opinion. I'd defer to Trent, but he's out. > Maybe ask Elly? Things like this which are objective-c wrappers around the Views version of a dialog generally live in chrome/browser/ui/cocoa, since they're only used in cocoa builds with views dialogs enabled - ie, a mac_views_browser=1 build won't use them. You can put certificate_viewer_mac.{h,mm} wherever you like.
Thanks all. Will submit with NOPRESUBMIT=true to avoid the error for the > 80 character include line. For reference, this is the file the error is in: https://codereview.chromium.org/1779383002/diff/730001/components/constrained... https://codereview.chromium.org/1779383002/diff/650001/chrome/chrome_browser_... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1779383002/diff/650001/chrome/chrome_browser_... chrome/chrome_browser_ui.gypi:2587: 'browser/ui/cocoa/certificate_viewer_mac_views.mm', On 2016/08/26 16:42:35, Elly Jones wrote: > On 2016/08/25 21:32:38, Robert Sesek wrote: > > On 2016/08/25 08:25:12, Patti Lor wrote: > > > On 2016/08/22 23:01:18, msw wrote: > > > > should this be moved to browser/ui/views? > > > > > > Oh, that's a good point. I'll let rsesek@ make the final decision on this, > but > > I > > > think I agree with you. It's weird though, I feel like if that's the case, > > where > > > should certificate_viewer_mac.{h,mm} go? Since it's not strictly for a Cocoa > > > build. > > > > > > Have made the move for now, along with certificate_viewer_mac.{h,mm} into > > > chrome/browser/ui/). > > > > Hm, I don't really have a strong opinion. I'd defer to Trent, but he's out. > > Maybe ask Elly? > > Things like this which are objective-c wrappers around the Views version of a > dialog generally live in chrome/browser/ui/cocoa, since they're only used in > cocoa builds with views dialogs enabled - ie, a mac_views_browser=1 build won't > use them. You can put certificate_viewer_mac.{h,mm} wherever you like. Thanks for taking a look Elly :) I'll leave it where it is then.
Description was changed from ========== MacViews: Remove constrained window dependencies for certificate viewer. Move constrained_window Mac dependencies to a Cocoa build only. To make the MacViews browser's certificate viewer work, refactor out common code taken from the original certificate viewer and subclass into SSLCertificateViewerCocoa and SSLCertificateViewerMacViews. The MacViews certificate viewer will be created via a new constrained_window method ShowWebModalDialogWithOverlayViews(). Similarly, NativeWebContentsModalDialogManagerViews is subclassed into a MacViews specific NativeWebContentsModalDialogManagerViewsMac, which handles hiding/showing the certificate viewer. Note that the certificate viewer is not the same as the Views one because OSX has its own, which is why there is special code for it needed here. BUG=587239 ========== to ========== MacViews: Remove constrained window dependencies for certificate viewer. Move constrained_window Mac dependencies to a Cocoa build only. To make the MacViews browser's certificate viewer work, refactor out common code taken from the original certificate viewer and subclass into SSLCertificateViewerCocoa and SSLCertificateViewerMacViews. The MacViews certificate viewer will be created via a new constrained_window method ShowWebModalDialogWithOverlayViews(). Similarly, NativeWebContentsModalDialogManagerViews is subclassed into a MacViews specific NativeWebContentsModalDialogManagerViewsMac, which handles hiding/showing the certificate viewer. Note that the certificate viewer is not the same as the Views one because OSX has its own, which is why there is special code for it needed here. BUG=587239 NOPRESUBMIT=true ==========
Description was changed from ========== MacViews: Remove constrained window dependencies for certificate viewer. Move constrained_window Mac dependencies to a Cocoa build only. To make the MacViews browser's certificate viewer work, refactor out common code taken from the original certificate viewer and subclass into SSLCertificateViewerCocoa and SSLCertificateViewerMacViews. The MacViews certificate viewer will be created via a new constrained_window method ShowWebModalDialogWithOverlayViews(). Similarly, NativeWebContentsModalDialogManagerViews is subclassed into a MacViews specific NativeWebContentsModalDialogManagerViewsMac, which handles hiding/showing the certificate viewer. Note that the certificate viewer is not the same as the Views one because OSX has its own, which is why there is special code for it needed here. BUG=587239 NOPRESUBMIT=true ========== to ========== MacViews: Remove constrained window dependencies for certificate viewer. Move constrained_window Mac dependencies to a Cocoa build only. To make the MacViews browser's certificate viewer work, refactor out common code taken from the original certificate viewer and subclass into SSLCertificateViewerCocoa and SSLCertificateViewerMacViews. The MacViews certificate viewer will be created via a new constrained_window method ShowWebModalDialogWithOverlayViews(). Similarly, NativeWebContentsModalDialogManagerViews is subclassed into a MacViews specific NativeWebContentsModalDialogManagerViewsMac, which handles hiding/showing the certificate viewer. Note that the certificate viewer is not the same as the Views one because OSX has its own, which is why there is special code for it needed here. BUG=587239 # Long include line in constrained_window_views.cc, wrapped in an ifdef. NOPRESUBMIT=true ==========
The CQ bit was checked by patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/1779383002/#ps730001 (title: "msw@ & rsesek@ review, move sheetDidEnd::: into separate SSLCertificateViewerMac declaration.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by patricialor@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MacViews: Remove constrained window dependencies for certificate viewer. Move constrained_window Mac dependencies to a Cocoa build only. To make the MacViews browser's certificate viewer work, refactor out common code taken from the original certificate viewer and subclass into SSLCertificateViewerCocoa and SSLCertificateViewerMacViews. The MacViews certificate viewer will be created via a new constrained_window method ShowWebModalDialogWithOverlayViews(). Similarly, NativeWebContentsModalDialogManagerViews is subclassed into a MacViews specific NativeWebContentsModalDialogManagerViewsMac, which handles hiding/showing the certificate viewer. Note that the certificate viewer is not the same as the Views one because OSX has its own, which is why there is special code for it needed here. BUG=587239 # Long include line in constrained_window_views.cc, wrapped in an ifdef. NOPRESUBMIT=true ========== to ========== MacViews: Remove constrained window dependencies for certificate viewer. Move constrained_window Mac dependencies to a Cocoa build only. To make the MacViews browser's certificate viewer work, refactor out common code taken from the original certificate viewer and subclass into SSLCertificateViewerCocoa and SSLCertificateViewerMacViews. The MacViews certificate viewer will be created via a new constrained_window method ShowWebModalDialogWithOverlayViews(). Similarly, NativeWebContentsModalDialogManagerViews is subclassed into a MacViews specific NativeWebContentsModalDialogManagerViewsMac, which handles hiding/showing the certificate viewer. Note that the certificate viewer is not the same as the Views one because OSX has its own, which is why there is special code for it needed here. BUG=587239 # Long include line in constrained_window_views.cc, wrapped in an ifdef. NOPRESUBMIT=true ==========
Message was sent while issue was closed.
Committed patchset #28 (id:730001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Remove constrained window dependencies for certificate viewer. Move constrained_window Mac dependencies to a Cocoa build only. To make the MacViews browser's certificate viewer work, refactor out common code taken from the original certificate viewer and subclass into SSLCertificateViewerCocoa and SSLCertificateViewerMacViews. The MacViews certificate viewer will be created via a new constrained_window method ShowWebModalDialogWithOverlayViews(). Similarly, NativeWebContentsModalDialogManagerViews is subclassed into a MacViews specific NativeWebContentsModalDialogManagerViewsMac, which handles hiding/showing the certificate viewer. Note that the certificate viewer is not the same as the Views one because OSX has its own, which is why there is special code for it needed here. BUG=587239 # Long include line in constrained_window_views.cc, wrapped in an ifdef. NOPRESUBMIT=true ========== to ========== MacViews: Remove constrained window dependencies for certificate viewer. Move constrained_window Mac dependencies to a Cocoa build only. To make the MacViews browser's certificate viewer work, refactor out common code taken from the original certificate viewer and subclass into SSLCertificateViewerCocoa and SSLCertificateViewerMacViews. The MacViews certificate viewer will be created via a new constrained_window method ShowWebModalDialogWithOverlayViews(). Similarly, NativeWebContentsModalDialogManagerViews is subclassed into a MacViews specific NativeWebContentsModalDialogManagerViewsMac, which handles hiding/showing the certificate viewer. Note that the certificate viewer is not the same as the Views one because OSX has its own, which is why there is special code for it needed here. BUG=587239 # Long include line in constrained_window_views.cc, wrapped in an ifdef. NOPRESUBMIT=true Committed: https://crrev.com/8f36f7b892825b3117d2b1d2cbd60b05ce6da4ba Cr-Commit-Position: refs/heads/master@{#414971} ==========
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/8f36f7b892825b3117d2b1d2cbd60b05ce6da4ba Cr-Commit-Position: refs/heads/master@{#414971} |
