Completed webkit radiusX, radiusY and rotationAngle handling.

Make touch orientation attributes visible in Blink.

Completed the path for touch orientation attributes (i.e. radiusX, radiusY and rotationAngle of a touch ellipse) from android to Blink.

BUG=381394
Committed: https://crrev.com/0ee22a13b49e9898c070d56655542f9f4843dc8f
Cr-Commit-Position: refs/heads/master@{#292923}

[mustaq, 2014-08-27 18:04:01]

mustaq@chromium.org changed reviewers:
+ darin@chromium.org, tdresser@chromium.org

[mustaq, 2014-08-27 18:04:01]

The result can be checked in Nexus 10---http://www.rbyers.net/paint.html already
has the corresponding JS changes.

The remaining test failures are cause by an existing implicit double-to-float
casting. The next upload will fix them.

[jdduke (slow), 2014-08-27 18:10:40]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2014-08-27 18:10:40]

Nice!

https://codereview.chromium.org/494833003/diff/180001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/494833003/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.h:74: size_t historical_index) const;
Where are these historical values used? I'd prefer we not expose them (anywhere)
unless absolutely necessary.

[mustaq, 2014-08-27 18:16:18]

mustaq@chromium.org changed reviewers:
- jdduke@chromium.org

[mustaq, 2014-08-27 18:16:58]

mustaq@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2014-08-27 18:21:10]

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android.cc (right):

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android.cc:295: if
(pointer_index < MAX_POINTERS_TO_CACHE)
I've seen devices return NaN here, which could be dangerous.  We should probably
check the result for NaN and return 0 in such case.

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
File content/browser/renderer_host/input/web_input_event_util.cc (right):

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
content/browser/renderer_host/input/web_input_event_util.cc:188:
event.GetAction(),
This indentation looks off by a couple spaces?

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
content/browser/renderer_host/input/web_input_event_util.cc:197: float
orientationDeg = event.GetOrientation(pointer_index) * 180.f / M_PI;
Nit: orientation_deg, though it's still not clear what exactly that means in
this context. It's probably worth retaining some of the comment I originally
used with this code
(https://codereview.chromium.org/178193022/diff/110001/content/browser/rendere...)

// MotionEvent provides a major axis orientation clockwise with respect to the	
// vertical of [-90, 90].  The proposed W3C extension specifies the angle that	
// the ellipse described by radiusX and radiusY is rotated clockwise about	
// its center, with a value of [0, 90], see	
// http://www.w3.org/TR/2011/WD-touch-events-20110505/.

[tdresser, 2014-08-27 18:31:35]

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
File content/browser/renderer_host/input/web_input_event_util.cc (right):

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
content/browser/renderer_host/input/web_input_event_util.cc:5: // MSVC++
requires this to be set before any other includes to get M_PI.
Add an
#include <cmath>

(Include what you use)

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
content/browser/renderer_host/input/web_input_event_util.cc:188:
event.GetAction(),
Revert indentation change.

https://codereview.chromium.org/494833003/diff/180001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.cc (left):

https://codereview.chromium.org/494833003/diff/180001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.cc:7: #include "base/logging.h"
"base/logging.h" defines DCHECK.

Include what you use!

https://codereview.chromium.org/494833003/diff/180001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.cc (right):

https://codereview.chromium.org/494833003/diff/180001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.cc:6: #define _USE_MATH_DEFINES
#include <cmath>

https://codereview.chromium.org/494833003/diff/180001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura_unittest.cc (right):

https://codereview.chromium.org/494833003/diff/180001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:6: #define _USE_MATH_DEFINES
#include <cmath>

[mustaq, 2014-08-27 19:08:57]

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
File content/browser/renderer_host/input/web_input_event_util.cc (right):

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
content/browser/renderer_host/input/web_input_event_util.cc:197: float
orientationDeg = event.GetOrientation(pointer_index) * 180.f / M_PI;
I totally agree, thanks for pointing to the original comments.

I was looking for a place to document the major/minor vs radiusX/radiusY
differences (along with the differences in angle definitions), but wasn't sure.
Do you think CreateWebTouchPoint() is the best place for all of these?

[jdduke (slow), 2014-08-27 19:16:13]

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
File content/browser/renderer_host/input/web_input_event_util.cc (right):

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
content/browser/renderer_host/input/web_input_event_util.cc:197: float
orientationDeg = event.GetOrientation(pointer_index) * 180.f / M_PI;
On 2014/08/27 19:08:57, mustaq wrote:
> I totally agree, thanks for pointing to the original comments.
> 
> I was looking for a place to document the major/minor vs radiusX/radiusY
> differences (along with the differences in angle definitions), but wasn't
sure.
> Do you think CreateWebTouchPoint() is the best place for all of these?

Yeah, this is probably as reasonable a place as any. I would still put a short
comment at the |MotionEvent::GetOrientation()| interface method  describing how
the orientation relates to the touch major value.

[mustaq, 2014-08-27 20:51:08]

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android.cc (right):

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android.cc:295: if
(pointer_index < MAX_POINTERS_TO_CACHE)
On 2014/08/27 18:21:09, jdduke wrote:
> I've seen devices return NaN here, which could be dangerous.  We should
probably
> check the result for NaN and return 0 in such case.

Done.

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
File content/browser/renderer_host/input/web_input_event_util.cc (right):

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
content/browser/renderer_host/input/web_input_event_util.cc:5: // MSVC++
requires this to be set before any other includes to get M_PI.
On 2014/08/27 18:31:34, tdresser wrote:
> Add an
> #include <cmath>
> 
> (Include what you use)

Done.

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
content/browser/renderer_host/input/web_input_event_util.cc:188:
event.GetAction(),
Done.

https://codereview.chromium.org/494833003/diff/180001/content/browser/rendere...
content/browser/renderer_host/input/web_input_event_util.cc:197: float
orientationDeg = event.GetOrientation(pointer_index) * 180.f / M_PI;
On 2014/08/27 19:16:13, jdduke wrote:
> On 2014/08/27 19:08:57, mustaq wrote:
> > I totally agree, thanks for pointing to the original comments.
> > 
> > I was looking for a place to document the major/minor vs radiusX/radiusY
> > differences (along with the differences in angle definitions), but wasn't
> sure.
> > Do you think CreateWebTouchPoint() is the best place for all of these?
> 
> Yeah, this is probably as reasonable a place as any. I would still put a short
> comment at the |MotionEvent::GetOrientation()| interface method  describing
how
> the orientation relates to the touch major value.

Done.

https://codereview.chromium.org/494833003/diff/180001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/494833003/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.h:74: size_t historical_index) const;
On 2014/08/27 18:10:40, jdduke wrote:
> Where are these historical values used? I'd prefer we not expose them
(anywhere)
> unless absolutely necessary.

We have one "real" reference to GetHistoricalTouchMajor:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/gesture_...
I think a GetHistoricalTouchMajor w/o a GetHistoricalTouch{Minor,Orientation}
makes the interface a bit too odd.

https://codereview.chromium.org/494833003/diff/180001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.cc (left):

https://codereview.chromium.org/494833003/diff/180001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.cc:7: #include "base/logging.h"
On 2014/08/27 18:31:35, tdresser wrote:
> "base/logging.h" defines DCHECK.
> 
> Include what you use!

Done.

https://codereview.chromium.org/494833003/diff/180001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.cc (right):

https://codereview.chromium.org/494833003/diff/180001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.cc:6: #define _USE_MATH_DEFINES
On 2014/08/27 18:31:35, tdresser wrote:
> #include <cmath>

Done.

https://codereview.chromium.org/494833003/diff/180001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura_unittest.cc (right):

https://codereview.chromium.org/494833003/diff/180001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:6: #define _USE_MATH_DEFINES
On 2014/08/27 18:31:35, tdresser wrote:
> #include <cmath>

Done.

[jdduke (slow), 2014-08-27 20:53:54]

https://codereview.chromium.org/494833003/diff/180001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/494833003/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.h:74: size_t historical_index) const;
On 2014/08/27 20:51:08, mustaq wrote:
> On 2014/08/27 18:10:40, jdduke wrote:
> > Where are these historical values used? I'd prefer we not expose them
> (anywhere)
> > unless absolutely necessary.
> 
> We have one "real" reference to GetHistoricalTouchMajor:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/gesture_...
> I think a GetHistoricalTouchMajor w/o a GetHistoricalTouch{Minor,Orientation}
> makes the interface a bit too odd.

We can add to the interface if/when we need it, let's remove it for now.

[tdresser, 2014-08-28 12:35:19]

LGTM, with nits (once Jared's comments are addressed)

Eventually I'd like the ScaleGestureDetector to no longer use radius
information, which would let us get rid of |GetHistoricalTouchMajor| as well.

https://codereview.chromium.org/494833003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/494833003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:6: #define
_USE_MATH_DEFINES
#include <cmath>

https://codereview.chromium.org/494833003/diff/220001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura_unittest.cc (right):

https://codereview.chromium.org/494833003/diff/220001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:256: rotation_angle = 23.f; //
Any positive acute angle
Although I don't see anything in the style guide against this comment style, its
much more common in the chromium code base to give comments their own line.

[mustaq, 2014-08-28 14:08:37]

https://codereview.chromium.org/494833003/diff/180001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/494833003/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.h:74: size_t historical_index) const;
On 2014/08/27 20:53:54, jdduke wrote:
> On 2014/08/27 20:51:08, mustaq wrote:
> > On 2014/08/27 18:10:40, jdduke wrote:
> > > Where are these historical values used? I'd prefer we not expose them
> > (anywhere)
> > > unless absolutely necessary.
> > 
> > We have one "real" reference to GetHistoricalTouchMajor:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/gesture_...
> > I think a GetHistoricalTouchMajor w/o a
GetHistoricalTouch{Minor,Orientation}
> > makes the interface a bit too odd.
> 
> We can add to the interface if/when we need it, let's remove it for now.

Makes sense since we are planning to nuke GetHistoricalTouchMajor. Done.

https://codereview.chromium.org/494833003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/494833003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:6: #define
_USE_MATH_DEFINES
On 2014/08/28 12:35:19, tdresser wrote:
> #include <cmath>

Done.

https://codereview.chromium.org/494833003/diff/220001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura_unittest.cc (right):

https://codereview.chromium.org/494833003/diff/220001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:256: rotation_angle = 23.f; //
Any positive acute angle
Removed all of them. Seems redundant now after adding the comment blocks in
web_input_event_util.cc:197 and motion_event.h:91.

On 2014/08/28 12:35:19, tdresser wrote:
> Although I don't see anything in the style guide against this comment style,
its
> much more common in the chromium code base to give comments their own line.

[jdduke (slow), 2014-08-28 15:31:58]

https://codereview.chromium.org/494833003/diff/220001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/494833003/diff/220001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.h:95: // Returns the orientation of the
major axis closkwise from vertical, in
clockwise

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android.cc (right):

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android.cc:296: return
cached_orientations_[pointer_index];
The cached orientation might be nan, so you might want to rearrange this to only
do the nan check in a single place.

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android.cc:299: return
isnan(orientation) ? 0 : orientation;
Hmm, I guess we have base::IsNaN for a reason (base/float_util.h), might as well
use that.

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android_unittest.cc
(right):

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android_unittest.cc:44: float
orientation1 = -0.1f;
Might as well make this std::numeric_limits<float>::quiet_NaN() to test the
logic.

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:128: if
(event_.touches[pointer_index].radiusX
Is this correct? Consider the case where |rotationAngle| is 0, radiusX = 5 and
radiusY = 10 (a vertically-aligned ellipse). This code will produce a
rotation_angle_rad of -M_PI_2, when it should really be zero (as orientation
defines the angle from the vertical of the *major* axis, which in this case is
defined by radiusY). I think there may be some issues with your MotionEventAura
math as well, please add tests where appropriate.

https://codereview.chromium.org/494833003/diff/240001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura_unittest.cc (right):

https://codereview.chromium.org/494833003/diff/240001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:265:
EXPECT_FLOAT_EQ(rotation_angle, event.GetOrientation(0) * 180 / M_PI);
The orientation needs a shift (because Orientation is with respect to the
vertical of the *major* axis, in this case radiusX).

https://codereview.chromium.org/494833003/diff/240001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:279:
EXPECT_FLOAT_EQ(rotation_angle, event.GetOrientation(1) * 180 / M_PI + 90);
The orientation shouldn't be shifted.

https://codereview.chromium.org/494833003/diff/240001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:291: rotation_angle,
raw_clone_aura->GetOrientation(1) * 180 / M_PI + 90);
Same as above.

https://codereview.chromium.org/494833003/diff/240001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:306:
EXPECT_FLOAT_EQ(rotation_angle, event.GetOrientation(1) * 180 / M_PI + 90);
Same as above.

[mustaq, 2014-08-28 18:21:14]

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:128: if
(event_.touches[pointer_index].radiusX
On 2014/08/28 15:31:57, jdduke wrote:
> Is this correct? Consider the case where |rotationAngle| is 0, radiusX = 5 and
> radiusY = 10 (a vertically-aligned ellipse). This code will produce a
> rotation_angle_rad of -M_PI_2, when it should really be zero (as orientation
> defines the angle from the vertical of the *major* axis, which in this case is
> defined by radiusY). I think there may be some issues with your
MotionEventAura
> math as well, please add tests where appropriate.

I think the rotation angle is measured from vertical, so rotationAngle would be
90 degrees in this case. Since exact 90 degrees is not allowed, we will have
angle=0 with swapped radii.

[jdduke (slow), 2014-08-28 18:23:51]

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/web_input_event_util.cc (right):

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/web_input_event_util.cc:206: // degrees,
where the x-axis makes the specified angle with vertical. See:
"the specified angle with the vertical": Where did you get that? 

That's not what I gather from
http://dvcs.w3.org/hg/webevents/raw-file/default/touchevents.html. radiusX is
aligned with the x-axis if the rotation is 0, "the angle (in degrees) that the
ellipse described by radiusX and radiusY is rotated clockwise about its center;
0 if no value is known. "

[mustaq, 2014-08-28 21:56:02]

https://codereview.chromium.org/494833003/diff/220001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/494833003/diff/220001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.h:95: // Returns the orientation of the
major axis closkwise from vertical, in
On 2014/08/28 15:31:57, jdduke wrote:
> clockwise

Done.

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android.cc (right):

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android.cc:296: return
cached_orientations_[pointer_index];
On 2014/08/28 15:31:57, jdduke wrote:
> The cached orientation might be nan, so you might want to rearrange this to
only
> do the nan check in a single place.

Done.

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android.cc:299: return
isnan(orientation) ? 0 : orientation;
On 2014/08/28 15:31:57, jdduke wrote:
> Hmm, I guess we have base::IsNaN for a reason (base/float_util.h), might as
well
> use that.

Done.

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android_unittest.cc
(right):

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android_unittest.cc:44: float
orientation1 = -0.1f;
On 2014/08/28 15:31:57, jdduke wrote:
> Might as well make this std::numeric_limits<float>::quiet_NaN() to test the
> logic.

Done.

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:128: if
(event_.touches[pointer_index].radiusX
On 2014/08/28 18:21:13, mustaq wrote:
> On 2014/08/28 15:31:57, jdduke wrote:
> > Is this correct? Consider the case where |rotationAngle| is 0, radiusX = 5
and
> > radiusY = 10 (a vertically-aligned ellipse). This code will produce a
> > rotation_angle_rad of -M_PI_2, when it should really be zero (as orientation
> > defines the angle from the vertical of the *major* axis, which in this case
is
> > defined by radiusY). I think there may be some issues with your
> MotionEventAura
> > math as well, please add tests where appropriate.
> 
> I think the rotation angle is measured from vertical, so rotationAngle would
be
> 90 degrees in this case. Since exact 90 degrees is not allowed, we will have
> angle=0 with swapped radii.

Fixed.

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/web_input_event_util.cc (right):

https://codereview.chromium.org/494833003/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/web_input_event_util.cc:206: // degrees,
where the x-axis makes the specified angle with vertical. See:
On 2014/08/28 18:23:50, jdduke wrote:
> "the specified angle with the vertical": Where did you get that? 
> 
> That's not what I gather from
> http://dvcs.w3.org/hg/webevents/raw-file/default/touchevents.html. radiusX is
> aligned with the x-axis if the rotation is 0, "the angle (in degrees) that the
> ellipse described by radiusX and radiusY is rotated clockwise about its
center;
> 0 if no value is known. "

My bad! This misunderstanding caused the remaining angle issues here. Fixed the
comment, and all of the issues.

https://codereview.chromium.org/494833003/diff/240001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura_unittest.cc (right):

https://codereview.chromium.org/494833003/diff/240001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:265:
EXPECT_FLOAT_EQ(rotation_angle, event.GetOrientation(0) * 180 / M_PI);
On 2014/08/28 15:31:57, jdduke wrote:
> The orientation needs a shift (because Orientation is with respect to the
> vertical of the *major* axis, in this case radiusX).

Done.

https://codereview.chromium.org/494833003/diff/240001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:279:
EXPECT_FLOAT_EQ(rotation_angle, event.GetOrientation(1) * 180 / M_PI + 90);
On 2014/08/28 15:31:57, jdduke wrote:
> The orientation shouldn't be shifted.

Done.

https://codereview.chromium.org/494833003/diff/240001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:291: rotation_angle,
raw_clone_aura->GetOrientation(1) * 180 / M_PI + 90);
On 2014/08/28 15:31:57, jdduke wrote:
> Same as above.

Done.

https://codereview.chromium.org/494833003/diff/240001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:306:
EXPECT_FLOAT_EQ(rotation_angle, event.GetOrientation(1) * 180 / M_PI + 90);
On 2014/08/28 15:31:57, jdduke wrote:
> Same as above.

Done.

[jdduke (slow), 2014-08-29 16:45:09]

Minor description nit, it helps to wrap the description at something less than
80 characters (72 is nice in case the patch is reverted).

https://codereview.chromium.org/494833003/diff/280001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android.cc (right):

https://codereview.chromium.org/494833003/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android.cc:378: float
MotionEventAndroid::ToValidFloat(float x) const {
Nit: This should probably be a free function in the anon namespace above as it's
effectively static.

https://codereview.chromium.org/494833003/diff/280001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android_unittest.cc
(right):

https://codereview.chromium.org/494833003/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android_unittest.cc:45: float
orientation1 = std::numeric_limits<float>::quiet_NaN();
Thanks.

https://codereview.chromium.org/494833003/diff/280001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/494833003/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:129: >=
event_.touches[pointer_index].radiusY) {
Nit: I would make this a strict inequality, that way, if radiusX==radiusY and
rotationAngle is 0 (probably a very common case), you get a returned orientation
of 0 (although I suppose the rotation doesn't matter much when the two are
equal).

https://codereview.chromium.org/494833003/diff/280001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.cc (right):

https://codereview.chromium.org/494833003/diff/280001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.cc:57: if (radius_x >= radius_y) {
tdresser@: Is it still the case that we see touches with a major radius of X but
a minor radius of 1? That might be confusing for some web apps, but I'm not sure
if that still holds and where that logic might originate (i.e., perhaps it
should be fixed further upstream?).

https://codereview.chromium.org/494833003/diff/280001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.h (right):

https://codereview.chromium.org/494833003/diff/280001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.h:68: float major_radius;
Any reason not to do this TODO now?

[mustaq, 2014-08-29 16:53:00]

https://codereview.chromium.org/494833003/diff/280001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/494833003/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:129: >=
event_.touches[pointer_index].radiusY) {
Yeah, just discovered the default=90 value an hour ago. Trying to fix such
corner cases now. See the current patch.

On 2014/08/29 16:45:08, jdduke wrote:
> Nit: I would make this a strict inequality, that way, if radiusX==radiusY and
> rotationAngle is 0 (probably a very common case), you get a returned
orientation
> of 0 (although I suppose the rotation doesn't matter much when the two are
> equal).

[mustaq, 2014-08-29 17:28:44]

https://codereview.chromium.org/494833003/diff/280001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.cc (right):

https://codereview.chromium.org/494833003/diff/280001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.cc:57: if (radius_x >= radius_y) {
See crbug.com/406004

On 2014/08/29 16:45:08, jdduke wrote:
> tdresser@: Is it still the case that we see touches with a major radius of X
but
> a minor radius of 1? That might be confusing for some web apps, but I'm not
sure
> if that still holds and where that logic might originate (i.e., perhaps it
> should be fixed further upstream?).

[jdduke (slow), 2014-08-29 17:30:42]

On 2014/08/29 17:28:44, mustaq wrote:
>
https://codereview.chromium.org/494833003/diff/280001/ui/events/gestures/moti...
> File ui/events/gestures/motion_event_aura.cc (right):
> 
>
https://codereview.chromium.org/494833003/diff/280001/ui/events/gestures/moti...
> ui/events/gestures/motion_event_aura.cc:57: if (radius_x >= radius_y) {
> See crbug.com/406004
> 
> On 2014/08/29 16:45:08, jdduke wrote:
> > tdresser@: Is it still the case that we see touches with a major radius of X
> but
> > a minor radius of 1? That might be confusing for some web apps, but I'm not
> sure
> > if that still holds and where that logic might originate (i.e., perhaps it
> > should be fixed further upstream?).

Thanks, let me know when you've made the other changes and I'll approve.

[mustaq, 2014-08-29 20:54:02]

All done, hopefully ;-)

https://codereview.chromium.org/494833003/diff/280001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android.cc (right):

https://codereview.chromium.org/494833003/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android.cc:378: float
MotionEventAndroid::ToValidFloat(float x) const {
On 2014/08/29 16:45:08, jdduke wrote:
> Nit: This should probably be a free function in the anon namespace above as
it's
> effectively static.

Done.

https://codereview.chromium.org/494833003/diff/280001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.h (right):

https://codereview.chromium.org/494833003/diff/280001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.h:68: float major_radius;
On 2014/08/29 16:45:09, jdduke wrote:
> Any reason not to do this TODO now?

Done. Wanted to investigate later if this private class could be removed
altogether. It's not my focus now.

[jdduke (slow), 2014-08-30 12:58:45]

lgtm, thanks!

[mustaq, 2014-09-02 14:02:33]

The CQ bit was checked by mustaq@chromium.org

[commit-bot: I haz the power, 2014-09-02 14:05:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mustaq@chromium.org/494833003/320001

[commit-bot: I haz the power, 2014-09-02 14:35:26]

Committed patchset #17 (id:320001) as f7ff513cdeb881497d4977355a965c33b5fb7980

[scottmg, 2014-09-02 18:53:03]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/531133002/ by scottmg@chromium.org.

The reason for reverting is: This causes Windows Debug dev builds to fail

[3978->3/4014 ~33] CXX
obj\content\browser\renderer_host\input\content.motion_event_web.obj
FAILED: ninja -t msvc -e environment.x86 --
"d:\src\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo
/showIncludes /FC
@obj\content\browser\renderer_host\input\content.motion_event_web.obj.rsp /c
..\..\content\browser\renderer_host\input\motion_event_web.cc
/Foobj\content\browser\renderer_host\input\content.motion_event_web.obj
/Fdobj\content\content.cc.pdb
d:\src\cr3\src\content\browser\renderer_host\input\motion_event_web.cc(124) :
error C2065: 'M_PI' : undeclared identifier
d:\src\cr3\src\content\browser\renderer_host\input\motion_event_web.cc(125) :
error C2065: 'M_PI_2' : undeclared identifier
d:\src\cr3\src\content\browser\renderer_host\input\motion_event_web.cc(133) :
error C2065: 'M_PI_2' : undeclared identifier

I suspect this is due to precompiled headers; the math.h has already been
included before _USE_MATH_DEFINES is defined in the .cc file..

[commit-bot: I haz the power, 2014-09-10 03:18:24]

Patchset 17 (id:??) landed as
https://crrev.com/0ee22a13b49e9898c070d56655542f9f4843dc8f
Cr-Commit-Position: refs/heads/master@{#292923}