Last Comment Bug 607854 - SVGTests interface is not implemented
: SVGTests interface is not implemented
Status: RESOLVED FIXED
ietestcenter
: dev-doc-complete
Product: Core
Classification: Components
Component: SVG (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: mozilla12
Assigned To: Robert Longson
:
Mentors:
Depends on: 441464
Blocks: svg11tests 441487 ietestcenter 652050 711958
  Show dependency treegraph
 
Reported: 2010-10-27 19:13 PDT by Cameron McCormack (:heycam)
Modified: 2013-02-06 01:09 PST (History)
9 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch with tests (170.08 KB, patch)
2011-11-16 02:36 PST, Robert Longson
no flags Details | Diff | Splinter Review
address review comments (175.28 KB, patch)
2011-12-04 10:07 PST, Robert Longson
no flags Details | Diff | Splinter Review
complete non-conditional processing string list infrastructure (176.75 KB, patch)
2011-12-08 12:43 PST, Robert Longson
jwatt: review+
Details | Diff | Splinter Review
update to tip (179.31 KB, patch)
2011-12-29 14:13 PST, Robert Longson
no flags Details | Diff | Splinter Review

Description Cameron McCormack (:heycam) 2010-10-27 19:13:27 PDT
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 2010-10-28 06:18:25 PDT
We need a nsSVGStringList class similar to the nsSVGLengthList class.
Comment 2 Cameron McCormack (:heycam) 2010-12-19 13:56:38 PST
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 2011-11-16 02:36:41 PST
Created attachment 574854 [details] [diff] [review]
patch with tests
Comment 4 Robert Longson 2011-11-16 02:46:41 PST
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) 2011-11-16 02:59:57 PST
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 2011-11-16 04:32:15 PST
(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] (catching up after vacation) 2011-11-20 09:59:39 PST
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 2011-12-04 10:07:21 PST
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.


> ::: 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 2011-12-08 12:43:44 PST
Created attachment 580150 [details] [diff] [review]
complete non-conditional processing string list infrastructure
Comment 10 Jonathan Watt [:jwatt] (catching up after vacation) 2011-12-19 06:40:02 PST
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 2011-12-19 06:43:43 PST
On the whole, I'd rather land it in 12.
Comment 12 Jonathan Watt [:jwatt] (catching up after vacation) 2011-12-19 06:54:02 PST
Okay.
Comment 13 Mozilla RelEng Bot 2011-12-21 03:00:58 PST
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/longsonr@gmail.com-9b381f8c0eda
Comment 14 Mozilla RelEng Bot 2011-12-21 04:30:30 PST
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/longsonr@gmail.com-72cfbf4932d8
Comment 15 Mozilla RelEng Bot 2011-12-22 12:30:48 PST
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/longsonr@gmail.com-f69ce23e119f
Comment 16 Robert Longson 2011-12-29 14:13:55 PST
Created attachment 584849 [details] [diff] [review]
update to tip
Comment 18 Phil Ringnalda (:philor, back in August) 2011-12-31 19:06:29 PST
https://hg.mozilla.org/mozilla-central/rev/883815d2edb2
Comment 19 Jeremie Patonnier :Jeremie 2012-01-01 08:19:40 PST
Documented here : https://developer.mozilla.org/en/DOM/SVGTests#section_5
and there : https://developer.mozilla.org/en/Firefox_12_for_developers

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