Introduce the MicButtonTargeter class

Introduce the MicButtonTargeter class

Introduce MicButtonTargeter, a derived class of MaskedViewTargeter,
to define the circular hit-test region of MicButton.

We will need to be able to access the hit test mask corresponding
to a view by calling into its installed MaskedViewTargeter, so
GetHitTestMask() should be made public. Furthermore, add a version
of GetEventTargeter() to EventTarget that returns a const pointer.

BUG=377537, 378485
TEST=SpeechViewTest.ClickMicButton

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273740

[tdanderson, 2014-05-27 16:33:33]

Sadrul, can you take a quick look at this? Ignore everything except for
speech_view.cc and my inline comment in view.cc (the rest is a copy of the
MaskedViewTargeter code currently in the CQ).

https://codereview.chromium.org/302653002/diff/1/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/302653002/diff/1/ui/views/view.cc#newcode1170
ui/views/view.cc:1170: return targeter_.get();
Added this seemingly necessary witchcraft for speech_view.cc line 103.

[tdanderson, 2014-05-27 16:35:02]

https://codereview.chromium.org/302653002/diff/1/ui/views/masked_view_targeter.h
File ui/views/masked_view_targeter.h (right):

https://codereview.chromium.org/302653002/diff/1/ui/views/masked_view_targete...
ui/views/masked_view_targeter.h:28: virtual bool GetHitTestMask(View* view,
gfx::Path* mask) const = 0;
One other change to note in this CL: I have made GetHitTestMask() public.

[tdanderson, 2014-05-28 16:39:13]

Sadrul, I rebased against ToT so this is now ready for formal review. If this
LGTY, I will first land the changes in masked_view_targeter.h and view.{h,cc} in
a separate patch.

[sadrul, 2014-05-28 22:14:51]

LGTM

Can you look at adding a test for the app_list change (from a quick look, it
doesn't look like we have tests for speech-view, but an app_list owner may have
suggestions)

[tdanderson, 2014-05-28 22:21:25]

xiyuan@, can you please look at the changes in ui/app_list? This is is a
follow-up on the change https://codereview.chromium.org/262883008/ which you
reviewed previously.

Also, do you have any suggestions for test coverage I should add here?

[xiyuan, 2014-05-28 23:01:43]

It might make sense to add a unit test for SpeechView itself. You can use
AppListTestViewDelegate (e.g. change it to add counter for
ToggleSpeechRecognition) to construct the SpeechView and simulate mouse clicks
to test whether the mask works as expected.

https://codereview.chromium.org/302653002/diff/40001/ui/app_list/views/speech...
File ui/app_list/views/speech_view.cc (right):

https://codereview.chromium.org/302653002/diff/40001/ui/app_list/views/speech...
ui/app_list/views/speech_view.cc:106: MicButton* b =
const_cast<MicButton*>(this);
const_cast makes me nervous.

Does it make sense to make MaskedViewTargeter::GetHitTestMask take a const
pointer?

[tdanderson, 2014-05-29 19:55:18]

xiyuan@, please take a look at my test in progress. Is this what you had in
mind? It doesn't work, though, because the SpeechView and its child container
both have width and height of 0 after layout (although the MicButton has the
correct dimensions). This means that trying to target the mic button with a
mouse event is not possible because its ancestor has a size of zero. Am I
missing something simple here?

https://codereview.chromium.org/302653002/diff/40001/ui/app_list/views/speech...
File ui/app_list/views/speech_view.cc (right):

https://codereview.chromium.org/302653002/diff/40001/ui/app_list/views/speech...
ui/app_list/views/speech_view.cc:106: MicButton* b =
const_cast<MicButton*>(this);
On 2014/05/28 23:01:44, xiyuan wrote:
> const_cast makes me nervous.
> 
> Does it make sense to make MaskedViewTargeter::GetHitTestMask take a const
> pointer?

Done.

[xiyuan, 2014-05-29 20:25:12]

Yes, the test is what I have in mind.

https://codereview.chromium.org/302653002/diff/60001/ui/app_list/views/speech...
File ui/app_list/views/speech_view.h (right):

https://codereview.chromium.org/302653002/diff/60001/ui/app_list/views/speech...
ui/app_list/views/speech_view.h:25: class SpeechView : public views::View,
#include "ui/app_list/app_list_export.h" and add APP_LIST_EXPORT to declaration.
Otherwise, app_list_unittests does not link in component build.

https://codereview.chromium.org/302653002/diff/60001/ui/app_list/views/speech...
File ui/app_list/views/speech_view_unittest.cc (right):

https://codereview.chromium.org/302653002/diff/60001/ui/app_list/views/speech...
ui/app_list/views/speech_view_unittest.cc:23: widget_ =
CreateTopLevelPlatformWidget();
By default, test widget is very small. Give it a big enough size,

e.g.
widget_->SetBounds(gfx::Rect(0, 0, 300, 200));

https://codereview.chromium.org/302653002/diff/60001/ui/app_list/views/speech...
ui/app_list/views/speech_view_unittest.cc:26: view_->Layout();
No need to call Layout explicitly. Instead, we should make the SpeechView fill
the content area,

e.g.
view_->SetBoundsRect(widget_->GetContentsView()->GetLocalBounds());

https://codereview.chromium.org/302653002/diff/60001/ui/app_list/views/speech...
ui/app_list/views/speech_view_unittest.cc:54: ui::MouseEvent
press(ui::ET_MOUSE_PRESSED, screen_bounds.CenterPoint(),
screen_bounds.CenterPoint(), 0, 0);
The mouse event should have ui::EF_LEFT_MOUSE_BUTTON in its flags. Otherwise,
button click does not fire.

[tdanderson, 2014-05-29 21:34:23]

xiyuan@, thank you very much for the helpful feedback. The test works now. Can
you please take another look?

https://codereview.chromium.org/302653002/diff/60001/ui/app_list/views/speech...
File ui/app_list/views/speech_view.h (right):

https://codereview.chromium.org/302653002/diff/60001/ui/app_list/views/speech...
ui/app_list/views/speech_view.h:25: class SpeechView : public views::View,
On 2014/05/29 20:25:12, xiyuan wrote:
> #include "ui/app_list/app_list_export.h" and add APP_LIST_EXPORT to
declaration.
> Otherwise, app_list_unittests does not link in component build.

Done.

https://codereview.chromium.org/302653002/diff/60001/ui/app_list/views/speech...
File ui/app_list/views/speech_view_unittest.cc (right):

https://codereview.chromium.org/302653002/diff/60001/ui/app_list/views/speech...
ui/app_list/views/speech_view_unittest.cc:23: widget_ =
CreateTopLevelPlatformWidget();
On 2014/05/29 20:25:12, xiyuan wrote:
> By default, test widget is very small. Give it a big enough size,
> 
> e.g.
> widget_->SetBounds(gfx::Rect(0, 0, 300, 200));

Done.

https://codereview.chromium.org/302653002/diff/60001/ui/app_list/views/speech...
ui/app_list/views/speech_view_unittest.cc:26: view_->Layout();
On 2014/05/29 20:25:12, xiyuan wrote:
> No need to call Layout explicitly. Instead, we should make the SpeechView fill
> the content area,
> 
> e.g.
> view_->SetBoundsRect(widget_->GetContentsView()->GetLocalBounds());

Done.

https://codereview.chromium.org/302653002/diff/60001/ui/app_list/views/speech...
ui/app_list/views/speech_view_unittest.cc:54: ui::MouseEvent
press(ui::ET_MOUSE_PRESSED, screen_bounds.CenterPoint(),
screen_bounds.CenterPoint(), 0, 0);
On 2014/05/29 20:25:12, xiyuan wrote:
> The mouse event should have ui::EF_LEFT_MOUSE_BUTTON in its flags. Otherwise,
> button click does not fire.

Done.

[xiyuan, 2014-05-29 21:41:07]

Cool. LGTM!

[tdanderson, 2014-05-29 21:53:04]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-05-29 21:55:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdanderson@chromium.org/302653002/80001

[tdanderson, 2014-05-29 22:43:16]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-05-29 22:45:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdanderson@chromium.org/302653002/100001

[commit-bot: I haz the power, 2014-05-30 07:01:22]

Change committed as 273740