Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1778643002:
Moved AppMenu ACTIVATED/DEACTIVATED ink drop handling in to MenuButton. (Closed)
DescriptionMoved AppMenu ACTIVATED/DEACTIVATED ink drop handling in to MenuButton.
The AppMenuButton and the ToolbarActionView buttons were both handling
the ink drop animating to/from the ACTIVATED/DEACTIVATED states. This
change pushes that logic up in the common ancestor MenuButton.
BUG=592058
TEST=MenuButtonTest.*
TEST=ToolbarActionViewUnitTest.NoCrashWhenDestroyingToolbarActionViewThatHasAPressedLock
Committed: https://crrev.com/3ead9e2c8b5d650d7d72868ab10f147a86642b97
Cr-Commit-Position: refs/heads/master@{#380626}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Added tests and addressed comments. #
Total comments: 3
Patch Set 3 : Added TestInkDropDelegate. #Patch Set 4 : Merge with master. #Patch Set 5 : Added explicit to PressStateMenuButtonListener ctor. #Patch Set 6 : Removed extra ACTION_PENDING animation trigger from MenuButton::OnMousePressed(). #Patch Set 7 : Fixed crash when destroying a ToolbarActionView. #Patch Set 8 : Fixed bugs found on try-bots and added smaller scoped tests to find them. #Patch Set 9 : Added a check for a null ThemeProvider in ToolbarActionView::GetInkDropBaseColor(). #Patch Set 10 : Attempted to fix MenuButtonTest.InkDropStateForMenuButtonActivationsWithListenerThatDontReleaseAllL… #Patch Set 11 : fixed compile time typo #
Total comments: 1
Messages
Total messages: 29 (9 generated)
bruthig@chromium.org changed reviewers: + sky@chromium.org
sky@, can you please have a look?
varkha@chromium.org changed reviewers: + varkha@chromium.org
https://codereview.chromium.org/1778643002/diff/1/ui/views/controls/button/me... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/1778643002/diff/1/ui/views/controls/button/me... ui/views/controls/button/menu_button.cc:373: const ButtonState previous_state = state(); Could you not call OnAction here? SetState doesn't seem to do anything with the ink drop. https://codereview.chromium.org/1778643002/diff/1/ui/views/controls/button/me... ui/views/controls/button/menu_button.cc:375: if (previous_state != STATE_PRESSED && ink_drop_delegate()) { nit: no need for parentheses. https://codereview.chromium.org/1778643002/diff/1/ui/views/controls/button/me... ui/views/controls/button/menu_button.cc:395: (desired_state == STATE_NORMAL || desired_state == STATE_HOVERED || Is this same as desired_state != STATE_PRESSED?
This is fine by me. How about test coverage?
Description was changed from ========== Moved AppMenu ACTIVATED/DEACTIVATED ink drop handling in to MenuButton. The AppMenuButton and the ToolbarActionView buttons were both handling the ink drop animating to/from the ACTIVATED/DEACTIVATED states. This change pushes that logic up in the common ancestor MenuButton. BUG=567252 TEST=manual ========== to ========== Moved AppMenu ACTIVATED/DEACTIVATED ink drop handling in to MenuButton. The AppMenuButton and the ToolbarActionView buttons were both handling the ink drop animating to/from the ACTIVATED/DEACTIVATED states. This change pushes that logic up in the common ancestor MenuButton. BUG=592058 TEST=MenuButtonTest.* ==========
I've addressed comments and added tests. sky@, varkha@, PTAL https://codereview.chromium.org/1778643002/diff/1/ui/views/controls/button/me... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/1778643002/diff/1/ui/views/controls/button/me... ui/views/controls/button/menu_button.cc:373: const ButtonState previous_state = state(); On 2016/03/08 21:47:58, varkha wrote: > Could you not call OnAction here? SetState doesn't seem to do anything with the > ink drop. Done. https://codereview.chromium.org/1778643002/diff/1/ui/views/controls/button/me... ui/views/controls/button/menu_button.cc:375: if (previous_state != STATE_PRESSED && ink_drop_delegate()) { On 2016/03/08 21:47:58, varkha wrote: > nit: no need for parentheses. Done. https://codereview.chromium.org/1778643002/diff/1/ui/views/controls/button/me... ui/views/controls/button/menu_button.cc:395: (desired_state == STATE_NORMAL || desired_state == STATE_HOVERED || On 2016/03/08 21:47:58, varkha wrote: > Is this same as desired_state != STATE_PRESSED? This has changed a bit, varkha@, please take a look.
https://codereview.chromium.org/1778643002/diff/20001/ui/views/controls/butto... File ui/views/controls/button/menu_button_unittest.cc (right): https://codereview.chromium.org/1778643002/diff/20001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:169: PressStateMenuButtonListener(bool release_lock) explicit https://codereview.chromium.org/1778643002/diff/20001/ui/views/views.gyp File ui/views/views.gyp (right): https://codereview.chromium.org/1778643002/diff/20001/ui/views/views.gyp#newc... ui/views/views.gyp:497: 'animation/test/test_ink_drop_delegate.cc', Did you forget to git add these files?
On 2016/03/09 16:21:38, sky wrote: > https://codereview.chromium.org/1778643002/diff/20001/ui/views/controls/butto... > File ui/views/controls/button/menu_button_unittest.cc (right): > > https://codereview.chromium.org/1778643002/diff/20001/ui/views/controls/butto... > ui/views/controls/button/menu_button_unittest.cc:169: > PressStateMenuButtonListener(bool release_lock) > explicit > > https://codereview.chromium.org/1778643002/diff/20001/ui/views/views.gyp > File ui/views/views.gyp (right): > > https://codereview.chromium.org/1778643002/diff/20001/ui/views/views.gyp#newc... > ui/views/views.gyp:497: 'animation/test/test_ink_drop_delegate.cc', > Did you forget to git add these files? My bad, added now.
LGTM
Description was changed from ========== Moved AppMenu ACTIVATED/DEACTIVATED ink drop handling in to MenuButton. The AppMenuButton and the ToolbarActionView buttons were both handling the ink drop animating to/from the ACTIVATED/DEACTIVATED states. This change pushes that logic up in the common ancestor MenuButton. BUG=592058 TEST=MenuButtonTest.* ========== to ========== Moved AppMenu ACTIVATED/DEACTIVATED ink drop handling in to MenuButton. The AppMenuButton and the ToolbarActionView buttons were both handling the ink drop animating to/from the ACTIVATED/DEACTIVATED states. This change pushes that logic up in the common ancestor MenuButton. BUG=592058 TEST=MenuButtonTest.* TEST=ToolbarActionViewUnitTest.NoCrashWhenDestroyingToolbarActionViewThatHasAPressedLock ==========
sky@, I know you already lgtm'd this but I had to add fix some failing tests and in the process I added a few new tests and tweaked some production code. Can you check out the delta since patch set 4? estade@, it appears that the ToolbarActionView's don't always have a ThemeProvider available. e.g. It was causing the MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction test. I've updated ToolbarActionView::GetInkDropBaseColor() slightly, can you please have a look? https://codereview.chromium.org/1778643002/diff/20001/ui/views/controls/butto... File ui/views/controls/button/menu_button_unittest.cc (right): https://codereview.chromium.org/1778643002/diff/20001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:169: PressStateMenuButtonListener(bool release_lock) On 2016/03/09 16:21:38, sky wrote: > explicit Done.
bruthig@chromium.org changed reviewers: + estade@chromium.org
(Actually adding estade@ this time...) FYI estade@, it appears that the ToolbarActionView's don't always have a ThemeProvider available. e.g. It was causing the MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction test. I've updated ToolbarActionView::GetInkDropBaseColor() slightly, thought we should re-visit this in a follow-up. WDYT?
SLGTM
The CQ bit was checked by bruthig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1778643002/#ps200001 (title: "fixed compile time typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778643002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778643002/200001
Message was sent while issue was closed.
Description was changed from ========== Moved AppMenu ACTIVATED/DEACTIVATED ink drop handling in to MenuButton. The AppMenuButton and the ToolbarActionView buttons were both handling the ink drop animating to/from the ACTIVATED/DEACTIVATED states. This change pushes that logic up in the common ancestor MenuButton. BUG=592058 TEST=MenuButtonTest.* TEST=ToolbarActionViewUnitTest.NoCrashWhenDestroyingToolbarActionViewThatHasAPressedLock ========== to ========== Moved AppMenu ACTIVATED/DEACTIVATED ink drop handling in to MenuButton. The AppMenuButton and the ToolbarActionView buttons were both handling the ink drop animating to/from the ACTIVATED/DEACTIVATED states. This change pushes that logic up in the common ancestor MenuButton. BUG=592058 TEST=MenuButtonTest.* TEST=ToolbarActionViewUnitTest.NoCrashWhenDestroyingToolbarActionViewThatHasAPressedLock ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Moved AppMenu ACTIVATED/DEACTIVATED ink drop handling in to MenuButton. The AppMenuButton and the ToolbarActionView buttons were both handling the ink drop animating to/from the ACTIVATED/DEACTIVATED states. This change pushes that logic up in the common ancestor MenuButton. BUG=592058 TEST=MenuButtonTest.* TEST=ToolbarActionViewUnitTest.NoCrashWhenDestroyingToolbarActionViewThatHasAPressedLock ========== to ========== Moved AppMenu ACTIVATED/DEACTIVATED ink drop handling in to MenuButton. The AppMenuButton and the ToolbarActionView buttons were both handling the ink drop animating to/from the ACTIVATED/DEACTIVATED states. This change pushes that logic up in the common ancestor MenuButton. BUG=592058 TEST=MenuButtonTest.* TEST=ToolbarActionViewUnitTest.NoCrashWhenDestroyingToolbarActionViewThatHasAPressedLock Committed: https://crrev.com/3ead9e2c8b5d650d7d72868ab10f147a86642b97 Cr-Commit-Position: refs/heads/master@{#380626} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/3ead9e2c8b5d650d7d72868ab10f147a86642b97 Cr-Commit-Position: refs/heads/master@{#380626}
Message was sent while issue was closed.
https://codereview.chromium.org/1778643002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1778643002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:122: return GetThemeProvider() this doesn't seem right to me. I think your change is causing GetInkDropBaseColor to be called before |this| is added to a widget, which we probably don't need/want. What was the callstack?
Message was sent while issue was closed.
On 2016/03/14 21:20:43, Evan Stade wrote: > https://codereview.chromium.org/1778643002/diff/200001/chrome/browser/ui/view... > File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): > > https://codereview.chromium.org/1778643002/diff/200001/chrome/browser/ui/view... > chrome/browser/ui/views/toolbar/toolbar_action_view.cc:122: return > GetThemeProvider() > this doesn't seem right to me. I think your change is causing > GetInkDropBaseColor to be called before |this| is added to a widget, which we > probably don't need/want. What was the callstack? Here's the stack trace, you can repro by adding a DCHECK(GetThemeProvider()); in ToolbarActionView::GetInkDropBaseColor() and running 'out/Debug/browser_tests --gtest_filter=*MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction*' #0 0x000000907a11 __interceptor_backtrace #1 0x7efe72b585ce base::debug::StackTrace::StackTrace() #2 0x7efe72c813ff logging::LogMessage::~LogMessage() #3 0x0000072a1555 ToolbarActionView::GetInkDropBaseColor() #4 0x7efe4f3fe7f6 views::InkDropHostView::CreateInkDropAnimation() #5 0x7efe4f458042 views::LabelButton::CreateInkDropAnimation() #6 0x7efe4f4587b3 views::LabelButton::CreateInkDropAnimation() #7 0x7efe4f3f8720 views::InkDropAnimationControllerImpl::CreateInkDropAnimation() #8 0x7efe4f3f83f9 views::InkDropAnimationControllerImpl::AnimateToState() #9 0x7efe4f3f060b views::ButtonInkDropDelegate::OnAction() #10 0x7efe4f460499 views::MenuButton::IncrementPressedLocked() #11 0x7efe4f4601ea views::MenuButton::PressedLock::PressedLock() #12 0x7efe4f46017f views::MenuButton::PressedLock::PressedLock() #13 0x0000072a3534 ToolbarActionView::OnPopupShown() #14 0x00000754e781 MediaRouterAction::OnPopupShown() #15 0x000007558add media_router::MediaRouterDialogControllerImpl::CreateMediaRouterDialog() #16 0x00000d283d76 media_router::MediaRouterDialogController::ShowMediaRouterDialog() #17 0x00000754d3ea MediaRouterAction::ExecuteAction() #18 0x0000033bca4c media_router::MediaRouterUIBrowserTest::ExecuteMediaRouterAction() #19 0x0000033bda5b _ZN4base8internal15RunnableAdapterIMN12media_router24MediaRouterUIBrowserTestEFvP13AppMenuButtonEE3RunIJRKS5_EEEvPS3_DpOT_ #20 0x0000033bd89f _ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMN12media_router24MediaRouterUIBrowserTestEFvP13AppMenuButtonEEEE8MakeItSoIJPS4_RKS6_EEEvS9_DpOT_ #21 0x0000033bd670 _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0ELm1EEEENS0_9BindStateINS0_15RunnableAdapterIMN12media_router24MediaRouterUIBrowserTestEFvP13AppMenuButtonEEEFvPS7_S9_EJSD_RS9_EEENS0_12InvokeHelperILb0EvSC_EEFvvEE3RunEPNS0_13BindStateBaseE #22 0x7efe72afb8b4 base::Callback<>::Run() #23 0x7efe72b66ae1 base::debug::TaskAnnotator::RunTask() #24 0x7efe72cd6552 base::MessageLoop::RunTask() #25 0x7efe72cd6e0a base::MessageLoop::DeferOrRunPendingTask() #26 0x7efe72cd7f78 base::MessageLoop::DoWork() #27 0x7efe72a95764 base::MessagePumpGlib::Run() #28 0x7efe72cd52d9 base::MessageLoop::RunHandler() #29 0x7efe72e2104c base::RunLoop::Run() #30 0x7efe4f783ee1 views::MenuMessageLoopAura::Run() #31 0x7efe4f4a0c90 views::MenuController::RunMessageLoop() #32 0x7efe4f49eb92 views::MenuController::Run() #33 0x7efe4f506000 views::internal::MenuRunnerImpl::RunMenuAt() #34 0x7efe4f50413e views::MenuRunner::RunMenuAt() #35 0x000007eef9a1 AppMenu::RunMenu() #36 0x0000072640a1 AppMenuButton::ShowMenu() #37 0x0000033b9fe5 media_router::MediaRouterUIBrowserTest::OpenMediaRouterDialogAndWaitForNewWebContents() #38 0x0000033b983d media_router::MediaRouterUIBrowserTest_OpenDialogWithMediaRouterAction_Test::RunTestOnMainThread() #39 0x00000653b783 InProcessBrowserTest::RunTestOnMainThreadLoop() #40 0x00000668b304 content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() #41 0x00000668f20b _ZN4base8internal15RunnableAdapterIMN7content15BrowserTestBaseEFvvEE3RunIJEEEvPS3_DpOT_ #42 0x00000668f09d _ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMN7content15BrowserTestBaseEFvvEEEE8MakeItSoIJPS4_EEEvS7_DpOT_ #43 0x00000668eea9 _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0EEEENS0_9BindStateINS0_15RunnableAdapterIMN7content15BrowserTestBaseEFvvEEEFvPS7_EJSB_EEENS0_12InvokeHelperILb0EvSA_EEFvvEE3RunEPNS0_13BindStateBaseE #44 0x000000aff044 base::Callback<>::Run() #45 0x0000058e1147 ChromeBrowserMainParts::PreMainMessageLoopRunImpl() #46 0x0000058dc82f ChromeBrowserMainParts::PreMainMessageLoopRun() #47 0x7efe6334c2ac content::BrowserMainLoop::PreMainMessageLoopRun() #48 0x7efe6335f28b _ZN4base8internal15RunnableAdapterIMN7content15BrowserMainLoopEFivEE3RunIJEEEiPS3_DpOT_ #49 0x7efe6335efcd _ZN4base8internal12InvokeHelperILb0EiNS0_15RunnableAdapterIMN7content15BrowserMainLoopEFivEEEE8MakeItSoIJPS4_EEEiS7_DpOT_ #50 0x7efe6335edc1 _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0EEEENS0_9BindStateINS0_15RunnableAdapterIMN7content15BrowserMainLoopEFivEEEFiPS7_EJNS0_17UnretainedWrapperIS7_EEEEENS0_12InvokeHelperILb0EiSA_EEFivEE3RunEPNS0_13BindStateBaseE #51 0x7efe646cf5d4 base::Callback<>::Run() #52 0x7efe654b3b87 content::StartupTaskRunner::RunAllTasksNow() #53 0x7efe63344fe2 content::BrowserMainLoop::CreateStartupTasks() #54 0x7efe633646c2 content::BrowserMainRunnerImpl::Initialize() #55 0x7efe633323aa content::BrowserMain() #56 0x7efe62cdb6bf content::RunNamedProcessTypeMain() #57 0x7efe62ce3352 content::ContentMainRunnerImpl::Run() #58 0x7efe62cd9d95 content::ContentMain() #59 0x00000668ab47 content::BrowserTestBase::SetUp() #60 0x000006535625 InProcessBrowserTest::SetUp() #61 0x000006b6623c testing::internal::HandleSehExceptionsInMethodIfSupported<>()
Message was sent while issue was closed.
On 2016/03/15 15:42:01, bruthig wrote: > On 2016/03/14 21:20:43, Evan Stade wrote: > > > https://codereview.chromium.org/1778643002/diff/200001/chrome/browser/ui/view... > > File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): > > > > > https://codereview.chromium.org/1778643002/diff/200001/chrome/browser/ui/view... > > chrome/browser/ui/views/toolbar/toolbar_action_view.cc:122: return > > GetThemeProvider() > > this doesn't seem right to me. I think your change is causing > > GetInkDropBaseColor to be called before |this| is added to a widget, which we > > probably don't need/want. What was the callstack? > > Here's the stack trace, you can repro by adding a DCHECK(GetThemeProvider()); in > ToolbarActionView::GetInkDropBaseColor() and running 'out/Debug/browser_tests > --gtest_filter=*MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction*' > > > #0 0x000000907a11 __interceptor_backtrace > #1 0x7efe72b585ce base::debug::StackTrace::StackTrace() > #2 0x7efe72c813ff logging::LogMessage::~LogMessage() > #3 0x0000072a1555 ToolbarActionView::GetInkDropBaseColor() > #4 0x7efe4f3fe7f6 views::InkDropHostView::CreateInkDropAnimation() > #5 0x7efe4f458042 views::LabelButton::CreateInkDropAnimation() > #6 0x7efe4f4587b3 views::LabelButton::CreateInkDropAnimation() > #7 0x7efe4f3f8720 > views::InkDropAnimationControllerImpl::CreateInkDropAnimation() > #8 0x7efe4f3f83f9 views::InkDropAnimationControllerImpl::AnimateToState() > #9 0x7efe4f3f060b views::ButtonInkDropDelegate::OnAction() > #10 0x7efe4f460499 views::MenuButton::IncrementPressedLocked() > #11 0x7efe4f4601ea views::MenuButton::PressedLock::PressedLock() > #12 0x7efe4f46017f views::MenuButton::PressedLock::PressedLock() > #13 0x0000072a3534 ToolbarActionView::OnPopupShown() > #14 0x00000754e781 MediaRouterAction::OnPopupShown() > #15 0x000007558add > media_router::MediaRouterDialogControllerImpl::CreateMediaRouterDialog() > #16 0x00000d283d76 > media_router::MediaRouterDialogController::ShowMediaRouterDialog() > #17 0x00000754d3ea MediaRouterAction::ExecuteAction() > #18 0x0000033bca4c > media_router::MediaRouterUIBrowserTest::ExecuteMediaRouterAction() > #19 0x0000033bda5b > _ZN4base8internal15RunnableAdapterIMN12media_router24MediaRouterUIBrowserTestEFvP13AppMenuButtonEE3RunIJRKS5_EEEvPS3_DpOT_ > #20 0x0000033bd89f > _ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMN12media_router24MediaRouterUIBrowserTestEFvP13AppMenuButtonEEEE8MakeItSoIJPS4_RKS6_EEEvS9_DpOT_ > #21 0x0000033bd670 > _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0ELm1EEEENS0_9BindStateINS0_15RunnableAdapterIMN12media_router24MediaRouterUIBrowserTestEFvP13AppMenuButtonEEEFvPS7_S9_EJSD_RS9_EEENS0_12InvokeHelperILb0EvSC_EEFvvEE3RunEPNS0_13BindStateBaseE > #22 0x7efe72afb8b4 base::Callback<>::Run() > #23 0x7efe72b66ae1 base::debug::TaskAnnotator::RunTask() > #24 0x7efe72cd6552 base::MessageLoop::RunTask() > #25 0x7efe72cd6e0a base::MessageLoop::DeferOrRunPendingTask() > #26 0x7efe72cd7f78 base::MessageLoop::DoWork() > #27 0x7efe72a95764 base::MessagePumpGlib::Run() > #28 0x7efe72cd52d9 base::MessageLoop::RunHandler() > #29 0x7efe72e2104c base::RunLoop::Run() > #30 0x7efe4f783ee1 views::MenuMessageLoopAura::Run() > #31 0x7efe4f4a0c90 views::MenuController::RunMessageLoop() > #32 0x7efe4f49eb92 views::MenuController::Run() > #33 0x7efe4f506000 views::internal::MenuRunnerImpl::RunMenuAt() > #34 0x7efe4f50413e views::MenuRunner::RunMenuAt() > #35 0x000007eef9a1 AppMenu::RunMenu() > #36 0x0000072640a1 AppMenuButton::ShowMenu() > #37 0x0000033b9fe5 > media_router::MediaRouterUIBrowserTest::OpenMediaRouterDialogAndWaitForNewWebContents() > #38 0x0000033b983d > media_router::MediaRouterUIBrowserTest_OpenDialogWithMediaRouterAction_Test::RunTestOnMainThread() > #39 0x00000653b783 InProcessBrowserTest::RunTestOnMainThreadLoop() > #40 0x00000668b304 content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() > #41 0x00000668f20b > _ZN4base8internal15RunnableAdapterIMN7content15BrowserTestBaseEFvvEE3RunIJEEEvPS3_DpOT_ > #42 0x00000668f09d > _ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMN7content15BrowserTestBaseEFvvEEEE8MakeItSoIJPS4_EEEvS7_DpOT_ > #43 0x00000668eea9 > _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0EEEENS0_9BindStateINS0_15RunnableAdapterIMN7content15BrowserTestBaseEFvvEEEFvPS7_EJSB_EEENS0_12InvokeHelperILb0EvSA_EEFvvEE3RunEPNS0_13BindStateBaseE > #44 0x000000aff044 base::Callback<>::Run() > #45 0x0000058e1147 ChromeBrowserMainParts::PreMainMessageLoopRunImpl() > #46 0x0000058dc82f ChromeBrowserMainParts::PreMainMessageLoopRun() > #47 0x7efe6334c2ac content::BrowserMainLoop::PreMainMessageLoopRun() > #48 0x7efe6335f28b > _ZN4base8internal15RunnableAdapterIMN7content15BrowserMainLoopEFivEE3RunIJEEEiPS3_DpOT_ > #49 0x7efe6335efcd > _ZN4base8internal12InvokeHelperILb0EiNS0_15RunnableAdapterIMN7content15BrowserMainLoopEFivEEEE8MakeItSoIJPS4_EEEiS7_DpOT_ > #50 0x7efe6335edc1 > _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0EEEENS0_9BindStateINS0_15RunnableAdapterIMN7content15BrowserMainLoopEFivEEEFiPS7_EJNS0_17UnretainedWrapperIS7_EEEEENS0_12InvokeHelperILb0EiSA_EEFivEE3RunEPNS0_13BindStateBaseE > #51 0x7efe646cf5d4 base::Callback<>::Run() > #52 0x7efe654b3b87 content::StartupTaskRunner::RunAllTasksNow() > #53 0x7efe63344fe2 content::BrowserMainLoop::CreateStartupTasks() > #54 0x7efe633646c2 content::BrowserMainRunnerImpl::Initialize() > #55 0x7efe633323aa content::BrowserMain() > #56 0x7efe62cdb6bf content::RunNamedProcessTypeMain() > #57 0x7efe62ce3352 content::ContentMainRunnerImpl::Run() > #58 0x7efe62cd9d95 content::ContentMain() > #59 0x00000668ab47 content::BrowserTestBase::SetUp() > #60 0x000006535625 InProcessBrowserTest::SetUp() > #61 0x000006b6623c testing::internal::HandleSehExceptionsInMethodIfSupported<>() yea, I think you need to change ToolbarActionView to not do this view_controller_->SetDelegate(this); or this UpdateState(); in the ctor. Instead, move that to ViewHierarchyChanged (see many other places like https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... )
Message was sent while issue was closed.
On 2016/03/15 19:30:58, Evan Stade wrote: > On 2016/03/15 15:42:01, bruthig wrote: > > On 2016/03/14 21:20:43, Evan Stade wrote: > > > > > > https://codereview.chromium.org/1778643002/diff/200001/chrome/browser/ui/view... > > > File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): > > > > > > > > > https://codereview.chromium.org/1778643002/diff/200001/chrome/browser/ui/view... > > > chrome/browser/ui/views/toolbar/toolbar_action_view.cc:122: return > > > GetThemeProvider() > > > this doesn't seem right to me. I think your change is causing > > > GetInkDropBaseColor to be called before |this| is added to a widget, which > we > > > probably don't need/want. What was the callstack? > > > > Here's the stack trace, you can repro by adding a DCHECK(GetThemeProvider()); > in > > ToolbarActionView::GetInkDropBaseColor() and running 'out/Debug/browser_tests > > --gtest_filter=*MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction*' > > > > > > #0 0x000000907a11 __interceptor_backtrace > > #1 0x7efe72b585ce base::debug::StackTrace::StackTrace() > > #2 0x7efe72c813ff logging::LogMessage::~LogMessage() > > #3 0x0000072a1555 ToolbarActionView::GetInkDropBaseColor() > > #4 0x7efe4f3fe7f6 views::InkDropHostView::CreateInkDropAnimation() > > #5 0x7efe4f458042 views::LabelButton::CreateInkDropAnimation() > > #6 0x7efe4f4587b3 views::LabelButton::CreateInkDropAnimation() > > #7 0x7efe4f3f8720 > > views::InkDropAnimationControllerImpl::CreateInkDropAnimation() > > #8 0x7efe4f3f83f9 views::InkDropAnimationControllerImpl::AnimateToState() > > #9 0x7efe4f3f060b views::ButtonInkDropDelegate::OnAction() > > #10 0x7efe4f460499 views::MenuButton::IncrementPressedLocked() > > #11 0x7efe4f4601ea views::MenuButton::PressedLock::PressedLock() > > #12 0x7efe4f46017f views::MenuButton::PressedLock::PressedLock() > > #13 0x0000072a3534 ToolbarActionView::OnPopupShown() > > #14 0x00000754e781 MediaRouterAction::OnPopupShown() > > #15 0x000007558add > > media_router::MediaRouterDialogControllerImpl::CreateMediaRouterDialog() > > #16 0x00000d283d76 > > media_router::MediaRouterDialogController::ShowMediaRouterDialog() > > #17 0x00000754d3ea MediaRouterAction::ExecuteAction() > > #18 0x0000033bca4c > > media_router::MediaRouterUIBrowserTest::ExecuteMediaRouterAction() > > #19 0x0000033bda5b > > > _ZN4base8internal15RunnableAdapterIMN12media_router24MediaRouterUIBrowserTestEFvP13AppMenuButtonEE3RunIJRKS5_EEEvPS3_DpOT_ > > #20 0x0000033bd89f > > > _ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMN12media_router24MediaRouterUIBrowserTestEFvP13AppMenuButtonEEEE8MakeItSoIJPS4_RKS6_EEEvS9_DpOT_ > > #21 0x0000033bd670 > > > _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0ELm1EEEENS0_9BindStateINS0_15RunnableAdapterIMN12media_router24MediaRouterUIBrowserTestEFvP13AppMenuButtonEEEFvPS7_S9_EJSD_RS9_EEENS0_12InvokeHelperILb0EvSC_EEFvvEE3RunEPNS0_13BindStateBaseE > > #22 0x7efe72afb8b4 base::Callback<>::Run() > > #23 0x7efe72b66ae1 base::debug::TaskAnnotator::RunTask() > > #24 0x7efe72cd6552 base::MessageLoop::RunTask() > > #25 0x7efe72cd6e0a base::MessageLoop::DeferOrRunPendingTask() > > #26 0x7efe72cd7f78 base::MessageLoop::DoWork() > > #27 0x7efe72a95764 base::MessagePumpGlib::Run() > > #28 0x7efe72cd52d9 base::MessageLoop::RunHandler() > > #29 0x7efe72e2104c base::RunLoop::Run() > > #30 0x7efe4f783ee1 views::MenuMessageLoopAura::Run() > > #31 0x7efe4f4a0c90 views::MenuController::RunMessageLoop() > > #32 0x7efe4f49eb92 views::MenuController::Run() > > #33 0x7efe4f506000 views::internal::MenuRunnerImpl::RunMenuAt() > > #34 0x7efe4f50413e views::MenuRunner::RunMenuAt() > > #35 0x000007eef9a1 AppMenu::RunMenu() > > #36 0x0000072640a1 AppMenuButton::ShowMenu() > > #37 0x0000033b9fe5 > > > media_router::MediaRouterUIBrowserTest::OpenMediaRouterDialogAndWaitForNewWebContents() > > #38 0x0000033b983d > > > media_router::MediaRouterUIBrowserTest_OpenDialogWithMediaRouterAction_Test::RunTestOnMainThread() > > #39 0x00000653b783 InProcessBrowserTest::RunTestOnMainThreadLoop() > > #40 0x00000668b304 content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() > > #41 0x00000668f20b > > > _ZN4base8internal15RunnableAdapterIMN7content15BrowserTestBaseEFvvEE3RunIJEEEvPS3_DpOT_ > > #42 0x00000668f09d > > > _ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMN7content15BrowserTestBaseEFvvEEEE8MakeItSoIJPS4_EEEvS7_DpOT_ > > #43 0x00000668eea9 > > > _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0EEEENS0_9BindStateINS0_15RunnableAdapterIMN7content15BrowserTestBaseEFvvEEEFvPS7_EJSB_EEENS0_12InvokeHelperILb0EvSA_EEFvvEE3RunEPNS0_13BindStateBaseE > > #44 0x000000aff044 base::Callback<>::Run() > > #45 0x0000058e1147 ChromeBrowserMainParts::PreMainMessageLoopRunImpl() > > #46 0x0000058dc82f ChromeBrowserMainParts::PreMainMessageLoopRun() > > #47 0x7efe6334c2ac content::BrowserMainLoop::PreMainMessageLoopRun() > > #48 0x7efe6335f28b > > > _ZN4base8internal15RunnableAdapterIMN7content15BrowserMainLoopEFivEE3RunIJEEEiPS3_DpOT_ > > #49 0x7efe6335efcd > > > _ZN4base8internal12InvokeHelperILb0EiNS0_15RunnableAdapterIMN7content15BrowserMainLoopEFivEEEE8MakeItSoIJPS4_EEEiS7_DpOT_ > > #50 0x7efe6335edc1 > > > _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0EEEENS0_9BindStateINS0_15RunnableAdapterIMN7content15BrowserMainLoopEFivEEEFiPS7_EJNS0_17UnretainedWrapperIS7_EEEEENS0_12InvokeHelperILb0EiSA_EEFivEE3RunEPNS0_13BindStateBaseE > > #51 0x7efe646cf5d4 base::Callback<>::Run() > > #52 0x7efe654b3b87 content::StartupTaskRunner::RunAllTasksNow() > > #53 0x7efe63344fe2 content::BrowserMainLoop::CreateStartupTasks() > > #54 0x7efe633646c2 content::BrowserMainRunnerImpl::Initialize() > > #55 0x7efe633323aa content::BrowserMain() > > #56 0x7efe62cdb6bf content::RunNamedProcessTypeMain() > > #57 0x7efe62ce3352 content::ContentMainRunnerImpl::Run() > > #58 0x7efe62cd9d95 content::ContentMain() > > #59 0x00000668ab47 content::BrowserTestBase::SetUp() > > #60 0x000006535625 InProcessBrowserTest::SetUp() > > #61 0x000006b6623c > testing::internal::HandleSehExceptionsInMethodIfSupported<>() > > yea, I think you need to change ToolbarActionView to not do this > > view_controller_->SetDelegate(this); > > or this > > UpdateState(); > > in the ctor. Instead, move that to ViewHierarchyChanged (see many other places > like > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > ) If you parse through what is happening in the MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction test you will find that it is indeed attempting to use a ToolbarActionView that is not hosted within a Widget. So I guess fundamentally I am questioning why are we ok with assuming a Widget is hosting the ToolbarActionView when GetInkDropBaseColor() is called? Although this assumption may always be true in production, it would be a lot easier to test the interaction of the ToolbarActionView together with the InkDrop(Hover|Animation) classes without requiring the overhead of a Widget to host them. Let me be clear here, my concern is more about about how the ThemeProviders are found and and used and it is obviously a much larger discussion than these one or two InkDrop related cases. I'm only calling attention to it here as a concrete example, for discussions sake, as to how these implicit dependencies cause tests to include more than they really should be. sky@, estade@, What are your thoughts on this? FYI I have posted a better fix here: https://codereview.chromium.org/1805393002/
Message was sent while issue was closed.
I wouldn't expect a browsertest to create an environment that doesn't match production. What is MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction doing that the button is not in a widget? -Scott On Thu, Mar 17, 2016 at 8:47 AM, <bruthig@chromium.org> wrote: > On 2016/03/15 19:30:58, Evan Stade wrote: >> On 2016/03/15 15:42:01, bruthig wrote: >> > On 2016/03/14 21:20:43, Evan Stade wrote: >> > > >> > >> > https://codereview.chromium.org/1778643002/diff/200001/chrome/browser/ui/view... >> > > File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): >> > > >> > > >> > >> > https://codereview.chromium.org/1778643002/diff/200001/chrome/browser/ui/view... >> > > chrome/browser/ui/views/toolbar/toolbar_action_view.cc:122: return >> > > GetThemeProvider() >> > > this doesn't seem right to me. I think your change is causing >> > > GetInkDropBaseColor to be called before |this| is added to a widget, >> > > which >> we >> > > probably don't need/want. What was the callstack? >> > >> > Here's the stack trace, you can repro by adding a > DCHECK(GetThemeProvider()); >> in >> > ToolbarActionView::GetInkDropBaseColor() and running > 'out/Debug/browser_tests >> > >> > --gtest_filter=*MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction*' >> > >> > >> > #0 0x000000907a11 __interceptor_backtrace >> > #1 0x7efe72b585ce base::debug::StackTrace::StackTrace() >> > #2 0x7efe72c813ff logging::LogMessage::~LogMessage() >> > #3 0x0000072a1555 ToolbarActionView::GetInkDropBaseColor() >> > #4 0x7efe4f3fe7f6 views::InkDropHostView::CreateInkDropAnimation() >> > #5 0x7efe4f458042 views::LabelButton::CreateInkDropAnimation() >> > #6 0x7efe4f4587b3 views::LabelButton::CreateInkDropAnimation() >> > #7 0x7efe4f3f8720 >> > views::InkDropAnimationControllerImpl::CreateInkDropAnimation() >> > #8 0x7efe4f3f83f9 >> > views::InkDropAnimationControllerImpl::AnimateToState() >> > #9 0x7efe4f3f060b views::ButtonInkDropDelegate::OnAction() >> > #10 0x7efe4f460499 views::MenuButton::IncrementPressedLocked() >> > #11 0x7efe4f4601ea views::MenuButton::PressedLock::PressedLock() >> > #12 0x7efe4f46017f views::MenuButton::PressedLock::PressedLock() >> > #13 0x0000072a3534 ToolbarActionView::OnPopupShown() >> > #14 0x00000754e781 MediaRouterAction::OnPopupShown() >> > #15 0x000007558add >> > media_router::MediaRouterDialogControllerImpl::CreateMediaRouterDialog() >> > #16 0x00000d283d76 >> > media_router::MediaRouterDialogController::ShowMediaRouterDialog() >> > #17 0x00000754d3ea MediaRouterAction::ExecuteAction() >> > #18 0x0000033bca4c >> > media_router::MediaRouterUIBrowserTest::ExecuteMediaRouterAction() >> > #19 0x0000033bda5b >> > >> > _ZN4base8internal15RunnableAdapterIMN12media_router24MediaRouterUIBrowserTestEFvP13AppMenuButtonEE3RunIJRKS5_EEEvPS3_DpOT_ >> > #20 0x0000033bd89f >> > >> > _ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMN12media_router24MediaRouterUIBrowserTestEFvP13AppMenuButtonEEEE8MakeItSoIJPS4_RKS6_EEEvS9_DpOT_ >> > #21 0x0000033bd670 >> > >> > _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0ELm1EEEENS0_9BindStateINS0_15RunnableAdapterIMN12media_router24MediaRouterUIBrowserTestEFvP13AppMenuButtonEEEFvPS7_S9_EJSD_RS9_EEENS0_12InvokeHelperILb0EvSC_EEFvvEE3RunEPNS0_13BindStateBaseE >> > #22 0x7efe72afb8b4 base::Callback<>::Run() >> > #23 0x7efe72b66ae1 base::debug::TaskAnnotator::RunTask() >> > #24 0x7efe72cd6552 base::MessageLoop::RunTask() >> > #25 0x7efe72cd6e0a base::MessageLoop::DeferOrRunPendingTask() >> > #26 0x7efe72cd7f78 base::MessageLoop::DoWork() >> > #27 0x7efe72a95764 base::MessagePumpGlib::Run() >> > #28 0x7efe72cd52d9 base::MessageLoop::RunHandler() >> > #29 0x7efe72e2104c base::RunLoop::Run() >> > #30 0x7efe4f783ee1 views::MenuMessageLoopAura::Run() >> > #31 0x7efe4f4a0c90 views::MenuController::RunMessageLoop() >> > #32 0x7efe4f49eb92 views::MenuController::Run() >> > #33 0x7efe4f506000 views::internal::MenuRunnerImpl::RunMenuAt() >> > #34 0x7efe4f50413e views::MenuRunner::RunMenuAt() >> > #35 0x000007eef9a1 AppMenu::RunMenu() >> > #36 0x0000072640a1 AppMenuButton::ShowMenu() >> > #37 0x0000033b9fe5 >> > >> > media_router::MediaRouterUIBrowserTest::OpenMediaRouterDialogAndWaitForNewWebContents() >> > #38 0x0000033b983d >> > >> > media_router::MediaRouterUIBrowserTest_OpenDialogWithMediaRouterAction_Test::RunTestOnMainThread() >> > #39 0x00000653b783 InProcessBrowserTest::RunTestOnMainThreadLoop() >> > #40 0x00000668b304 >> > content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() >> > #41 0x00000668f20b >> > >> > _ZN4base8internal15RunnableAdapterIMN7content15BrowserTestBaseEFvvEE3RunIJEEEvPS3_DpOT_ >> > #42 0x00000668f09d >> > >> > _ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMN7content15BrowserTestBaseEFvvEEEE8MakeItSoIJPS4_EEEvS7_DpOT_ >> > #43 0x00000668eea9 >> > >> > _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0EEEENS0_9BindStateINS0_15RunnableAdapterIMN7content15BrowserTestBaseEFvvEEEFvPS7_EJSB_EEENS0_12InvokeHelperILb0EvSA_EEFvvEE3RunEPNS0_13BindStateBaseE >> > #44 0x000000aff044 base::Callback<>::Run() >> > #45 0x0000058e1147 ChromeBrowserMainParts::PreMainMessageLoopRunImpl() >> > #46 0x0000058dc82f ChromeBrowserMainParts::PreMainMessageLoopRun() >> > #47 0x7efe6334c2ac content::BrowserMainLoop::PreMainMessageLoopRun() >> > #48 0x7efe6335f28b >> > >> > _ZN4base8internal15RunnableAdapterIMN7content15BrowserMainLoopEFivEE3RunIJEEEiPS3_DpOT_ >> > #49 0x7efe6335efcd >> > >> > _ZN4base8internal12InvokeHelperILb0EiNS0_15RunnableAdapterIMN7content15BrowserMainLoopEFivEEEE8MakeItSoIJPS4_EEEiS7_DpOT_ >> > #50 0x7efe6335edc1 >> > >> > _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0EEEENS0_9BindStateINS0_15RunnableAdapterIMN7content15BrowserMainLoopEFivEEEFiPS7_EJNS0_17UnretainedWrapperIS7_EEEEENS0_12InvokeHelperILb0EiSA_EEFivEE3RunEPNS0_13BindStateBaseE >> > #51 0x7efe646cf5d4 base::Callback<>::Run() >> > #52 0x7efe654b3b87 content::StartupTaskRunner::RunAllTasksNow() >> > #53 0x7efe63344fe2 content::BrowserMainLoop::CreateStartupTasks() >> > #54 0x7efe633646c2 content::BrowserMainRunnerImpl::Initialize() >> > #55 0x7efe633323aa content::BrowserMain() >> > #56 0x7efe62cdb6bf content::RunNamedProcessTypeMain() >> > #57 0x7efe62ce3352 content::ContentMainRunnerImpl::Run() >> > #58 0x7efe62cd9d95 content::ContentMain() >> > #59 0x00000668ab47 content::BrowserTestBase::SetUp() >> > #60 0x000006535625 InProcessBrowserTest::SetUp() >> > #61 0x000006b6623c >> testing::internal::HandleSehExceptionsInMethodIfSupported<>() >> >> yea, I think you need to change ToolbarActionView to not do this >> >> view_controller_->SetDelegate(this); >> >> or this >> >> UpdateState(); >> >> in the ctor. Instead, move that to ViewHierarchyChanged (see many other >> places >> like >> > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... >> ) > > If you parse through what is happening in the > MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction test you will find > that > it is indeed attempting to use a ToolbarActionView that is not hosted within > a > Widget. So I guess fundamentally I am questioning why are we ok with > assuming a > Widget is hosting the ToolbarActionView when GetInkDropBaseColor() is > called? > Although this assumption may always be true in production, it would be a lot > easier to test the interaction of the ToolbarActionView together with the > InkDrop(Hover|Animation) classes without requiring the overhead of a Widget > to > host them. Let me be clear here, my concern is more about about how the > ThemeProviders are found and and used and it is obviously a much larger > discussion than these one or two InkDrop related cases. I'm only calling > attention to it here as a concrete example, for discussions sake, as to how > these implicit dependencies cause tests to include more than they really > should > be. > > sky@, estade@, What are your thoughts on this? > > FYI I have posted a better fix here: > https://codereview.chromium.org/1805393002/ > > https://codereview.chromium.org/1778643002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
I'm pretty sure it is in a widget, it's just not yet in a widget when this function is called. ergo, this function is called too early. > So I guess fundamentally I am questioning why are we ok with assuming a Widget is hosting the ToolbarActionView when GetInkDropBaseColor() is called? It's more like asserting than assuming. This is what happens in prod (or should happen in prod...). There's no point in calling GetInkDropBaseColor() when there's no widget, so we shouldn't support it.
Message was sent while issue was closed.
Never mind my question here. I saw the patch which explains how the button wasn't in a widget. -Scott On Thu, Mar 17, 2016 at 2:07 PM, Scott Violet <sky@chromium.org> wrote: > I wouldn't expect a browsertest to create an environment that doesn't > match production. What is > MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction doing that > the button is not in a widget? > > -Scott > > On Thu, Mar 17, 2016 at 8:47 AM, <bruthig@chromium.org> wrote: >> On 2016/03/15 19:30:58, Evan Stade wrote: >>> On 2016/03/15 15:42:01, bruthig wrote: >>> > On 2016/03/14 21:20:43, Evan Stade wrote: >>> > > >>> > >>> >> https://codereview.chromium.org/1778643002/diff/200001/chrome/browser/ui/view... >>> > > File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): >>> > > >>> > > >>> > >>> >> https://codereview.chromium.org/1778643002/diff/200001/chrome/browser/ui/view... >>> > > chrome/browser/ui/views/toolbar/toolbar_action_view.cc:122: return >>> > > GetThemeProvider() >>> > > this doesn't seem right to me. I think your change is causing >>> > > GetInkDropBaseColor to be called before |this| is added to a widget, >>> > > which >>> we >>> > > probably don't need/want. What was the callstack? >>> > >>> > Here's the stack trace, you can repro by adding a >> DCHECK(GetThemeProvider()); >>> in >>> > ToolbarActionView::GetInkDropBaseColor() and running >> 'out/Debug/browser_tests >>> > >>> > --gtest_filter=*MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction*' >>> > >>> > >>> > #0 0x000000907a11 __interceptor_backtrace >>> > #1 0x7efe72b585ce base::debug::StackTrace::StackTrace() >>> > #2 0x7efe72c813ff logging::LogMessage::~LogMessage() >>> > #3 0x0000072a1555 ToolbarActionView::GetInkDropBaseColor() >>> > #4 0x7efe4f3fe7f6 views::InkDropHostView::CreateInkDropAnimation() >>> > #5 0x7efe4f458042 views::LabelButton::CreateInkDropAnimation() >>> > #6 0x7efe4f4587b3 views::LabelButton::CreateInkDropAnimation() >>> > #7 0x7efe4f3f8720 >>> > views::InkDropAnimationControllerImpl::CreateInkDropAnimation() >>> > #8 0x7efe4f3f83f9 >>> > views::InkDropAnimationControllerImpl::AnimateToState() >>> > #9 0x7efe4f3f060b views::ButtonInkDropDelegate::OnAction() >>> > #10 0x7efe4f460499 views::MenuButton::IncrementPressedLocked() >>> > #11 0x7efe4f4601ea views::MenuButton::PressedLock::PressedLock() >>> > #12 0x7efe4f46017f views::MenuButton::PressedLock::PressedLock() >>> > #13 0x0000072a3534 ToolbarActionView::OnPopupShown() >>> > #14 0x00000754e781 MediaRouterAction::OnPopupShown() >>> > #15 0x000007558add >>> > media_router::MediaRouterDialogControllerImpl::CreateMediaRouterDialog() >>> > #16 0x00000d283d76 >>> > media_router::MediaRouterDialogController::ShowMediaRouterDialog() >>> > #17 0x00000754d3ea MediaRouterAction::ExecuteAction() >>> > #18 0x0000033bca4c >>> > media_router::MediaRouterUIBrowserTest::ExecuteMediaRouterAction() >>> > #19 0x0000033bda5b >>> > >>> >> _ZN4base8internal15RunnableAdapterIMN12media_router24MediaRouterUIBrowserTestEFvP13AppMenuButtonEE3RunIJRKS5_EEEvPS3_DpOT_ >>> > #20 0x0000033bd89f >>> > >>> >> _ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMN12media_router24MediaRouterUIBrowserTestEFvP13AppMenuButtonEEEE8MakeItSoIJPS4_RKS6_EEEvS9_DpOT_ >>> > #21 0x0000033bd670 >>> > >>> >> _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0ELm1EEEENS0_9BindStateINS0_15RunnableAdapterIMN12media_router24MediaRouterUIBrowserTestEFvP13AppMenuButtonEEEFvPS7_S9_EJSD_RS9_EEENS0_12InvokeHelperILb0EvSC_EEFvvEE3RunEPNS0_13BindStateBaseE >>> > #22 0x7efe72afb8b4 base::Callback<>::Run() >>> > #23 0x7efe72b66ae1 base::debug::TaskAnnotator::RunTask() >>> > #24 0x7efe72cd6552 base::MessageLoop::RunTask() >>> > #25 0x7efe72cd6e0a base::MessageLoop::DeferOrRunPendingTask() >>> > #26 0x7efe72cd7f78 base::MessageLoop::DoWork() >>> > #27 0x7efe72a95764 base::MessagePumpGlib::Run() >>> > #28 0x7efe72cd52d9 base::MessageLoop::RunHandler() >>> > #29 0x7efe72e2104c base::RunLoop::Run() >>> > #30 0x7efe4f783ee1 views::MenuMessageLoopAura::Run() >>> > #31 0x7efe4f4a0c90 views::MenuController::RunMessageLoop() >>> > #32 0x7efe4f49eb92 views::MenuController::Run() >>> > #33 0x7efe4f506000 views::internal::MenuRunnerImpl::RunMenuAt() >>> > #34 0x7efe4f50413e views::MenuRunner::RunMenuAt() >>> > #35 0x000007eef9a1 AppMenu::RunMenu() >>> > #36 0x0000072640a1 AppMenuButton::ShowMenu() >>> > #37 0x0000033b9fe5 >>> > >>> >> media_router::MediaRouterUIBrowserTest::OpenMediaRouterDialogAndWaitForNewWebContents() >>> > #38 0x0000033b983d >>> > >>> >> media_router::MediaRouterUIBrowserTest_OpenDialogWithMediaRouterAction_Test::RunTestOnMainThread() >>> > #39 0x00000653b783 InProcessBrowserTest::RunTestOnMainThreadLoop() >>> > #40 0x00000668b304 >>> > content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() >>> > #41 0x00000668f20b >>> > >>> >> _ZN4base8internal15RunnableAdapterIMN7content15BrowserTestBaseEFvvEE3RunIJEEEvPS3_DpOT_ >>> > #42 0x00000668f09d >>> > >>> >> _ZN4base8internal12InvokeHelperILb0EvNS0_15RunnableAdapterIMN7content15BrowserTestBaseEFvvEEEE8MakeItSoIJPS4_EEEvS7_DpOT_ >>> > #43 0x00000668eea9 >>> > >>> >> _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0EEEENS0_9BindStateINS0_15RunnableAdapterIMN7content15BrowserTestBaseEFvvEEEFvPS7_EJSB_EEENS0_12InvokeHelperILb0EvSA_EEFvvEE3RunEPNS0_13BindStateBaseE >>> > #44 0x000000aff044 base::Callback<>::Run() >>> > #45 0x0000058e1147 ChromeBrowserMainParts::PreMainMessageLoopRunImpl() >>> > #46 0x0000058dc82f ChromeBrowserMainParts::PreMainMessageLoopRun() >>> > #47 0x7efe6334c2ac content::BrowserMainLoop::PreMainMessageLoopRun() >>> > #48 0x7efe6335f28b >>> > >>> >> _ZN4base8internal15RunnableAdapterIMN7content15BrowserMainLoopEFivEE3RunIJEEEiPS3_DpOT_ >>> > #49 0x7efe6335efcd >>> > >>> >> _ZN4base8internal12InvokeHelperILb0EiNS0_15RunnableAdapterIMN7content15BrowserMainLoopEFivEEEE8MakeItSoIJPS4_EEEiS7_DpOT_ >>> > #50 0x7efe6335edc1 >>> > >>> >> _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0EEEENS0_9BindStateINS0_15RunnableAdapterIMN7content15BrowserMainLoopEFivEEEFiPS7_EJNS0_17UnretainedWrapperIS7_EEEEENS0_12InvokeHelperILb0EiSA_EEFivEE3RunEPNS0_13BindStateBaseE >>> > #51 0x7efe646cf5d4 base::Callback<>::Run() >>> > #52 0x7efe654b3b87 content::StartupTaskRunner::RunAllTasksNow() >>> > #53 0x7efe63344fe2 content::BrowserMainLoop::CreateStartupTasks() >>> > #54 0x7efe633646c2 content::BrowserMainRunnerImpl::Initialize() >>> > #55 0x7efe633323aa content::BrowserMain() >>> > #56 0x7efe62cdb6bf content::RunNamedProcessTypeMain() >>> > #57 0x7efe62ce3352 content::ContentMainRunnerImpl::Run() >>> > #58 0x7efe62cd9d95 content::ContentMain() >>> > #59 0x00000668ab47 content::BrowserTestBase::SetUp() >>> > #60 0x000006535625 InProcessBrowserTest::SetUp() >>> > #61 0x000006b6623c >>> testing::internal::HandleSehExceptionsInMethodIfSupported<>() >>> >>> yea, I think you need to change ToolbarActionView to not do this >>> >>> view_controller_->SetDelegate(this); >>> >>> or this >>> >>> UpdateState(); >>> >>> in the ctor. Instead, move that to ViewHierarchyChanged (see many other >>> places >>> like >>> >> https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... >>> ) >> >> If you parse through what is happening in the >> MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction test you will find >> that >> it is indeed attempting to use a ToolbarActionView that is not hosted within >> a >> Widget. So I guess fundamentally I am questioning why are we ok with >> assuming a >> Widget is hosting the ToolbarActionView when GetInkDropBaseColor() is >> called? >> Although this assumption may always be true in production, it would be a lot >> easier to test the interaction of the ToolbarActionView together with the >> InkDrop(Hover|Animation) classes without requiring the overhead of a Widget >> to >> host them. Let me be clear here, my concern is more about about how the >> ThemeProviders are found and and used and it is obviously a much larger >> discussion than these one or two InkDrop related cases. I'm only calling >> attention to it here as a concrete example, for discussions sake, as to how >> these implicit dependencies cause tests to include more than they really >> should >> be. >> >> sky@, estade@, What are your thoughts on this? >> >> FYI I have posted a better fix here: >> https://codereview.chromium.org/1805393002/ >> >> https://codereview.chromium.org/1778643002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
