Change View::HitTest to View::HitTestRect

Change View::HitTest to View::HitTestRect

This change makes HitTest more general by allowing hit tests on rects. Hit testing on a point can be done by calling View::HitTestPoint, which calls View::HitTestRect with a rect of width and height 1.
Since gesture events can now store a bounding box to represent the area of the gesture
rather than just a single point, this change is needed for the implementation of 
gesture target fuzzing in views (https://chromiumcodereview.appspot.com/10790019/).

BUG=129794
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150954

[tdanderson, 2012-08-07 16:18:28]

Please let me know your thoughts on this patch, especially my comment about an
empty vs. non-empty rect in view.cc.

The existing views_unittests pass with this patch, but I have not yet added any
of my own (I'd like to wait and see what you have to say about this patch
first).

One other question: should NonClientHitTest(Point) be changed as well?

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc:343: if (r.y()
> tabstrip_bounds.bottom())
Should this instead be r.y() + r.height() ?

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:413: if (r.y() >
tabstrip_bounds.bottom())
Should this instead be r.y() + r.height() ?

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar_view.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar_view.cc:772: if (rect.y() <
kContentShadowHeight) {
Should this instead be rect.y() + rect.height() ?

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

https://chromiumcodereview.appspot.com/10827198/diff/1/ui/views/view.cc#newco...
ui/views/view.cc:779: GetLocalBounds().Intersects(r)) {
I made the decision to represent a point using a rect of width and height 0. But
on second thought, after making this change I realize that doing so introduces
the inconvenience of needing to call Contains if the rect is empty vs. calling
Intersects if it is not. If I instead represent a point as a rect having width
and height 1, Intersects can be called in both cases. Let me know if you agree
with this change and I will make it.

https://chromiumcodereview.appspot.com/10827198/diff/1/ui/views/view.cc#newco...
ui/views/view.cc:798: return !!RectInRegion(rgn, rect);
I have not tested that above two lines of code are correct.

[sky, 2012-08-07 20:11:43]

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc:343: if (r.y()
> tabstrip_bounds.bottom())
On 2012/08/07 16:18:28, tdanderson wrote:
> Should this instead be r.y() + r.height() ?

Yes, I believe it should be r.bottom().

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc:349: gfx::Point
browser_view_point(gfx::Point(r.x(), r.y()));
I don't think this is right here, it only tests the origin.
IsPositionInWindowCaption likely needs to be updated too.

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:413: if (r.y() >
tabstrip_bounds.bottom())
On 2012/08/07 16:18:28, tdanderson wrote:
> Should this instead be r.y() + r.height() ?

Same comments in this file as the ash file.

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:168: if
(!HitTest(gfx::Rect(event.location(), gfx::Size(0, 0))))
Based on how many calls of this we have I think its worth adding two:

bool HitTestPoint(const gfx::Point& point) {
  return HitTestRect(gfx::Rect(point, whatever size you decide);
}

The key thing is the first is non-virtual.

virtual bool HitRectRect(...);

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar_view.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar_view.cc:772: if (rect.y() <
kContentShadowHeight) {
On 2012/08/07 16:18:28, tdanderson wrote:
> Should this instead be rect.y() + rect.height() ?

I think this one wants want like you have.

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

https://chromiumcodereview.appspot.com/10827198/diff/1/ui/views/view.cc#newco...
ui/views/view.cc:779: GetLocalBounds().Intersects(r)) {
On 2012/08/07 16:18:28, tdanderson wrote:
> I made the decision to represent a point using a rect of width and height 0.
But
> on second thought, after making this change I realize that doing so introduces
> the inconvenience of needing to call Contains if the rect is empty vs. calling
> Intersects if it is not. If I instead represent a point as a rect having width
> and height 1, Intersects can be called in both cases. Let me know if you agree
> with this change and I will make it.

I like whatever is easiest all around. If you say a rect of 1x1 is easier than
0x0, go with that.

https://chromiumcodereview.appspot.com/10827198/diff/1/ui/views/view.cc#newco...
ui/views/view.cc:798: return !!RectInRegion(rgn, rect);
On 2012/08/07 16:18:28, tdanderson wrote:
> I have not tested that above two lines of code are correct.

I don't think they are. Shouldn't rect be a value and you do &rect to
RectInRegion? Also, nuke the !! it's hard to read.

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

https://chromiumcodereview.appspot.com/10827198/diff/1/ui/views/view.h#newcod...
ui/views/view.h:512: // Convenience to test whether a point is within this
view's bounds
Update description.

https://chromiumcodereview.appspot.com/10827198/diff/1/ui/views/window/non_cl...
File ui/views/window/non_client_view.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/ui/views/window/non_cl...
ui/views/window/non_client_view.cc:254: return
!GetWidget()->client_view()->bounds().Contains(gfx::Point(r.x(),
r.origin()

[tdanderson, 2012-08-08 23:47:50]

Please let me know what you think of patch set 2.

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc:343: if (r.y()
> tabstrip_bounds.bottom())
On 2012/08/07 20:11:43, sky wrote:
> On 2012/08/07 16:18:28, tdanderson wrote:
> > Should this instead be r.y() + r.height() ?
> 
> Yes, I believe it should be r.bottom().

Done.

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc:349: gfx::Point
browser_view_point(gfx::Point(r.x(), r.y()));
On 2012/08/07 20:11:43, sky wrote:
> I don't think this is right here, it only tests the origin.
> IsPositionInWindowCaption likely needs to be updated too.

I passed in the center point since TabStrip::IsPositionInWindowCaption uses
View::GetEventHandlerForPoint, which means that there is no reason to pass a
rect since we do not have a GetEventHandlerForRect or a general GetEventHandler
implemented.

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:413: if (r.y() >
tabstrip_bounds.bottom())
On 2012/08/07 20:11:43, sky wrote:
> On 2012/08/07 16:18:28, tdanderson wrote:
> > Should this instead be r.y() + r.height() ?
> 
> Same comments in this file as the ash file.

Done.

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:419: gfx::Point
browser_view_point(gfx::Point(r.x(), r.y()));
I am also passing through the center point here for the same reasons as in the
ash file. Please let me know if you feel this is acceptable.

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:168: if
(!HitTest(gfx::Rect(event.location(), gfx::Size(0, 0))))
On 2012/08/07 20:11:43, sky wrote:
> Based on how many calls of this we have I think its worth adding two:
> 
> bool HitTestPoint(const gfx::Point& point) {
>   return HitTestRect(gfx::Rect(point, whatever size you decide);
> }
> 
> The key thing is the first is non-virtual.
> 
> virtual bool HitRectRect(...);

Done.

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar_view.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar_view.cc:772: if (rect.y() <
kContentShadowHeight) {
On 2012/08/07 20:11:43, sky wrote:
> On 2012/08/07 16:18:28, tdanderson wrote:
> > Should this instead be rect.y() + rect.height() ?
> 
> I think this one wants want like you have.

Ok, I left this alone.

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

https://chromiumcodereview.appspot.com/10827198/diff/1/ui/views/view.cc#newco...
ui/views/view.cc:779: GetLocalBounds().Intersects(r)) {
On 2012/08/07 20:11:43, sky wrote:
> On 2012/08/07 16:18:28, tdanderson wrote:
> > I made the decision to represent a point using a rect of width and height 0.
> But
> > on second thought, after making this change I realize that doing so
introduces
> > the inconvenience of needing to call Contains if the rect is empty vs.
calling
> > Intersects if it is not. If I instead represent a point as a rect having
width
> > and height 1, Intersects can be called in both cases. Let me know if you
agree
> > with this change and I will make it.
> 
> I like whatever is easiest all around. If you say a rect of 1x1 is easier than
> 0x0, go with that.

I have gone with 1x1 instead of 0x0 so that HitTestRect can call Intersects in
all cases (rather than needing to call Contains in the special case of an empty
rect).

https://chromiumcodereview.appspot.com/10827198/diff/1/ui/views/view.cc#newco...
ui/views/view.cc:798: return !!RectInRegion(rgn, rect);
On 2012/08/07 20:11:43, sky wrote:
> On 2012/08/07 16:18:28, tdanderson wrote:
> > I have not tested that above two lines of code are correct.
> 
> I don't think they are. Shouldn't rect be a value and you do &rect to
> RectInRegion? Also, nuke the !! it's hard to read.

Changed. (I thought that the !! was needed to force the (int) BOOL to bool?)

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

https://chromiumcodereview.appspot.com/10827198/diff/1/ui/views/view.h#newcod...
ui/views/view.h:512: // Convenience to test whether a point is within this
view's bounds
On 2012/08/07 20:11:43, sky wrote:
> Update description.

Done.

https://chromiumcodereview.appspot.com/10827198/diff/1/ui/views/window/non_cl...
File ui/views/window/non_client_view.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/ui/views/window/non_cl...
ui/views/window/non_client_view.cc:254: return
!GetWidget()->client_view()->bounds().Contains(gfx::Point(r.x(),
On 2012/08/07 20:11:43, sky wrote:
> r.origin()

Done.

[sky, 2012-08-09 00:30:14]

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc:349: gfx::Point
browser_view_point(gfx::Point(r.x(), r.y()));
On 2012/08/08 23:47:51, tdanderson wrote:
> On 2012/08/07 20:11:43, sky wrote:
> > I don't think this is right here, it only tests the origin.
> > IsPositionInWindowCaption likely needs to be updated too.
> 
> I passed in the center point since TabStrip::IsPositionInWindowCaption uses
> View::GetEventHandlerForPoint, which means that there is no reason to pass a
> rect since we do not have a GetEventHandlerForRect or a general
GetEventHandler
> implemented.

Can you add a TODO then so that you don't miss updating it when we get
GetEventHandlerForRect? Alternatively add it know, but had it call with the
center so that you can get this working. I'm fine with either approach for now.

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

https://chromiumcodereview.appspot.com/10827198/diff/1/ui/views/view.cc#newco...
ui/views/view.cc:798: return !!RectInRegion(rgn, rect);
On 2012/08/08 23:47:51, tdanderson wrote:
> On 2012/08/07 20:11:43, sky wrote:
> > On 2012/08/07 16:18:28, tdanderson wrote:
> > > I have not tested that above two lines of code are correct.
> > 
> > I don't think they are. Shouldn't rect be a value and you do &rect to
> > RectInRegion? Also, nuke the !! it's hard to read.
> 
> Changed. (I thought that the !! was needed to force the (int) BOOL to bool?)

It is, I'm suggesting coverting to == or !=

[sky, 2012-08-09 00:33:51]

This is the last of the comments I have.

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

https://chromiumcodereview.appspot.com/10827198/diff/4002/ui/views/view.h#new...
ui/views/view.h:512: // A convenience function which calls HitTestRect with a
rect of size
Use () when referring to methods, eg HitTestRect() here and 1035 & 1041.

[tdanderson, 2012-08-09 16:16:46]

I have addressed all of your comments, so this patch should be good to go.

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/ash/browser_non_client_frame_view_ash.cc:349: gfx::Point
browser_view_point(gfx::Point(r.x(), r.y()));
On 2012/08/09 00:30:14, sky wrote:
> On 2012/08/08 23:47:51, tdanderson wrote:
> > On 2012/08/07 20:11:43, sky wrote:
> > > I don't think this is right here, it only tests the origin.
> > > IsPositionInWindowCaption likely needs to be updated too.
> > 
> > I passed in the center point since TabStrip::IsPositionInWindowCaption uses
> > View::GetEventHandlerForPoint, which means that there is no reason to pass a
> > rect since we do not have a GetEventHandlerForRect or a general
> GetEventHandler
> > implemented.
> 
> Can you add a TODO then so that you don't miss updating it when we get
> GetEventHandlerForRect? Alternatively add it know, but had it call with the
> center so that you can get this working. I'm fine with either approach for
now.

I have added a TODO(tdanderson) for now and will update this when I resume work
on views fuzzing.

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://chromiumcodereview.appspot.com/10827198/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:419: gfx::Point
browser_view_point(gfx::Point(r.x(), r.y()));
On 2012/08/08 23:47:51, tdanderson wrote:
> I am also passing through the center point here for the same reasons as in the
> ash file. Please let me know if you feel this is acceptable.

Also added a TODO here as well.

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

https://chromiumcodereview.appspot.com/10827198/diff/1/ui/views/view.cc#newco...
ui/views/view.cc:798: return !!RectInRegion(rgn, rect);
On 2012/08/09 00:30:14, sky wrote:
> On 2012/08/08 23:47:51, tdanderson wrote:
> > On 2012/08/07 20:11:43, sky wrote:
> > > On 2012/08/07 16:18:28, tdanderson wrote:
> > > > I have not tested that above two lines of code are correct.
> > > 
> > > I don't think they are. Shouldn't rect be a value and you do &rect to
> > > RectInRegion? Also, nuke the !! it's hard to read.
> > 
> > Changed. (I thought that the !! was needed to force the (int) BOOL to bool?)
> 
> It is, I'm suggesting coverting to == or !=

Done.

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

https://chromiumcodereview.appspot.com/10827198/diff/4002/ui/views/view.h#new...
ui/views/view.h:512: // A convenience function which calls HitTestRect with a
rect of size
On 2012/08/09 00:33:52, sky wrote:
> Use () when referring to methods, eg HitTestRect() here and 1035 & 1041.

Done.

[sky, 2012-08-09 16:37:52]

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

https://chromiumcodereview.appspot.com/10827198/diff/11001/ui/views/view.cc#n...
ui/views/view.cc:794: const RECT r(rect.x(), rect.y(), rect.width(),
rect.height());
Use rect.ToRect() (especially since the code you have here creates the wrong
rect, it's left/right/top/bottom).

[tdanderson, 2012-08-09 17:00:19]

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

https://chromiumcodereview.appspot.com/10827198/diff/11001/ui/views/view.cc#n...
ui/views/view.cc:794: const RECT r(rect.x(), rect.y(), rect.width(),
rect.height());
On 2012/08/09 16:37:53, sky wrote:
> Use rect.ToRect() (especially since the code you have here creates the wrong
> rect, it's left/right/top/bottom).

Done.

[sky, 2012-08-09 18:31:57]

LGTM

[commit-bot: I haz the power, 2012-08-09 18:35:16]

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

[commit-bot: I haz the power, 2012-08-09 18:57:41]

Try job failure for 10827198-44 (retry) on win for step "runhooks" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[tdanderson, 2012-08-09 21:01:27]

Patch set 5 renames HitTest to HitTestPoint where the trybots reported a
failure. AFAICT these are the only places where I forgot to update the function
call with the new name.

[sky, 2012-08-09 21:43:22]

SLGTM

[commit-bot: I haz the power, 2012-08-09 21:48:40]

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

[commit-bot: I haz the power, 2012-08-10 00:43:26]

Change committed as 150954