607854 - SVGTests interface is not implemented

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Cameron McCormack (:heycam)]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6
Build Identifier: 

None of the members of the SVGTests interface are implemented.
http://www.w3.org/TR/SVG/types.html#InterfaceSVGTests

This causes this IE Test Center test to fail:
http://samples.msdn.microsoft.com/ietestcenter/svg112/svg_harness.htm?url=./svg112/svg/chapter_04.1.svg

and its corresponding one in the official SVG 1.1 2ed test suite (reviewed, not yet accepted):

http://dev.w3.org/SVG/profiles/1.1F2/test/svg/types-dom-svgstringlist-01-f.svg


Reproducible: Always

Comment 1

[Robert Longson [:longsonr]]

We need a nsSVGStringList class similar to the nsSVGLengthList class.

Comment 2

[Cameron McCormack (:heycam)]

http://dev.w3.org/SVG/profiles/1.1F2/test/svg/types-dom-06-f.svg also fails, which is an accepted test.

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch with tests

Comment 4

[Robert Longson [:longsonr]]

Links in comment 0 and comment 2 pass with this patch.

Does not implement bug 631437 since stringList is a list of strings rather than a list of anim/baseval objects. I'd need to implement the following method...

nsAString&
DOMSVGTests::GetItemWithoutAddRef(PRUint32 aIndex)

And what would I return if aIndex was outside the list bounds. Unlike the other lists nsnull is not an option.

Comment 5

[Cameron McCormack (:heycam)]

What about having a static nsAString "" that you return, and making DOMSVGStringList::GetItem check if GetItemWithoutAddRef returns exactly that object (so that it can return NS_ERROR_DOM_INDEX_SIZE_ERR)?

Comment 6

[Robert Longson [:longsonr]]

(In reply to Cameron McCormack (:heycam) from comment #5)
> What about having a static nsAString "" that you return, and making
> DOMSVGStringList::GetItem check if GetItemWithoutAddRef returns exactly that
> object (so that it can return NS_ERROR_DOM_INDEX_SIZE_ERR)?

Possibly, I'd want to punt that to a follow up though.

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 574854 [details] [diff] [review]
patch with tests

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

Really great job on this Robert! I've still to review the tests, but here are my thoughts on the code changes...

Random passing thought: it would be nice if we didn't have to QI to DOMSVGTests internally, but I guess that doesn't matter too much.

::: content/svg/content/src/DOMSVGStringList.cpp
@@ +81,5 @@
> +}
> +
> +DOMSVGStringList::~DOMSVGStringList()
> +{
> +  // Script no longer has any references to us, to our value object.

Should just drop the "to our value object".

@@ +101,5 @@
> +{
> +  if (mVal->Length() > 0) {
> +    mVal->Clear();
> +    mElement->DidChangeStringList(mAttrEnum, true);
> +  }

Just call Clear() here.

::: content/svg/content/src/DOMSVGStringList.h
@@ +57,5 @@
> + *
> + * See the architecture comment in DOMSVGAnimatedLengthList.h.
> + *
> + * Our DOM items are created lazily on demand as and when script requests them.
> + */

Please make this comment:

/**
 * Class DOMSVGStringList
 *
 * This class is used to create the DOM tearoff objects that wrap internal
 * SVGPathData objects.
 *
 * See the architecture comment in DOMSVGAnimatedLengthList.h first (that's
 * LENGTH list), then continue reading the remainder of this comment.
 *
 * The architecture of this class is similar to that of DOMSVGLengthList
 * except for two important aspects:
 *
 * First, since there is no nsIDOMSVGAnimatedStringList interface in SVG, we
 * have no parent DOMSVGAnimatedStringList (unlike DOMSVGLengthList which has
 * a parent DOMSVGAnimatedLengthList class). As a consequence, much of the
 * logic that would otherwise be in DOMSVGAnimatedStringList (and is in
 * DOMSVGAnimatedLengthList) is contained in this class.
 *
 * Second, since there is no nsIDOMSVGString interface in SVG, we have no
 * DOMSVGString items to maintain. As far as script is concerned, objects
 * of this class contain a list of strings, not a list of mutable objects
 * like the other SVG list types. As a result, unlike the other SVG list
 * types, this class does not create its items lazily on demand and store
 * them so it can return the same objects each time. It simply returns a new
 * string each time any given item is requested.
 */

@@ +96,5 @@
> +
> +  // Weak refs to our DOMSVGStringList object. This object is a
> +  // friend and takes care of clearing this pointer when it dies, making
> +  // these true weak references.
> +  SVGStringList *mVal;

The SVGStringList does not clear this pointer. It doesn't need to since it can't die until we die (due to our mElement strong ref).

In any case, I don't think we should have this member. For consistency with the other SVG list classes, I think we should have a InternalList() method instead.

@@ +99,5 @@
> +  // these true weak references.
> +  SVGStringList *mVal;
> +
> +  // Strong ref to our element to keep it alive. We hold this not only for
> +  // ourself, but also for our value and all of its items.

This should just be "Strong ref to our element to keep it alive." The rest of the comment does not apply to this list type.

::: content/svg/content/src/DOMSVGTests.cpp
@@ +99,5 @@
> +  for (PRUint32 i = 0; i < ArrayLength(sStringListInfo); i++) {
> +    if (aAttribute == *sStringListInfo[i].mName) {
> +      return true;
> +    }
> +  }

This seems like a bit of a hazard. I can see someone adding support for a new string list attribute in future, and not being aware that here we consider all string lists to be conditional processing attributes.

::: content/svg/content/src/DOMSVGTests.h
@@ +93,5 @@
> +
> +  /**
> +   * Serialises the conditional processing attribute.
> +   */
> +  nsIAtom* GetValue(PRUint8 aAttrEnum, nsAString& aValue) const;

I'm not really liking the way this returns an arbitrary piece of information. How about Calling this GetValueAndAttrName and make it an out param? (Otherwise a comment would really be needed saying what this magic atom being returned is.

@@ +98,5 @@
> +
> +  /**
> +   * Unsets a conditional processing attribute.
> +   */
> +  PRUint32 ClearValue(const nsIAtom* aAttribute);

As stated below, I'm not sure we need this method, but if so it would be good to add a line about what the PRUint32 value that it returns is. (Also again, I'm not a fan of returning a random piece of info that seems unrelated to the method name.)

::: content/svg/content/src/SVGStringList.cpp
@@ +71,5 @@
> +    if (i != last) {
> +      if (aIsCommaSeparated) {
> +        aValue.Append(',');
> +      }
> +      aValue.Append(' ');

Slightly more compact and efficient as:

  aValue.Append(aIsCommaSeparated ? ', ': ' ');

::: content/svg/content/src/nsSVGElement.cpp
@@ -285,5 @@
> -    if (parent &&
> -        parent->NodeInfo()->Equals(nsGkAtoms::svgSwitch, kNameSpaceID_SVG)) {
> -      static_cast<nsSVGSwitchElement*>(parent)->MaybeInvalidate();
> -    }
> -  }

Note to self: check that there's an invalidation test for this.

@@ +726,5 @@
> +    nsCOMPtr<DOMSVGTests> tests(do_QueryInterface(this));
> +    if (tests && tests->IsConditionalProcessingAttribute(aName)) {
> +      PRUint32 i = tests->ClearValue(aName);
> +      DidChangeStringList(i, false);
> +      return;

Seems like DOMTests::ClearValue doesn't do anything special, and just resets the string lists. Why don't you add a GetStringListInfo() method and iterate through the string lists attributes looking for one to reset like we do with the other list types?

::: content/svg/content/src/nsSVGFeatures.h
@@ +52,5 @@
>     * @param aFeature one of the feature strings specified at
>     *    http://www.w3.org/TR/SVG11/feature.html
>     */
>    static bool
> +  HasFeature(nsISupports* aObject, const nsAString& aFeature);

Seems to me that "have" is right, since we're talking about "do we/does our implementation have this feature?" *shrug*

::: content/svg/content/src/nsSVGSwitchElement.cpp
@@ -178,5 @@
> -      if (nsSVGFeatures::PassesConditionalProcessingTests(
> -            child, nsSVGFeatures::kIgnoreSystemLanguage)) {
> -        nsAutoString value;
> -        if (child->GetAttr(kNameSpaceID_None, nsGkAtoms::systemLanguage,
> -                           value)) {

I think removing this check is right. The spec says that if the attribute is not set, the element matches.

@@ +207,1 @@
>        }

Similarly, this seems wrong to me. We need that check where it was to get the "if the attribute is not set, it matches" behavior. And it's not clear to me that if a child of a switch does not support the SVGTests interface that we should be using that as the matching child.

@@ +215,5 @@
> +    if (!child->IsElement()) {
> +      continue;
> +    }
> +    nsCOMPtr<DOMSVGTests> tests(do_QueryInterface(child));
> +    if (!tests || tests->PassesConditionalProcessingTests(&acceptLangs)) {

Again, I'm not sure why we're returning the child if it doesn't support SVGTests.

::: dom/interfaces/svg/nsIDOMSVGStringList.idl
@@ +39,5 @@
> +
> +[scriptable, uuid(481f01a5-0bbb-4abf-8623-f3c2fb5642a9)]
> +interface nsIDOMSVGStringList : nsISupports
> +{ 
> +  readonly attribute unsigned long numberOfItems;

Need to add the line:

  readonly attribute unsigned long length;  // synonym for numberOfItems

as the other list types have.

Comment 8

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to Jonathan Watt [:jwatt] from comment #7)
> @@ +101,5 @@
> > +{
> > +  if (mVal->Length() > 0) {
> > +    mVal->Clear();
> > +    mElement->DidChangeStringList(mAttrEnum, true);
> > +  }
> 
> Just call Clear() here.

We need the DidChangeStringList as this is an external DOM update and the object may need to redraw. I've left this pretty much as is.


> ::: content/svg/content/src/DOMSVGTests.cpp
> @@ +99,5 @@
> > +  for (PRUint32 i = 0; i < ArrayLength(sStringListInfo); i++) {
> > +    if (aAttribute == *sStringListInfo[i].mName) {
> > +      return true;
> > +    }
> > +  }
> 
> This seems like a bit of a hazard. I can see someone adding support for a
> new string list attribute in future, and not being aware that here we
> consider all string lists to be conditional processing attributes.

DOMSVGTests is the conditional processing class so this belongs here. Other string list attributes would be processed elsewhere. I've added some basic infrastructure for that now.

> 
> I'm not really liking the way this returns an arbitrary piece of
> information. How about Calling this GetValueAndAttrName and make it an out
> param? (Otherwise a comment would really be needed saying what this magic
> atom being returned is.

I rewrote this.

> 
> Slightly more compact and efficient as:
> 
>   aValue.Append(aIsCommaSeparated ? ', ': ' ');

Unfortunately that doesn't compile as character literals can only have one character and if you make it a string and change to AppendLiteral it doesn't compile either so I've left this as is.

> Note to self: check that there's an invalidation test for this.

There is. dynamic-switch-xx I think.

> 
> Seems to me that "have" is right, since we're talking about "do we/does our
> implementation have this feature?" *shrug*

Yes, but the SVG DOM interface is HasFeature so I changed it to be consistent with that.

> I think removing this check is right. The spec says that if the attribute is
> not set, the element matches.

IsExplicitlySet is the equivalent code now. This functionality hasn't changed.

> Similarly, this seems wrong to me. We need that check where it was to get
> the "if the attribute is not set, it matches" behavior. And it's not clear
> to me that if a child of a switch does not support the SVGTests interface
> that we should be using that as the matching child.

That's what we used to do.

> 
> Again, I'm not sure why we're returning the child if it doesn't support
> SVGTests.

We used to do that so I'm not changing this.

> 
> Need to add the line:
> 
>   readonly attribute unsigned long length;  // synonym for numberOfItems
> 
> as the other list types have.

That will need to be a follow up. See previous comments.

Comment 9

[Robert Longson [:longsonr]]

Attachment: complete non-conditional processing string list infrastructure

Comment 10

[Jonathan Watt [:jwatt]]

Comment on attachment 580150 [details] [diff] [review]
complete non-conditional processing string list infrastructure

r=jwatt

(In reply to Robert Longson from comment #8)
> Created attachment 578916 [details] [diff] [review]
> address review comments
> 
> (In reply to Jonathan Watt [:jwatt] from comment #7)
> > @@ +101,5 @@
> > > +{
> > > +  if (mVal->Length() > 0) {
> > > +    mVal->Clear();
> > > +    mElement->DidChangeStringList(mAttrEnum, true);
> > > +  }
> > 
> > Just call Clear() here.
> 
> We need the DidChangeStringList as this is an external DOM update and the
> object may need to redraw. I've left this pretty much as is.

Sorry, I put that comment at the wrong point. I meant to put it after the mVal->Clear() call in Initialize, but never mind, this was just a nit for consistency with the other DOM classes.

> > Again, I'm not sure why we're returning the child if it doesn't support
> > SVGTests.
> 
> We used to do that so I'm not changing this.

Ah, I misread the:

  if (!ElementSupportsAttributes(aContent->Tag(), ATTRS_CONDITIONAL)) {
    return true;
  }

in nsSVGFeatures::PassesConditionalProcessingTests as returning false. Never mind that comment then!

Do you want me to try and land this today? If so, have you run it through Try, or do I need to do that?

Comment 11

[Robert Longson [:longsonr]]

On the whole, I'd rather land it in 12.

Comment 12

[Jonathan Watt [:jwatt]]

Okay.

Comment 13

[Mozilla RelEng Bot]

Try run for 9b381f8c0eda is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9b381f8c0eda
Results (out of 19 total builds):
    success: 15
    warnings: 2
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9b381f8c0eda

Comment 14

[Mozilla RelEng Bot]

Try run for 72cfbf4932d8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=72cfbf4932d8
Results (out of 26 total builds):
    success: 17
    warnings: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-72cfbf4932d8

Comment 15

[Mozilla RelEng Bot]

Try run for f69ce23e119f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f69ce23e119f
Results (out of 27 total builds):
    success: 20
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f69ce23e119f

Comment 16

[Robert Longson [:longsonr]]

Attachment: update to tip

Comment 17

[Robert Longson [:longsonr]]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/883815d2edb2

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/883815d2edb2

Comment 19

[Jeremie Patonnier :Jeremie]

Documented here : https://developer.mozilla.org/en/DOM/SVGTests#section_5
and there : https://developer.mozilla.org/en/Firefox_12_for_developers