Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(450)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 365753002: MicButton should implement MaskedTargeterDelegate (Closed)

Created:
6 years, 5 months ago by tdanderson
Modified:
6 years, 5 months ago
Reviewers:
sky
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina, ben+views_chromium.org, tdanderson+views_chromium.org, sadrul
Project:
chromium
Visibility:
Public.

Description

MicButton should implement MaskedTargeterDelegate Define the hit test mask for MicButton by implementing MaskedTargeterDelegate instead of overriding View::HasHitTestMask() and View::GetHitTestMaskDeprecated(). BUG=388838 TEST=SpeechViewTest.ClickMicButton Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282395

Patch Set 1 #

Patch Set 2 : avoid static_cast when setting the targeter #

Patch Set 3 : rebase against ToT #

Patch Set 4 : re-upload #

Patch Set 5 : moved MicButton class declaration #

Patch Set 6 : re-upload #

Total comments: 2

Patch Set 7 : comments addressed #

Total comments: 2

Patch Set 8 : local variable to avoid static_cast #

Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -14 lines) Patch
M ui/app_list/views/speech_view.cc View 1 2 3 4 5 6 7 5 chunks +13 lines, -14 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
tdanderson
Scott, can you please take a look?
6 years, 5 months ago (2014-07-04 01:25:33 UTC) #1
sky
https://chromiumcodereview.appspot.com/365753002/diff/100001/ui/app_list/views/speech_view.h File ui/app_list/views/speech_view.h (right): https://chromiumcodereview.appspot.com/365753002/diff/100001/ui/app_list/views/speech_view.h#newcode27 ui/app_list/views/speech_view.h:27: class APP_LIST_EXPORT MicButton : public views::ImageButton, I would keep ...
6 years, 5 months ago (2014-07-09 19:26:05 UTC) #2
tdanderson
Scott, please take another look. https://codereview.chromium.org/365753002/diff/100001/ui/app_list/views/speech_view.h File ui/app_list/views/speech_view.h (right): https://codereview.chromium.org/365753002/diff/100001/ui/app_list/views/speech_view.h#newcode27 ui/app_list/views/speech_view.h:27: class APP_LIST_EXPORT MicButton : ...
6 years, 5 months ago (2014-07-09 20:05:11 UTC) #3
sky
LGTM https://codereview.chromium.org/365753002/diff/120001/ui/app_list/views/speech_view.cc File ui/app_list/views/speech_view.cc (right): https://codereview.chromium.org/365753002/diff/120001/ui/app_list/views/speech_view.cc#newcode140 ui/app_list/views/speech_view.cc:140: new views::ViewTargeter(static_cast<MicButton*>(mic_button_)))); I'm not a fan of the ...
6 years, 5 months ago (2014-07-10 15:46:00 UTC) #4
tdanderson
https://codereview.chromium.org/365753002/diff/120001/ui/app_list/views/speech_view.cc File ui/app_list/views/speech_view.cc (right): https://codereview.chromium.org/365753002/diff/120001/ui/app_list/views/speech_view.cc#newcode140 ui/app_list/views/speech_view.cc:140: new views::ViewTargeter(static_cast<MicButton*>(mic_button_)))); On 2014/07/10 15:46:00, sky wrote: > I'm ...
6 years, 5 months ago (2014-07-10 16:53:45 UTC) #5
tdanderson
The CQ bit was checked by tdanderson@chromium.org
6 years, 5 months ago (2014-07-10 16:54:01 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tdanderson@chromium.org/365753002/140001
6 years, 5 months ago (2014-07-10 16:55:57 UTC) #7
commit-bot: I haz the power
6 years, 5 months ago (2014-07-10 19:26:55 UTC) #8
Message was sent while issue was closed. 
Change committed as 282395

Powered by Google App Engine
This is Rietveld 408576698