Views fuzzing for Aura and Windows

Touch and Mouse event handlers are now synchronized. In the case of touch, we consider any view that intersects the touch location, and select the one that has the greatest overlap.

This builds off of the work done by tdanderson in https://chromiumcodereview.appspot.com/10790019/

BUG=129794

[girard, 2012-09-18 16:58:52]

This is a work in progress, but contains most of the code needed to get
views/fuzzing working, both in Windows and ChromeOS.  

I've done a quick self-review, and I'm seeking design feedback.

https://chromiumcodereview.appspot.com/10938009/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://chromiumcodereview.appspot.com/10938009/diff/12002/chrome/browser/ui/...
chrome/browser/ui/views/tabs/tab_strip.cc:1348: double CoverageProportion(const
gfx::Rect& rect_one,
Move this into the anonymous namespace at the top of the file.

https://chromiumcodereview.appspot.com/10938009/diff/12002/chrome/browser/ui/...
chrome/browser/ui/views/tabs/tab_strip.cc:1403: if (view)
This should only return if MOUSE.  Otherwise continue considering other options.

https://chromiumcodereview.appspot.com/10938009/diff/12002/chrome/browser/ui/...
chrome/browser/ui/views/tabs/tab_strip.cc:1407: Tab* tab =
FindTabForEvent(rect);
This logic should be expanded here, so we can consider all the alternatives.

https://chromiumcodereview.appspot.com/10938009/diff/12002/chrome/browser/ui/...
chrome/browser/ui/views/tabs/tab_strip.cc:1409: return
ConvertRectToViewAndGetEventHandler(this, tab, rect, type);
Ditto.

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.h
File ui/views/view.h (right):

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.h#ne...
ui/views/view.h:1056: public:
make protected.
This will mess up all the other headers, but it's the right thing to do.

[sadrul, 2012-09-18 17:29:22]

http://codereview.chromium.org/10938009/diff/12002/ui/views/view.cc
File ui/views/view.cc (left):

http://codereview.chromium.org/10938009/diff/12002/ui/views/view.cc#oldcode762
ui/views/view.cc:762: return
child->GetEventHandlerForPoint(point_in_child_coords);
It is important to continue to use the hit-testing for points. And I think we
should do this for rects too. (i.e. the hit-test related interface also needs to
be updated to be able to handle a rect)

http://codereview.chromium.org/10938009/diff/12002/ui/views/view.cc
File ui/views/view.cc (right):

http://codereview.chromium.org/10938009/diff/12002/ui/views/view.cc#newcode684
ui/views/view.cc:684: rect->set_origin(rect_location);
Just converting the origin isn't sufficient. You would want to transform the
whole rect (see ConvertRectToParent, for example).

http://codereview.chromium.org/10938009/diff/12002/ui/views/view.cc#newcode813
ui/views/view.cc:813: View* View::GetEventHandlerForRect(const gfx::Rect& rect)
{
Without changes in RootView, it is difficult to understand exactly how you are
planning on using these ForPoint/ForRect versions. But it feels like in the
current implementation, it would be better to just keep ForPoint as it is, and
introduce ForRect separately.

[tdanderson, 2012-09-18 19:19:02]

https://chromiumcodereview.appspot.com/10938009/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://chromiumcodereview.appspot.com/10938009/diff/12002/chrome/browser/ui/...
chrome/browser/ui/views/tabs/tab_strip.cc:1368: int touch_size = 24;
I'm not sure where this 24 comes from. Can you please include a comment?

https://chromiumcodereview.appspot.com/10938009/diff/12002/chrome/browser/ui/...
chrome/browser/ui/views/tabs/tab_strip.cc:1377: int closest_distance = INT_MAX;
Are you planning on using these in the same way as in view.cc?

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.cc
File ui/views/view.cc (right):

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.cc#n...
ui/views/view.cc:814: // TODO: Consider expanding small (single point)
rectangles.
What do you mean by "expanding" small rectangles?

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.cc#n...
ui/views/view.cc:819: View* closest_view = NULL;
I think calling this |closest_view| in your current implementation is a bit
misleading, since this variable actually represents the View having the largest
overlap proportion among the Views which have a non-empty intersection with the
touch region.

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.cc#n...
ui/views/view.cc:853: } else if (!closest_view || proportion > max_proportion) {
I am worried about the case where there are >=2 views which are completely
contained within the touch region. In this case shouldn't we use distance to
center line to break ties?

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.cc#n...
ui/views/view.cc:866: if (closest_untouched_view)
It seems this is only reached if a touch region doesn't actually intersect any
view element. Is this even possible?

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.h
File ui/views/view.h (right):

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.h#ne...
ui/views/view.h:420: gfx::Rect* rect);
nit: spacing

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.h#ne...
ui/views/view.h:1067: // rectangles, NULL is returned.
I think this description is from my old patch. Are you still planning on using
the kFuzzingOverlapPercentage threshold in any way?

[sky, 2012-09-18 19:51:41]

I only looked at View. You need to deal with HitTest before you can continue. In
fact you might want to do that in its own patch.

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.cc
File ui/views/view.cc (right):

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.cc#n...
ui/views/view.cc:818: View* View::GetEventHandler(const gfx::Rect& rect,
EventType type) {
ordering doesn't match header.

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.h
File ui/views/view.h (right):

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.h#ne...
ui/views/view.h:514: // Returns the appropriate event handler for the given
touch rect.
If this is specific to touch, it should be named so, eg GetEventHandlerForTouch.

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.h#ne...
ui/views/view.h:1056: public:
On 2012/09/18 16:58:52, girard wrote:
> make protected.
> This will mess up all the other headers, but it's the right thing to do.

Huh? Move the enum and function to the right sections. Arbitrarily adding new
sections doesn't scale.

https://chromiumcodereview.appspot.com/10938009/diff/12002/ui/views/view.h#ne...
ui/views/view.h:1058: enum EventType { MOUSE, TOUCH };
Make these more descriptive. Maybe EventHandlerType EVENT_HANDLER_TYPE_MOUSE and
EVENT_HANDLER_TYPE_TOUCH.