Do not call into EventTargeter methods from ViewTargeter

Do not call into EventTargeter methods from ViewTargeter

Now that hit-testing for views (crbug.com/388838) and
event-targeting for views (crbug.com/391845) use
ViewTargeterDelegate to define custom behaviour for
these operations (instead of the now-obsolete
approach of subclassing ViewTargeter itself), we
should not be calling into EventTargeter for this
purpose. Changes in this CL:

* Finding the target of a scroll event is now
  performed correctly by using the logic in
  ViewTargeterDelegate and its overrides.

* The dead code
  ViewTargeter::SubtreeCanAcceptEvent(),
  ViewTargeter::EventLocationInsideBounds(),
  and ViewTargeter::BoundsForEvent() have been
  removed along with their corresponding tests.

* The class-level documentation for ViewTargeter
  and ViewTargeterDelegate has been updated
  to clarify the purpose of these classes.

BUG=395387, 370579
TEST=ViewTargeterTest.*

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

[tdanderson, 2014-07-26 18:08:29]

Sadrul, can you please take a look?

[sadrul, 2014-07-29 18:06:50]

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

https://codereview.chromium.org/426443002/diff/1/ui/views/view_targeter.cc#ne...
ui/views/view_targeter.cc:60: return
root->GetEventHandlerForPoint(scroll.location());
This should directly call into the delegate, something like:

  return root->GetEffectiveViewTargeter()->TargetForRect(...)

This would normally just do:

  Targeter* targeter = root->targeter();
  if (!targeter)
    targeter = this;
  return targeter->TargetForRect(...);

But because the 'effective' view-targeter goes upto the root-view, we should
look it up from |root|.

https://codereview.chromium.org/426443002/diff/1/ui/views/view_targeter.h
File ui/views/view_targeter.h (left):

https://codereview.chromium.org/426443002/diff/1/ui/views/view_targeter.h#old...
ui/views/view_targeter.h:45: virtual bool SubtreeCanAcceptEvent(
How about keeping these with NOTREACHED()?

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

https://codereview.chromium.org/426443002/diff/1/ui/views/view_targeter.h#new...
ui/views/view_targeter.h:16: // A ViewTargeter is installed on a derived class
of View that wishes to use
's/derived class of//'

You should be able to install it on any View, right?

https://codereview.chromium.org/426443002/diff/1/ui/views/view_targeter_deleg...
File ui/views/view_targeter_delegate.h (right):

https://codereview.chromium.org/426443002/diff/1/ui/views/view_targeter_deleg...
ui/views/view_targeter_delegate.h:22: // ViewTargeter on themselves.
s/Derived classes of View/Views/

[tdanderson, 2014-07-29 19:40:45]

Hi Sadrul, please take another look.

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

https://codereview.chromium.org/426443002/diff/1/ui/views/view_targeter.cc#ne...
ui/views/view_targeter.cc:60: return
root->GetEventHandlerForPoint(scroll.location());
On 2014/07/29 18:06:50, sadrul wrote:
> This should directly call into the delegate, something like:
> 
>   return root->GetEffectiveViewTargeter()->TargetForRect(...)
> 
> This would normally just do:
> 
>   Targeter* targeter = root->targeter();
>   if (!targeter)
>     targeter = this;
>   return targeter->TargetForRect(...);
> 
> But because the 'effective' view-targeter goes upto the root-view, we should
> look it up from |root|.

Is your suggestion about correctness or is it about design? Because I think I
disagree either way. The code should be correct as-is, and as a matter of design
I would rather call into View::GetEventHandlerForPoint() (which everyone else
already does anyway) and keep call sites of ViewTargeter::TargetForRect() to a
minimum.

https://codereview.chromium.org/426443002/diff/1/ui/views/view_targeter.h
File ui/views/view_targeter.h (left):

https://codereview.chromium.org/426443002/diff/1/ui/views/view_targeter.h#old...
ui/views/view_targeter.h:45: virtual bool SubtreeCanAcceptEvent(
On 2014/07/29 18:06:50, sadrul wrote:
> How about keeping these with NOTREACHED()?

Sure, seems reasonable. We can remove them entirely when crbug.com/395388 is
addressed.

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

https://codereview.chromium.org/426443002/diff/1/ui/views/view_targeter.h#new...
ui/views/view_targeter.h:16: // A ViewTargeter is installed on a derived class
of View that wishes to use
On 2014/07/29 18:06:50, sadrul wrote:
> 's/derived class of//'
> 
> You should be able to install it on any View, right?

Yep, this was careless wording on my part. Fixed.

https://codereview.chromium.org/426443002/diff/1/ui/views/view_targeter_deleg...
File ui/views/view_targeter_delegate.h (right):

https://codereview.chromium.org/426443002/diff/1/ui/views/view_targeter_deleg...
ui/views/view_targeter_delegate.h:22: // ViewTargeter on themselves.
On 2014/07/29 18:06:50, sadrul wrote:
> s/Derived classes of View/Views/

Done.

[tdanderson, 2014-08-01 15:53:07]

Sadrul, please see my question below:

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

https://codereview.chromium.org/426443002/diff/1/ui/views/view_targeter.cc#ne...
ui/views/view_targeter.cc:60: return
root->GetEventHandlerForPoint(scroll.location());
On 2014/07/29 19:40:45, tdanderson wrote:
> On 2014/07/29 18:06:50, sadrul wrote:
> > This should directly call into the delegate, something like:
> > 
> >   return root->GetEffectiveViewTargeter()->TargetForRect(...)
> > 
> > This would normally just do:
> > 
> >   Targeter* targeter = root->targeter();
> >   if (!targeter)
> >     targeter = this;
> >   return targeter->TargetForRect(...);
> > 
> > But because the 'effective' view-targeter goes upto the root-view, we should
> > look it up from |root|.
> 
> Is your suggestion about correctness or is it about design? Because I think I
> disagree either way. The code should be correct as-is, and as a matter of
design
> I would rather call into View::GetEventHandlerForPoint() (which everyone else
> already does anyway) and keep call sites of ViewTargeter::TargetForRect() to a
> minimum.

I forget the outcome of our in-person chat about this. Is
this OK to leave as-is or is it something we definitely
need changed to your suggestion?

[sadrul, 2014-08-01 15:58:45]

On 2014/08/01 15:53:07, tdanderson wrote:
> Sadrul, please see my question below:
> 
> https://codereview.chromium.org/426443002/diff/1/ui/views/view_targeter.cc
> File ui/views/view_targeter.cc (right):
> 
>
https://codereview.chromium.org/426443002/diff/1/ui/views/view_targeter.cc#ne...
> ui/views/view_targeter.cc:60: return
> root->GetEventHandlerForPoint(scroll.location());
> On 2014/07/29 19:40:45, tdanderson wrote:
> > On 2014/07/29 18:06:50, sadrul wrote:
> > > This should directly call into the delegate, something like:
> > > 
> > >   return root->GetEffectiveViewTargeter()->TargetForRect(...)
> > > 
> > > This would normally just do:
> > > 
> > >   Targeter* targeter = root->targeter();
> > >   if (!targeter)
> > >     targeter = this;
> > >   return targeter->TargetForRect(...);
> > > 
> > > But because the 'effective' view-targeter goes upto the root-view, we
should
> > > look it up from |root|.
> > 
> > Is your suggestion about correctness or is it about design? Because I think
I
> > disagree either way. The code should be correct as-is, and as a matter of
> design
> > I would rather call into View::GetEventHandlerForPoint() (which everyone
else
> > already does anyway) and keep call sites of ViewTargeter::TargetForRect() to
a
> > minimum.
> 
> I forget the outcome of our in-person chat about this. Is
> this OK to leave as-is or is it something we definitely
> need changed to your suggestion?

We want to have only a small dependency on View from ViewTargeter. So we should
directly call into TargetForRect() from ViewTargeter.

[tdanderson, 2014-08-01 17:04:05]

Sadrul, comments have been addressed. Can you please take another look?

[sadrul, 2014-08-05 16:41:17]

LGTM

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

https://codereview.chromium.org/426443002/diff/40001/ui/views/view.cc#newcode...
ui/views/view.cc:1034: ViewTargeter* view_targeter = targeter();
Do a DCHECK() here that GetWidget() is not null

[tdanderson, 2014-08-05 17:18:09]

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

https://codereview.chromium.org/426443002/diff/40001/ui/views/view.cc#newcode...
ui/views/view.cc:1034: ViewTargeter* view_targeter = targeter();
On 2014/08/05 16:41:17, sadrul wrote:
> Do a DCHECK() here that GetWidget() is not null

Done.

[tdanderson, 2014-08-05 17:18:13]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-08-05 17:19:50]

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

[commit-bot: I haz the power, 2014-08-05 20:56:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42741)

[tdanderson, 2014-08-05 22:00:35]

The CQ bit was unchecked by tdanderson@chromium.org

[tdanderson, 2014-08-05 22:00:37]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-08-05 22:03:26]

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

[commit-bot: I haz the power, 2014-08-05 22:28:52]

Change committed as 287620