SVGTests interface is not implemented

RESOLVED FIXED in mozilla12

Status

()

Core
SVG
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: heycam, Assigned: Robert Longson)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla12
x86
Windows 7
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: ietestcenter)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
Blocks: 554013
Whiteboard: ietestcenter
(Assignee)

Comment 1

7 years ago
We need a nsSVGStringList class similar to the nsSVGLengthList class.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

6 years ago
http://dev.w3.org/SVG/profiles/1.1F2/test/svg/types-dom-06-f.svg also fails, which is an accepted test.
Blocks: 512501
(Assignee)

Comment 3

6 years ago
Created attachment 574854 [details] [diff] [review]
patch with tests
Assignee: nobody → longsonr
Attachment #574854 - Flags: review?(jwatt)
(Assignee)

Comment 4

6 years ago
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.
(Reporter)

Comment 5

6 years ago
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)?
(Assignee)

Comment 6

6 years ago
(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 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.
(Assignee)

Comment 8

6 years ago
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.
Attachment #574854 - Attachment is obsolete: true
Attachment #578916 - Flags: review?(jwatt)
Attachment #574854 - Flags: review?(jwatt)
(Assignee)

Comment 9

5 years ago
Created attachment 580150 [details] [diff] [review]
complete non-conditional processing string list infrastructure
Attachment #578916 - Attachment is obsolete: true
Attachment #580150 - Flags: review?(jwatt)
Attachment #578916 - Flags: review?(jwatt)

Updated

5 years ago
Blocks: 711958
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?
Attachment #580150 - Flags: review?(jwatt) → review+
(Assignee)

Comment 11

5 years ago
On the whole, I'd rather land it in 12.
Okay.

Comment 13

5 years ago
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

5 years ago
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

5 years ago
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
(Assignee)

Comment 16

5 years ago
Created attachment 584849 [details] [diff] [review]
update to tip
Attachment #580150 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/883815d2edb2
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla12
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
(Assignee)

Updated

5 years ago
Blocks: 652050
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/883815d2edb2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Documented here : https://developer.mozilla.org/en/DOM/SVGTests#section_5
and there : https://developer.mozilla.org/en/Firefox_12_for_developers
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

5 years ago
Blocks: 441487
(Assignee)

Updated

4 years ago
Depends on: 441464
You need to log in before you can comment on or make changes to this bug.