setOrientTo{Auto|Angle} should reflect on orient attribute

third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp

 /*
  * Copyright (C) 2004, 2005, 2006, 2007, 2008 Nikolas Zimmermann <zimmermann@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007 Rob Buis <buis@kde.org>
  * Copyright (C) Research In Motion Limited 2009-2010. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
@@ skipping 98 matching lines @@
 
     if (change.byParser)
         return;
 
     if (LayoutObject* object = layoutObject())
         object->setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::ChildChanged);
 }
 
 void SVGMarkerElement::setOrientToAuto()
 {
+    if (Attribute* orientAttribute = ensureUniqueElementData().attributes().find(SVGNames::orientAttr))

[fs, 2016/02/19 10:33:13]

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.

[Shanmuga Pandi, 2016/02/19 11:30:29]

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.

[fs, 2016/02/19 12:38:35]

Yes, calling setAttribute would be preferable to manipulating the attribute
collection manually.

[Shanmuga Pandi, 2016/02/19 14:46:44]

Acknowledged.

[Shanmuga Pandi, 2016/02/22 06:45:07]

Done.

+        orientAttribute->setValue("auto");

[rwlbuis, 2016/02/17 22:38:02]

Have you considered using setAttribute?

[fs, 2016/02/19 10:33:13]

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.

[Shanmuga Pandi, 2016/02/19 11:30:29]

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.

[fs, 2016/02/19 12:38:35]

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.)

[Shanmuga Pandi, 2016/02/19 14:46:44]

Yes. Copy-Paste problem... It will go through SVGAnimatedEnumeration.

+
     m_orientAngle->baseValue()->orientType()->setEnumValue(SVGMarkerOrientAuto);
     invalidateSVGAttributes();
     svgAttributeBaseValChanged(SVGNames::orientAttr);
 }
 
 void SVGMarkerElement::setOrientToAngle(PassRefPtrWillBeRawPtr<SVGAngleTearOff> angle)
 {
     ASSERT(angle);
     RefPtrWillBeRawPtr<SVGAngle> target = angle->target();
+    if (Attribute* orientAttribute = ensureUniqueElementData().attributes().find(SVGNames::orientAttr))
+        orientAttribute->setValue(AtomicString(target->valueAsString()));
+
     m_orientAngle->baseValue()->newValueSpecifiedUnits(target->unitType(), target->valueInSpecifiedUnits());
     invalidateSVGAttributes();
     svgAttributeBaseValChanged(SVGNames::orientAttr);
 }
 
 LayoutObject* SVGMarkerElement::createLayoutObject(const ComputedStyle&)
 {
     return new LayoutSVGResourceMarker(this);
 }
 
 bool SVGMarkerElement::selfHasRelativeLengths() const
 {
     return m_refX->currentValue()->isRelative()
         || m_refY->currentValue()->isRelative()
         || m_markerWidth->currentValue()->isRelative()
         || m_markerHeight->currentValue()->isRelative();
 }
 
 } // namespace blink