views: Update event-related API to use PointF/RectF instead of Point/Rect.

views: Update event-related API to use PointF/RectF instead of Point/Rect.

 * View
   - GetEventHandlerForRect()
   - GetEventHandlerForPoint()
   - HitTestPoint()
   - HitTestRect()
   - ExceededDragThreshold()
   - GetDragOperations()
   - WriteDragData()
   - DoDrag()

 * DragController
   - WriteDragDataForView()
   - GetDragOperationsForView()
   - CanStartDragForView()

 * TabStrip
   - IsPositionInWindowCaption()
   - IsRectInWindowCaption()

BUG=337824

[tdresser, 2014-05-02 13:19:05]

Should we modify some existing tests to use non-integral floating point values?

https://codereview.chromium.org/265713007/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/265713007/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:331: gfx::RectF
rect_in_window_switcher_coords_f(rect);
|rect_in_window_switcher_coords_f| should probably be renamed
|rect_in_window_switcher_coords|, as we're moving away from using the "_f"
suffix.

https://codereview.chromium.org/265713007/diff/1/ui/views/rect_based_targetin...
File ui/views/rect_based_targeting_utils.cc (right):

https://codereview.chromium.org/265713007/diff/1/ui/views/rect_based_targetin...
ui/views/rect_based_targeting_utils.cc:13: return rect.width() == 1.f &&
rect.height() == 1.f;
Is comparing equality of floats here acceptable, or should we use a proper
floating point comparison?

[sky, 2014-05-02 14:42:16]

Sadrul, could you outline the motivation for going floating point
here? Are you looking at moving views to floating point as well?

  -Scott

On Thu, May 1, 2014 at 11:42 PM,  <sadrul@chromium.org> wrote:
> Reviewers: sky, tdresser,
>
> Description:
> views: Update event-related API to use PointF/RectF instead of Point/Rect.
>
>  * View
>    - GetEventHandlerForRect()
>    - GetEventHandlerForPoint()
>    - HitTestPoint()
>    - HitTestRect()
>    - ExceededDragThreshold()
>    - GetDragOperations()
>    - WriteDragData()
>    - DoDrag()
>
>  * DragController
>    - WriteDragDataForView()
>    - GetDragOperationsForView()
>    - CanStartDragForView()
>
>  * TabStrip
>    - IsPositionInWindowCaption()
>    - IsRectInWindowCaption()
>
> BUG=337824
>
> Please review this at https://codereview.chromium.org/265713007/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+200, -202 lines):
>   M ash/drag_drop/drag_drop_controller_unittest.cc
>   M ash/drag_drop/drag_drop_interactive_uitest.cc
>   M ash/frame/custom_frame_view_ash.h
>   M ash/frame/custom_frame_view_ash.cc
>   M ash/shelf/shelf_button.h
>   M ash/shelf/shelf_button.cc
>   M chrome/browser/ui/views/autofill/autofill_dialog_views.h
>   M chrome/browser/ui/views/autofill/autofill_dialog_views.cc
>   M chrome/browser/ui/views/autofill/decorated_textfield.h
>   M chrome/browser/ui/views/autofill/decorated_textfield.cc
>   M chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
>   M chrome/browser/ui/views/bookmarks/bookmark_bar_view.h
>   M chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
>   M chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h
>   M chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc
>   M chrome/browser/ui/views/frame/glass_browser_frame_view.h
>   M chrome/browser/ui/views/frame/opaque_browser_frame_view.h
>   M chrome/browser/ui/views/frame/opaque_browser_frame_view.cc
>   M chrome/browser/ui/views/location_bar/location_bar_view.h
>   M chrome/browser/ui/views/location_bar/location_bar_view.cc
>   M chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h
>   M chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc
>   M chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc
>   M chrome/browser/ui/views/profiles/avatar_menu_button.h
>   M chrome/browser/ui/views/profiles/avatar_menu_button.cc
>   M chrome/browser/ui/views/tabs/tab.cc
>   M chrome/browser/ui/views/tabs/tab_strip.h
>   M chrome/browser/ui/views/tabs/tab_strip.cc
>   M chrome/browser/ui/views/toolbar/browser_actions_container.h
>   M chrome/browser/ui/views/toolbar/browser_actions_container.cc
>   M chrome/browser/ui/views/toolbar/toolbar_origin_chip_view.h
>   M chrome/browser/ui/views/toolbar/toolbar_origin_chip_view.cc
>   M chrome/browser/ui/views/toolbar/toolbar_view.h
>   M chrome/browser/ui/views/toolbar/toolbar_view.cc
>   M ui/app_list/views/speech_view.cc
>   M ui/base/dragdrop/drag_utils_aura.cc
>   M ui/gfx/skia_util.h
>   M ui/gfx/skia_util.cc
>   M ui/message_center/views/bounded_label.h
>   M ui/message_center/views/bounded_label.cc
>   M ui/message_center/views/notification_view.h
>   M ui/message_center/views/notification_view.cc
>   M ui/views/controls/image_view.h
>   M ui/views/controls/image_view.cc
>   M ui/views/controls/label.h
>   M ui/views/controls/label.cc
>   M ui/views/controls/link.h
>   M ui/views/controls/link.cc
>   M ui/views/controls/textfield/textfield.h
>   M ui/views/controls/textfield/textfield.cc
>   M ui/views/controls/textfield/textfield_unittest.cc
>   M ui/views/drag_controller.h
>   M ui/views/rect_based_targeting_utils.h
>   M ui/views/rect_based_targeting_utils.cc
>   M ui/views/view.h
>   M ui/views/view.cc
>   M ui/views/widget/root_view.cc
>   M ui/views/window/non_client_view.h
>   M ui/views/window/non_client_view.cc
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2014-05-02 18:19:49]

On 2014/05/02 14:42:16, sky wrote:
> Sadrul, could you outline the motivation for going floating point
> here? Are you looking at moving views to floating point as well?

tldr: Some, but not necessarily all of it.

For Events, we now have floating-point locations. We need to clean-up the
left-over TODOs from that change (Tim has been working on this:
https://codereview.chromium.org/139983009/). I want to avoid having to get the
event-location and always have to do a gfx::ToFlooredPoint() on it immediately.
So I am trying to change the views API that deal with events to also use
floating points. This also ties in well with doing the coordinate-conversion
correctly (https://code.google.com/p/chromium/issues/detail?id=364074). The
primary motivations for these are developer-friendliness/code-cleanliness, and
correctness. (it may make sense to eventually convert all of views to use
floats, but I am not targeting that at the moment).

[sky, 2014-05-02 21:21:15]

On 2014/05/02 18:19:49, sadrul wrote:
> On 2014/05/02 14:42:16, sky wrote:
> > Sadrul, could you outline the motivation for going floating point
> > here? Are you looking at moving views to floating point as well?
> 
> tldr: Some, but not necessarily all of it.
> 
> For Events, we now have floating-point locations. We need to clean-up the
> left-over TODOs from that change (Tim has been working on this:
> https://codereview.chromium.org/139983009/). I want to avoid having to get the
> event-location and always have to do a gfx::ToFlooredPoint() on it
immediately.
> So I am trying to change the views API that deal with events to also use
> floating points. This also ties in well with doing the coordinate-conversion
> correctly (https://code.google.com/p/chromium/issues/detail?id=364074). The
> primary motivations for these are developer-friendliness/code-cleanliness, and
> correctness. (it may make sense to eventually convert all of views to use
> floats, but I am not targeting that at the moment).

If you're not going to make everything floating point, is there any value in
converting part of it? Fractional hit testing only matters if we can position
views at fractional points, right?

[sadrul, 2014-05-05 19:40:10]

On 2014/05/02 21:21:15, sky wrote:
> On 2014/05/02 18:19:49, sadrul wrote:
> > On 2014/05/02 14:42:16, sky wrote:
> > > Sadrul, could you outline the motivation for going floating point
> > > here? Are you looking at moving views to floating point as well?
> > 
> > tldr: Some, but not necessarily all of it.
> > 
> > For Events, we now have floating-point locations. We need to clean-up the
> > left-over TODOs from that change (Tim has been working on this:
> > https://codereview.chromium.org/139983009/). I want to avoid having to get
the
> > event-location and always have to do a gfx::ToFlooredPoint() on it
> immediately.
> > So I am trying to change the views API that deal with events to also use
> > floating points. This also ties in well with doing the coordinate-conversion
> > correctly (https://code.google.com/p/chromium/issues/detail?id=364074). The
> > primary motivations for these are developer-friendliness/code-cleanliness,
and
> > correctness. (it may make sense to eventually convert all of views to use
> > floats, but I am not targeting that at the moment).
> 
> If you're not going to make everything floating point, is there any value in
> converting part of it? Fractional hit testing only matters if we can position
> views at fractional points, right?

Right. But since we can have (and do want) fractional locations for events, it
makes sense to have at least some of the API be also aware of fractional
locations. I do agree that having only parts of the API deal with fractional
points is not a very nice state. It may make sense to try to convert all of the
API to be aware of this, although that does sound like quite a lot of work, but
perhaps it's worth it? The alternative is to require the code that deal with
event locations to always do a gfx::ToFlooredPoint() immediately
(https://codereview.chromium.org/139983009/), or add a
LocatedEvent::GetLocation() [sic] that does this. While I don't like the latter
approach, I could be convinced that it is the lesser of the evils.

[sky, 2014-05-05 20:55:38]

Until we have a compelling reason to actually consume fractional
locations in views code I'm for the simple approach. I tend to prefer
having new methods for getting the floating point locations and making
the current API returned floored locations.

On Mon, May 5, 2014 at 12:40 PM,  <sadrul@chromium.org> wrote:
> On 2014/05/02 21:21:15, sky wrote:
>>
>> On 2014/05/02 18:19:49, sadrul wrote:
>> > On 2014/05/02 14:42:16, sky wrote:
>> > > Sadrul, could you outline the motivation for going floating point
>> > > here? Are you looking at moving views to floating point as well?
>> >
>> > tldr: Some, but not necessarily all of it.
>> >
>> > For Events, we now have floating-point locations. We need to clean-up
>> > the
>> > left-over TODOs from that change (Tim has been working on this:
>> > https://codereview.chromium.org/139983009/). I want to avoid having to
>> > get
>
> the
>>
>> > event-location and always have to do a gfx::ToFlooredPoint() on it
>> immediately.
>> > So I am trying to change the views API that deal with events to also use
>> > floating points. This also ties in well with doing the
>> > coordinate-conversion
>> > correctly (https://code.google.com/p/chromium/issues/detail?id=364074).
>> > The
>> > primary motivations for these are
>> > developer-friendliness/code-cleanliness,
>
> and
>>
>> > correctness. (it may make sense to eventually convert all of views to
>> > use
>> > floats, but I am not targeting that at the moment).
>
>
>> If you're not going to make everything floating point, is there any value
>> in
>> converting part of it? Fractional hit testing only matters if we can
>> position
>> views at fractional points, right?
>
>
> Right. But since we can have (and do want) fractional locations for events,
> it
> makes sense to have at least some of the API be also aware of fractional
> locations. I do agree that having only parts of the API deal with fractional
> points is not a very nice state. It may make sense to try to convert all of
> the
> API to be aware of this, although that does sound like quite a lot of work,
> but
> perhaps it's worth it? The alternative is to require the code that deal with
> event locations to always do a gfx::ToFlooredPoint() immediately
> (https://codereview.chromium.org/139983009/), or add a
> LocatedEvent::GetLocation() [sic] that does this. While I don't like the
> latter
> approach, I could be convinced that it is the lesser of the evils.
>
> https://codereview.chromium.org/265713007/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2014-05-05 22:49:48]

On 2014/05/05 20:55:38, sky wrote:
> Until we have a compelling reason to actually consume fractional
> locations in views code I'm for the simple approach. I tend to prefer
> having new methods for getting the floating point locations and making
> the current API returned floored locations.

We already have LocatedEvent::location() and LocatedEvent::location_f(). Perhaps
we should change to:

  LocatedEvent:
    const gfx::Point& location() const { return location_; }
    const gfx::PointF& location_f() const { return location_f_; }

    gfx::Point location_;
    gfx::Point location_f_;

LocatedEvent::location_ gets updated from location_f_ in the ctor (instead of
doing the flooring in every call to location()). Does that sound reasonable?

> 
> On Mon, May 5, 2014 at 12:40 PM,  <mailto:sadrul@chromium.org> wrote:
> > On 2014/05/02 21:21:15, sky wrote:
> >>
> >> On 2014/05/02 18:19:49, sadrul wrote:
> >> > On 2014/05/02 14:42:16, sky wrote:
> >> > > Sadrul, could you outline the motivation for going floating point
> >> > > here? Are you looking at moving views to floating point as well?
> >> >
> >> > tldr: Some, but not necessarily all of it.
> >> >
> >> > For Events, we now have floating-point locations. We need to clean-up
> >> > the
> >> > left-over TODOs from that change (Tim has been working on this:
> >> > https://codereview.chromium.org/139983009/). I want to avoid having to
> >> > get
> >
> > the
> >>
> >> > event-location and always have to do a gfx::ToFlooredPoint() on it
> >> immediately.
> >> > So I am trying to change the views API that deal with events to also use
> >> > floating points. This also ties in well with doing the
> >> > coordinate-conversion
> >> > correctly (https://code.google.com/p/chromium/issues/detail?id=364074).
> >> > The
> >> > primary motivations for these are
> >> > developer-friendliness/code-cleanliness,
> >
> > and
> >>
> >> > correctness. (it may make sense to eventually convert all of views to
> >> > use
> >> > floats, but I am not targeting that at the moment).
> >
> >
> >> If you're not going to make everything floating point, is there any value
> >> in
> >> converting part of it? Fractional hit testing only matters if we can
> >> position
> >> views at fractional points, right?
> >
> >
> > Right. But since we can have (and do want) fractional locations for events,
> > it
> > makes sense to have at least some of the API be also aware of fractional
> > locations. I do agree that having only parts of the API deal with fractional
> > points is not a very nice state. It may make sense to try to convert all of
> > the
> > API to be aware of this, although that does sound like quite a lot of work,
> > but
> > perhaps it's worth it? The alternative is to require the code that deal with
> > event locations to always do a gfx::ToFlooredPoint() immediately
> > (https://codereview.chromium.org/139983009/), or add a
> > LocatedEvent::GetLocation() [sic] that does this. While I don't like the
> > latter
> > approach, I could be convinced that it is the lesser of the evils.
> >
> > https://codereview.chromium.org/265713007/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-05-06 00:28:59]

On Mon, May 5, 2014 at 3:49 PM,  <sadrul@chromium.org> wrote:
> On 2014/05/05 20:55:38, sky wrote:
>>
>> Until we have a compelling reason to actually consume fractional
>> locations in views code I'm for the simple approach. I tend to prefer
>> having new methods for getting the floating point locations and making
>> the current API returned floored locations.
>
>
> We already have LocatedEvent::location() and LocatedEvent::location_f().
> Perhaps
> we should change to:
>
>   LocatedEvent:
>     const gfx::Point& location() const { return location_; }
>     const gfx::PointF& location_f() const { return location_f_; }
>
>     gfx::Point location_;
>     gfx::Point location_f_;
>
> LocatedEvent::location_ gets updated from location_f_ in the ctor (instead
> of
> doing the flooring in every call to location()). Does that sound reasonable?

SGTM
>
>
>
>> On Mon, May 5, 2014 at 12:40 PM,  <mailto:sadrul@chromium.org> wrote:
>> > On 2014/05/02 21:21:15, sky wrote:
>> >>
>> >> On 2014/05/02 18:19:49, sadrul wrote:
>> >> > On 2014/05/02 14:42:16, sky wrote:
>> >> > > Sadrul, could you outline the motivation for going floating point
>> >> > > here? Are you looking at moving views to floating point as well?
>> >> >
>> >> > tldr: Some, but not necessarily all of it.
>> >> >
>> >> > For Events, we now have floating-point locations. We need to clean-up
>> >> > the
>> >> > left-over TODOs from that change (Tim has been working on this:
>> >> > https://codereview.chromium.org/139983009/). I want to avoid having
>> >> > to
>> >> > get
>> >
>> > the
>> >>
>> >> > event-location and always have to do a gfx::ToFlooredPoint() on it
>> >> immediately.
>> >> > So I am trying to change the views API that deal with events to also
>> >> > use
>> >> > floating points. This also ties in well with doing the
>> >> > coordinate-conversion
>> >> > correctly
>> >> > (https://code.google.com/p/chromium/issues/detail?id=364074).
>> >> > The
>> >> > primary motivations for these are
>> >> > developer-friendliness/code-cleanliness,
>> >
>> > and
>> >>
>> >> > correctness. (it may make sense to eventually convert all of views to
>> >> > use
>> >> > floats, but I am not targeting that at the moment).
>> >
>> >
>> >> If you're not going to make everything floating point, is there any
>> >> value
>> >> in
>> >> converting part of it? Fractional hit testing only matters if we can
>> >> position
>> >> views at fractional points, right?
>> >
>> >
>> > Right. But since we can have (and do want) fractional locations for
>> > events,
>> > it
>> > makes sense to have at least some of the API be also aware of fractional
>> > locations. I do agree that having only parts of the API deal with
>> > fractional
>> > points is not a very nice state. It may make sense to try to convert all
>> > of
>> > the
>> > API to be aware of this, although that does sound like quite a lot of
>> > work,
>> > but
>> > perhaps it's worth it? The alternative is to require the code that deal
>> > with
>> > event locations to always do a gfx::ToFlooredPoint() immediately
>> > (https://codereview.chromium.org/139983009/), or add a
>> > LocatedEvent::GetLocation() [sic] that does this. While I don't like the
>> > latter
>> > approach, I could be convinced that it is the lesser of the evils.
>> >
>> > https://codereview.chromium.org/265713007/
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
>
> https://codereview.chromium.org/265713007/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.