Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1805393002:
Added MediaRouterUIBrowserTest::toolbar_action_view_ to a Widget. (Closed)
DescriptionAdded MediaRouterUIBrowserTest::toolbar_action_view_ to a Widget.
The MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction test was failing to
find the correct ThemeProvider in ToolbarActionView::GetInkDropBaseColor()
due this change https://codereview.chromium.org/1778643002/. As a short term
work around the ToolbarActionView::GetInkDropBaseColor() fell back on
CustomButton::GetInkDropBaseColor() when the ThemeProvider couldn't be found.
This change implements the better solution.
TEST=MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction
Committed: https://crrev.com/ad0d763c39a8d2666fecf418bf6c47c5c8a44566
Cr-Commit-Position: refs/heads/master@{#382052}
Patch Set 1 #Patch Set 2 : Rearranged ToolbarActionView::ViewHierarchyChanged(). #Patch Set 3 : Proposed solution. #
Total comments: 10
Patch Set 4 : Merge with master #Patch Set 5 : Addressed estade@'s comments from patch set 3. #
Total comments: 2
Patch Set 6 : Updated incorrect doc. #
Messages
Total messages: 21 (6 generated)
Description was changed from ========== Removed null checks for GetThemeProvider() in GetInkDropBaseColor(). BUG= ========== to ========== Added MediaRouterUIBrowserTest::toolbar_action_view_ to a Widget. The MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction test was failing to find the correct ThemeProvider in ToolbarActionView::GetInkDropBaseColor() due this change https://codereview.chromium.org/1778643002/. As a short term work around the ToolbarActionView::GetInkDropBaseColor() fell back on CustomButton::GetInkDropBaseColor() when the ThemeProvider couldn't be found. This change implements the better solution. TEST=MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction ==========
bruthig@chromium.org changed reviewers: + estade@chromium.org, sky@chromium.org
This is a follow up to the discussion here: https://codereview.chromium.org/1778643002/. sky@, estade@ PTAL.
LGTM
ok, ignore what i said on the other thread. This approach lg https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:245: DCHECK(GetThemeProvider()); the style guide says not to dcheck right before derefing https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:102: scoped_ptr<ToolbarActionView> toolbar_action_view_; is this still the right ownership model? I'd expect that setcontentsview took ownership (except if you change owned_by_client_) https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:105: scoped_ptr<views::Widget> toolbar_action_view_widget_; likewise aren't widgets usually self-owned? You just call Close when you don't want it any more. https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:122: DCHECK(GetThemeProvider()); ditto
https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:245: DCHECK(GetThemeProvider()); On 2016/03/17 21:41:35, Evan Stade wrote: > the style guide says not to dcheck right before derefing Yeah, I know that the style guide says to avoid this, and I was hoping this would start a discussion. I argue that they are super useful because I spent almost a day tracking down this error with manual log lines because there was no stack trace in the logs when this was failing on the bots. Unfortunately the logs have been purged for that particular run so I can't show you the proof. Is there an alternative to solution to this?
https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:245: DCHECK(GetThemeProvider()); On 2016/03/18 00:22:41, bruthig wrote: > On 2016/03/17 21:41:35, Evan Stade wrote: > > the style guide says not to dcheck right before derefing > > Yeah, I know that the style guide says to avoid this, and I was hoping this > would start a discussion. > I argue that they are super useful because I spent almost a day tracking down > this error with manual log lines because there was no stack trace in the logs > when this was failing on the bots. Unfortunately the logs have been purged for > that particular run so I can't show you the proof. Is there an alternative to > solution to this? add a debug line: base::debug::StackTrace().Print();
On 2016/03/18 01:20:22, Evan Stade wrote: > https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): > > https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:245: > DCHECK(GetThemeProvider()); > On 2016/03/18 00:22:41, bruthig wrote: > > On 2016/03/17 21:41:35, Evan Stade wrote: > > > the style guide says not to dcheck right before derefing > > > > Yeah, I know that the style guide says to avoid this, and I was hoping this > > would start a discussion. > > I argue that they are super useful because I spent almost a day tracking down > > this error with manual log lines because there was no stack trace in the logs > > when this was failing on the bots. Unfortunately the logs have been purged for > > that particular run so I can't show you the proof. Is there an alternative to > > solution to this? > > add a debug line: base::debug::StackTrace().Print(); or are you talking about how to find that it was crashing here in the first place. Wasn't it failing locally? Would you get a stack trace with this fn in it somewhere? I thought the trace I looked at made it fairly obvious. You can also use gdb which helps sometimes.
On 2016/03/18 01:22:40, Evan Stade wrote: > On 2016/03/18 01:20:22, Evan Stade wrote: > > > https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... > > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): > > > > > https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... > > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:245: > > DCHECK(GetThemeProvider()); > > On 2016/03/18 00:22:41, bruthig wrote: > > > On 2016/03/17 21:41:35, Evan Stade wrote: > > > > the style guide says not to dcheck right before derefing > > > > > > Yeah, I know that the style guide says to avoid this, and I was hoping this > > > would start a discussion. > > > I argue that they are super useful because I spent almost a day tracking > down > > > this error with manual log lines because there was no stack trace in the > logs > > > when this was failing on the bots. Unfortunately the logs have been purged > for > > > that particular run so I can't show you the proof. Is there an alternative > to > > > solution to this? > > > > add a debug line: base::debug::StackTrace().Print(); > > or are you talking about how to find that it was crashing here in the first > place. Wasn't it failing locally? Would you get a stack trace with this fn in it > somewhere? I thought the trace I looked at made it fairly obvious. You can also > use gdb which helps sometimes. I was talking about finding that it was crashing here in the first place. It was failing locally but gdb wasn't trapping the crash. Locally I was getting the following stack trace: #0 0x82fd13e (/work/chrome/src/out/Debug/browser_tests+0x82fd13e) #1 0x7fffcdda14a2 (/work/chrome/src/out/Debug/lib/libviews.so+0x1ca4a2) #2 0x7fffcddf1799 (/work/chrome/src/out/Debug/lib/libviews.so+0x21a799) #3 0x7fffcddf1982 (/work/chrome/src/out/Debug/lib/libviews.so+0x21a982) #4 0x7fffcdd992b6 (/work/chrome/src/out/Debug/lib/libviews.so+0x1c22b6) #5 0x7fffcdd99040 (/work/chrome/src/out/Debug/lib/libviews.so+0x1c2040) #6 0x7fffcdd920f2 (/work/chrome/src/out/Debug/lib/libviews.so+0x1bb0f2) #7 0x7fffcddf98b8 (/work/chrome/src/out/Debug/lib/libviews.so+0x2228b8) #8 0x7fffcddf9609 (/work/chrome/src/out/Debug/lib/libviews.so+0x222609) #9 0x7fffcddf959e (/work/chrome/src/out/Debug/lib/libviews.so+0x22259e) #10 0x82fec71 (/work/chrome/src/out/Debug/browser_tests+0x82fec71) #11 0x8554d3c (/work/chrome/src/out/Debug/browser_tests+0x8554d3c) #12 0x855fc6b (/work/chrome/src/out/Debug/browser_tests+0x855fc6b) #13 0xe5e53e6 (/work/chrome/src/out/Debug/browser_tests+0xe5e53e6) #14 0x8554152 (/work/chrome/src/out/Debug/browser_tests+0x8554152) #15 0x332bc89 (/work/chrome/src/out/Debug/browser_tests+0x332bc89) #16 0x332cc77 (/work/chrome/src/out/Debug/browser_tests+0x332cc77) #17 0x332ca96 (/work/chrome/src/out/Debug/browser_tests+0x332ca96) #18 0x332c89a (/work/chrome/src/out/Debug/browser_tests+0x332c89a) #19 0x7ffff5e99da3 (/work/chrome/src/out/Debug/lib/libbase.so+0x21eda3) #20 0x7ffff5f00c60 (/work/chrome/src/out/Debug/lib/libbase.so+0x285c60) #21 0x7ffff603d1b7 (/work/chrome/src/out/Debug/lib/libbase.so+0x3c21b7) #22 0x7ffff603da19 (/work/chrome/src/out/Debug/lib/libbase.so+0x3c2a19) #23 0x7ffff603dda5 (/work/chrome/src/out/Debug/lib/libbase.so+0x3c2da5) #24 0x7ffff5e222d4 (/work/chrome/src/out/Debug/lib/libbase.so+0x1a72d4) #25 0x7ffff5e23d58 (/work/chrome/src/out/Debug/lib/libbase.so+0x1a8d58) #26 0x7fffc990ae03 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x48e03) I know it's possible to convert this to human readable symbols and line numbers but I am not sure how. I'd be happy to learn tho.
On 2016/03/18 14:49:52, bruthig wrote: > On 2016/03/18 01:22:40, Evan Stade wrote: > > On 2016/03/18 01:20:22, Evan Stade wrote: > > > > > > https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... > > > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): > > > > > > > > > https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... > > > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:245: > > > DCHECK(GetThemeProvider()); > > > On 2016/03/18 00:22:41, bruthig wrote: > > > > On 2016/03/17 21:41:35, Evan Stade wrote: > > > > > the style guide says not to dcheck right before derefing > > > > > > > > Yeah, I know that the style guide says to avoid this, and I was hoping > this > > > > would start a discussion. > > > > I argue that they are super useful because I spent almost a day tracking > > down > > > > this error with manual log lines because there was no stack trace in the > > logs > > > > when this was failing on the bots. Unfortunately the logs have been purged > > for > > > > that particular run so I can't show you the proof. Is there an > alternative > > to > > > > solution to this? > > > > > > add a debug line: base::debug::StackTrace().Print(); > > > > or are you talking about how to find that it was crashing here in the first > > place. Wasn't it failing locally? Would you get a stack trace with this fn in > it > > somewhere? I thought the trace I looked at made it fairly obvious. You can > also > > use gdb which helps sometimes. > > I was talking about finding that it was crashing here in the first place. It was > failing locally but gdb wasn't trapping the crash. Locally I was getting the > following stack trace: > #0 0x82fd13e (/work/chrome/src/out/Debug/browser_tests+0x82fd13e) > #1 0x7fffcdda14a2 (/work/chrome/src/out/Debug/lib/libviews.so+0x1ca4a2) > #2 0x7fffcddf1799 (/work/chrome/src/out/Debug/lib/libviews.so+0x21a799) > #3 0x7fffcddf1982 (/work/chrome/src/out/Debug/lib/libviews.so+0x21a982) > #4 0x7fffcdd992b6 (/work/chrome/src/out/Debug/lib/libviews.so+0x1c22b6) > #5 0x7fffcdd99040 (/work/chrome/src/out/Debug/lib/libviews.so+0x1c2040) > #6 0x7fffcdd920f2 (/work/chrome/src/out/Debug/lib/libviews.so+0x1bb0f2) > #7 0x7fffcddf98b8 (/work/chrome/src/out/Debug/lib/libviews.so+0x2228b8) > #8 0x7fffcddf9609 (/work/chrome/src/out/Debug/lib/libviews.so+0x222609) > #9 0x7fffcddf959e (/work/chrome/src/out/Debug/lib/libviews.so+0x22259e) > #10 0x82fec71 (/work/chrome/src/out/Debug/browser_tests+0x82fec71) > #11 0x8554d3c (/work/chrome/src/out/Debug/browser_tests+0x8554d3c) > #12 0x855fc6b (/work/chrome/src/out/Debug/browser_tests+0x855fc6b) > #13 0xe5e53e6 (/work/chrome/src/out/Debug/browser_tests+0xe5e53e6) > #14 0x8554152 (/work/chrome/src/out/Debug/browser_tests+0x8554152) > #15 0x332bc89 (/work/chrome/src/out/Debug/browser_tests+0x332bc89) > #16 0x332cc77 (/work/chrome/src/out/Debug/browser_tests+0x332cc77) > #17 0x332ca96 (/work/chrome/src/out/Debug/browser_tests+0x332ca96) > #18 0x332c89a (/work/chrome/src/out/Debug/browser_tests+0x332c89a) > #19 0x7ffff5e99da3 (/work/chrome/src/out/Debug/lib/libbase.so+0x21eda3) > #20 0x7ffff5f00c60 (/work/chrome/src/out/Debug/lib/libbase.so+0x285c60) > #21 0x7ffff603d1b7 (/work/chrome/src/out/Debug/lib/libbase.so+0x3c21b7) > #22 0x7ffff603da19 (/work/chrome/src/out/Debug/lib/libbase.so+0x3c2a19) > #23 0x7ffff603dda5 (/work/chrome/src/out/Debug/lib/libbase.so+0x3c2da5) > #24 0x7ffff5e222d4 (/work/chrome/src/out/Debug/lib/libbase.so+0x1a72d4) > #25 0x7ffff5e23d58 (/work/chrome/src/out/Debug/lib/libbase.so+0x1a8d58) > #26 0x7fffc990ae03 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x48e03) > > I know it's possible to convert this to human readable symbols and line numbers > but I am not sure how. I'd be happy to learn tho. debug build. gdb also works with release builds to give you fn names if not line numbers.
On 2016/03/18 15:12:03, Evan Stade wrote: > On 2016/03/18 14:49:52, bruthig wrote: > > On 2016/03/18 01:22:40, Evan Stade wrote: > > > On 2016/03/18 01:20:22, Evan Stade wrote: > > > > > > > > > > https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... > > > > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... > > > > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:245: > > > > DCHECK(GetThemeProvider()); > > > > On 2016/03/18 00:22:41, bruthig wrote: > > > > > On 2016/03/17 21:41:35, Evan Stade wrote: > > > > > > the style guide says not to dcheck right before derefing > > > > > > > > > > Yeah, I know that the style guide says to avoid this, and I was hoping > > this > > > > > would start a discussion. > > > > > I argue that they are super useful because I spent almost a day tracking > > > down > > > > > this error with manual log lines because there was no stack trace in the > > > logs > > > > > when this was failing on the bots. Unfortunately the logs have been > purged > > > for > > > > > that particular run so I can't show you the proof. Is there an > > alternative > > > to > > > > > solution to this? > > > > > > > > add a debug line: base::debug::StackTrace().Print(); > > > > > > or are you talking about how to find that it was crashing here in the first > > > place. Wasn't it failing locally? Would you get a stack trace with this fn > in > > it > > > somewhere? I thought the trace I looked at made it fairly obvious. You can > > also > > > use gdb which helps sometimes. > > > > I was talking about finding that it was crashing here in the first place. It > was > > failing locally but gdb wasn't trapping the crash. Locally I was getting the > > following stack trace: > > #0 0x82fd13e (/work/chrome/src/out/Debug/browser_tests+0x82fd13e) > > #1 0x7fffcdda14a2 (/work/chrome/src/out/Debug/lib/libviews.so+0x1ca4a2) > > #2 0x7fffcddf1799 (/work/chrome/src/out/Debug/lib/libviews.so+0x21a799) > > #3 0x7fffcddf1982 (/work/chrome/src/out/Debug/lib/libviews.so+0x21a982) > > #4 0x7fffcdd992b6 (/work/chrome/src/out/Debug/lib/libviews.so+0x1c22b6) > > #5 0x7fffcdd99040 (/work/chrome/src/out/Debug/lib/libviews.so+0x1c2040) > > #6 0x7fffcdd920f2 (/work/chrome/src/out/Debug/lib/libviews.so+0x1bb0f2) > > #7 0x7fffcddf98b8 (/work/chrome/src/out/Debug/lib/libviews.so+0x2228b8) > > #8 0x7fffcddf9609 (/work/chrome/src/out/Debug/lib/libviews.so+0x222609) > > #9 0x7fffcddf959e (/work/chrome/src/out/Debug/lib/libviews.so+0x22259e) > > #10 0x82fec71 (/work/chrome/src/out/Debug/browser_tests+0x82fec71) > > #11 0x8554d3c (/work/chrome/src/out/Debug/browser_tests+0x8554d3c) > > #12 0x855fc6b (/work/chrome/src/out/Debug/browser_tests+0x855fc6b) > > #13 0xe5e53e6 (/work/chrome/src/out/Debug/browser_tests+0xe5e53e6) > > #14 0x8554152 (/work/chrome/src/out/Debug/browser_tests+0x8554152) > > #15 0x332bc89 (/work/chrome/src/out/Debug/browser_tests+0x332bc89) > > #16 0x332cc77 (/work/chrome/src/out/Debug/browser_tests+0x332cc77) > > #17 0x332ca96 (/work/chrome/src/out/Debug/browser_tests+0x332ca96) > > #18 0x332c89a (/work/chrome/src/out/Debug/browser_tests+0x332c89a) > > #19 0x7ffff5e99da3 (/work/chrome/src/out/Debug/lib/libbase.so+0x21eda3) > > #20 0x7ffff5f00c60 (/work/chrome/src/out/Debug/lib/libbase.so+0x285c60) > > #21 0x7ffff603d1b7 (/work/chrome/src/out/Debug/lib/libbase.so+0x3c21b7) > > #22 0x7ffff603da19 (/work/chrome/src/out/Debug/lib/libbase.so+0x3c2a19) > > #23 0x7ffff603dda5 (/work/chrome/src/out/Debug/lib/libbase.so+0x3c2da5) > > #24 0x7ffff5e222d4 (/work/chrome/src/out/Debug/lib/libbase.so+0x1a72d4) > > #25 0x7ffff5e23d58 (/work/chrome/src/out/Debug/lib/libbase.so+0x1a8d58) > > #26 0x7fffc990ae03 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x48e03) > > > > I know it's possible to convert this to human readable symbols and line > numbers > > but I am not sure how. I'd be happy to learn tho. > > debug build. gdb also works with release builds to give you fn names if not line > numbers. since it's a browser test, you might need --single_process for gdb to work well
estade > > debug build. gdb also works with release builds to give you fn names if not > line > > numbers. That stack trace was from a debug build. > > since it's a browser test, you might need --single_process for gdb to work well This did the trick, thanks. https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:245: DCHECK(GetThemeProvider()); On 2016/03/18 01:20:22, Evan Stade wrote: > On 2016/03/18 00:22:41, bruthig wrote: > > On 2016/03/17 21:41:35, Evan Stade wrote: > > > the style guide says not to dcheck right before derefing Removed. https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:102: scoped_ptr<ToolbarActionView> toolbar_action_view_; On 2016/03/17 21:41:35, Evan Stade wrote: > is this still the right ownership model? I'd expect that setcontentsview took > ownership (except if you change owned_by_client_) Good catch, updated. PTAL https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:105: scoped_ptr<views::Widget> toolbar_action_view_widget_; On 2016/03/17 21:41:35, Evan Stade wrote: > likewise aren't widgets usually self-owned? You just call Close when you don't > want it any more. Is this what you are suggesting, using a raw pointer and calling Close()? https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1805393002/diff/30001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:122: DCHECK(GetThemeProvider()); On 2016/03/17 21:41:35, Evan Stade wrote: > ditto Removed.
lgtm https://codereview.chromium.org/1805393002/diff/70001/chrome/browser/ui/views... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/1805393002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:100: // ToolbarActionView constructed to set the delegate on |mr_action|. this doc doesn't match the actual var name
https://codereview.chromium.org/1805393002/diff/70001/chrome/browser/ui/views... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/1805393002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:100: // ToolbarActionView constructed to set the delegate on |mr_action|. On 2016/03/18 15:45:15, Evan Stade wrote: > this doc doesn't match the actual var name Done.
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, estade@chromium.org Link to the patchset: https://codereview.chromium.org/1805393002/#ps90001 (title: "Updated incorrect doc.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1805393002/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805393002/90001
Message was sent while issue was closed.
Description was changed from ========== Added MediaRouterUIBrowserTest::toolbar_action_view_ to a Widget. The MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction test was failing to find the correct ThemeProvider in ToolbarActionView::GetInkDropBaseColor() due this change https://codereview.chromium.org/1778643002/. As a short term work around the ToolbarActionView::GetInkDropBaseColor() fell back on CustomButton::GetInkDropBaseColor() when the ThemeProvider couldn't be found. This change implements the better solution. TEST=MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction ========== to ========== Added MediaRouterUIBrowserTest::toolbar_action_view_ to a Widget. The MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction test was failing to find the correct ThemeProvider in ToolbarActionView::GetInkDropBaseColor() due this change https://codereview.chromium.org/1778643002/. As a short term work around the ToolbarActionView::GetInkDropBaseColor() fell back on CustomButton::GetInkDropBaseColor() when the ThemeProvider couldn't be found. This change implements the better solution. TEST=MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction ==========
Message was sent while issue was closed.
Committed patchset #6 (id:90001)
Message was sent while issue was closed.
Description was changed from ========== Added MediaRouterUIBrowserTest::toolbar_action_view_ to a Widget. The MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction test was failing to find the correct ThemeProvider in ToolbarActionView::GetInkDropBaseColor() due this change https://codereview.chromium.org/1778643002/. As a short term work around the ToolbarActionView::GetInkDropBaseColor() fell back on CustomButton::GetInkDropBaseColor() when the ThemeProvider couldn't be found. This change implements the better solution. TEST=MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction ========== to ========== Added MediaRouterUIBrowserTest::toolbar_action_view_ to a Widget. The MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction test was failing to find the correct ThemeProvider in ToolbarActionView::GetInkDropBaseColor() due this change https://codereview.chromium.org/1778643002/. As a short term work around the ToolbarActionView::GetInkDropBaseColor() fell back on CustomButton::GetInkDropBaseColor() when the ThemeProvider couldn't be found. This change implements the better solution. TEST=MediaRouterUIBrowserTest.OpenDialogWithMediaRouterAction Committed: https://crrev.com/ad0d763c39a8d2666fecf418bf6c47c5c8a44566 Cr-Commit-Position: refs/heads/master@{#382052} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ad0d763c39a8d2666fecf418bf6c47c5c8a44566 Cr-Commit-Position: refs/heads/master@{#382052} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
