New issue
Advanced search Search tips

Issue 811859 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

OmniboxPopupContentsViewTest.PopupAlignment/Rounded fails under --mash

Project Member Reported by jamescook@chromium.org, Feb 13 2018

Issue description

I'm not sure why this is different under mash_browser_tests (which run browser_tests --mash). Maybe it's touching an aura window property that is only supported in the window manager, hence the browser can't see it?

https://ci.chromium.org/buildbot/chromium.fyi/Mojo%20ChromiumOS/26942

Probably failing since it landed with https://chromium.googlesource.com/chromium/src/+/933f229bfdfa33dd7d5776bb4abf73078ea03c17

[ RUN      ] OmniboxPopupContentsViewTest.PopupAlignment/Rounded
[6026:6026:0213/092526.326416:WARNING:chrome_browser_main_chromeos.cc(611)] Running as stub user with profile dir: test-user
[6055:6055:0213/092526.971904:ERROR:shell_delegate_mus.cc(71)] Not implemented reached in virtual void ash::ShellDelegateMus::PreInit()
[6055:6055:0213/092527.011829:ERROR:layer_tree_host_impl.cc(2648)] Forcing zero-copy tile initialization as worker context is missing
[6026:6044:0213/092527.014112:ERROR:logging_chrome.cc(218)] Unable to create symlink /b/s/w/it9qOhWR/.org.chromium.Chromium.D1hBFN/ddnjqLl/test-user/chrome_debug.log pointing at /b/s/w/it9qOhWR/.org.chromium.Chromium.D1hBFN/ddnjqLl/test-user/chrome_debug_20180213-092527: No such file or directory (2)
[6055:6055:0213/092527.020030:ERROR:wallpaper_controller.cc(1247)] User is ephemeral or guest! Fallback to default wallpaper.
[6026:6026:0213/092527.031708:WARNING:user_session_manager.cc(1044)] Attempting to save user password for non enterprise user.
[6055:6055:0213/092527.084036:ERROR:shell_delegate_mus.cc(61)] Not implemented reached in virtual bool ash::ShellDelegateMus::CanShowWindowForUser(aura::Window *) const
[6026:6026:0213/092527.125792:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[6026:6026:0213/092527.126035:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[6026:6026:0213/092527.126386:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[6026:6026:0213/092527.127148:ERROR:network_type_pattern.cc(134)] NetworkTypePattern: wifi: Can not match empty type.
[6026:6026:0213/092527.158555:ERROR:remote_text_input_client.cc(135)] Not implemented reached in virtual void RemoteTextInputClient::OnInputMethodChanged()
[6026:6026:0213/092527.158587:ERROR:remote_text_input_client.cc(92)] Not implemented reached in virtual bool RemoteTextInputClient::HasCompositionText() const
[6026:6026:0213/092527.158597:ERROR:remote_text_input_client.cc(98)] Not implemented reached in virtual bool RemoteTextInputClient::GetTextRange(gfx::Range *) const
[6026:6026:0213/092527.158607:ERROR:remote_text_input_client.cc(153)] Not implemented reached in virtual void RemoteTextInputClient::EnsureCaretNotInRect(const gfx::Rect &)
[6026:6026:0213/092527.174141:ERROR:layer_tree_host_impl.cc(2648)] Forcing zero-copy tile initialization as worker context is missing
[6026:6026:0213/092527.200195:ERROR:layer_tree_host_impl.cc(2648)] Forcing zero-copy tile initialization as worker context is missing
[1:10:0213/092527.211597:ERROR:layer_tree_host_impl.cc(2648)] Forcing zero-copy tile initialization as worker context is missing
[6026:6085:0213/092527.238611:WARNING:simple_synchronous_entry.cc(1236)] Could not open platform files for entry.
[6026:6026:0213/092527.215225:FATAL:rounded_omnibox_results_frame.cc(132)] Check failed: shadow_layer.
#0 0x000003ed2eac base::debug::StackTrace::StackTrace()
#1 0x000003eed14b logging::LogMessage::~LogMessage()
#2 0x00000728c4dc RoundedOmniboxResultsFrame::AddedToWidget()
#3 0x000004e20db5 views::View::AddChildViewAt()
#4 0x000004e2b75a views::internal::RootView::SetContentsView()
#5 0x000004e2f174 views::Widget::SetContentsView()
#6 0x000007287c79 OmniboxPopupContentsView::UpdatePopupAppearance()
#7 0x000006bed4c8 OmniboxPopupModel::OnResultChanged()
#8 0x000006be7e40 OmniboxEditModel::OnCurrentMatchChanged()
#9 0x000006c21a21 OmniboxController::OnResultChanged()
#10 0x000006bbdfd1 AutocompleteController::UpdateResult()
#11 0x000006bbd97c AutocompleteController::Start()
#12 0x000006be50ac OmniboxEditModel::StartAutocomplete()
#13 0x0000013561fe OmniboxPopupContentsViewTest_PopupAlignment_Test::RunTestOnMainThread()
#14 0x00000450e4d1 content::BrowserTestBase::ProxyRunTestOnMainThreadLoop()
#15 0x0000040066ce ChromeBrowserMainParts::PreMainMessageLoopRunImpl()
#16 0x00000400546a ChromeBrowserMainParts::PreMainMessageLoopRun()
#17 0x0000019a4fd3 chromeos::ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun()
#18 0x00000275fcf1 content::BrowserMainLoop::PreMainMessageLoopRun()
#19 0x000002b9a7c5 content::StartupTaskRunner::RunAllTasksNow()
#20 0x00000275e2ad content::BrowserMainLoop::CreateStartupTasks()
#21 0x0000027627b3 content::BrowserMainRunnerImpl::Initialize()
#22 0x00000275bec2 content::BrowserMain()
#23 0x000003e9e731 content::ContentMainRunnerImpl::Run()
#24 0x000006183c37 service_manager::Main()
#25 0x000003e9cd24 content::ContentMain()
#26 0x00000450e13b content::BrowserTestBase::SetUp()
#27 0x000003fbfadb InProcessBrowserTest::SetUp()
#28 0x000001d25491 testing::Test::Run()
#29 0x000001d26050 testing::TestInfo::Run()
#30 0x000001d26537 testing::TestCase::Run()
#31 0x000001d2daa7 testing::internal::UnitTestImpl::RunAllTests()
#32 0x000001d2d6f7 testing::UnitTest::Run()
#33 0x000003fd4292 base::TestSuite::Run()
#34 0x000003ec6d99 ChromeTestSuiteRunner::RunTestSuite()
#35 0x000003ec6d10 (anonymous namespace)::MusTestLauncherDelegate::RunTestSuite()
#36 0x00000454294c content::LaunchTests()
#37 0x000003ec7264 LaunchChromeTests()
#38 0x000003ec6aa6 RunMashBrowserTests()

sky/estade, are shadow layers available to the browser?

 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 13 2018

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

commit 036f1a7835baee73ebb8268273a08ffd8b282a94
Author: James Cook <jamescook@chromium.org>
Date: Tue Feb 13 19:21:49 2018

cros: Skip OmniboxPopupContentsViewTest.PopupAlignment/Rounded on mash

Fails under browser_tests --mash, see bug. Disable on the FYI bot.

NOTRY=true
TBR=sky@chromium.org

Bug:  811859 
Test: browser_tests --mash
Change-Id: I0c5856fba03fb8038ec677489e9430c96d7b5995
Reviewed-on: https://chromium-review.googlesource.com/916630
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536417}
[modify] https://crrev.com/036f1a7835baee73ebb8268273a08ffd8b282a94/testing/buildbot/filters/mojo.fyi.mash.browser_tests.filter

Cc: -tapted@chromium.org
Owner: tapted@chromium.org
Status: Assigned (was: Untriaged)
tapted: I'm assigning to you to remove it from our triage queue. But feel free to reassign as necessary or to unassign again if you feel it should go back to our triage process after more information is available.

Comment 3 by tapted@chromium.org, Feb 14 2018

Hm - the code is using wm::ShadowController::GetShadowForWindow(..) in order to call Shadow::SetRoundedCornerRadius(..)

It would make sense that the shadow controller exists only in the mash window server. But I guess it currently only supports propagating the shadow elevation, not its corner radius.

Not much is using Shadow::SetRoundedCornerRadius():

 chrome/browser/ui/views/omnibox/rounded_omnibox_results_frame.cc:  shadow_layer->SetRoundedCornerRadius(
 components/exo/shell_surface_base.cc:      shadow->SetRoundedCornerRadius(0);


From my limited mash knowledge, it seems like the way forward is something like

 WM_CORE_EXPORT void SetWindowCornerRadius(aura::Window* window, int radius);
 WM_CORE_EXPORT extern const aura::WindowProperty<int>* const kWindowCornerRadiusKey;


Currently windows are either square, or the shadows are drawn inside the Widget's bounds. This means, e.g., you can click on the --secondary-ui-md shadow quite far below the bookmark bubble and still "hit" the bubble rather than dismiss it.

With shadows and corner radii getting bigger that's going to get more noticeable.

Follow-up question is whether we also need to expose this in Widget::InitParams, similar to the shadow_elevation.


(possible?) Alternative: Make wm::ShadowController do more. I.e. it currently picks a shadow elevation based on aura::client::WINDOW_TYPE_FOO. Would it make sense for it to also pick a corner radius?  (would it then also be responsible for clipping to that corner radius....?).

Let me know if you have any tips :)

Otherwise, I'll see if I can get wm::SetWindowCornerRadius(..) working for the new omnibox popup style under Mash.

Comment 4 by estade@google.com, Feb 14 2018

I like adding to InitParams. It's not something that I would see changing after init.
InitParams SGTM too.

Comment 6 by tapted@chromium.org, Feb 20 2018

Status: Started (was: Assigned)
Ok - I think I've got this going in https://chromium-review.googlesource.com/c/chromium/src/+/925943

Turns out: every window in Mus has *two* shadows -- one in each process. Except, the shadow in the browser process is clipped to the window bounds, so you can only see it if your window has rounded corners :).  That was a fun glitch to diagnose....

(So was missing that static_cast<PrimitiveType> - what a trap!)

There are some oddities around whether this should be an aura::Window property and/or described as an _InitProperty in the mojom. I went for an approach that simplifies the mash plumbing -- diverging from this seems to require a lot of boilerplate.
Screenshot from 2018-02-20 17-36-52.png
4.3 KB View Download
Components: -Internals>MUS Internals>Services>WindowService
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 28 2018

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

commit 0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38
Author: Trent Apted <tapted@chromium.org>
Date: Wed Feb 28 08:12:44 2018

Add an aura property for window corner radius. Propagate to mus.

Fix a bug in Mash where a root window would be given two shadows:
one each process. The one in the browser process would be clipped
to the root window bounds, so is only visible when the window has
rounded corners.

The corner radius is currently only used for window server shadows.
(E.g. it's not used in aura::Window::HitTest()).

Bug:  811859 , 801583
Change-Id: I260b0ff71e01181952d0ef4dfbfebd8ae95133b5
Reviewed-on: https://chromium-review.googlesource.com/925943
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539754}
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/chrome/browser/ui/views/omnibox/rounded_omnibox_results_frame.cc
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/components/exo/client_controlled_shell_surface_unittest.cc
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/components/exo/shell_surface_base.cc
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/services/ui/public/interfaces/window_manager.mojom
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/testing/buildbot/filters/mash.browser_tests.filter
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/ui/aura/client/aura_constants.cc
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/ui/aura/client/aura_constants.h
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/ui/aura/mus/property_converter.cc
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/ui/aura/mus/window_tree_client_unittest.cc
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/ui/views/mus/mus_client.cc
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/ui/views/test/views_test_base.h
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/ui/views/test/widget_test.h
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/ui/views/test/widget_test_aura.cc
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/ui/views/test/widget_test_mac.mm
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/ui/views/widget/native_widget_aura.cc
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/ui/views/widget/widget.h
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/ui/views/widget/widget_unittest.cc
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/ui/wm/core/shadow_controller.cc
[modify] https://crrev.com/0ec2d0ee6bbf66f2ad447b960ea0d3793c458a38/ui/wm/core/shadow_controller_unittest.cc

This is fixed. The event generator stuff is tracked in Issue 814675.
Status: Fixed (was: Started)

Sign in to add a comment