Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1699893003:
setOrientTo{Auto|Angle} should reflect on orient attribute (Closed)
|
Created:
4 years, 10 months ago by Shanmuga Pandi Modified:
4 years, 10 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionsetOrientTo{Auto|Angle} should reflect on orient attribute
Earlier, setOrientTo{Auto|Angle} modifies only the SVGAngle value and units,
but not changing orient attribute value.
As per the spec, it should set the value on orient attribute.
https://www.w3.org/TR/svg-markers/#__svg__SVGMarkerElement__setOrientToAuto
BUG=586330
Committed: https://crrev.com/cffddfc0131ffda1f890dc244e4624abf5e742ce
Cr-Commit-Position: refs/heads/master@{#376718}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Added setAttribute #Patch Set 3 : fixed nits #
Total comments: 4
Patch Set 4 : Align with review comments #
Messages
Total messages: 25 (8 generated)
Description was changed from
==========
setOrientTo{Auto|Angle} should reflect on orient attribute
BUG=586330
==========
to
==========
setOrientTo{Auto|Angle} should reflect on orient attribute
Earlier, setOrientTo{Auto|Angle} modifies only the SVGAngle value and units, but
not orient attribute.
As per the spec, it should set the value on orient attribute.
https://www.w3.org/TR/svg-markers/#__svg__SVGMarkerElement__setOrientToAuto
BUG=586330
R=SamsungPeerReview
==========
shanmuga.m@samsung.com changed reviewers: + rob.buis@samsung.com
Description was changed from
==========
setOrientTo{Auto|Angle} should reflect on orient attribute
Earlier, setOrientTo{Auto|Angle} modifies only the SVGAngle value and units, but
not orient attribute.
As per the spec, it should set the value on orient attribute.
https://www.w3.org/TR/svg-markers/#__svg__SVGMarkerElement__setOrientToAuto
BUG=586330
R=SamsungPeerReview
==========
to
==========
setOrientTo{Auto|Angle} should reflect on orient attribute
Earlier, setOrientTo{Auto|Angle} modifies only the SVGAngle value and units,
but not changing orient attribute value.
As per the spec, it should set the value on orient attribute.
https://www.w3.org/TR/svg-markers/#__svg__SVGMarkerElement__setOrientToAuto
BUG=586330
R=SamsungPeerReview
==========
PTAL!!
https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp (right): https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp:120: orientAttribute->setValue("auto"); Have you considered using setAttribute?
On 2016/02/17 22:38:02, rwlbuis wrote: > https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp (right): > > https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp:120: > orientAttribute->setValue("auto"); > Have you considered using setAttribute? Yes. I have considered setAttribute also.... That also worked. But that does extra things like attribute validation and parsing, error checking etc., which are not required in this case. Let me know your opinion. Thanks
On 2016/02/18 05:17:45, Shanmuga Pandi wrote: > On 2016/02/17 22:38:02, rwlbuis wrote: > > > https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp (right): > > > > > https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp:120: > > orientAttribute->setValue("auto"); > > Have you considered using setAttribute? > > Yes. I have considered setAttribute also.... That also worked. > But that does extra things like attribute validation and parsing, error checking > etc., which are not required in this case. > Let me know your opinion. > Thanks Okay, I think it may be too level, I only see this being used in HTMLSelectElement.cpp. Anyway, apart from that it looks good to me, you can add SVG reviewers now :)
On 2016/02/18 16:47:12, rwlbuis wrote: > On 2016/02/18 05:17:45, Shanmuga Pandi wrote: > > On 2016/02/17 22:38:02, rwlbuis wrote: > > > > > > https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... > > > File third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp (right): > > > > > > > > > https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp:120: > > > orientAttribute->setValue("auto"); > > > Have you considered using setAttribute? > > > > Yes. I have considered setAttribute also.... That also worked. > > But that does extra things like attribute validation and parsing, error > checking > > etc., which are not required in this case. > > Let me know your opinion. > > Thanks > > Okay, I think it may be too level, I only see this being used in Oops, meant low-level.
Description was changed from
==========
setOrientTo{Auto|Angle} should reflect on orient attribute
Earlier, setOrientTo{Auto|Angle} modifies only the SVGAngle value and units,
but not changing orient attribute value.
As per the spec, it should set the value on orient attribute.
https://www.w3.org/TR/svg-markers/#__svg__SVGMarkerElement__setOrientToAuto
BUG=586330
R=SamsungPeerReview
==========
to
==========
setOrientTo{Auto|Angle} should reflect on orient attribute
Earlier, setOrientTo{Auto|Angle} modifies only the SVGAngle value and units,
but not changing orient attribute value.
As per the spec, it should set the value on orient attribute.
https://www.w3.org/TR/svg-markers/#__svg__SVGMarkerElement__setOrientToAuto
BUG=586330
==========
shanmuga.m@samsung.com changed reviewers: + ed@chromium.org, fs@opera.com, pdr@chromium.org
+ Reviewers PTAL!!
https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html (right): https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html:4: <svg width="1" height="1" visibility="hidden"> Specifying 'visibility' here is pretty unnecessary because there is nothing in the <svg> fragment that will render anyway. Also, 'height' (or 'width') can just be set to zero - the only real use of this fragment is to have a SVGSVGElement anyway (which could be achieved without one in the document as well.) https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html:5: <marker markerWidth="1" markerHeight="1"> None of this should be needed - and it's probably even preferable not to have it - see below. https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html:15: }, "Tests with setAttribute"); This test seems pretty redundant in the context of what we're testing. (Testing that setAttribute works is likely covered already.) https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html:20: markerElement.setOrientToAngle(svgAngle); These tests are dependent on the result of the previous test (and without verifying that the element is in an expected state.) I think it'd be better to have each subtest create a new <marker> element and test what it likes on that one. (That'd likely expose one issue with the fix in it's current form.) https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp (right): https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp:119: if (Attribute* orientAttribute = ensureUniqueElementData().attributes().find(SVGNames::orientAttr)) This will not do the right thing if this method is called when an 'orient' attribute has not been specified (in which case the value of the attribute should still be updated.) It also looks bit hacky TBH. https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp:120: orientAttribute->setValue("auto"); On 2016/02/17 at 22:38:02, rwlbuis wrote: > Have you considered using setAttribute? Using setAttribute would likely work fine in these two methods although I believe it'll miss the bigger issue. For instance consider this (rough equivalent) script: markerElement.orientType.baseVal = 1; markerElement.getAttribute('orient'); // returns? So the wider issue of attribute synchronization should probably be considered here as well.
https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp (right): https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp:119: if (Attribute* orientAttribute = ensureUniqueElementData().attributes().find(SVGNames::orientAttr)) On 2016/02/19 10:33:13, fs wrote: > This will not do the right thing if this method is called when an 'orient' > attribute has not been specified (in which case the value of the attribute > should still be updated.) It also looks bit hacky TBH. Yes. you are right.. I got it.. In case 'orient' attrribute is not specified before, then we can call attributes().append(SVGNames::orientAttr, value).. If you think it looks bit hacky, then can we simply call setAttribute() as Rob suggested. https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp:120: orientAttribute->setValue("auto"); On 2016/02/19 10:33:13, fs wrote: > On 2016/02/17 at 22:38:02, rwlbuis wrote: > > Have you considered using setAttribute? > > Using setAttribute would likely work fine in these two methods although I > believe it'll miss the bigger issue. For instance consider this (rough > equivalent) script: > > markerElement.orientType.baseVal = 1; > markerElement.getAttribute('orient'); // returns? > > So the wider issue of attribute synchronization should probably be considered > here as well. Hmm.. The part here "markerElement.orientType.baseVal = 1;" will be handled by SVGAngleTearOff::setValue() and adding setAttribute/equivalent from here, again its more of hacky.
https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp (right): https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp:119: if (Attribute* orientAttribute = ensureUniqueElementData().attributes().find(SVGNames::orientAttr)) On 2016/02/19 at 11:30:29, Shanmuga Pandi wrote: > On 2016/02/19 10:33:13, fs wrote: > > This will not do the right thing if this method is called when an 'orient' > > attribute has not been specified (in which case the value of the attribute > > should still be updated.) It also looks bit hacky TBH. > > Yes. you are right.. I got it.. > In case 'orient' attrribute is not specified before, then we can call attributes().append(SVGNames::orientAttr, value).. > > If you think it looks bit hacky, then can we simply call setAttribute() as Rob suggested. Yes, calling setAttribute would be preferable to manipulating the attribute collection manually. https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp:120: orientAttribute->setValue("auto"); On 2016/02/19 at 11:30:29, Shanmuga Pandi wrote: > On 2016/02/19 10:33:13, fs wrote: > > On 2016/02/17 at 22:38:02, rwlbuis wrote: > > > Have you considered using setAttribute? > > > > Using setAttribute would likely work fine in these two methods although I > > believe it'll miss the bigger issue. For instance consider this (rough > > equivalent) script: > > > > markerElement.orientType.baseVal = 1; > > markerElement.getAttribute('orient'); // returns? > > > > So the wider issue of attribute synchronization should probably be considered > > here as well. > > Hmm.. > The part here "markerElement.orientType.baseVal = 1;" will be handled by SVGAngleTearOff::setValue() and adding setAttribute/equivalent from here, again its more of hacky. My point is that doing that will show the same issue as calling this method (because the sequence of operations is almost exactly the same), and setAttribute should definitely not be added there since that would defeat the attribute synchronization lazyness. It will not go through SVGAngleTearOff, since orientType is an SVGAnimatedEnumeration (and hence it's baseVal is an SVGEnumeration.)
https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp (right): https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp:119: if (Attribute* orientAttribute = ensureUniqueElementData().attributes().find(SVGNames::orientAttr)) On 2016/02/19 12:38:35, fs wrote: > On 2016/02/19 at 11:30:29, Shanmuga Pandi wrote: > > On 2016/02/19 10:33:13, fs wrote: > > > This will not do the right thing if this method is called when an 'orient' > > > attribute has not been specified (in which case the value of the attribute > > > should still be updated.) It also looks bit hacky TBH. > > > > Yes. you are right.. I got it.. > > In case 'orient' attrribute is not specified before, then we can call > attributes().append(SVGNames::orientAttr, value).. > > > > If you think it looks bit hacky, then can we simply call setAttribute() as Rob > suggested. > > Yes, calling setAttribute would be preferable to manipulating the attribute > collection manually. Acknowledged. https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp:120: orientAttribute->setValue("auto"); On 2016/02/19 12:38:35, fs wrote: > On 2016/02/19 at 11:30:29, Shanmuga Pandi wrote: > > On 2016/02/19 10:33:13, fs wrote: > > > On 2016/02/17 at 22:38:02, rwlbuis wrote: > > > > Have you considered using setAttribute? > > > > > > Using setAttribute would likely work fine in these two methods although I > > > believe it'll miss the bigger issue. For instance consider this (rough > > > equivalent) script: > > > > > > markerElement.orientType.baseVal = 1; > > > markerElement.getAttribute('orient'); // returns? > > > > > > So the wider issue of attribute synchronization should probably be > considered > > > here as well. > > > > Hmm.. > > The part here "markerElement.orientType.baseVal = 1;" will be handled by > SVGAngleTearOff::setValue() and adding setAttribute/equivalent from here, again > its more of hacky. > > My point is that doing that will show the same issue as calling this method > (because the sequence of operations is almost exactly the same), and > setAttribute should definitely not be added there since that would defeat the > attribute synchronization lazyness. It will not go through SVGAngleTearOff, > since orientType is an SVGAnimatedEnumeration (and hence it's baseVal is an > SVGEnumeration.) Yes. Copy-Paste problem... It will go through SVGAnimatedEnumeration.
PTAL! https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html (right): https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html:4: <svg width="1" height="1" visibility="hidden"> On 2016/02/19 10:33:13, fs wrote: > Specifying 'visibility' here is pretty unnecessary because there is nothing in > the <svg> fragment that will render anyway. Also, 'height' (or 'width') can just > be set to zero - the only real use of this fragment is to have a SVGSVGElement > anyway (which could be achieved without one in the document as well.) Acknowledged. https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html:5: <marker markerWidth="1" markerHeight="1"> On 2016/02/19 10:33:13, fs wrote: > None of this should be needed - and it's probably even preferable not to have it > - see below. Done. https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html:15: }, "Tests with setAttribute"); On 2016/02/19 10:33:12, fs wrote: > This test seems pretty redundant in the context of what we're testing. (Testing > that setAttribute works is likely covered already.) Acknowledged. https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html:20: markerElement.setOrientToAngle(svgAngle); On 2016/02/19 10:33:13, fs wrote: > These tests are dependent on the result of the previous test (and without > verifying that the element is in an expected state.) I think it'd be better to > have each subtest create a new <marker> element and test what it likes on that > one. (That'd likely expose one issue with the fix in it's current form.) Done. https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp (right): https://codereview.chromium.org/1699893003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp:119: if (Attribute* orientAttribute = ensureUniqueElementData().attributes().find(SVGNames::orientAttr)) On 2016/02/19 12:38:35, fs wrote: > On 2016/02/19 at 11:30:29, Shanmuga Pandi wrote: > > On 2016/02/19 10:33:13, fs wrote: > > > This will not do the right thing if this method is called when an 'orient' > > > attribute has not been specified (in which case the value of the attribute > > > should still be updated.) It also looks bit hacky TBH. > > > > Yes. you are right.. I got it.. > > In case 'orient' attrribute is not specified before, then we can call > attributes().append(SVGNames::orientAttr, value).. > > > > If you think it looks bit hacky, then can we simply call setAttribute() as Rob > suggested. > > Yes, calling setAttribute would be preferable to manipulating the attribute > collection manually. Done.
https://codereview.chromium.org/1699893003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html (right): https://codereview.chromium.org/1699893003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html:12: }, "Tests with setOrientToAngle"); Maybe more descriptive as "getAttribute('orient') after setOrientToAngle" or similar. (Since that's what's being tested, and "Tests" is kind of left "hanging".) https://codereview.chromium.org/1699893003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html:18: }, "Tests with setOrientToAuto"); Ditto.
https://codereview.chromium.org/1699893003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html (right): https://codereview.chromium.org/1699893003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html:12: }, "Tests with setOrientToAngle"); On 2016/02/22 11:43:31, fs wrote: > Maybe more descriptive as "getAttribute('orient') after setOrientToAngle" or > similar. (Since that's what's being tested, and "Tests" is kind of left > "hanging".) Done. https://codereview.chromium.org/1699893003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/markers/marker-orientation-set.html:18: }, "Tests with setOrientToAuto"); On 2016/02/22 11:43:31, fs wrote: > Ditto. Done.
lgtm
The CQ bit was checked by shanmuga.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699893003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699893003/60001
Message was sent while issue was closed.
Description was changed from
==========
setOrientTo{Auto|Angle} should reflect on orient attribute
Earlier, setOrientTo{Auto|Angle} modifies only the SVGAngle value and units,
but not changing orient attribute value.
As per the spec, it should set the value on orient attribute.
https://www.w3.org/TR/svg-markers/#__svg__SVGMarkerElement__setOrientToAuto
BUG=586330
==========
to
==========
setOrientTo{Auto|Angle} should reflect on orient attribute
Earlier, setOrientTo{Auto|Angle} modifies only the SVGAngle value and units,
but not changing orient attribute value.
As per the spec, it should set the value on orient attribute.
https://www.w3.org/TR/svg-markers/#__svg__SVGMarkerElement__setOrientToAuto
BUG=586330
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from
==========
setOrientTo{Auto|Angle} should reflect on orient attribute
Earlier, setOrientTo{Auto|Angle} modifies only the SVGAngle value and units,
but not changing orient attribute value.
As per the spec, it should set the value on orient attribute.
https://www.w3.org/TR/svg-markers/#__svg__SVGMarkerElement__setOrientToAuto
BUG=586330
==========
to
==========
setOrientTo{Auto|Angle} should reflect on orient attribute
Earlier, setOrientTo{Auto|Angle} modifies only the SVGAngle value and units,
but not changing orient attribute value.
As per the spec, it should set the value on orient attribute.
https://www.w3.org/TR/svg-markers/#__svg__SVGMarkerElement__setOrientToAuto
BUG=586330
Committed: https://crrev.com/cffddfc0131ffda1f890dc244e4624abf5e742ce
Cr-Commit-Position: refs/heads/master@{#376718}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/cffddfc0131ffda1f890dc244e4624abf5e742ce Cr-Commit-Position: refs/heads/master@{#376718} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
