New issue
Advanced search Search tips

Issue 780409 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug

Blocking:
issue 775848



Sign in to add a comment

Fix (mostly Cocoa) tests that fail on Mac when ShowAllDialogsWithViewsToolkit is enabled

Project Member Reported by tapted@chromium.org, Nov 1 2017

Issue description

Chrome Version       : 64
OS Version: OS X 10.12.6

A lot of these are failing just because the Cocoa UI/window isn't being created, so there's nothing to test.

Fix is largely going to be a ScopedFeatureList that disables SecondaryUiMd when testing specifically Cocoa UI. But there may be some things not specifically testing Cocoa UI that need extra coverage.


bot run: https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/578638


failing tests:

browser_tests:
BrowserWindowControllerTest.InfoBarTipStretchedWhenBookmarkBarStatusChanged
ContentSettingDecorationTest.DecorationPress
ExtensionMessageBubbleBrowserTestMac.* (12)
FirstRunMasterPrefsImportNothing.ImportNothingAndShowNewTabPage
FirstRunMasterPrefsWithTrackedPreferencesInstance/FirstRunMasterPrefsWithTrackedPreferences.TrackedPreferencesSurviveFirstRun/0
FirstRunMasterPrefsWithTrackedPreferencesInstance/FirstRunMasterPrefsWithTrackedPreferences.TrackedPreferencesSurviveFirstRun/1
ManagePasswordsBubbleTest.* (4)
NtpBubbleBrowserTestMac.* (2)
ProfileSigninConfirmationHelperBrowserTest.* (2)
StartupBrowserCreatorFirstRunTest.* (2)

interactive_ui_tests:
AutofillInteractiveTest.AutofillAfterTranslate

native_theme_unittests:
NativeThemeMacTest.SystemColorSpotChecks

unit_tests:
BookmarkBubbleControllerTest.TestBubbleWindow
ManagePasswordsBubbleCocoaTest.* (8)
PermissionSelectorButtonTest.* (3)
TranslateBubbleControllerTest.* (4)
 
Blocking: 775848
First pass https://chromium-review.googlesource.com/c/chromium/src/+/748487

should fix unit_tests:
 BookmarkBubbleControllerTest.TestBubbleWindow
 ManagePasswordsBubbleCocoaTest.* (8)
 PermissionSelectorButtonTest.* (3)
 TranslateBubbleControllerTest.* (4)
and browser_tests:
 ManagePasswordsBubbleTest.* (4)
 ExtensionMessageBubbleBrowserTestMac.* (12)

remaining (feel free to grab any and update this bug..):

BrowserWindowControllerTest.InfoBarTipStretchedWhenBookmarkBarStatusChanged
 - might actually be a bug

ContentSettingDecorationTest.DecorationPress
 - I think we want ContentSettingBubbleViewsBridge::Show(..) to return a gfx::NativeWindow so that content_setting_decoration.mm can store it. Annoying it's just for a test though.

this bunch:
  FirstRun*
  NtpBubbleBrowserTestMac.* (2)
  ProfileSigninConfirmationHelperBrowserTest.* (2)
  StartupBrowserCreatorFirstRunTest.* (2)
  AutofillInteractiveTest.AutofillAfterTranslate
 - I think these are creating random/unowned/unparented windows. Aura relies on some code in application_lifetime_aura that calls views::Widget::CloseAllSecondaryWidgets(). I think we need to call that on Mac as well. (which also requires implementing it!)

native_theme_unittests:
  NativeThemeMacTest.SystemColorSpotChecks
 - haven't looked at this yet, but it's probably a matter of changing the things that are spot-checked in the test
Components: Internals>Views
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 1 2017

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

commit 5da19f1b92ef9faa281ba146b15338aa2105ed29
Author: Trent Apted <tapted@chromium.org>
Date: Wed Nov 01 22:46:38 2017

Disable SecondaryUiMd in some tests that only test Cocoa UI.

These test harnesses specifically test Cocoa dialogs that are not shown
when SecondaryUiMd is enabled.

Bug:  780409 
Change-Id: Ibdce953bad102464bed17b998c4fcabdf64ace53
Reviewed-on: https://chromium-review.googlesource.com/748487
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513306}
[modify] https://crrev.com/5da19f1b92ef9faa281ba146b15338aa2105ed29/chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller_unittest.mm
[modify] https://crrev.com/5da19f1b92ef9faa281ba146b15338aa2105ed29/chrome/browser/ui/cocoa/extensions/extension_message_bubble_browsertest_mac.mm
[modify] https://crrev.com/5da19f1b92ef9faa281ba146b15338aa2105ed29/chrome/browser/ui/cocoa/page_info/permission_selector_button_unittest.mm
[modify] https://crrev.com/5da19f1b92ef9faa281ba146b15338aa2105ed29/chrome/browser/ui/cocoa/passwords/passwords_bubble_browsertest.mm
[modify] https://crrev.com/5da19f1b92ef9faa281ba146b15338aa2105ed29/chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa_unittest.mm
[modify] https://crrev.com/5da19f1b92ef9faa281ba146b15338aa2105ed29/chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 2 2017

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

commit ecd252f35b6e8671d0a3e387ea8ad246859ecaee
Author: Justin Donnelly <jdonnelly@chromium.org>
Date: Thu Nov 02 01:01:34 2017

Revert "Disable SecondaryUiMd in some tests that only test Cocoa UI."

This reverts commit 5da19f1b92ef9faa281ba146b15338aa2105ed29.

Reason for revert: Reverting on suspicion of breaking ExtensionMessageBubbleBrowserTestMac.TestClickingLearnMoreButton and other bubble tests on several Mac bots. See, for example:

https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.12%20Tests/builds/6896

-- Chromium sheriff

Original change's description:
> Disable SecondaryUiMd in some tests that only test Cocoa UI.
> 
> These test harnesses specifically test Cocoa dialogs that are not shown
> when SecondaryUiMd is enabled.
> 
> Bug:  780409 
> Change-Id: Ibdce953bad102464bed17b998c4fcabdf64ace53
> Reviewed-on: https://chromium-review.googlesource.com/748487
> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
> Commit-Queue: Trent Apted <tapted@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#513306}

TBR=ellyjones@chromium.org,tapted@chromium.org

Change-Id: I75466a2200968ce45b018dd9558191bafe98288c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  780409 
Reviewed-on: https://chromium-review.googlesource.com/749853
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513351}
[modify] https://crrev.com/ecd252f35b6e8671d0a3e387ea8ad246859ecaee/chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller_unittest.mm
[modify] https://crrev.com/ecd252f35b6e8671d0a3e387ea8ad246859ecaee/chrome/browser/ui/cocoa/extensions/extension_message_bubble_browsertest_mac.mm
[modify] https://crrev.com/ecd252f35b6e8671d0a3e387ea8ad246859ecaee/chrome/browser/ui/cocoa/page_info/permission_selector_button_unittest.mm
[modify] https://crrev.com/ecd252f35b6e8671d0a3e387ea8ad246859ecaee/chrome/browser/ui/cocoa/passwords/passwords_bubble_browsertest.mm
[modify] https://crrev.com/ecd252f35b6e8671d0a3e387ea8ad246859ecaee/chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa_unittest.mm
[modify] https://crrev.com/ecd252f35b6e8671d0a3e387ea8ad246859ecaee/chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm

OK!

 https://chromium-review.googlesource.com/c/chromium/src/+/750601 - tests in c/b/ui/cocoa that only care about cocoa code
 https://chromium-review.googlesource.com/c/chromium/src/+/752782 - BrowserWindowControllerTest.InfoBarTipStretchedWhenBookmarkBarStatusChanged
 https://chromium-review.googlesource.com/c/chromium/src/+/752905 - ContentSettingDecorationTest.DecorationPress
 https://chromium-review.googlesource.com/c/chromium/src/+/753061 - NativeThemeMacTest.SystemColorSpotChecks
 https://chromium-review.googlesource.com/c/chromium/src/+/752902 - a bunch of things wanting views::Widget::CloseAllSecondaryWidgets()

 https://chromium-review.googlesource.com/c/chromium/src/+/750444 - CL with jobs flipping the flag on Mac!
Oh! There's one more - AutofillInteractiveTest.AutofillAfterTranslate .. I haven't got it working yet. There's a weird renderer crash at translate_helper.cc(336) when it checks for TranslateHelper::IsTranslateLibAvailable()

WIP: https://chromium-review.googlesource.com/752783

possible alternative to get the test passing -> https://chromium-review.googlesource.com/752908
yah - AutofillAfterTranslate is the only error in the trial flag flip at https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/580652
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 3 2017

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

commit 187e23a1b6de89d7a2b79b4f47c3bfaafa1b59ec
Author: Trent Apted <tapted@chromium.org>
Date: Fri Nov 03 21:22:11 2017

Skip AutofillInteractiveTest.AutofillAfterTranslate on MacViews

The test flips ::switches::kEnableTranslateNewUX which is never going
to launch on Mac as Cocoa UI.

On MacViews, kEnableTranslateNewUX will engage the MacViews bubble UI
which isn't ready and isn't used yet. The Cocoa browser still uses an
InfoBar, even with MacViews/Harmony. See  http://crbug.com/781134 . So
only continue when testing the Cocoa UI (i.e. the one that won't launch).

Bug:  780409 ,  781134 
Change-Id: Iaf5aa156f687578e632894f80c9ce6a9b57ccc03
Reviewed-on: https://chromium-review.googlesource.com/752908
Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513914}
[modify] https://crrev.com/187e23a1b6de89d7a2b79b4f47c3bfaafa1b59ec/chrome/browser/autofill/autofill_interactive_uitest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Nov 3 2017

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

commit 1d2d5ce69bbc0c4cb07204aed7dad82ace633eee
Author: Trent Apted <tapted@chromium.org>
Date: Fri Nov 03 21:48:44 2017

Reland: Disable SecondaryUiMd in some tests that only test Cocoa UI.

These test harnesses specifically test Cocoa dialogs that are not shown
when SecondaryUiMd is enabled.

First attempt suffered from a subtle trap in SetUp() ordering when using
feature lists in browser_tests (crbug/722409).

Bug:  780409 
Change-Id: I5129e8b6a76e3b60b1ea6f32fcabd2804bdb6cb9
Reviewed-on: https://chromium-review.googlesource.com/750601
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513923}
[modify] https://crrev.com/1d2d5ce69bbc0c4cb07204aed7dad82ace633eee/chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller_unittest.mm
[modify] https://crrev.com/1d2d5ce69bbc0c4cb07204aed7dad82ace633eee/chrome/browser/ui/cocoa/extensions/extension_message_bubble_browsertest_mac.mm
[modify] https://crrev.com/1d2d5ce69bbc0c4cb07204aed7dad82ace633eee/chrome/browser/ui/cocoa/page_info/permission_selector_button_unittest.mm
[modify] https://crrev.com/1d2d5ce69bbc0c4cb07204aed7dad82ace633eee/chrome/browser/ui/cocoa/passwords/passwords_bubble_browsertest.mm
[modify] https://crrev.com/1d2d5ce69bbc0c4cb07204aed7dad82ace633eee/chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa_unittest.mm
[modify] https://crrev.com/1d2d5ce69bbc0c4cb07204aed7dad82ace633eee/chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm

Project Member

Comment 11 by bugdroid1@chromium.org, Nov 3 2017

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

commit 49bbbf39fc02d2c4fbe41f7e30b685e05b77d85d
Author: Trent Apted <tapted@chromium.org>
Date: Fri Nov 03 22:03:16 2017

Remove NativeThemeMacTest.SystemColorSpotChecks

This is more of a "change detector" test that isn't adding much value.
It's fragile to macOS changes out of our control, and breaks under
Harmony. The particular constant it checks, kColorId_WindowBackground,
is only used by the IME window on ChromeOS.

The test now fails because Harmony adopts a consistent dialog background
color across all platforms and OS versions; kColorId_WindowBackground is
fixed.

There are other system colors that Harmony does adopt, but they are
configurable in System Preferences, so checking them may cause a test
to fail on developer machines or misconfigured bots.

Bug:  780409 
Change-Id: I374dad77fb00fbe5ebcc772b97e8acd3fb768564
Reviewed-on: https://chromium-review.googlesource.com/753061
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513924}
[modify] https://crrev.com/49bbbf39fc02d2c4fbe41f7e30b685e05b77d85d/ui/native_theme/native_theme_mac_unittest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Nov 4 2017

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

commit a7026b7f4a0665a8f7bdb17e4050dd98511f05e9
Author: Trent Apted <tapted@chromium.org>
Date: Fri Nov 03 23:01:55 2017

MacViews: Implement Widget::CloseAllSecondaryWidgets()

Views Widgets host ui::Compositors that talk to the GPU process, whose host
complains if it is destroyed while in-use. By this point, all browser
windows are closed, which tear down any Widgets parented to them. This will
additionally close any unparented, non-Browser Widgets.

Test: WidgetTest.CloseAllSecondaryWidgets
Bug:  780409 
Change-Id: Ieea01134a843abb04816c82933f3a3a301cd2647
Reviewed-on: https://chromium-review.googlesource.com/752902
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513932}
[modify] https://crrev.com/a7026b7f4a0665a8f7bdb17e4050dd98511f05e9/chrome/browser/lifetime/application_lifetime_mac.mm
[modify] https://crrev.com/a7026b7f4a0665a8f7bdb17e4050dd98511f05e9/ui/views/widget/native_widget_mac.mm
[modify] https://crrev.com/a7026b7f4a0665a8f7bdb17e4050dd98511f05e9/ui/views/widget/widget_unittest.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Nov 4 2017

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

commit b03bc3afcf0dbf003eb3989f21e7a42e87697b10
Author: Trent Apted <tapted@chromium.org>
Date: Fri Nov 03 23:02:06 2017

MacViews: Fix ContentSettingDecorationTest.DecorationPress with SecondaryUiMd

The ContentSettingDecoration is in the browser, so still used in MacViews.

Follow the pattern used by the Cocoa bubble, and retain a window. This
change also reduces the cases where a bubble reopens (rather than
dismissing) when clicking the decoration a second time. However, there's
an orthogonal issue where this may still occur depending on the order of
events (the views bubble dismisses on focus loss, so may be hidden by
the time the mouse event is processed on the decoration).

It's also a bit weird that the NSWindow is retained for the lifetime of the
decoration, but that's what Cocoa does, and it doesn't matter for views
so long as the window isn't reshown.

Bug:  780409 
Change-Id: I7d46904d3c1ae8f2dd482537126908cf00389fd9
Reviewed-on: https://chromium-review.googlesource.com/752905
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513933}
[modify] https://crrev.com/b03bc3afcf0dbf003eb3989f21e7a42e87697b10/chrome/browser/ui/cocoa/browser_dialogs_views_mac.cc
[modify] https://crrev.com/b03bc3afcf0dbf003eb3989f21e7a42e87697b10/chrome/browser/ui/cocoa/browser_dialogs_views_mac.h
[modify] https://crrev.com/b03bc3afcf0dbf003eb3989f21e7a42e87697b10/chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm

Project Member

Comment 14 by bugdroid1@chromium.org, Nov 4 2017

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

commit 1d64d9d0dacfaee4e4ddb209307dcfdc6f628b3e
Author: Trent Apted <tapted@chromium.org>
Date: Fri Nov 03 23:03:13 2017

Fix BrowserWindowControllerTest.InfoBarTipStretchedWhenBookmarkBarStatusChanged

Infobars used to anchor from the same point as PageInfo. Now they don't.
I renamed the method used in the browser code, but the test file still
tests expectations against the old name.

Bug:  780409 
Change-Id: I2b1130bdd637719cc51f9a642710061bbc2c37c1
Reviewed-on: https://chromium-review.googlesource.com/752782
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513935}
[modify] https://crrev.com/1d64d9d0dacfaee4e4ddb209307dcfdc6f628b3e/chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm

Status: Fixed (was: Started)

Sign in to add a comment