We should at least fix the crashes as high-priority now; the failures don't have to block other work, but crashes can mean we're doing something wrong.

Allen, throwing this at you for the moment since I don't think you have other P1 Harmony stuff going on.

For the referenced tests which are failing or crashing, the issue is related to the lack of the arrow around the border of the bubble. Those arrows are images that are not loaded in MD mode, which returns a nullptr to the tests. I suspect some of the other failing tests are experiencing similar issues.

It doesn't seem right to merely disable whole tests when in MD mode, unless the test is explicitly for something that is wholly disabled in MD mode. For instance, the GetBoundsOriginsTest does a lot of work to pre-calculate various bounds and positions for different modes, many of which have to take into account the images. The test itself seems to still be valid even without the arrow images, yet the calculates would largely be very different.

I suspect this effort could quickly turn herculean if there are many tests that fall into similar situations.

So the crash is because the test is trying to explicitly dereference properties (e.g. the height) of the returned arrow images?

These sound to me a bit like change-detector tests.  If there is value in them, I'd think we'd want to calculate things like "the origin is at least as far over as the anchor" or "the window fits on the screen", but not things like exact expected sizes.

When fixing up tabstrip tests for top chrome MD I ended up ripping out a lot of stuff that calculated precise tab bounds and tried to replace with things like "this tab is not left of that one".

Yes, the test code is dereferencing properties of internal::BorderImages returned from BubbleBorder::GetImagesForTest();

As far as merely being a change-detector test, I'm not sure that characterization is completely accurate. The code does a lot to calculate things based on the size of the images returned from internal::BorderImages. I suspect I could return an instance of that class that has 0 for sizes and the calculations may simply work.

I was just highlighting that this may not be a "spend a few days and fix up the tests" task.

I'm optimistic we can at least get the tests to not crash in a couple of days, and get an idea of what we might want to do in principle with these tests.  Actually doing it definitely does seem like it could be a longer process.

I'm still not sure from your descriptions that we really want these tests doing all these calculations, though.  If tests are just rebuilding the calculations of the code, that doesn't seem helpful.  It seems like what we want tested are the basic core principles the dialogs are supposed to uphold, not necessarily specific layout details.

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

commit 794b29fa006a89abb7b415d3cb6aaf7bc9decd01
Author: Allen Bauer <kylixrd@chromium.org>
Date: Wed Aug 16 13:44:21 2017

Skip/alter tests in BubbleFrameViewTest.GetUpdatedWindowBoundsCenterArrows which fail because of MD mode.

Bug:  713030 
Change-Id: Idf185f57cd3c718cbb79a18c4a6bf197c65309c9
Reviewed-on: https://chromium-review.googlesource.com/602379
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Valery Arkhangorodsky <varkha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494761}
[modify] https://crrev.com/794b29fa006a89abb7b415d3cb6aaf7bc9decd01/ui/views/bubble/bubble_frame_view_unittest.cc

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

commit be0868764b1797345b832a30d54f5e00b6efd4df
Author: Allen Bauer <kylixrd@chromium.org>
Date: Fri Aug 18 14:43:37 2017

BubbleBorderTest is now working in both MD and non-MD modes.

Bug:  713030 
Change-Id: I0d48bc1c5a7d83a6aa2b626c946d2e25db9c8d8a
Reviewed-on: https://chromium-review.googlesource.com/600832
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495547}
[modify] https://crrev.com/be0868764b1797345b832a30d54f5e00b6efd4df/ui/views/bubble/bubble_border_unittest.cc

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

commit 47d6522a58399d49d8da7014f98acbba6f84b6b5
Author: Allen Bauer <kylixrd@chromium.org>
Date: Fri Sep 15 14:22:38 2017

Fixed TableViewTest to work in MD mode.

Bug:  713030 
Change-Id: I6aa86b54e4e5fccadb38d26afae70a70faec897d
Reviewed-on: https://chromium-review.googlesource.com/604378
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502245}
[modify] https://crrev.com/47d6522a58399d49d8da7014f98acbba6f84b6b5/ui/views/controls/table/table_view_unittest.cc
[modify] https://crrev.com/47d6522a58399d49d8da7014f98acbba6f84b6b5/ui/views/widget/widget.cc

The NextAction date has arrived: 2017-09-29

https://chromium-review.googlesource.com/c/chromium/src/+/725205 is a CL to flip the flag, it has some failures. Any help getting those passing would be appreciated!

Note we can probably ignore ChromeOS specific ones. And Gerrit isn't good at clearing out jobs after a rebase. Anyway, I think the failures are all

unit_tests:

BrowserViewTest.BrowserViewLayout
GlobalErrorBubbleViewTest.Basic
PageInfoBubbleViewTest.SetPermissionInfo
PermissionMenuModelTest.TestDefault
PermissionMenuModelTest.TestDefaultMediaHttp
PermissionMenuModelTest.TestIncognitoNotifications
PermissionMenuModelTest.TestSubresourceFilter


views_unittests
DialogTest.BoundsAccommodateTitle  (linux only)

I ran these tests on Windows/Linux/Mac and cannot get them to fail. I made sure I've add --enable-features=SecondaryUiMd to the command line.

I'm sure I may have missed something.

hehe, you probably encountered  Issue 778492  "views_unittests and others ignore --enable-features=MyNewFeature command line arguments". Which I'm about to land a fix for.

There's actually quite a few more views_unittests failing - I think because --secondary-ui-md set a static boolean at an odd time, --secondary-ui-md wasn't enough to put all the tests into MD mode properly. But the feature flag is set earlier and more reliably.

Current failures:

5 tests failed:
    BubbleFrameViewTest.GetUpdatedWindowBounds (../../ui/views/bubble/bubble_frame_view_unittest.cc:192)
    BubbleFrameViewTest.GetUpdatedWindowBoundsCenterArrows (../../ui/views/bubble/bubble_frame_view_unittest.cc:365)
    DialogTest.BoundsAccommodateTitle (../../ui/views/window/dialog_delegate_unittest.cc:307)
    LabelButtonTest.ButtonStyleIsDefaultStyle (../../ui/views/controls/button/label_button_unittest.cc:389)
    LabelButtonTest.HighlightedButtonStyle (../../ui/views/controls/button/label_button_unittest.cc:413)

Fix for tests ignoring -enable-features is in (r512005)

PageInfoBubbleViewTest.SetPermissionInfo is disabled on Mac, and BrowserViewTest.BrowserViewLayout doesn't exist on Mac (there is no BrowserView) - any takers ?


*.BoundsAccommodateTitle: https://chromium-review.googlesource.com/c/chromium/src/+/741381

LabelButtonTest.*: https://chromium-review.googlesource.com/c/chromium/src/+/741401  (Mac-specific probably)

The BubbleFrameViewTest.* have evaporated - maybe it was a temporary regression


PermissionMenuModelTest.* fail on a DCHECK
 FATAL:page_info_ui.cc(305)] Check failed: false.
 in PageInfoUI::PermissionActionToUIString(), when the default switch case gets hit.

Probably due to this:

  // The Material UI for site settings uses comboboxes instead of menubuttons,
  // which means the elements of the menu themselves have to be shorter, instead
  // of simply setting a shorter label on the menubutton.
  if (ui::MaterialDesignController::IsSecondaryUiMaterial()) {
    label = PageInfoUI::PermissionActionToUIString(
        profile, permission_.type, CONTENT_SETTING_DEFAULT,
        effective_default_setting, permission_.source);
  }

I think all the failing PermissionMenuModelTest just need to set permission.source = content_settings::SETTING_SOURCE_USER or similar.



GlobalErrorBubbleViewTest.Basic

failure is

../../chrome/browser/ui/views/global_error_bubble_view_unittest.cc:124: Failure
Value of: view_->ShouldShowWindowIcon()
  Actual: false
Expected: true

Fix for GlobalErrorBubbleViewTest.Basic: https://chromium-review.googlesource.com/c/chromium/src/+/742182

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

commit 50ce2bb504c401aa2b97b644a3214eb7bda3bfb2
Author: Allen Bauer <kylixrd@chromium.org>
Date: Fri Oct 27 23:30:53 2017

Update GlobalErrorBubbleViewTest.Basic to also work properly in SecondaryUiMd mode.

Bug:  713030 
Change-Id: Iac3fe3d706408ae6dd664cbaa15394057fb6f2c1
Reviewed-on: https://chromium-review.googlesource.com/742182
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512346}
[modify] https://crrev.com/50ce2bb504c401aa2b97b644a3214eb7bda3bfb2/chrome/browser/ui/views/global_error_bubble_view_unittest.cc

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

commit be56a2a50547baf10ff3dbaf3398dc09ec11d584
Author: Trent Apted <tapted@chromium.org>
Date: Sun Oct 29 23:23:41 2017

Fix DialogTest.BoundsAccommodateTitle under SecondaryUiMd

Under MD, the close button takes up more vertical space than the
dialog title, so it dominates the title insets.

Make the test work for both by removing the close buttons from the layout.

Bug:  713030 
Change-Id: Ibead5d002dcbe36d6fda65a3a9a8e01695e4f1a8
Reviewed-on: https://chromium-review.googlesource.com/741381
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512424}
[modify] https://crrev.com/be56a2a50547baf10ff3dbaf3398dc09ec11d584/ui/views/window/dialog_delegate_unittest.cc

BrowserViewLayout failure is ../../chrome/browser/ui/views/frame/browser_view_unittest.cc:121: Failure
      Expected: chrome::kMinimumBookmarkBarHeight
      Which is: 26
To be equal to: bookmark_bar->GetMinimumSize().height()
      Which is: 30
[  FAILED  ] BrowserViewTest.BrowserViewLayout (1531 ms)

This might actually be a bug.

The code is

int BookmarkBarView::GetPreferredHeight() const {
  int height = chrome::kMinimumBookmarkBarHeight;
  for (int i = 0; i < child_count(); ++i) {
    const views::View* view = child_at(i);
    if (view->visible())
      height = std::max(view->GetPreferredSize().height(), height);
  }
  return height;
}

I guess the bookmarks bar buttons under MD are taller, but I don't know if we want to be taking up more space in the bookmarks bar.

I haven't had time to look at this yet.


PermissionMenuModelTest.* fix is in https://chromium-review.googlesource.com/c/chromium/src/+/742863

I think these are the last ones.

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

commit acdee60f605282e5c83b38e94347548083d4e498
Author: Allen Bauer <kylixrd@chromium.org>
Date: Mon Oct 30 23:48:24 2017

Fix PermissionMenuModelTest for SecondaryUiMd mode.

Under MD, these unit tests invoke PageInfoUI::PermissionActionToUIString(..)
which require the permission to have a source set. Use
content_settings::SETTING_SOURCE_USER.

Bug:  713030 
Change-Id: I2ce0ece216dddc9dd1aff26b5dc166ce1ae437e5
Reviewed-on: https://chromium-review.googlesource.com/742347
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512676}
[modify] https://crrev.com/acdee60f605282e5c83b38e94347548083d4e498/chrome/browser/ui/page_info/permission_menu_model_unittest.cc

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

commit c7cb3e9057a8420d0f54db86dc581bc33b94e4c9
Author: Trent Apted <tapted@chromium.org>
Date: Tue Oct 31 01:49:27 2017

Fix LabelButtonTest.ButtonStyleIsDefaultStyle,HighlightedButtonStyle

Since r478352 SecondaryUI buttons on Mac have all been MdTextButtons,
so we don't really care how regular LabelButtons look on Mac.

Remove some mac-specific logic from PlatformStyle and the
label_button_unittest.cc to make these tests pass. Some tests
currently fail just because the text color changes from solid
black/white to something different under MD due to logic in
ui::NativeTheme

Bug:  713030 
Change-Id: I3b925918024d6c260da007ceb96900a2afb557a4
Reviewed-on: https://chromium-review.googlesource.com/741401
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512704}
[modify] https://crrev.com/c7cb3e9057a8420d0f54db86dc581bc33b94e4c9/ui/views/controls/button/blue_button_unittest.cc
[modify] https://crrev.com/c7cb3e9057a8420d0f54db86dc581bc33b94e4c9/ui/views/controls/button/label_button.cc
[modify] https://crrev.com/c7cb3e9057a8420d0f54db86dc581bc33b94e4c9/ui/views/controls/button/label_button_unittest.cc
[modify] https://crrev.com/c7cb3e9057a8420d0f54db86dc581bc33b94e4c9/ui/views/style/platform_style.cc
[modify] https://crrev.com/c7cb3e9057a8420d0f54db86dc581bc33b94e4c9/ui/views/style/platform_style.h
[modify] https://crrev.com/c7cb3e9057a8420d0f54db86dc581bc33b94e4c9/ui/views/style/platform_style_mac.mm

Fix for BrowserViewTest.BrowserViewLayout is in https://chromium-review.googlesource.com/c/chromium/src/+/746062

Then we're nearly there! Last one is PageInfoBubbleViewTest.SetPermissionInfo, which is disabled on Mac.

https://build.chromium.org/p/tryserver.chromium.win/builders/win7_chromium_rel_ng/builds/30985

[ RUN      ] PageInfoBubbleViewTest.SetPermissionInfo
../../chrome/browser/ui/views/page_info/page_info_bubble_view_unittest.cc(233): error:       Expected: base::ASCIIToUTF16("Allow")
      Which is: L"Allow"
To be equal to: api_->GetPermissionButtonTextAt(0)
      Which is: L"Block"
../../chrome/browser/ui/views/page_info/page_info_bubble_view_unittest.cc(239): error:       Expected: base::ASCIIToUTF16("Ask")
      Which is: L"Ask"
To be equal to: api_->GetPermissionButtonTextAt(0)
      Which is: L"Block"
[  FAILED  ] PageInfoBubbleViewTest.SetPermissionInfo (66 ms)

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

commit 10fd09104eed55326bc2f38fa2fab4154c052ab0
Author: Trent Apted <tapted@chromium.org>
Date: Wed Nov 01 03:51:17 2017

Fix BrowserViewTest.BrowserViewLayout with SecondaryUiMd

BookmarkBarInstructionsView::CalculatePreferredSize() was growing 25->30
pixels under SecondaryUiMd because Labels pick up a default minimum line
height under Harmony.

BookmarkBarInstructionsView should be using style::CONTEXT_BUTTON so that
its text is consistent with the LabelButtons that normally appear in the
bookmarks toolbar. That TextContext doesn't set line heights, so the
height now stays the same.

Bug:  713030 
Change-Id: I5386755a8f1e77d91d95a9f03b86ce22d5f31c61
Reviewed-on: https://chromium-review.googlesource.com/746062
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513068}
[modify] https://crrev.com/10fd09104eed55326bc2f38fa2fab4154c052ab0/chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc

Fix for PageInfoBubbleViewTest.SetPermissionInfo is in https://chromium-review.googlesource.com/c/chromium/src/+/748861 . Sadly, fixing the test exposed a memory leak in the release code that only surfaces with SecondaryUiMd

https://logs.chromium.org/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux%2Flinux_chromium_asan_rel_ng%2F480976%2F%2B%2Frecipes%2Fsteps%2Funit_tests__with_patch_%2F0%2Flogs%2FPageInfoBubbleViewTest.SetPermissionInfo%2F0

I think the problem is with http://cs.chromium.org/PermissionSelectorRow::~PermissionSelectorRow() -- it has a big comment that starts "// Gross.". Trying out the fix now...


Also with the revised plan to flip not just SecondaryUiMd but also ShowAllDialogsWithViewsToolkit on Mac, there are more problems. https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/578638 -- a lot of these seem to be Cocoa-specific tests that end up trying to test the toolkit-views dialogs. Disabling the feature in the .mm files will hopefully make them go away..

So to get this flipped on Windows/Linux, it might be a good idea just to exclude Mac for the first landing.

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

commit d34d8303ef116967f7a50a2734bc43338c11deb1
Author: Trent Apted <tapted@chromium.org>
Date: Wed Nov 01 23:15:16 2017

Fix PageInfoBubbleViewTest.SetPermissionInfo with SecondaryUiMd

Under MD, the changed setting is always read from the combobox selected
index when changed in the UI. PermissionSelectorRow::PermissionChanged()
ignores the argument, except to detect whether it is the default.

Also PermissionMenuModel gets strings using PageInfoUI::
PermissionActionToUIString(..) rather than directly from the
ResourceBundle.

Ensure the unit test acconts for these.

Test exposed a memory leak with SecondaryUiMd, which enables views::
Combobox for the selectors. Combobox needs to be deleted before its
model is destroyed. View::RemoveChildView doesn't delete its argument,
but simply calling delete view does end up calling RemoveChildView().

Bug:  713030 ,  780408 
Change-Id: I373cf0d40316bb2fc42e76bb470d1977c0678e2b
Reviewed-on: https://chromium-review.googlesource.com/748861
Reviewed-by: Lucas Garron <lgarron@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513315}
[modify] https://crrev.com/d34d8303ef116967f7a50a2734bc43338c11deb1/chrome/browser/ui/views/page_info/page_info_bubble_view_unittest.cc
[modify] https://crrev.com/d34d8303ef116967f7a50a2734bc43338c11deb1/chrome/browser/ui/views/page_info/permission_selector_row.cc

Are there anymore tests that are failing under SecondaryUiMd? Should this now be marked as "Fixed"?

I hope not! This has been running on the bots for a couple of weeks. I don't think we disabled any tests.