[svg2] Make 'x' and 'y' presentation attributes

Source/core/svg/SVGForeignObjectElement.cpp

 /*
  * Copyright (C) 2006 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nikolas Zimmermann <zimmermann@kde.org>
  *
  * 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.
  *
  * This library is distributed in the hope that it will be useful,
@@ skipping 56 matching lines @@
     return supportedAttributes.contains<SVGAttributeHashTranslator>(attrName);
 }
 
 void SVGForeignObjectElement::parseAttribute(const QualifiedName& name, const AtomicString& value)
 {
     parseAttributeNew(name, value);
 }
 
 bool SVGForeignObjectElement::isPresentationAttribute(const QualifiedName& name) const
 {
-    if (name == SVGNames::widthAttr || name == SVGNames::heightAttr)
+    if (name == SVGNames::xAttr || name == SVGNames::yAttr
+        || name == SVGNames::widthAttr || name == SVGNames::heightAttr)
         return true;
     return SVGGraphicsElement::isPresentationAttribute(name);
 }
 
 void SVGForeignObjectElement::collectStyleForPresentationAttribute(const QualifiedName& name, const AtomicString& value, MutableStylePropertySet* style)
 {
-    if (name == SVGNames::widthAttr || name == SVGNames::heightAttr) {
+    RefPtrWillBeRawPtr<SVGAnimatedPropertyBase> property = propertyFromAttribute(name);
+    if (property == m_width || property == m_height) {
         RefPtrWillBeRawPtr<SVGLength> length = SVGLength::create(LengthModeOther);
         TrackExceptionState exceptionState;
         length->setValueAsString(value, exceptionState);
         if (!exceptionState.hadException()) {
-            if (name == SVGNames::widthAttr)
+            if (property == m_width)

[fs, 2015/02/04 13:24:53]

Maybe we could split this part out and land separately? I.e. use the "new"
approach, which should allow this to be:

if (property == m_width)
    addSVGLengthPropertyToPresentationAttributeStyle(..., CSSPropertyWidth,
m_width);
else if (property == m_height)
    addSVGLengthPropertyToPresentationAtt...

IIRC the the original structure with parsing was to avoid "injecting" keywords
('auto' and 'inherit' I guess), but that shouldn't be the case when using the
SVGLength - right?

(see comment about this new helper method below.)

[Erik Dahlström (inactive), 2015/02/05 16:09:02]

That shouldn't be the case no, but we should allow them in the future I think. A
move to parsing with CSS would provide that automatically.

Yes, I'd like to convert the width/height ones to follow the same pattern, but
wasn't sure if it should be done in this patch or not.

                 addPropertyToPresentationAttributeStyle(style, CSSPropertyWidth, value);
-            else if (name == SVGNames::heightAttr)
+            else if (property == m_height)
                 addPropertyToPresentationAttributeStyle(style, CSSPropertyHeight, value);
         }
+    } else if (property == m_x) {
+        addPropertyToPresentationAttributeStyle(style, CSSPropertyX, m_x->currentValue()->valueInSpecifiedUnits(), m_x->currentValue()->cssUnitTypeQuirk());

[fs, 2015/02/04 13:24:53]

Maybe turn this "pattern" into:

void addSVGLengthPropertyToPresentationAttributeStyle(MutableStylePropertySet*,
CSSPropertyID, SVGLength&)

(on SVGElement?)

then the quirks-handling would be contained in a single spot too hopefully.

[Erik Dahlström (inactive), 2015/02/05 16:09:02]

Done.

+    } else if (property == m_y) {
+        addPropertyToPresentationAttributeStyle(style, CSSPropertyY, m_y->currentValue()->valueInSpecifiedUnits(), m_y->currentValue()->cssUnitTypeQuirk());
     } else {
         SVGGraphicsElement::collectStyleForPresentationAttribute(name, value, style);
     }
 }
 
 void SVGForeignObjectElement::svgAttributeChanged(const QualifiedName& attrName)
 {
     if (!isSupportedAttribute(attrName)) {
         SVGGraphicsElement::svgAttributeChanged(attrName);
         return;
     }
 
-    if (attrName == SVGNames::widthAttr || attrName == SVGNames::heightAttr) {
-        invalidateSVGPresentationAttributeStyle();
-        setNeedsStyleRecalc(LocalStyleChange, StyleChangeReasonForTracing::create(StyleChangeReason::SVGContainerSizeChange));
-    }
-
     SVGElement::InvalidationGuard invalidationGuard(this);
 
+    bool widthOrHeight = attrName == SVGNames::widthAttr

[fs, 2015/02/04 13:24:53]

Maybe do the something similar for 'x' and 'y' (and then maybe drop
isLengthAttribute)

+        || attrName == SVGNames::heightAttr;
+
+    if (attrName == SVGNames::xAttr || attrName == SVGNames::yAttr || widthOrHeight) {
+        invalidateSVGPresentationAttributeStyle();
+        setNeedsStyleRecalc(LocalStyleChange,
+            widthOrHeight ? StyleChangeReasonForTracing::create(StyleChangeReason::SVGContainerSizeChange) : StyleChangeReasonForTracing::fromAttribute(attrName));
+    }
+
     bool isLengthAttribute = attrName == SVGNames::xAttr
-                          || attrName == SVGNames::yAttr
-                          || attrName == SVGNames::widthAttr
-                          || attrName == SVGNames::heightAttr;
+        || attrName == SVGNames::yAttr
+        || widthOrHeight;
 
     if (isLengthAttribute) {
         updateRelativeLengthsInformation();
         if (RenderObject* renderer = this->renderer())
             markForLayoutAndParentResourceInvalidation(renderer);
     }
 }
 
 RenderObject* SVGForeignObjectElement::createRenderer(RenderStyle*)
 {
@@ skipping 20 matching lines @@
 
 bool SVGForeignObjectElement::selfHasRelativeLengths() const
 {
     return m_x->currentValue()->isRelative()
         || m_y->currentValue()->isRelative()
         || m_width->currentValue()->isRelative()
         || m_height->currentValue()->isRelative();
 }
 
 } // namespace blink