Closed Bug 1062106 Opened 10 years ago Closed 7 years ago

Remove special handling of attributeType="XML" when animating CSS properties

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: birtles, Assigned: mantaroh)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 4 obsolete files)

Bug 1062106 part 1 - Remove special handling of attributeType='XML'.
59 bytes, text/x-review-board-request
birtles
: review+
longsonr
: review+
Details
Bug 1062106 part 2 - Remove unused SVG Attribute Animations cascade level.
59 bytes, text/x-review-board-request
birtles
: review+
dbaron
: review+
xidorn
: review+
Details
Bug 1062106 part 3 - Remove SMIL MappedAttribute mechanism.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1062106 part 4 - Remove GetTargetAttributeType() from svg element.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
Currently there is a subtle difference between the animation of the following two elements:

  <animate attributeType="XML" attributeName="fill" ... />
  <animate attributeType="CSS" attributeName="fill" ... />

  (If attributeType is not specified it defaults to "auto" which will have the same behavior as "CSS" for CSS properties.)

They end up applying at different points in the cascade (XML->high-level presentational hint, CSS->just above inline style) and their implementation is quite different. 

Whether this distinction was intended by the spec is unclear. See Daniel's comments in bug 1057231 comment 17 which includes posts to www-svg that received no response.

I propose we remove this distinction and use the "CSS" behavior everywhere because:

1) I'd like to make "attributeType" obsolete. It was only added to disambiguate when an element had an attribute name that overlaps with a CSS property and both are animatable. In SVG we're promoting most animatable attributes to properties (i.e. making them presentation attributes) so the chance of overlap should be reduced to a small subset of animatable attributes that don't get promoted (e.g. 'class', 'href', 'd'). Hence it seems like we should be able to remove this attribute.

2) It would simplify implementation considerably: if not in terms of sheer code size at least in terms of conceptual complexity (it took me a few hours to work out which code paths were related to which mode in bug 977991 comment 27 and I'm pretty sure I didn't get it right).

3) I think SVG animations and CSS animations should operate at the same level in the cascade so they can be better integrated and to give authors more consistent behavior. That is the approach Web Animations is taking and I'd rather not add a flag to Web Animations to reproduce this subtle difference if I can avoid it.

I don't expect content to be relying on this distinction. Based on Daniel's investigation (http://lists.w3.org/Archives/Public/www-svg/2010Jan/0067.html) it seems UAs are already inconsistent in what they do here. Furthermore, there is an ongoing investigation to re-implement SVG Animation in Blink entirely in Javascript and I think it's unlikely to reproduce this subtlety.
(In reply to Brian Birtles (:birtles) from comment #0)
> I don't expect content to be relying on this distinction. Based on Daniel's
> investigation
> (http://lists.w3.org/Archives/Public/www-svg/2010Jan/0067.html) it seems UAs
> are already inconsistent in what they do here.

FWIW: I just tested current Blink (Version 39.0.2145.4 dev (64-bit)), and they appear to behave as if attributeType did not exist. i.e. they match the suggested behavior in this bug, and they match the Adobe SVG Viewer behavior (*not* the WebKit behavior) described in that ^ www-svg post.

In particular, Blink/ASV differ from current Gecko on my chart[1] in cases (c) and (e).  In those cases, there's an attributeType="XML" animation, which Gecko stomps on, because we have an attributeType="CSS" animation (in (c)) or a "style" attribute (in (d)), both of which apply with higher priority in the CSS cascade, and hence stomp on the results of the XML-attribute animation.

But if we were to ignore attributeType, then that animation would shine through (and turn the rect blue) in these cases.  And that appears to be what Blink does.

[1] http://people.mozilla.org/~dholbert/tests/smil/compat_tests/xmlVsCssChart_v1.svg
I've always thought that what Brian proposes in comment 0 is how the spec should have been defined, though I recall that Dirk was in favour of being able to animate the presentation attributes separately from the properties.  I think it's a worthwhile simplification to make if we can.
(In reply to Daniel Holbert [:dholbert] from comment #1)
> (In reply to Brian Birtles (:birtles) from comment #0)
> > I don't expect content to be relying on this distinction. Based on Daniel's
> > investigation
> > (http://lists.w3.org/Archives/Public/www-svg/2010Jan/0067.html) it seems UAs
> > are already inconsistent in what they do here.
> 
> FWIW: I just tested current Blink (Version 39.0.2145.4 dev (64-bit)), and
> they appear to behave as if attributeType did not exist. i.e. they match the
> suggested behavior in this bug, and they match the Adobe SVG Viewer behavior
> (*not* the WebKit behavior) described in that ^ www-svg post.

Blink and WebKit should behave the same here (looking at the relevant source right now). If an author specifies XML and it is a CSS property, then we do animate the property and not the attribute. This is even true for the new created SVG CSS properties x,y,width,height,rx,ry,cx,cy and r in WebKit.

I am supportive for Brian's proposal.
(In reply to Cameron McCormack (:heycam) from comment #2)
> I've always thought that what Brian proposes in comment 0 is how the spec
> should have been defined, though I recall that Dirk was in favour of being
> able to animate the presentation attributes separately from the properties. 
> I think it's a worthwhile simplification to make if we can.

I am not, it is just the way that I interpreted the specification. If it didn't have this intention, attributeType would have been meaningless from the beginning.
(In reply to Dirk Schulze from comment #3)
> (In reply to Daniel Holbert [:dholbert] from comment #1)
> > FWIW: I just tested current Blink (Version 39.0.2145.4 dev (64-bit)), and
> > they appear to behave as if attributeType did not exist. i.e. they match the
> > suggested behavior in this bug, and they match the Adobe SVG Viewer behavior
> > (*not* the WebKit behavior) described in that ^ www-svg post.
> 
> Blink and WebKit should behave the same here

Ah, that's good news.

(In comment 1, the "WebKit" behavior I was comparing Blink against was the behavior documented in my chart of results in http://lists.w3.org/Archives/Public/www-svg/2010Jan/0067.html . It sounds like things have changed since then, though.)
Maybe you misinterpreted the results or you are not testing what you intend to test.

<rect xmlns="http://www.w3.org/2000/svg" fill="rgb(200,0,0)" width="50" height="50">
      <animate attributeType="CSS" attributeName="fill" to="rgb(0,200,0)" begin="0s" dur="1s" fill="freeze"/>
      <animate attributeType="XML" attributeName="fill" to="rgb(0,0,200)" begin="0s" dur="1s" fill="freeze"/>
</rect>

This is blue in WebKit and IIRC it always was. The text says: "Competing 'fill', XML top of sandwich"

In the case above the animation is not additive and we ignore the specified "XML". That is why the second animation wins. It is not visible in the WebInspector since we do it on the override style as the spec requires us to do.
Ok, a) is gray and e) is blue as well now. Maybe there was a bug in WebKit at the time after all :) The results of WebKit and Blink match the results of ASV.
Basically, in gecko, the behavior of attributeType is as follow:
1) attributeType="XML":
 a) use nsHTMLCSSStyleSheet as StyleRuleProcessor.[1]
 b) Lookup property from attribute name when composing. [2]

2) attributeType="CSS"
 a) use SVGAttrAnimationRuleProcessor as StyleRuleProcessor.[1]
 b) Lookup property from attribute name when composing if attribute is mapped. [2][3]

[1] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/layout/style/nsStyleSet.cpp#1592,1602
[2] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/smil/nsSMILCompositor.cpp#127
[3] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/svg/nsSVGElement.cpp#2551

Perhaps we can merge 1-a and 1-2 steps to procedure of SVGAttAniamtionRuleProcessor.
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
Umm,,
In the case of property is 'width' or 'height', the compositor will create SMIL Attribute from SVGElement.[1]

[1] https://dxr.mozilla.org/mozilla-central/rev/d4b3146a5567a7ddbcdfa5244945db55616cb8d1/dom/smil/nsSMILCompositor.cpp#137

We may go through this pass when property is 'width' or 'height' or SVGMotion and etc..(For detail, see SVGElement::GetAnimatedAttr)

I think that we can drop mIsCSS property from nsSMILTargetIdentifier, and I think we can decide that get SMIL Attribute from where.

Furthermore, I think that we can use eRestyle_StyleAttribute_Animations instead of eRestyle_SVGAttrAnimations, and I think we can drop it, since difference of each restyle hint is as follow:

eRestyle_SVGAttrAnimations         : Specify it when adding restyle tracker.[2]
eRestyle_StyleAttribute_Animations : Specify it when adding restyle tracker.[2]
                                     Specify it when setting smil css declaration.[3]

So I think that we won't need to separate this restyle hint.

[2] https://dxr.mozilla.org/mozilla-central/rev/d4b3146a5567a7ddbcdfa5244945db55616cb8d1/dom/smil/nsSMILAnimationController.cpp#740
[3] https://dxr.mozilla.org/mozilla-central/rev/d4b3146a5567a7ddbcdfa5244945db55616cb8d1/dom/base/Element.cpp#1986

I made the rough implementation.

Brian,
How do you think this changeset?
I'm not sure that this change cause performance issue, so I wish you can point out my wrong, if there are misunderstand.
Attachment #8820155 - Flags: feedback?(bbirtles)
Comment on attachment 8820155 [details] [diff] [review]
(WIP)Bug 1062106 - Remove special handling of attributeType="XML"  and drop eRestyle_SVGAttrAnimations.

Review of attachment 8820155 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good but:

* I don't really follow all the complexity of the width/height on outer SVG so we'll probably need to do some investigation into what is required there
* I *think* we want all these animations of properties to end up in the style attribute level of the cascade for now (although we probably want to change that to be the animations level of the cascade eventually)
* I suspect there's even more we can drop including SheetType::SVGAttrAnimation and perhaps even SheetType::SVGAttrAnimation

::: dom/smil/nsSMILCompositor.cpp
@@ +138,5 @@
>      return mKey.mElement->GetAnimatedAttr(mKey.mAttributeNamespaceID,
>                                            mKey.mAttributeName);
>    }
> +
> +  return new nsSMILCSSProperty(propId, mKey.mElement.get());

I wonder if the following would work:

  nsISMILAttr*
  nsSMILCompositor::CreateSMILAttr()
  {
    nsCSSPropertyID propId =
      nsCSSProps::LookupProperty(nsDependentAtomString(mKey.mAttributeName),
                                CSSEnabledState::eForAllContent);

    if (nsSMILCSSProperty::IsPropertyAnimatable(propId)) {
      return new nsSMILCSSProperty(propId, mKey.mElement.get());
    }

    return mKey.mElement->GetAnimatedAttr(mKey.mAttributeNamespaceID,
                                          mKey.mAttributeName);
  }

I'm trying to follow exactly the history of bug 764851 and I still can't quite determine if this works. I think it's probably easiest to just try the above with the test cases from bug 764851 and see if it breaks.

Perhaps it won't work because if we only change style, then the check to see whether or not the attribute is set (introduced in bug 668163) will fail?

Ideally, it would be nice to drop the section about mapped attributes at the start of nsSVGElement::GetAnimatedAttr (and even drop nsSMILMappedAttribute altogether?) but I haven't really grasped the detail of bug 668163.

::: layout/style/nsStyleSet.cpp
@@ +1477,5 @@
>  static const CascadeLevel gCascadeLevels[] = {
>    { SheetType::Agent,            false, false, nsRestyleHint(0) },
>    { SheetType::User,             false, false, nsRestyleHint(0) },
>    { SheetType::PresHint,         false, false, nsRestyleHint(0) },
> +  { SheetType::SVGAttrAnimation, false, false, eRestyle_StyleAttribute_Animations },

I think we should drop this line, right? I think the idea is that all SVG animations of properties would go in the style attribute level of the cascade (or perhaps in future they should be part of the animation level of the cascade).
Attachment #8820155 - Flags: feedback?(bbirtles) → feedback+
width/height on outer svg is mapped to style i.e. you can say style="width:100%" on an outer <svg> elment and it works the same as width="100%".
width on say a foreignObject element does not work like this (although in SVG 2 it should do so).
The code you're looking at is there specifically to support animation of width/height on outer <svg> elements per bug 764851.
That code contains a reftest so if you break it you should get a test failure. 
Per bug 764851 http://hoffmann.bplaced.net/svgtest/widthheight02.svg should animate over time. You should check that before landing this bug.
Umm, I investigated this problem again.

(In reply to Brian Birtles (:birtles, away most of Jan 21 - Feb 1) from comment #10)
> Comment on attachment 8820155 [details] [diff] [review]
> (WIP)Bug 1062106 - Remove special handling of attributeType="XML" and drop
> eRestyle_SVGAttrAnimations.
> 
> Review of attachment 8820155 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good but:
> 
> * I don't really follow all the complexity of the width/height on outer SVG
> so we'll probably need to do some investigation into what is required there

As mentioned Robert in comment #11, we will need to mapped to style when specifying width/height attribute explicitly.


> ::: dom/smil/nsSMILCompositor.cpp
> @@ +138,5 @@ 
> I wonder if the following would work:
> 
> nsISMILAttr*
> nsSMILCompositor::CreateSMILAttr()
> {
> nsCSSPropertyID propId =
> nsCSSProps::LookupProperty(nsDependentAtomString(mKey.mAttributeName),
> CSSEnabledState::eForAllContent);
> 
> if (nsSMILCSSProperty::IsPropertyAnimatable(propId)) {
> return new nsSMILCSSProperty(propId, mKey.mElement.get());
> }
> 
> return mKey.mElement->GetAnimatedAttr(mKey.mAttributeNamespaceID,
> mKey.mAttributeName);
> }

If we use above example, following test case is failed.
-----------------------------------
<svg>
 <rect width="100px" height="100px" fill="red">
  <animate attributeName="width" to="200px" dur="1s"/>
  <animate attributeName="height" to="200px" dur="1s"/>
 </rect>
</svg>
-----------------------------------

In fact, we used SVGElement::GetAnimatedAttr in the above sample, in the current implementation.
So I think that we will need to special handling when attributeName is width or height.
I'm back to this bug.

(In reply to Brian Birtles (:birtles) from comment #10)
> * I don't really follow all the complexity of the width/height on outer SVG
> so we'll probably need to do some investigation into what is required there

I'm going to remove this handling, But maybe I think that we will need to this handling since we can't get the attribute value when restyling.

WIP:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d48944c18e8647d1232e6c62bc676398593b5f1
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #14)
> I'm going to remove this handling, But maybe I think that we will need to
> this handling since we can't get the attribute value when restyling.

Investigation Memo:
We treat width/height as presentation attribute when owner element is the outermost SVG element(i.e. SVGSVGElement).
The following table shows that how is gecko treat each SVG's attribute.
+-------------------------------------+- Use MappedAttribute? -+- Animation as CSS? (*1) -+
|1. SVG Presentation Attribute | yes | yes |
|2. width / height of Non-Outer SVG | no | yes |
|3. width / height of Outer SVG | yes | yes |
+-------------------------------------+------------------------+--------------------------+
*1: This means that we will animate as attributeType='CSS'.

For example, 'fill' attribute treat as No.1, rect element's width treats as No.2, outer SVG element's height treats as No.3.

I think that we have better to use mapped attribute when specifying width/height attribute of the outer SVG element since we calculate intrinsic width/height value from cached value.[1] 
If we use the width/height value as SVG's attribute, we couldn't store the cache value.

[1] https://dxr.mozilla.org/mozilla-central/rev/d29f84406483c721a13cf9a52936ecced0c5c98a/layout/generic/nsFrame.cpp#5094
I wrote the patch of removing special handling of attributeType. And I think that we can't remove restyle hint of eRestyle_SVGAttrAnimations  since we need to leave mapped attribute for width/height of outer-<svg> element.

So in this bug, we remove the attribtueType's special handling only.

If target attribute is mapping to style, we use eRestyle_StyleAttr_Animations hint. Other hand, we use eResyle_SVGAttr_Animations hint.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b47c94803a8533c73f49df6caadc57dca5da9c5a
Comment on attachment 8820155 [details] [diff] [review]
(WIP)Bug 1062106 - Remove special handling of attributeType="XML"  and drop eRestyle_SVGAttrAnimations.

>-  } else {
>+  nsCSSPropertyID propId =
>+    nsCSSProps::LookupProperty(nsDependentAtomString(mKey.mAttributeName),
>+                               CSSEnabledState::eForAllContent);
>+
>+  // If we can't find the attribute name or CSS Property,
>+  // We should search for matching attribute on the target element.
>+  // https://svgwg.org/specs/animations/#TargetAttributes
>+  if (mKey.mAttributeName == nsGkAtoms::width ||
>+      mKey.mAttributeName == nsGkAtoms::height ||
>+      !nsSMILCSSProperty::IsPropertyAnimatable(propId)) {
>     return mKey.mElement->GetAnimatedAttr(mKey.mAttributeNamespaceID,
>                                           mKey.mAttributeName);

If the property returned false from nsSMILCSSProperty::IsPropertyAnimatable
we would previously return nullptr; Why are you changing this logic so it
doesn't any more?

>   }
>-  return nullptr;
>+
>+  return new nsSMILCSSProperty(propId, mKey.mElement.get());
> }
>
Marking as P1 and moving to CSS Parsing & Computation since this blocks stylo work that we want to have done before Taipei.
Component: SVG → CSS Parsing and Computation
Priority: -- → P1

        Attachment #8820155 -
        Attachment is obsolete: true

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
  

        Attachment #8846541 -
        Attachment is obsolete: true

    
  

        Attachment #8846542 -
        Attachment is obsolete: true

    
  

        Attachment #8846543 -
        Attachment is obsolete: true

    
    

    
  
I think we can drop SVGAnimationElement::GetTargetAttributeType() and SVGAnimateMotionElement::GetTargetAttributeType() too. Probably as a separate patch.

    
    

    
  
(In reply to Robert Longson from comment #17)
> Comment on attachment 8820155 [details] [diff] [review]
> (WIP)Bug 1062106 - Remove special handling of attributeType="XML"  and drop
> eRestyle_SVGAttrAnimations.
> 
> >-  } else {
> >+  nsCSSPropertyID propId =
> >+    nsCSSProps::LookupProperty(nsDependentAtomString(mKey.mAttributeName),
> >+                               CSSEnabledState::eForAllContent);
> >+
> >+  // If we can't find the attribute name or CSS Property,
> >+  // We should search for matching attribute on the target element.
> >+  // https://svgwg.org/specs/animations/#TargetAttributes
> >+  if (mKey.mAttributeName == nsGkAtoms::width ||
> >+      mKey.mAttributeName == nsGkAtoms::height ||
> >+      !nsSMILCSSProperty::IsPropertyAnimatable(propId)) {
> >     return mKey.mElement->GetAnimatedAttr(mKey.mAttributeNamespaceID,
> >                                           mKey.mAttributeName);
> 
> If the property returned false from nsSMILCSSProperty::IsPropertyAnimatable
> we would previously return nullptr; Why are you changing this logic so it
> doesn't any more?
> 
> >   }
> >-  return nullptr;
> >+
> >+  return new nsSMILCSSProperty(propId, mKey.mElement.get());
> > }
> >

Commenting just on the most recent patch, previously we had the following arrangement:

(I'm ignoring the special handling for width/height here and below, but I believe it's
correct.)

  nsSMILAnimationController::GetTargetIdentifierForAnimation
    attributeType=auto
      isCSS = IsPropertyAnimatable(prop)
    attributeType=CSS
      isCSS = true

  nsSMILCompositor::CreateSMILAttr
    if (isCSS)
      return IsPropertyAnimatable(prop) ? nsSMILCSSProperty : null
    else
      return GetAnimatedAttr

So CreateSMILAttr will only return null in two cases:

  (a) attributeType = CSS, IsPropertyAnimatable(attributeName) is false
  (b) attributeType = auto, IsPropertyAnimatable(attributeName) is false, and
      GetAnimatedAttr returns null

That is, when attributeType=auto, we only set isCSS if IsPropertyAnimatable returns true.
As a result, the check in CreateSMILAttr for IsPropertyAnimatable will never return
false for the attributeType=auto case.

The behaviour as of this latest patch is:

  if (IsPropertyAnimatable(prop))
    return nsSMILCSSProperty
  else
    return GetAnimatedAttr

That is, we will only return null for (b) from above which seems consistent with
assuming attributeType=auto everywhere.

    
    

    
  
Comment on attachment 8846914 [details]
Bug 1062106 part 1 - Remove special handling of attributeType='XML'.

https://reviewboard.mozilla.org/r/119930/#review121824

r=me with the following comments addressed.

::: commit-message-f9362:3
(Diff revision 1)
> +This patch drops the special handling of attributeType='XML' and change the restyle hint of SMIL.
> +This mean that we treat as attributeType='auto' always.

It doesn't change the restyle hint. It just means that we always use attributeType='CSS' for CSS properties so that we always end up using the eRestyle_StyleAttribute_Animations restyle hint, rather than sometimes using the eRestyle_StyleAttribute_Animations restyle hint, and sometimes using the eRestyle_SVGAttrAnimations hint.

So I would say,

"This patch removes handling of the 'attributeType' attribute so that we behave as if attributeType is always "auto". This means that for CSS properties we always animate them as CSS properties (i.e. we animate them as part of the SMIL override stylesheet) rather than mapped attributes.

The one special case is width/height on an outer SVG. Previously we animated this as a mapped attribute since Web compatibility requires that the width/height on an outer SVG, when set explicitly, are mapped to style. However, we can produce the same behavior by animating these as CSS properties (as opposed to mapped attributes). There is no observable difference in results returned by the SVG DOM APIs, only the level at which the result is added to the cascade: the SMIL override stylesheet instead of the attribute animation presentation hint level.

As part of this patch, we animate width/height on outer SVG elements as CSS properties as opposed to mapped attributes both for consistency and also so we can remove the animated mapped attribute code altogether."

::: dom/smil/nsSMILCompositor.cpp:136
(Diff revision 1)
> +    // The easiest way to test for an outer SVG element, is just to see if it
> +    // is an element mapping its width/height attribute to style.
> +    bool animateAsAttr = (mKey.mAttributeName == nsGkAtoms::width ||
> +                          mKey.mAttributeName == nsGkAtoms::height) &&
> +                         !mKey.mElement->IsAttributeMapped(mKey.mAttributeName);
> +    bool animateOuterElement =
> +      mKey.mElement->GetNameSpaceID() != kNameSpaceID_SVG;
> +
> +    if (!animateAsAttr || animateOuterElement) {

I think we can simplify this as follows:

Make the last part of the comment just:

  // The easiest way to test for an outer SVG element, is to see if it
  // is an SVG-namespace element mapping its width/height to style.

Then change the animateAsAttr check to:

  bool animateAsAttr = (mKey.mAttributeName == nsGkAtoms::width ||
                        mKey.mAttributeName == nsGkAtoms::height) &&
                       !(mKey.mElement->GetNameSpaceID() == kNameSpaceID_SVG &&
                         mKey.mElement->IsAttributeMapped(mKey.mAttributeName));

Then the final check is just:

  if (!animateAsAttr) {
    return new ...
  }

::: layout/reftests/svg/smil/anim-remove-6.svg:4
(Diff revision 1)
>  <svg xmlns="http://www.w3.org/2000/svg"
>       xmlns:xlink="http://www.w3.org/1999/xlink"
>       onload="go()">
> -  <!-- In this test, we change an animation element's "attributeType" attribute
> +  <!-- In this tests, we change an animation element's "attributeType" attribute

I think it should be just 'test', no need to add an 's'

::: layout/reftests/svg/smil/anim-remove-6.svg:6
(Diff revision 1)
> -       there's no CSS property that matches the attributeName that we're
> -       animating, "x").  We verify that animation effects are removed from the
> +       (since in SVG, there's no CSS property that match the attributeName that
> +       we're animating, "x"). We verify that animation effects are removed from

The parenthetical should probably become:

"(since there is no animatable 'x' attribute in the xlink namespace on a 'rect')"

        Attachment #8846914 -
        Flags: review?(bbirtles) → review+

    
    

    
  
Comment on attachment 8846915 [details]
Bug 1062106 part 2 - Remove unused SVG Attribute Animations cascade level.

https://reviewboard.mozilla.org/r/119932/#review121828

It seems odd to me that we remove all references to SVGAttrAnimationRuleProcessor in this patch but delete the files in the next patch (along with other unrelated changes). Let's drop SVGAttrAnimationRuleProcessor in this patch.

r=me with that but dbaron should probably check this too in case there are other places that need to be updated too (apart from what's in the next patch)

::: commit-message-f8d6d:3
(Diff revision 1)
> +We can drop the eRestyle_SVGAttrAnimations restyle hint since we will treat SVG Animation as usual style animation in the previous patch.
> +This patch removes this restyle hint.
> +
> +Notice: We should remove literal string which mapped literal of restyle hint. This is using by testing.(RestyleManager::RestyleHintToString)

"The previous patch in this series converted all uses of mapped attributes for animation, to be animated as CSS properties (that is, to be treated as targeting the SMIL override stylesheet, rather than special SVG Animation presentation hints in the cascade).

As a result, we no longer need the SVG Animation presentation hints level of the cascade, the corresponding rule processor: SVGAttrAnimationRuleProcessor, or the corresponding eRestyle_SVGAttrAnimations restyle hint."

I don't think we need to call out the changes to RestyleManager::RestyleHintToString.

::: layout/style/nsStyleSet.cpp:1138
(Diff revision 1)
>    bool haveImportantUserRules = !aRuleWalker->GetCheckForImportantRules();
>  
>    aRuleWalker->SetLevel(SheetType::PresHint, false, false);
>    if (mRuleProcessors[SheetType::PresHint])
>      (*aCollectorFunc)(mRuleProcessors[SheetType::PresHint], aData);
>  

Drop this blank line (so that |lastPresHintRN| is flash with the other three lines concerned with processing preshints).

        Attachment #8846915 -
        Flags: review?(bbirtles) → review+

        Attachment #8846915 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 8846916 [details]
Bug 1062106 part 3 - Remove SMIL MappedAttribute mechanism.

https://reviewboard.mozilla.org/r/119934/#review121832

::: commit-message-a9505:2
(Diff revision 1)
> +Bug 1062106 part 3 - Remove SMIL MappedAttribute mechanism. r?birtles
> +We can remove unnecesasary SMILMappedAttribute and SMILAttrAnimationRuleProcessor since we drop the code of using it.

s/since we drop the code of using it/since earlier patches in this series mean this code is no longer used/

::: dom/smil/nsSMILTimeContainer.cpp:14
(Diff revision 1)
>  #include "nsSMILTimedElement.h"
>  #include <algorithm>
>  
>  #include "mozilla/AutoRestore.h"
>  
> +using namespace mozilla;

(For my own reference, I believe this is needed since this patch deletes some files that means the chunking of the universal build changes exposing the fact that we were relying on this declaration from another file.)

        Attachment #8846916 -
        Flags: review?(bbirtles) → review+

    
    

    
  
Thanks for doing this. Also, please do create a part 4 for dropping GetTargetAttributeType as per comment 26.

    
    

    
  
Comment on attachment 8846915 [details]
Bug 1062106 part 2 - Remove unused SVG Attribute Animations cascade level.

https://reviewboard.mozilla.org/r/119932/#review121834

::: commit-message-f8d6d:1
(Diff revision 1)
> +Bug 1062106 part 2 - Remove unnecessary eStyle_SVGAttribute_Animation restyle hint. r?birtles
> +
> +We can drop the eRestyle_SVGAttrAnimations restyle hint since we will treat SVG Animation as usual style animation in the previous patch.
> +This patch removes this restyle hint.

This commit message is an incorrect description of what this patch does.  It's removing *substantially* more than this.

Please update with an accurate commit message and request review again.

        Attachment #8846915 -
        Flags: review?(dbaron) → review-

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8847022 [details]
Bug 1062106 part 4 - Remove GetTargetAttributeType() from svg element.

https://reviewboard.mozilla.org/r/120054/#review121878

        Attachment #8847022 -
        Flags: review?(bbirtles) → review+

    
    

    
  
Comment on attachment 8846915 [details]
Bug 1062106 part 2 - Remove unused SVG Attribute Animations cascade level.

https://reviewboard.mozilla.org/r/119932/#review122094

The first line of the commit message (which is the summary; only the first line is shown by many tools) is still inaccurate, in that it's only describing a small part of what the patch does.

The primary thing that this patch is removing is a level of the cascade that is specific to SVG Attribute animations; the restyle hint just comes along with that.

        Attachment #8846915 -
        Flags: review?(dbaron) → review-

    
    

    
  
Comment on attachment 8846915 [details]
Bug 1062106 part 2 - Remove unused SVG Attribute Animations cascade level.

https://reviewboard.mozilla.org/r/119932/#review122096

::: commit-message-f8d6d:5
(Diff revision 2)
> +Bug 1062106 part 2 - Remove unnecessary eStyle_SVGAttribute_Animation restyle hint. r?birtles, dbaron
> +
> +The previous patch in this series converted all uses of mapped attributes for animation, to be animated as CSS properties (that is, to be treated as presentation hints in the cascade).
> +
> +As result, we no longer need the SVG Animation presentation hints level of the cascade, the corresponding rule processor: SVGAttrAnimationRuleProcessor, or the corresponding eRestyle_SVGAttrAnimations restyle hint.

Also, another obviously missing change is removing SVGAttrAnimation from https://searchfox.org/mozilla-central/source/layout/style/SheetType.h .  Once you remove that, the compiler may lead you to some other missing changes.

    
    

    
  
If you move to CSS animating the length/width and I get the animVal of those attributes during the animation will they reflect the values at that point of the animation? 

Are there any tests of that? If not a mochitest might be nice. It may well be possible to modify one of the existing mochitests.
Flags: needinfo?(mantaroh)

    
    

    
  
Comment on attachment 8846914 [details]
Bug 1062106 part 1 - Remove special handling of attributeType='XML'.

https://reviewboard.mozilla.org/r/119930/#review122234

::: layout/reftests/svg/smil/anim-remove-6.svg:6
(Diff revision 2)
>  <svg xmlns="http://www.w3.org/2000/svg"
>       xmlns:xlink="http://www.w3.org/1999/xlink"
>       onload="go()">
>    <!-- In this test, we change an animation element's "attributeType" attribute
> -       to "CSS", which invalidates a completed, frozen animation (since in SVG,
> -       there's no CSS property that matches the attributeName that we're
> +       to refer to another namespace, which invalidates a completed, frozen animation
> +       (since there is no animatable 'x' attribute in the xlink namecpace on a rect)

namespace is not spelled correctly

::: layout/reftests/svg/smil/anim-remove-6.svg:16
(Diff revision 2)
>        // Seek animation before we start tweaking things, to make sure we've
>        // already started sampling it.
>        document.documentElement.setCurrentTime(2.0);
>  
>        var anim = document.getElementById("anim");
> -      anim.setAttributeNS(null, "attributeType", "CSS");
> +      anim.setAttributeNS(null, "attributeName", "xlink:x");

The comment above suggests the xlink namespace but the code uses the null namespace

    
    

    
  
(In reply to Robert Longson from comment #40)
> If you move to CSS animating the length/width and I get the animVal of those
> attributes during the animation will they reflect the values at that point
> of the animation? 

No, but they're already don't reflect it.

See for example: http://codepen.io/birtles/pen/wJgpyP
(You'll need to open your console to the result, sorry.)

With the existing setup, we never reflect the animation change back into the length member. So it's already a pseudo-mapped attribute for the purposes of animation as far as I can tell.

    
    

    
  
(In reply to Robert Longson from comment #41)
> ::: layout/reftests/svg/smil/anim-remove-6.svg:16
> (Diff revision 2)
> >        // Seek animation before we start tweaking things, to make sure we've
> >        // already started sampling it.
> >        document.documentElement.setCurrentTime(2.0);
> >  
> >        var anim = document.getElementById("anim");
> > -      anim.setAttributeNS(null, "attributeType", "CSS");
> > +      anim.setAttributeNS(null, "attributeName", "xlink:x");
> 
> The comment above suggests the xlink namespace but the code uses the null
> namespace

I think it's right? It doesn't change the namespace of the attributeName attribute, but of the attribute that attributeName refers to (i.e. from 'x' to 'xlink:x' -- SMIL uses qnames to refer to attributes and the 'xlink' prefix is already mapped to the xlink namespace earlier in the test file).

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Robert Longson from comment #40)
> If you move to CSS animating the length/width and I get the animVal of those
> attributes during the animation will they reflect the values at that point
> of the animation? 
> 
> Are there any tests of that? If not a mochitest might be nice. It may well
> be possible to modify one of the existing mochitests.

As Brian mentioned in comment #42, we don't reflect animation value.
This behavior doesn't change before applying this patch, so I think that we can use current mochitests.
Flags: needinfo?(mantaroh)

    
    

    
  
Comment on attachment 8846914 [details]
Bug 1062106 part 1 - Remove special handling of attributeType='XML'.

https://reviewboard.mozilla.org/r/119930/#review122234

Thank you so much.

I updated this patch.
Could you please confirm again?

> The comment above suggests the xlink namespace but the code uses the null namespace

As Brian mentioned in comment #43, we already mapped xlink on declaration of svg element.

    
    

    
  
Comment on attachment 8846915 [details]
Bug 1062106 part 2 - Remove unused SVG Attribute Animations cascade level.

https://reviewboard.mozilla.org/r/119932/#review122094

Thanks, dbaron,

I fixed the commit message to what is this patch doing.

    
    

    
  
Comment on attachment 8846915 [details]
Bug 1062106 part 2 - Remove unused SVG Attribute Animations cascade level.

https://reviewboard.mozilla.org/r/119932/#review122348

::: commit-message-f8d6d:1
(Diff revision 3)
> +Bug 1062106 part 2 -Remove unused SVG Attribute Animations cascade process. r?birtles, dbaron

"process" -> "level"

::: commit-message-f8d6d:3
(Diff revision 3)
> +The previous patch in this series converted all uses of mapped attributes for animation, to be animated as CSS properties (that is, to be treated as presentation hints in the cascade).
> +
> +As result, we no longer need the SVG Animation presentation hints level of the cascade, the corresponding rule processor: SVGAttrAnimationRuleProcessor, or the corresponding eRestyle_SVGAttrAnimations restyle hint.
> +So this patch removes these unused rule processor and restyle hint.

These lines should be wrapped at 72-78 characters rather than very long.

And the last two should either be combined in a single paragraph, or have a blank line between them.

::: commit-message-f8d6d:3
(Diff revision 3)
> +Bug 1062106 part 2 -Remove unused SVG Attribute Animations cascade process. r?birtles, dbaron
> +
> +The previous patch in this series converted all uses of mapped attributes for animation, to be animated as CSS properties (that is, to be treated as presentation hints in the cascade).

remove the comma in "animation, to"

::: commit-message-f8d6d:5
(Diff revision 3)
> +Bug 1062106 part 2 -Remove unused SVG Attribute Animations cascade process. r?birtles, dbaron
> +
> +The previous patch in this series converted all uses of mapped attributes for animation, to be animated as CSS properties (that is, to be treated as presentation hints in the cascade).
> +
> +As result, we no longer need the SVG Animation presentation hints level of the cascade, the corresponding rule processor: SVGAttrAnimationRuleProcessor, or the corresponding eRestyle_SVGAttrAnimations restyle hint.

Either replace the : with a , or remove the : and put SVGAttrAnimationRuleProcessor in ().

        Attachment #8846915 -
        Flags: review?(dbaron) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8846915 [details]
Bug 1062106 part 2 - Remove unused SVG Attribute Animations cascade level.

https://reviewboard.mozilla.org/r/119932/#review122348

Thanks dbaron,

I address the comment.

    
    

    
  
Comment on attachment 8846914 [details]
Bug 1062106 part 1 - Remove special handling of attributeType='XML'.

https://reviewboard.mozilla.org/r/119930/#review121824

> I think we can simplify this as follows:
> 
> Make the last part of the comment just:
> 
>   // The easiest way to test for an outer SVG element, is to see if it
>   // is an SVG-namespace element mapping its width/height to style.
> 
> Then change the animateAsAttr check to:
> 
>   bool animateAsAttr = (mKey.mAttributeName == nsGkAtoms::width ||
>                         mKey.mAttributeName == nsGkAtoms::height) &&
>                        !(mKey.mElement->GetNameSpaceID() == kNameSpaceID_SVG &&
>                          mKey.mElement->IsAttributeMapped(mKey.mAttributeName));
> 
> Then the final check is just:
> 
>   if (!animateAsAttr) {
>     return new ...
>   }

Some test failed with this code since animateAsAttr will be true when we will animate out of <svg> element.[1] (e.g. an attribute name is 'width' and a namespace is kNameSpaceID_None.)

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d7ca3d749f63e4207c10bba87505d0e87740ee8


So I changed the condition like following:

    bool animateAsAttr = (mKey.mAttributeName == nsGkAtoms::width ||
                          mKey.mAttributeName == nsGkAtoms::height) &&
                         mKey.mElement->GetNameSpaceID() == kNameSpaceID_SVG &&
                         !mKey.mElement->IsAttributeMapped(mKey.mAttributeName);

    
    

    
  
Comment on attachment 8846914 [details]
Bug 1062106 part 1 - Remove special handling of attributeType='XML'.

https://reviewboard.mozilla.org/r/119930/#review123728

::: dom/smil/nsSMILCompositor.cpp:133
(Diff revision 4)
> -  if (mKey.mIsCSS) {
> +  nsCSSPropertyID propID =
> -    nsCSSPropertyID propId =
> -      nsCSSProps::LookupProperty(nsDependentAtomString(mKey.mAttributeName),
> +    nsCSSProps::LookupProperty(nsDependentAtomString(mKey.mAttributeName),
> -                                 CSSEnabledState::eForAllContent);
> +                               CSSEnabledState::eForAllContent);
> -    if (nsSMILCSSProperty::IsPropertyAnimatable(propId)) {
> -      return new nsSMILCSSProperty(propId, mKey.mElement.get());
> +  if (nsSMILCSSProperty::IsPropertyAnimatable(propID)) {
> +    // If we are animating the 'width' or 'height' of an outer SVG

I think we need to check here whether the attribute is mapped too.

a) mapped and animateabable -> same logic you already have i.e. test (nsSMILCSSProperty:IsPropertyAnimatable...) and return nsSMILCSSProperty or mKey.mElement->GetAnimatedAttr depending on the animateAsAttrTest

b) mapped and not animatable -> return null

c) not mapped -> return mKey.mElement->GetAnimatedAttr(mKey.mAttributeNamespaceID,

        Attachment #8846914 -
        Flags: review?(longsonr) → review-

    
    

    
  
There's an IsAttributeMapped(aName) method you can use (see nsSVGElement::GetAnimatedAttr before you removed that logic)

    
    

    
  
(In reply to Robert Longson from comment #59)
> Comment on attachment 8846914 [details]
> Bug 1062106 part 1 - Remove special handling of attributeType='XML'.
> 
> https://reviewboard.mozilla.org/r/119930/#review123728
> 
> ::: dom/smil/nsSMILCompositor.cpp:133
> (Diff revision 4)
> > -  if (mKey.mIsCSS) {
> > +  nsCSSPropertyID propID =
> > -    nsCSSPropertyID propId =
> > -      nsCSSProps::LookupProperty(nsDependentAtomString(mKey.mAttributeName),
> > +    nsCSSProps::LookupProperty(nsDependentAtomString(mKey.mAttributeName),
> > -                                 CSSEnabledState::eForAllContent);
> > +                               CSSEnabledState::eForAllContent);
> > -    if (nsSMILCSSProperty::IsPropertyAnimatable(propId)) {
> > -      return new nsSMILCSSProperty(propId, mKey.mElement.get());
> > +  if (nsSMILCSSProperty::IsPropertyAnimatable(propID)) {
> > +    // If we are animating the 'width' or 'height' of an outer SVG
> 
> I think we need to check here whether the attribute is mapped too.
> 
> a) mapped and animateabable -> same logic you already have i.e. test
> (nsSMILCSSProperty:IsPropertyAnimatable...) and return nsSMILCSSProperty or
> mKey.mElement->GetAnimatedAttr depending on the animateAsAttrTest
> 
> b) mapped and not animatable -> return null
> 
> c) not mapped -> return
> mKey.mElement->GetAnimatedAttr(mKey.mAttributeNamespaceID,

Sorry, long weekend here so I'm probably overlooking something obvious. What's the case that's missing?

I thought the logic we wanted was something like:

 It's a CSS property name that's animatable:
   width/height on outer SVG -> new nsSMILCSSProperty
   width/height on an HTML element -> new nsSMILCSSProperty
   width/height on other SVG element -> Call GetAnimatedAttr in case elem has an animatable width/height attribute

 It's a CSS property name that's not animatable:
   Call GetAnimatedAttr in case it also happens to be an animatable attribute

 It's not a CSS property name that's animatable:
   Call GetAnimatedAttr to see if it's an animatable attribute

Is the problem we might have mapped attribute names that don't correspond to CSS property names?

    
    

    
  
I thought a CSS property that's not animatable would need a null return to ensure we don't animate it (that's what we used to do). If you're sure that's not the case then it's fine to land as is.

    
    

    
  
Thanks Robert, Brian,

(In reply to Robert Longson from comment #62)
> I thought a CSS property that's not animatable would need a null return to
> ensure we don't animate it (that's what we used to do). If you're sure
> that's not the case then it's fine to land as is.

The current condition which we return a null on CreateSMILAttr is
mKey.isCSS == true && nsSMILCSSProperty::IsAnimatableProperty == false [1].

Then we need have the following condition to pass above condition:
1. Animate width or height of an HTML element [2].
2. Specify the attributeType='CSS' [2].

[1] https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/dom/smil/nsSMILCompositor.cpp#130-136
[2] https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/dom/smil/nsSMILAnimationController.cpp#703,712

The condition 1) is animating width or height attribute, this attribute is animatable, so we animate as CSS always. The condition 2) will disappear since we will remove attributeType. So I think we will not need to return a null on CreateSMILAttr. 

Could you point out my mistakes?

    
    

    
  
Condition 1 is satisfied by a couple of mapped attributes also i.e. direction and unicode-bidi. My concern is that these should not suddenly become animatable.

    
    

    
  
Regarding "a CSS property that's not animatable", I guess that means a situation where:

a) nsCSSProps::LookupProperty(mKey.mAttributeName) does *not* return eCSSProperty_UNKNOWN, and
b) nsSMILCSSProperty::IsPropertyAnimatable returns false.

Prior to this patch, if attributeType == auto we would set |isCSS| to false in that case since we
have code in nsSMILAnimationController::GetTargetIdentifierForAnimation that basically looks like
this:

  if (attributeType == eSMILTargetAttrType_auto) {
    nsCSSPropertyID prop = nsCSSProps::LookupProperty(attributeName);
    isCSS = nsSMILCSSProperty::IsPropertyAnimatable(prop);
  } else {
    isCSS = (attributeType == eSMILTargetAttrType_CSS);
  }

If attributeType == CSS we'd just set |isCSS| to true.

Then in CreateSMILAttr we have (again, prior to this patch):

  if (mKey.mIsCSS) {
    nsCSSPropertyID propId =
      nsCSSProps::LookupProperty(mKey.mAttributeName...);
    if (nsSMILCSSProperty::IsPropertyAnimatable(propId)) {
      return new nsSMILCSSProperty(propId...);
    }
  } else {
    return mKey.mElement->GetAnimatedAttr(...);
  }
  return nullptr;

So, for the attributeType == auto case, mIsCSS is false, so we'd just end up returning
mKey.mElement->GetAnimatedAttr(...).

For the attributeType == CSS case, we would return nullptr, however.

With this patch, we have roughly:

  nsCSSPropertyID propID =
    nsCSSProps::LookupProperty(mKey.mAttributeName...);
  if (nsSMILCSSProperty::IsPropertyAnimatable(propID)) {
    return new nsSMILCSSProperty(propID...);
  }

  return mKey.mElement->GetAnimatedAttr(mKey.mAttributeNamespaceID,

which means that we will end up returning mKey.mElement->GetAnimatedAttr(...) which is equivalent to
the attributeType == auto case from before.

In effect, this patch just makes us always apply the attributeType="auto" behaviour.

    
    

    
  
(In reply to Robert Longson from comment #64)
> Condition 1 is satisfied by a couple of mapped attributes also i.e.
> direction and unicode-bidi. My concern is that these should not suddenly
> become animatable.

Right, these will only suddenly become animatable if the content explicitly sets attributeType="CSS". If the content does not specify attributeType or specifies attributeType="auto", then they are already animated.

    
    

    
  
Comment on attachment 8846914 [details]
Bug 1062106 part 1 - Remove special handling of attributeType='XML'.

https://reviewboard.mozilla.org/r/119930/#review124296

::: commit-message-f9362:3
(Diff revision 4)
> +This patch removes handling of the 'attributeType' attribute so that we behave as if attributeType is always 'auto'. This means that for CSS properties we always animate them as CSS properties (i.e. we animate them as part of the SMIL override stylesheet) rather than mapped attributes.
> +
> +The one special case is width/height on an outer SVG. Previously we animated this as a mapped attribute since Web compatibility requires that the width/height on an outer SVG, when set explicitly, are mapped to style.
> +However, we can produce the same behavior by animating these as CSS properties (as opposed to mapped attributes). There is no observable difference in results returned by the SVG DOM APIs, only the level at which the result is added to the cascade: the SMIL override stylesheet instead of the attribute animation presentation hint level.
> +
> +As part of this patch, we animate width/height on outer SVG elements as CSS properties as opposed to mapped attributes both for consistency and also so we can remove the animated mapped attribute code altogether.

This lines should be wrapped to fit within 80 characters.

    
    

    
  
(In reply to Brian Birtles (:birtles) from comment #66)
> 
> Right, these will only suddenly become animatable if the content explicitly
> sets attributeType="CSS". If the content does not specify attributeType or
> specifies attributeType="auto", then they are already animated.

Which is perhaps an existing bug and therefore does not need to be fixed in this one.

    
    

    
  
(In reply to Robert Longson from comment #64)
> Condition 1 is satisfied by a couple of mapped attributes also i.e.
> direction and unicode-bidi. My concern is that these should not suddenly
> become animatable.

For the specific case of unicode-bidi and direction, as of this patch, there should be nothing in GetAnimatedAttr that matches these names (since we drop the mapped attribute case) so we should still effectively be returning null for those.

We could add an explicit check for LookupProperty() != eCSSProperty_UNKNOWN and !IsPropertyAnimatable() and make sure to return null in that case. However, I don't think that's the behavior we want. That's because if we ever introduce a non-SMIL-animatable CSS property that overlaps with an animatable attribute name, we would no longer be able to animate that attribute. Conversely, and more concretely, suppose we introduce a 'direction' attribute that *is* supposed to be animatable by SMIL, I think we would want to call GetAnimatedAttr for that despite the fact that LookupProperty() != eCSSProperty_UNKNOWN and !IsPropertyAnimatable().

    
    

    
  
(In reply to Brian Birtles (:birtles) from comment #69)
> (In reply to Robert Longson from comment #64)
> > Condition 1 is satisfied by a couple of mapped attributes also i.e.
> > direction and unicode-bidi. My concern is that these should not suddenly
> > become animatable.
> 
> For the specific case of unicode-bidi and direction, as of this patch, there
> should be nothing in GetAnimatedAttr that matches these names (since we drop
> the mapped attribute case) so we should still effectively be returning null
> for those.
> 

OK, I'm convinced.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
I guess the last r+ was through Bugzilla and MozReview lost it when you updated the patches :/

    
    

    
  
Comment on attachment 8846914 [details]
Bug 1062106 part 1 - Remove special handling of attributeType='XML'.

https://reviewboard.mozilla.org/r/119930/#review124330

        Attachment #8846914 -
        Flags: review?(longsonr) → review+

    
    

    
  
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a565aca3013c
part 1 - Remove special handling of attributeType='XML'. r=birtles,longsonr+218550
https://hg.mozilla.org/integration/autoland/rev/e77bfa57be61
part 2 - Remove unused SVG Attribute Animations cascade level. r=birtles,dbaron
https://hg.mozilla.org/integration/autoland/rev/7682b2da0437
part 3 - Remove SMIL MappedAttribute mechanism. r=birtles
https://hg.mozilla.org/integration/autoland/rev/7ac1fffb6a87
part 4 - Remove GetTargetAttributeType() from svg element. r=birtles

    
    

    
  
Thanks, Robert, Brian.

I landed it.

    
    

    
  
(In reply to Iris Hsiao [:ihsiao] from comment #79)
> backed out for build bustage like
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=85256460&repo=autoland&lineNumber=14771
> 
> https://hg.mozilla.org/integration/autoland/rev/63cbef012e2a

The cause of this bustage is that we removed the SVGAttrAnimationRuleProcessor. And the include order of unified build was misaligned.
The ServoMediaRule use the ServoMediaList header file, but it doesn't include this header. So we will need to specify this header files.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3f80468ecc5afeabd07c0a76e7a5f062fd5df51
Flags: needinfo?(mantaroh)

    
    

    
  
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #80)
> (In reply to Iris Hsiao [:ihsiao] from comment #79)
> > backed out for build bustage like
> > https://treeherder.mozilla.org/logviewer.
> > html#?job_id=85256460&repo=autoland&lineNumber=14771
> > 
> > https://hg.mozilla.org/integration/autoland/rev/63cbef012e2a
> 
> The cause of this bustage is that we removed the
> SVGAttrAnimationRuleProcessor. And the include order of unified build was
> misaligned.
> The ServoMediaRule use the ServoMediaList header file, but it doesn't
> include this header. So we will need to specify this header files.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f3f80468ecc5afeabd07c0a76e7a5f062fd5df51

I've overlooked the signs of servo debug build failure.

linux 64 stylo debug build failed.
https://treeherder.mozilla.org/logviewer.html#?job_id=85462093&repo=try&lineNumber=15630

---------------------------
INFO -  Executing /home/worker/workspace/build/src/obj-firefox/dist/bin/xpcshell -g /home/worker/workspace/build/src/obj-firefox/dist/bin/ -a /home/worker/workspace/build/src/obj-firefox/dist/bin/ -f /home/worker/workspace/build/src/toolkit/mozapps/installer/precompile_cache.js -e precompile_startupcache("resource://gre/");
INFO -  thread '<unnamed>' panicked at 'assertion failed: `(left == right)` (left: `64`, right: `128`): RESTYLE_STYLE_ATTRIBUTE', /home/worker/workspace/build/src/servo/components/style/restyle_hints.rs:76
---------------------------

In this file, we check the restyle hint value. So I should modify the restyle hint value in this file.[1]

[1] https://hg.mozilla.org/mozilla-central/annotate/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/servo/components/style/restyle_hints.rs#l53

I retry: https://treeherder.mozilla.org/#/jobs?repo=try&revision=892cfc1370a3ab3a905fc9447978542b4eb8dc85

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #81)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #80)
> > (In reply to Iris Hsiao [:ihsiao] from comment #79)
> > > backed out for build bustage like
> > > https://treeherder.mozilla.org/logviewer.
> > > html#?job_id=85256460&repo=autoland&lineNumber=14771
> > > 
> > > https://hg.mozilla.org/integration/autoland/rev/63cbef012e2a
> > 
> > The cause of this bustage is that we removed the
> > SVGAttrAnimationRuleProcessor. And the include order of unified build was
> > misaligned.
> > The ServoMediaRule use the ServoMediaList header file, but it doesn't
> > include this header. So we will need to specify this header files.
> > 
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=f3f80468ecc5afeabd07c0a76e7a5f062fd5df51
> 
> I've overlooked the signs of servo debug build failure.
> 
> linux 64 stylo debug build failed.
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=85462093&repo=try&lineNumber=15630
> 
> ---------------------------
> INFO -  Executing
> /home/worker/workspace/build/src/obj-firefox/dist/bin/xpcshell -g
> /home/worker/workspace/build/src/obj-firefox/dist/bin/ -a
> /home/worker/workspace/build/src/obj-firefox/dist/bin/ -f
> /home/worker/workspace/build/src/toolkit/mozapps/installer/precompile_cache.
> js -e precompile_startupcache("resource://gre/");
> INFO -  thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
> (left: `64`, right: `128`): RESTYLE_STYLE_ATTRIBUTE',
> /home/worker/workspace/build/src/servo/components/style/restyle_hints.rs:76
> ---------------------------
> 
> In this file, we check the restyle hint value. So I should modify the
> restyle hint value in this file.[1]
> 
> [1]
> https://hg.mozilla.org/mozilla-central/annotate/
> 05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/servo/components/style/
> restyle_hints.rs#l53
> 
> I retry:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=892cfc1370a3ab3a905fc9447978542b4eb8dc85

xidorn,

I modified the code related with servo because I removed the SVGAttrAnimationRuleProcessor.
Could you please confirm this change?

    
    

    
  
Comment on attachment 8846915 [details]
Bug 1062106 part 2 - Remove unused SVG Attribute Animations cascade level.

https://reviewboard.mozilla.org/r/119932/#review125150

The Servo change looks fine. Note that you need to strip that part before landing it in Gecko, and you need to land the Servo change separately as a Servo pull request.

In general, we land Servo pull request first, and when the pull request gets merged, we land the Gecko change immediately to minimize the bustage on Stylo.

        Attachment #8846915 -
        Flags: review?(xidorn+moz) → review+

    
    

    
  
Thanks, xidorn

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #87)
> Comment on attachment 8846915 [details]
> Bug 1062106 part 2 - Remove unused SVG Attribute Animations cascade level.
> 
> https://reviewboard.mozilla.org/r/119932/#review125150
> 
> The Servo change looks fine. Note that you need to strip that part before
> landing it in Gecko, and you need to land the Servo change separately as a
> Servo pull request.
> 
> In general, we land Servo pull request first, and when the pull request gets
> merged, we land the Gecko change immediately to minimize the bustage on
> Stylo.

OK.
I decided that I leave the declaration of SVGAttrAnimations hint on gecko, and I'll remove this restyle hint declaration together with a servo at another bug.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #93)
> I leaved the SVGAttrAnimations declartion.
> 
> try
> :https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0b0598617ae5526cdb2577fda57ae0a6653dfe24

Oh, I forgot the remove the test from reftest-stylo.list.
fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=536a578142bcd4b9e3d478347e0fba7787672fc8

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/00c615691a6b
part 1 - Remove special handling of attributeType='XML'. r=birtles,longsonr+218550
https://hg.mozilla.org/integration/autoland/rev/b55c9e8adf4f
part 2 - Remove unused SVG Attribute Animations cascade level. r=birtles,dbaron,xidorn
https://hg.mozilla.org/integration/autoland/rev/b63a6a20dea6
part 3 - Remove SMIL MappedAttribute mechanism. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d307e9e3d791
part 4 - Remove GetTargetAttributeType() from svg element. r=birtles

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: