MacViews: Refactor certificate viewer tests to work for MacViews browser.

Mac: Use a generic interface for testing the certificate viewer.

Refactor the Mac certificate viewer browser tests to work on a generic interface
in preparation for http://crrev.com/1779383002, which refactors the certificate
viewer to remove some Cocoa dependencies for the MacViews browser.

Additionally, remove unneeded code used to resize/reposition the certificate
viewer after switching tabs by ignoring mouse events while the certificate
viewer is hidden instead.

BUG=613880
Committed: https://crrev.com/63b0c3d1ec281400262336497fa9803841b8469c
Cr-Commit-Position: refs/heads/master@{#400627}

[Patti Lor, 2016-06-07 01:52:28]

patricialor@chromium.org changed reviewers:
+ tapted@chromium.org

[Patti Lor, 2016-06-07 01:52:29]

Hi Trent, PTAL. I wasn't able to use WebContentsModalDialogManager::TestApi to
get the child_dialogs_ like you suggested, but have done something kind of
similar with [...->GetTopLevelNativeWindow() childWindows].

[tapted, 2016-06-07 05:33:43]

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm (right):

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:31: void
CheckCertificateViewerVisibility(gfx::NativeWindow overlay_window,
nit: these can just use NSWindow instead of gfx::NativeWindow which is a bit
more abstract. more in SLCertificateViewerMacTest.HideShow

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:39:
EXPECT_EQ(ignore_events, [overlay_window ignoresMouseEvents]);
One thing to be aware of: the output of the tests in the bots is just the line
number, so we don't know if a failure on this line was from a call with visible
= true or visible = false. Sometimes that's important to know.

There's an easy fix though - at the start of this function, do

  SCOPED_TRACE(testing::Message() << "visible=" << visible);

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:41:
ASSERT_TRUE(dialog_sheet);
EXPECT_TRUE

(ASSERT has odd semantics -- it's basically "EXPECT(..); (if it fails) return;"
e.g. it only works in functions that return `void` and it doesn't terminate the
program. Usually we just go with EXPECT, even if a crash would result later)

But I think this particular check makes more sense in the test body (see below),
so it can be removed from here

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:45: } // namespace
nit: blank line before (there should be one at the top of the namespace too)

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:87:
ASSERT_TRUE((num_child_windows + 1) == [[window childWindows] count]);
EXPECT_EQ(num_child_windows + 1, [[window childWindows] count]));

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:88:
gfx::NativeWindow overlay_window = [[window childWindows] lastObject];
comment before:

// Assume the last child is the overlay window that was added.

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:89:
gfx::NativeWindow dialog_sheet = [overlay_window attachedSheet];
EXPECT_TRUE(dialog_sheet) after this

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:90: NSRect
sheetFrame = [dialog_sheet frame];
sheetFrame -> sheet_frame

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:107:
test_api.CloseAllDialogs();
was this needed for something? (comment?)

[Patti Lor, 2016-06-08 00:14:38]

Thanks, PTAL!

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm (right):

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:31: void
CheckCertificateViewerVisibility(gfx::NativeWindow overlay_window,
On 2016/06/07 05:33:43, tapted wrote:
> nit: these can just use NSWindow instead of gfx::NativeWindow which is a bit
> more abstract. more in SLCertificateViewerMacTest.HideShow

Done.

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:39:
EXPECT_EQ(ignore_events, [overlay_window ignoresMouseEvents]);
On 2016/06/07 05:33:42, tapted wrote:
> One thing to be aware of: the output of the tests in the bots is just the line
> number, so we don't know if a failure on this line was from a call with
visible
> = true or visible = false. Sometimes that's important to know.
> 
> There's an easy fix though - at the start of this function, do
> 
>   SCOPED_TRACE(testing::Message() << "visible=" << visible);
> 

Done.

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:41:
ASSERT_TRUE(dialog_sheet);
On 2016/06/07 05:33:43, tapted wrote:
> EXPECT_TRUE
> 
> (ASSERT has odd semantics -- it's basically "EXPECT(..); (if it fails)
return;"
> e.g. it only works in functions that return `void` and it doesn't terminate
the
> program. Usually we just go with EXPECT, even if a crash would result later)
> 
> But I think this particular check makes more sense in the test body (see
below),
> so it can be removed from here

Done.

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:45: } // namespace
On 2016/06/07 05:33:43, tapted wrote:
> nit: blank line before (there should be one at the top of the namespace too)

Done.

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:87:
ASSERT_TRUE((num_child_windows + 1) == [[window childWindows] count]);
On 2016/06/07 05:33:42, tapted wrote:
> EXPECT_EQ(num_child_windows + 1, [[window childWindows] count]));

Done.

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:88:
gfx::NativeWindow overlay_window = [[window childWindows] lastObject];
On 2016/06/07 05:33:43, tapted wrote:
> comment before:
> 
> // Assume the last child is the overlay window that was added.

Done.

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:89:
gfx::NativeWindow dialog_sheet = [overlay_window attachedSheet];
On 2016/06/07 05:33:42, tapted wrote:
> EXPECT_TRUE(dialog_sheet) after this

Done.

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:90: NSRect
sheetFrame = [dialog_sheet frame];
On 2016/06/07 05:33:43, tapted wrote:
> sheetFrame -> sheet_frame

Done.

https://codereview.chromium.org/2039823002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:107:
test_api.CloseAllDialogs();
On 2016/06/07 05:33:43, tapted wrote:
> was this needed for something? (comment?)

Nope, removed.

[tapted, 2016-06-10 03:25:48]

given there's some hold ups on the other CL, we might want to get this out of
the way first by rebasing it on master - call it something like `Mac: Use a
generic interface for testing the certificate viewer`.

[Patti Lor, 2016-06-17 01:28:36]

Description was changed from

==========
MacViews: Refactor certificate viewer tests to work for MacViews browser.

Follow up from http://crrev.com/1779383002, which refactors the certificate
viewer to remove some Cocoa dependencies for the MacViews browser. This CL
updates SSLCertificateViewerMacTest.* browser_tests to work for both Cocoa and
MacViews browser builds.

BUG=613880
==========

to

==========
Mac: Use a generic interface for testing the certificate viewer.

Refactor the Mac certificate viewer browser tests to work on a generic interface
in preparation for http://crrev.com/1779383002, which refactors the certificate
viewer to remove some Cocoa dependencies for the MacViews browser.

Additionally, remove unneeded code used to resize/reposition the certificate
viewer after switching tabs by ignoring mouse events while the certificate
viewer is hidden instead.

BUG=613880
==========

[Patti Lor, 2016-06-17 05:52:34]

Rebased back onto master :) I had to add a fix back in for the certificate
viewer not ignoring mouse events while it was hidden to keep the test the same
(which was fixed another way in https://codereview.chromium.org/2034993002,
which is why some of the code is being deleted). Alternatively, we could keep
the fix in the other CL and delete the expectations checking for that particular
property in the test.

Thanks!

[tapted, 2016-06-17 06:11:34]

lgtm with some minor tweaks and a Q

the fix in https://codereview.chromium.org/2034993002 isn't as nice -
kWindowSizeDeterminedLater is a 1x1+0+0 window (and yeah - if you switch tabs
with a certificate viewer open you can't click the bottom-left corner of of your
screen :o)

https://codereview.chromium.org/2039823002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/certificate_viewer_mac.h (right):

https://codereview.chromium.org/2039823002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac.h:41: - (NSWindow*)overlayWindow;
can this be removed? (from the .mm too)

https://codereview.chromium.org/2039823002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (left):

https://codereview.chromium.org/2039823002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac.mm:184: // If the overlay frame's
size has changed, the position offsets need to be
So does this happen automatically? (i.e. does the sheet always reposition itself
appropriately)

https://codereview.chromium.org/2039823002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm (right):

https://codereview.chromium.org/2039823002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:23: typedef
InProcessBrowserTest SSLCertificateViewerMacTest;
nit: using SSLCertificateViewerMacTest = InProcessBrowserTest;

(it's the latest trend)

[Patti Lor, 2016-06-20 03:42:29]

Thanks Trent! Will land now.

https://codereview.chromium.org/2039823002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/certificate_viewer_mac.h (right):

https://codereview.chromium.org/2039823002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac.h:41: - (NSWindow*)overlayWindow;
On 2016/06/17 06:11:33, tapted wrote:
> can this be removed? (from the .mm too)

Done.

https://codereview.chromium.org/2039823002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/certificate_viewer_mac.mm (left):

https://codereview.chromium.org/2039823002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac.mm:184: // If the overlay frame's
size has changed, the position offsets need to be
On 2016/06/17 06:11:33, tapted wrote:
> So does this happen automatically? (i.e. does the sheet always reposition
itself
> appropriately)

Yep, the certificate viewer position is controlled by
ConstrainedWindowSheetController
updateSheetPosition/overlayWindowFrameForParentView (see
constrained_window_sheet_controller.mm).

https://codereview.chromium.org/2039823002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm (right):

https://codereview.chromium.org/2039823002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/certificate_viewer_mac_browsertest.mm:23: typedef
InProcessBrowserTest SSLCertificateViewerMacTest;
On 2016/06/17 06:11:33, tapted wrote:
> nit: using SSLCertificateViewerMacTest = InProcessBrowserTest;
> 
> (it's the latest trend)

Done.

[Patti Lor, 2016-06-20 04:30:25]

The CQ bit was checked by patricialor@chromium.org

[commit-bot: I haz the power, 2016-06-20 04:30:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2039823002/60001

[commit-bot: I haz the power, 2016-06-20 05:22:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-20 05:23:34]

Description was changed from

==========
Mac: Use a generic interface for testing the certificate viewer.

Refactor the Mac certificate viewer browser tests to work on a generic interface
in preparation for http://crrev.com/1779383002, which refactors the certificate
viewer to remove some Cocoa dependencies for the MacViews browser.

Additionally, remove unneeded code used to resize/reposition the certificate
viewer after switching tabs by ignoring mouse events while the certificate
viewer is hidden instead.

BUG=613880
==========

to

==========
Mac: Use a generic interface for testing the certificate viewer.

Refactor the Mac certificate viewer browser tests to work on a generic interface
in preparation for http://crrev.com/1779383002, which refactors the certificate
viewer to remove some Cocoa dependencies for the MacViews browser.

Additionally, remove unneeded code used to resize/reposition the certificate
viewer after switching tabs by ignoring mouse events while the certificate
viewer is hidden instead.

BUG=613880

Committed: https://crrev.com/63b0c3d1ec281400262336497fa9803841b8469c
Cr-Commit-Position: refs/heads/master@{#400627}
==========

[commit-bot: I haz the power, 2016-06-20 05:23:35]

Patchset 4 (id:??) landed as
https://crrev.com/63b0c3d1ec281400262336497fa9803841b8469c
Cr-Commit-Position: refs/heads/master@{#400627}