Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
303263008:
[SVG2] Add support for the 'turn' unit in <angle>. (Closed)
|
Created:
6 years, 6 months ago by Erik Dahlström (inactive) Modified:
6 years, 6 months ago CC:
blink-reviews, ed+blinkwatch_opera.com, f(malita), gyuyoung.kim_webkit.org, Stephen Chennney, kouhei+svg_chromium.org, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
Description[SVG2] Add support for the 'turn' unit in <angle>.
Spec:
https://svgwg.org/svg2-draft/painting.html#OrientAttribute
http://www.w3.org/TR/2012/WD-css3-values-20120308/#angles
BUG=377514
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176014
Patch Set 1 #Patch Set 2 : add reftest #
Total comments: 4
Patch Set 3 : review fixes #Patch Set 4 : add unitType and const tests #Patch Set 5 : compilefix :P #
Total comments: 7
Patch Set 6 : fix nits #Patch Set 7 : hide the internals of SVGAngle #
Total comments: 9
Patch Set 8 : review fixes #
Total comments: 4
Patch Set 9 : fix nits #
Total comments: 4
Patch Set 10 : fixups for MathExtras #
Messages
Total messages: 40 (0 generated)
https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle.h File Source/core/svg/SVGAngle.h (right): https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... Source/core/svg/SVGAngle.h:71: SVG_ANGLETYPE_TURN = 5 Should this be exposed on the DOM SVGAngle interface? Not in the spec. ATM (AFAICS), but I suspect you'll still get unitType === 5 if you access SVGMarkerElement.orientAngle for instance? Some rudimentary tests for that (and the conversions) would probably be good (in either case.)
https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... File Source/core/svg/SVGAngle.cpp (right): https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... Source/core/svg/SVGAngle.cpp:288: m_valueInSpecifiedUnits = deg2grad(turn2deg(m_valueInSpecifiedUnits)); Is this just m_valueInSpecifiedUnits * 400? Could we just introduce turn2grad?
https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... File Source/core/svg/SVGAngle.cpp (right): https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... Source/core/svg/SVGAngle.cpp:288: m_valueInSpecifiedUnits = deg2grad(turn2deg(m_valueInSpecifiedUnits)); On 2014/06/02 13:56:49, pdr wrote: > Is this just m_valueInSpecifiedUnits * 400? Could we just introduce turn2grad? Done. https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle.h File Source/core/svg/SVGAngle.h (right): https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... Source/core/svg/SVGAngle.h:71: SVG_ANGLETYPE_TURN = 5 On 2014/06/02 12:49:39, fs wrote: > Should this be exposed on the DOM SVGAngle interface? Not in the spec. ATM > (AFAICS), but I suspect you'll still get unitType === 5 if you access > SVGMarkerElement.orientAngle for instance? Will raise this in the WG, but I think we should still go ahead with it. > Some rudimentary tests for that (and the conversions) would probably be good (in > either case.) Added.
On 2014/06/03 15:30:27, Erik Dahlström wrote: > https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... > File Source/core/svg/SVGAngle.cpp (right): > > https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... > Source/core/svg/SVGAngle.cpp:288: m_valueInSpecifiedUnits = > deg2grad(turn2deg(m_valueInSpecifiedUnits)); > On 2014/06/02 13:56:49, pdr wrote: > > Is this just m_valueInSpecifiedUnits * 400? Could we just introduce turn2grad? > > Done. > > https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle.h > File Source/core/svg/SVGAngle.h (right): > > https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... > Source/core/svg/SVGAngle.h:71: SVG_ANGLETYPE_TURN = 5 > On 2014/06/02 12:49:39, fs wrote: > > Should this be exposed on the DOM SVGAngle interface? Not in the spec. ATM > > (AFAICS), but I suspect you'll still get unitType === 5 if you access > > SVGMarkerElement.orientAngle for instance? > > Will raise this in the WG, but I think we should still go ahead with it. > > > Some rudimentary tests for that (and the conversions) would probably be good > (in > > either case.) > > Added. LGTM
On 2014/06/03 15:39:56, pdr wrote: > On 2014/06/03 15:30:27, Erik Dahlström wrote: > > > https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... > > File Source/core/svg/SVGAngle.cpp (right): > > > > > https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... > > Source/core/svg/SVGAngle.cpp:288: m_valueInSpecifiedUnits = > > deg2grad(turn2deg(m_valueInSpecifiedUnits)); > > On 2014/06/02 13:56:49, pdr wrote: > > > Is this just m_valueInSpecifiedUnits * 400? Could we just introduce > turn2grad? > > > > Done. > > > > > https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle.h > > File Source/core/svg/SVGAngle.h (right): > > > > > https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... > > Source/core/svg/SVGAngle.h:71: SVG_ANGLETYPE_TURN = 5 > > On 2014/06/02 12:49:39, fs wrote: > > > Should this be exposed on the DOM SVGAngle interface? Not in the spec. ATM > > > (AFAICS), but I suspect you'll still get unitType === 5 if you access > > > SVGMarkerElement.orientAngle for instance? > > > > Will raise this in the WG, but I think we should still go ahead with it. > > > > > Some rudimentary tests for that (and the conversions) would probably be good > > (in > > > either case.) > > > > Added. > > LGTM no LGTM if the comment means you add it. We do not add new types to SVG DOM. Instead it should return not supported.
On 2014/06/03 15:49:43, krit wrote: > On 2014/06/03 15:39:56, pdr wrote: > > On 2014/06/03 15:30:27, Erik Dahlström wrote: > > > > > > https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... > > > File Source/core/svg/SVGAngle.cpp (right): > > > > > > > > > https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... > > > Source/core/svg/SVGAngle.cpp:288: m_valueInSpecifiedUnits = > > > deg2grad(turn2deg(m_valueInSpecifiedUnits)); > > > On 2014/06/02 13:56:49, pdr wrote: > > > > Is this just m_valueInSpecifiedUnits * 400? Could we just introduce > > turn2grad? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle.h > > > File Source/core/svg/SVGAngle.h (right): > > > > > > > > > https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... > > > Source/core/svg/SVGAngle.h:71: SVG_ANGLETYPE_TURN = 5 > > > On 2014/06/02 12:49:39, fs wrote: > > > > Should this be exposed on the DOM SVGAngle interface? Not in the spec. ATM > > > > (AFAICS), but I suspect you'll still get unitType === 5 if you access > > > > SVGMarkerElement.orientAngle for instance? > > > > > > Will raise this in the WG, but I think we should still go ahead with it. > > > > > > > Some rudimentary tests for that (and the conversions) would probably be > good > > > (in > > > > either case.) > > > > > > Added. > > > > LGTM > > no LGTM if the comment means you add it. We do not add new types to SVG DOM. > Instead it should return not supported. not LGTM
https://codereview.chromium.org/303263008/diff/80001/LayoutTests/fast/svg/svg... File LayoutTests/fast/svg/svgangle.html (right): https://codereview.chromium.org/303263008/diff/80001/LayoutTests/fast/svg/svg... LayoutTests/fast/svg/svgangle.html:92: throw e; Nit: Looks having no try-catch block would give this function the same behavior. https://codereview.chromium.org/303263008/diff/80001/LayoutTests/fast/svg/svg... LayoutTests/fast/svg/svgangle.html:104: catch(e) {} Nit: AFAIK, the harness will catch exceptions (and fail the test), so I think a lot of these try-catch blocks could just be removed. I suppose catching means you don't get the "Threw FooBar..." type message on failure though... https://codereview.chromium.org/303263008/diff/80001/Source/core/svg/SVGAngle... File Source/core/svg/SVGAngle.cpp (right): https://codereview.chromium.org/303263008/diff/80001/Source/core/svg/SVGAngle... Source/core/svg/SVGAngle.cpp:358: break; Nit: This should still be unreachable (due to the unitType == m_unitType check before the switch.) Similarly for the other cases.
On 2014/06/03 15:49:59, krit wrote: > On 2014/06/03 15:49:43, krit wrote: > > On 2014/06/03 15:39:56, pdr wrote: > > > On 2014/06/03 15:30:27, Erik Dahlström wrote: > https://codereview.chromium.org/303263008/diff/20001/Source/core/svg/SVGAngle... > > > > Source/core/svg/SVGAngle.h:71: SVG_ANGLETYPE_TURN = 5 > > > > On 2014/06/02 12:49:39, fs wrote: > > > > > Should this be exposed on the DOM SVGAngle interface? Not in the spec. > ATM > > > > > (AFAICS), but I suspect you'll still get unitType === 5 if you access > > > > > SVGMarkerElement.orientAngle for instance? > > > > > > > > Will raise this in the WG, but I think we should still go ahead with it. > > > > > > > > > Some rudimentary tests for that (and the conversions) would probably be > > good > > > > (in > > > > > either case.) > > > > > > > > Added. > > > > > > LGTM > > > > no LGTM if the comment means you add it. We do not add new types to SVG DOM. > > Instead it should return not supported. > > not LGTM Could just "normalize" to 'deg' on parsing?
On 2014/06/03 15:49:43, krit wrote: > no LGTM if the comment means you add it. We do not add new types to SVG DOM. > Instead it should return not supported. As bad as the SVG DOM is, this seems like an unnecessary inconsistency. Are users best served by having 4/5 of the types work properly?
On 2014/06/03 15:54:31, pdr wrote: > On 2014/06/03 15:49:43, krit wrote: > > no LGTM if the comment means you add it. We do not add new types to SVG DOM. > > Instead it should return not supported. > > As bad as the SVG DOM is, this seems like an unnecessary inconsistency. I agree, it's a minor addition in any case. > Are users best served by having 4/5 of the types work properly? I wouldn't think so, but I do understand Dirk's reluctance to adding to SVG DOM.
https://codereview.chromium.org/303263008/diff/80001/LayoutTests/fast/svg/svg... File LayoutTests/fast/svg/svgangle.html (right): https://codereview.chromium.org/303263008/diff/80001/LayoutTests/fast/svg/svg... LayoutTests/fast/svg/svgangle.html:104: catch(e) {} On 2014/06/03 15:52:48, fs wrote: > Nit: AFAIK, the harness will catch exceptions (and fail the test), so I think a > lot of these try-catch blocks could just be removed. I suppose catching means > you don't get the "Threw FooBar..." type message on failure though... Yes, the output from failures looked (much) cleaner with catching in place. I wanted to compare line-by-line with the other browsers and that was made harder by having an arbitrary number of stack trace lines on each failed test. https://codereview.chromium.org/303263008/diff/80001/Source/core/svg/SVGAngle... File Source/core/svg/SVGAngle.cpp (right): https://codereview.chromium.org/303263008/diff/80001/Source/core/svg/SVGAngle... Source/core/svg/SVGAngle.cpp:358: break; On 2014/06/03 15:52:48, fs wrote: > Nit: This should still be unreachable (due to the unitType == m_unitType check > before the switch.) Similarly for the other cases. The other cases I agree with, but for this one (due to fall-through) we will actually I hit this assert for the case of one being SVG_ANGLETYPE_UNSPECIFIED and the other SVG_ANGLETYPE_DEG.
The CQ bit was checked by ed@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ed@opera.com/303263008/100001
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: dschulze@chromium.org. Please make sure to get positive review before another CQ attempt.
LGTM. I agree w/ pdr on the enum-issue. https://codereview.chromium.org/303263008/diff/80001/LayoutTests/fast/svg/svg... File LayoutTests/fast/svg/svgangle.html (right): https://codereview.chromium.org/303263008/diff/80001/LayoutTests/fast/svg/svg... LayoutTests/fast/svg/svgangle.html:104: catch(e) {} On 2014/06/04 09:28:21, Erik Dahlström wrote: > On 2014/06/03 15:52:48, fs wrote: > > Nit: AFAIK, the harness will catch exceptions (and fail the test), so I think > a > > lot of these try-catch blocks could just be removed. I suppose catching means > > you don't get the "Threw FooBar..." type message on failure though... > > Yes, the output from failures looked (much) cleaner with catching in place. I > wanted to compare line-by-line with the other browsers and that was made harder > by having an arbitrary number of stack trace lines on each failed test. Yepp, I can see that. Ok. https://codereview.chromium.org/303263008/diff/80001/Source/core/svg/SVGAngle... File Source/core/svg/SVGAngle.cpp (right): https://codereview.chromium.org/303263008/diff/80001/Source/core/svg/SVGAngle... Source/core/svg/SVGAngle.cpp:358: break; On 2014/06/04 09:28:21, Erik Dahlström wrote: > On 2014/06/03 15:52:48, fs wrote: > > Nit: This should still be unreachable (due to the unitType == m_unitType check > > before the switch.) Similarly for the other cases. > > The other cases I agree with, but for this one (due to fall-through) we will > actually I hit this assert for the case of one being SVG_ANGLETYPE_UNSPECIFIED > and the other SVG_ANGLETYPE_DEG. Ah, yes, this particular spot had a higher-level fall-through...
The CQ bit was checked by ed@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ed@opera.com/303263008/100001
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: dschulze@chromium.org. Please make sure to get positive review before another CQ attempt.
On 2014/06/03 15:54:31, pdr wrote: > On 2014/06/03 15:49:43, krit wrote: > > no LGTM if the comment means you add it. We do not add new types to SVG DOM. > > Instead it should return not supported. > > As bad as the SVG DOM is, this seems like an unnecessary inconsistency. Are > users best served by having 4/5 of the types work properly? We agreed in the WG at multiple occasion that we will not add new types to SVG DOM. SVG DOM will be deprecated and it doesn't make sense to change the API. I am fine with having the enumeration be extended as long as the public interface stays untouched. (This is probably harder to do then work around the assertion. Not sure though.) With this change you are actively changing the exposed API which even needs an Ok from the API owners. I object to exposing new enumeration values. CSS has more CSS units that we would need to add back to SVG DOM than just 'turn'. As comparison: CSS Transforms requires to use SVG_TRANSFORM_UNKNOWN.
On 2014/06/04 11:54:13, krit wrote: > On 2014/06/03 15:54:31, pdr wrote: > > On 2014/06/03 15:49:43, krit wrote: > > > no LGTM if the comment means you add it. We do not add new types to SVG DOM. > > > Instead it should return not supported. > > > > As bad as the SVG DOM is, this seems like an unnecessary inconsistency. Are > > users best served by having 4/5 of the types work properly? > > We agreed in the WG at multiple occasion that we will not add new types to SVG > DOM. SVG DOM will be deprecated and it doesn't make sense to change the API. Not sure about multiple times, but I did find http://www.w3.org/2013/07/11-svg-minutes.html#item05. > I am fine with having the enumeration be extended as long as the public > interface stays untouched. (This is probably harder to do then work around the > assertion. Not sure though.) Fair enough. I don't like it much, but OTOH I don't like IDL constants much either. > With this change you are actively changing the exposed API which even needs an > Ok from the API owners. I object to exposing new enumeration values. CSS has > more CSS units that we would need to add back to SVG DOM than just 'turn'. No other missing units for <angle> values though. I think we should add all the other <length> units, but that's something for another review. > As comparison: CSS Transforms requires to use SVG_TRANSFORM_UNKNOWN. Ok, I see what you mean now.
On 2014/06/04 13:42:39, Erik Dahlström wrote: > On 2014/06/04 11:54:13, krit wrote: > > On 2014/06/03 15:54:31, pdr wrote: > > > On 2014/06/03 15:49:43, krit wrote: > > > > no LGTM if the comment means you add it. We do not add new types to SVG > DOM. > > > > Instead it should return not supported. > > > > > > As bad as the SVG DOM is, this seems like an unnecessary inconsistency. Are > > > users best served by having 4/5 of the types work properly? > > > > We agreed in the WG at multiple occasion that we will not add new types to SVG > > DOM. SVG DOM will be deprecated and it doesn't make sense to change the API. Hiding the internals from the SVG DOM makes for some ugly code IMHO, but PTAL.
https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... File Source/core/svg/SVGAngleTearOff.cpp (right): https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... Source/core/svg/SVGAngleTearOff.cpp:114: exceptionState.throwDOMException(SyntaxError, "The value provided ('" + value + "') is invalid."); Hmm, the "damage" has been done here already (state modified). Save and rollback? https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... File Source/core/svg/SVGAngleTearOff.h (right): https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... Source/core/svg/SVGAngleTearOff.h:71: bool isValidAngle() { return target()->unitType() <= SVGAngle::SVG_ANGLETYPE_GRAD; } Nit: isValidAngleType / isValidAngleUnit ? (Could be private and const?)
https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... File Source/core/svg/SVGAngleTearOff.cpp (right): https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... Source/core/svg/SVGAngleTearOff.cpp:114: exceptionState.throwDOMException(SyntaxError, "The value provided ('" + value + "') is invalid."); On 2014/06/10 11:13:30, fs wrote: > Hmm, the "damage" has been done here already (state modified). Save and > rollback? Does unknown unit type need to imply that we do not support the unit as string in general? Internally we still do after all. It is logical to not accept it and would be more consistent either I guess. https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... File Source/core/svg/SVGAngleTearOff.h (right): https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... Source/core/svg/SVGAngleTearOff.h:68: String valueAsString() { return isValidAngle() ? target()->valueAsString() : String::number(0); } Exposing more unit types is one thing. Do we need to be restrictive on valueAsString?
https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... File Source/core/svg/SVGAngleTearOff.cpp (right): https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... Source/core/svg/SVGAngleTearOff.cpp:114: exceptionState.throwDOMException(SyntaxError, "The value provided ('" + value + "') is invalid."); On 2014/06/10 11:13:30, fs wrote: > Hmm, the "damage" has been done here already (state modified). Save and > rollback? Done. https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... Source/core/svg/SVGAngleTearOff.cpp:114: exceptionState.throwDOMException(SyntaxError, "The value provided ('" + value + "') is invalid."); On 2014/06/10 11:45:48, krit wrote: > On 2014/06/10 11:13:30, fs wrote: > > Hmm, the "damage" has been done here already (state modified). Save and > > rollback? > > Does unknown unit type need to imply that we do not support the unit as string > in general? Internally we still do after all. If we want to behave as if the "turn" unit wasn't supported to a user of the SVGAngle DOM interface then this is necessary. My preference is still that we should follow the existing architecture of SVGAngle (extending it with the new IDL constant), and then deprecating the whole interface as soon as we have a proper replacement API (if any). > It is logical to not accept it and would be more consistent either I guess. I don't fully understand what you mean. https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... File Source/core/svg/SVGAngleTearOff.h (right): https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... Source/core/svg/SVGAngleTearOff.h:68: String valueAsString() { return isValidAngle() ? target()->valueAsString() : String::number(0); } On 2014/06/10 11:45:48, krit wrote: > Exposing more unit types is one thing. Do we need to be restrictive on > valueAsString? Not necessarily, only if we want to preserve existing (public) behavior. The spec is fairly clear on what should happen ("SVGAngle.valueAsString: The angle value as a string value, in the units expressed by unitType.") Since unitType is "unknown" the returned string is the default value (0 in this case). https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... Source/core/svg/SVGAngleTearOff.h:71: bool isValidAngle() { return target()->unitType() <= SVGAngle::SVG_ANGLETYPE_GRAD; } On 2014/06/10 11:13:30, fs wrote: > Nit: isValidAngleType / isValidAngleUnit ? > > (Could be private and const?) Private yes, const no, the target() method is non-const.
https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... File Source/core/svg/SVGAngleTearOff.h (right): https://codereview.chromium.org/303263008/diff/120001/Source/core/svg/SVGAngl... Source/core/svg/SVGAngleTearOff.h:71: bool isValidAngle() { return target()->unitType() <= SVGAngle::SVG_ANGLETYPE_GRAD; } On 2014/06/10 16:57:42, Erik Dahlström wrote: > On 2014/06/10 11:13:30, fs wrote: > > Nit: isValidAngleType / isValidAngleUnit ? > > > > (Could be private and const?) > > Private yes, const no, the target() method is non-const. Ok. https://codereview.chromium.org/303263008/diff/140001/Source/core/svg/SVGAngl... File Source/core/svg/SVGAngleTearOff.cpp (right): https://codereview.chromium.org/303263008/diff/140001/Source/core/svg/SVGAngl... Source/core/svg/SVGAngleTearOff.cpp:112: String old = target()->valueAsString(); Nit: s/old/oldValue/ (or something) https://codereview.chromium.org/303263008/diff/140001/Source/core/svg/SVGAngl... Source/core/svg/SVGAngleTearOff.cpp:117: target()->setValueAsString(old, IGNORE_EXCEPTION); // rollback to old value Nit: ASSERT_NO_EXCEPTION might be even better?
https://codereview.chromium.org/303263008/diff/140001/Source/core/svg/SVGAngl... File Source/core/svg/SVGAngleTearOff.cpp (right): https://codereview.chromium.org/303263008/diff/140001/Source/core/svg/SVGAngl... Source/core/svg/SVGAngleTearOff.cpp:112: String old = target()->valueAsString(); On 2014/06/11 07:29:07, fs wrote: > Nit: s/old/oldValue/ (or something) Done. https://codereview.chromium.org/303263008/diff/140001/Source/core/svg/SVGAngl... Source/core/svg/SVGAngleTearOff.cpp:117: target()->setValueAsString(old, IGNORE_EXCEPTION); // rollback to old value On 2014/06/11 07:29:07, fs wrote: > Nit: ASSERT_NO_EXCEPTION might be even better? Done.
On 2014/06/11 08:04:28, Erik Dahlström wrote: Lgtm from me
The CQ bit was checked by ed@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ed@opera.com/303263008/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7432) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7437)
On 2014/06/11 09:23:30, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > blink_presubmit on tryserver.blink > (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7437) Needs review for WebKit/Source/wtf/MathExtras.h.
lgtm https://codereview.chromium.org/303263008/diff/160001/Source/wtf/MathExtras.h File Source/wtf/MathExtras.h (right): https://codereview.chromium.org/303263008/diff/160001/Source/wtf/MathExtras.h... Source/wtf/MathExtras.h:211: inline double turn2grad(double t) { return t * 400.0; } No need to add .0 http://dev.chromium.org/blink/coding-style#TOC-Floating-point-literals https://codereview.chromium.org/303263008/diff/160001/Source/wtf/MathExtras.h... Source/wtf/MathExtras.h:212: inline double grad2turn(double g) { return g / 400.f; } No need to add .f https://codereview.chromium.org/303263008/diff/160001/Source/wtf/MathExtras.h... Source/wtf/MathExtras.h:222: inline float turn2grad(float t) { return t * 400.0f; } No need to add .0f https://codereview.chromium.org/303263008/diff/160001/Source/wtf/MathExtras.h... Source/wtf/MathExtras.h:223: inline float grad2turn(float g) { return g / 400.f; } No need to add .f
The CQ bit was checked by ed@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ed@opera.com/303263008/180001
Message was sent while issue was closed.
Change committed as 176014 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
