With the revert, the failures disappeared in
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/mac-cocoa-rel/1150

(There is another new failure that's likely unrelated).

Thanks for reverting, Sam.

This is really complicated:
 - Original patch landed in: r587490 (70.0.3538.0)
 - Reverted in:              r588581 (71.0.3543.0, not yet cut)

M70 branch point happened in between, so the original is still in the branch.

We need it in the branch (otherwise Linux PWA windows are completely ugly) so I'd like to not immediately revert the branch. Currently trying to figure out if it's just the test that's crashing or if there's a legitimate new crash in BrowserNonClientFrameView::GetFrameColor on Mac Cocoa.

The two failing tests:

[ RUN      ] BrowserNonClientFrameViewBrowserTest.BrowserFrameColorThemed
BrowserTestBase received signal: Segmentation fault: 11. Backtrace:
3   ???                                 0x0000000000000020 0x0 + 32
4   browser_tests                       0x0000000107665bc0 BrowserNonClientFrameViewBrowserTest_BrowserFrameColorThemed_Test::RunTestOnMainThread() + 144

[ RUN      ] BrowserNonClientFrameViewBrowserTest.BookmarkAppFrameColorSystemTheme
BrowserTestBase received signal: Segmentation fault: 11. Backtrace:
3   ???                                 0x00007fe6898fc080 0x0 + 140628127105152
4   browser_tests                       0x000000010d60b11c BrowserNonClientFrameView::GetFrameColor(BrowserNonClientFrameView::ActiveState) const + 28
5   browser_tests                       0x000000010813838a BrowserNonClientFrameViewBrowserTest_BookmarkAppFrameColorSystemTheme_Test::RunTestOnMainThread() + 106

It's hard to tell whether this is a new failure in GetFrameColor, because these tests are brand new in r587490 and this code was untested before then. And I don't have any stack traces with debugging symbols.

However, since this is a crash in Views code which probably doesn't run on Mac Cocoa, I assume there is nothing to worry about in production, and we can just disable the tests in Cocoa (or on Mac).

Which begs the question: why is BrowserNonClientFrameViewTest running at all on the mac-cocoa-rel bot?

+robliao +ccameron can you shed any light on this? Do you think it's safe to disable (i.e., early return) these views-only tests on Mac Cocoa?

+robliao +ccameron

Don't worry: tapted@ helped me out and I found test::ScopedMacViewsBrowserMode. Seems like this test simply doesn't make sense on Mac Cocoa so just forcing it to run in Views mode is fine. And tested on a Mac that this resolves the issue.

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

commit 9e6f06a2d395fd32c5dc2112f77d6ad15cfebc9a
Author: Samuel Huang <huangs@chromium.org>
Date: Tue Sep 04 17:49:13 2018

Revert "Fix Linux hosted app / PWA window frame color with GTK theme."

This reverts commit cac6956430e23423886b7124b42b5a9450ccc21f.

Reason for revert: Suspected of causing consistent failure for mac-cocoa-rel tests

BrowserNonClientFrameViewBrowserTest.BrowserFrameColorThemed
BrowserNonClientFrameViewBrowserTest.BookmarkAppFrameColorSystemTheme

starting from:

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/mac-cocoa-rel/1090

Original change's description:
> Fix Linux hosted app / PWA window frame color with GTK theme.
>
> The title text color and side/bottom frame color now match the GTK
> theme, rather than the app's theme color (which clashed with the title
> bar background color, that would use the native color scheme regardless
> of the app theme color).
>
> Adds a browser test for GetFrameColor.
>
> Bug:  878636 
> Change-Id: If5eaca1bc800f6c52f3f49918f455ffa2d881016
> Reviewed-on: https://chromium-review.googlesource.com/1195226
> Commit-Queue: Matt Giuca <mgiuca@chromium.org>
> Reviewed-by: Bret Sepulveda <bsep@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#587490}

TBR=mgiuca@chromium.org,bsep@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  878636 ,  880363 
Change-Id: I999ae00f1bb5f1963e77c8394c47b8516a036343
Reviewed-on: https://chromium-review.googlesource.com/1204450
Commit-Queue: Samuel Huang <huangs@chromium.org>
Reviewed-by: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588581}
[modify] https://crrev.com/9e6f06a2d395fd32c5dc2112f77d6ad15cfebc9a/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
[modify] https://crrev.com/9e6f06a2d395fd32c5dc2112f77d6ad15cfebc9a/chrome/browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc
[modify] https://crrev.com/9e6f06a2d395fd32c5dc2112f77d6ad15cfebc9a/chrome/browser/ui/views/frame/opaque_browser_frame_view_linux.cc

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

commit d4f9bbd56e958e973205b04288c90d790c23afdc
Author: Matt Giuca <mgiuca@chromium.org>
Date: Thu Sep 06 01:53:29 2018

[reland] Fix Linux hosted app / PWA window frame color with GTK theme.

This is a reland of cac6956430e23423886b7124b42b5a9450ccc21f (r587490,
reverted in r588581).

The reland fixes a browser test crash on Mac Cocoa (because it's running
Views-only code) ( https://crbug.com/880363 ). Added a
ScopedMacViewsBrowserMode to force the test to run in Views mode.

Original change's description:

The title text color and side/bottom frame color now match the GTK
theme, rather than the app's theme color (which clashed with the title
bar background color, that would use the native color scheme regardless
of the app theme color).

Adds a browser test for GetFrameColor.

Bug:  878636 ,  880363 
Change-Id: I21003e78b4232cc6b8583267b08e2b37a823cfed
Reviewed-on: https://chromium-review.googlesource.com/1205972
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589086}
[modify] https://crrev.com/d4f9bbd56e958e973205b04288c90d790c23afdc/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
[modify] https://crrev.com/d4f9bbd56e958e973205b04288c90d790c23afdc/chrome/browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc
[modify] https://crrev.com/d4f9bbd56e958e973205b04288c90d790c23afdc/chrome/browser/ui/views/frame/opaque_browser_frame_view_linux.cc

Keeping this open while I keep an eye on https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/mac-cocoa-rel

Notes on merge (it's complicated):

- r587490 landed before M70 branch point.
- r588581 reverted after M70 branch point.
- r589086 landed today, not changing any production code but fixing tests so mac-cocoa-rel doesn't crash.

That means there's nothing to merge to M70 for production code, but mac-cocoa-rel would crash if it were run on the M70 branch. I'm not sure whether that's something we expect to pass (it doesn't seem that mac-cocoa-rel runs on the beta builders).

If we merge, I'll need to do a manual CL because r589086 won't cleanly apply. The merge CL would just be adding these 5 lines to the test:
https://chromium-review.googlesource.com/c/chromium/src/+/1205972/3..4/chrome/browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc

Successful build on https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/mac-cocoa-rel/1165

Requesting a merge to M70 branch, just to fix the test (see #10). Not sure if this is needed; it depends if we need tests to pass on mac-cocoa-rel on the M70 branch.

No need to test on Canary (since the production code isn't affected). We know it's correct because the bot passed.

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

+ellyjones - do we care about mac-cocoa tests failing starting M70? Could those tests be deleted, once we rollout MacViews too 100% M69?

#13: We do not - I'll mark this for later deletion.

Since I used test::ScopedMacViewsBrowserMode (which has a TODO to be deleted along with Mac Cocoa), I think you shouldn't have to make a note of anything. You'll naturally come across this when you delete test::ScopedMacViewsBrowserMode and should be able to just remove that one-line reference from BrowserNonClientFrameViewBrowserTest.

If we don't care about mac cocoa tests failing on the M70 branch (and this isn't going to be showing up red in the release manager's build tree) then we shouldn't have to merge anything.

per #14/15, rejecting merge.