823394 - Convert SVGElement to WebIDL bindings

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Part 1: Merge nsSVGStylableElement/nsSVGElement and nsIDOMSVGStylable/nsIDOMSVGElement

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Add WebIDL API and enable binding

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 694703 [details] [diff] [review]
Part 1: Merge nsSVGStylableElement/nsSVGElement and nsIDOMSVGStylable/nsIDOMSVGElement

Hmm.  This will make it so there is no longer an SVGStylable interface visible in script, right?  Do other UAs have such an interface visible?  Do SVG elements claim to be instances of SVGStylable in other UAs?

Comment 4

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Comment on attachment 694703 [details] [diff] [review]
> Part 1: Merge nsSVGStylableElement/nsSVGElement and
> nsIDOMSVGStylable/nsIDOMSVGElement
> 
> Hmm.  This will make it so there is no longer an SVGStylable interface
> visible in script, right?  Do other UAs have such an interface visible?  Do
> SVG elements claim to be instances of SVGStylable in other UAs?

Neither Webkit nor Chrome have SVGStylable.

Comment 5

[Cameron McCormack (:heycam)]

SVGStylable was one of the multiply inherited interfaces, and there are no constants on it that people would have been directly getting off the interface object, so I think it's fine to remove.  (SVGStylable has just been merged directly into SVGElement in SVG 2.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 694703 [details] [diff] [review]
Part 1: Merge nsSVGStylableElement/nsSVGElement and nsIDOMSVGStylable/nsIDOMSVGElement

OK.  Watch out for tests that test for class and style NOT being present on some SVG elements!

You need to rev the nsIDOMSVGElement IID.

If we don't plan to do getPresentationAttribute, why not just leave it off?  That should have the same "throws when called" effect... ;)

Shouldn't the custom GetStyle quickstub be fixed to use nsSVGElement instead of being removed?

r=me with those nits

Comment 7

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> 
> If we don't plan to do getPresentationAttribute, why not just leave it off? 
> That should have the same "throws when called" effect... ;)

Isn't there a difference when testing SVGElement.getPresentationAttribute?
I wanted to preserve existing behavior but I can remove it if it doesn't matter

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 694717 [details] [diff] [review]
Part 2: Add WebIDL API and enable binding

>+++ b/content/svg/content/src/nsSVGElement.cpp
>+already_AddRefed<nsICSSDeclaration>
>+nsSVGElement::GetStyle(ErrorResult& rv)

Why not return nsICSSDeclaration* here instead?

>+NS_IMETHODIMP nsSVGElement::GetXmlspace(nsAString & aXmlspace)

I'd rather we added stuff like this in a separate bug, not least because it's not clear to me whether we want to add it.... and if we do, I'm not the right reviewer.

> nsSVGElement::GetOwnerSVGElement(nsIDOMSVGSVGElement * *aOwnerSVGElement)
>-  if (*aOwnerSVGElement || Tag() == nsGkAtoms::svg) {
>+  if (!ownerSVGElement || (ownerSVGElement && Tag() != nsGkAtoms::svg)) {

If you meant to just invert the test, wouldn't it become:

  if (!ownerSVGElement && Tag() != nsGkAtoms::svg)

?  Why did you change it to what you changed it to?

>+already_AddRefed<nsSVGElement>
>+nsSVGElement::GetViewportElement()
>+{
>+  nsCOMPtr<nsSVGElement> elem =
>+    do_QueryInterface(SVGContentUtils::GetNearestViewportElement(this).get());
>+  return elem.get();

Even if we posit that the QI there is kosher (which it probably is), this code first leaks a ref to the element then return a non-addrefed ptr as an addreffed one.  The two cancel out, but I'd really rather not have someone come along and fix one of those thing but not the other.  Please just rewrite this sanely?

>+already_AddRefed<nsIDOMSVGAnimatedString>
>+nsSVGElement::ClassName()

Any reason GetClassName doesn't call this?

>+nsSVGElement::GetXmlbase(nsAString & aXMLBase)

Again, please separate bug for adding things.  Note that you added this one in webidl but not XPIDL, unlike xmlspace.

>+nsSVGElement::GetXmllang(nsAString & aXMLLang)

And this.

I'd like to see an updated patch, and ideally an interdiff, here....

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Isn't there a difference when testing SVGElement.getPresentationAttribute?

Yes.  OK, let's leave it and file a followup bug on removing.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, one more thing.  It looks like I was wrong about whether you need the NEW_BINDING bits.  See bug 821606 comment 9.

Comment 11

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> Comment on attachment 694717 [details] [diff] [review]
> 
> >+NS_IMETHODIMP nsSVGElement::GetXmlspace(nsAString & aXmlspace)
> 
> I'd rather we added stuff like this in a separate bug, not least because
> it's not clear to me whether we want to add it.... and if we do, I'm not the
> right reviewer.

This was already implemented, I just moved it from nsIDOMSVGStyleElement
 
> > nsSVGElement::GetOwnerSVGElement(nsIDOMSVGSVGElement * *aOwnerSVGElement)
> >-  if (*aOwnerSVGElement || Tag() == nsGkAtoms::svg) {
> >+  if (!ownerSVGElement || (ownerSVGElement && Tag() != nsGkAtoms::svg)) {
> 
> If you meant to just invert the test, wouldn't it become:
> 
>   if (!ownerSVGElement && Tag() != nsGkAtoms::svg)
> 
> ?  Why did you change it to what you changed it to?

No idea why I did that.

> 
> Again, please separate bug for adding things.  Note that you added this one
> in webidl but not XPIDL, unlike xmlspace.

I'll file a followup for these two.

Comment 12

[David Zbarsky (:dzbarsky)]

Attachment: Part 2 interdiff

Comment 13

[Boris Zbarsky [:bzbarsky]]

> This was already implemented, I just moved it from nsIDOMSVGStyleElement

Right, so it was implemented only on <svg:style>...  and you moved it to all SVG elements.  Separate bug, I say.  ;)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 695104 [details] [diff] [review]
Part 2 interdiff

r=me modulo the xmlspace thing

Comment 15

[Robert Longson [:longsonr]]

Comment on attachment 694703 [details] [diff] [review]
Part 1: Merge nsSVGStylableElement/nsSVGElement and nsIDOMSVGStylable/nsIDOMSVGElement

> nsresult
> nsSVGElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
>                          nsIContent* aBindingParent,
>                          bool aCompileEventHandlers)
> {
>   nsresult rv = nsSVGElementBase::BindToTree(aDocument, aParent,
>                                              aBindingParent,
>                                              aCompileEventHandlers);
>@@ -267,16 +330,21 @@ nsSVGElement::ParseAttribute(int32_t aNa
>                              const nsAString& aValue,
>                              nsAttrValue& aResult)
> {
>   nsresult rv = NS_OK;
>   bool foundMatch = false;
>   bool didSetResult = false;
> 
>   if (aNamespaceID == kNameSpaceID_None) {
>+    if (aAttribute == nsGkAtoms::_class) {
>+      mClassAttribute.SetBaseValue(aValue, this, false);
>+      aResult.ParseAtomArray(aValue);
>+      return true;
>+    }

Should probably go at the end of the if (aNamespaceID == kNameSpaceID_None) { as class animation is pretty rare.

> void
> nsSVGElement::UnsetAttrInternal(int32_t aNamespaceID, nsIAtom* aName,
>                                 bool aNotify)
> {
>   // XXXbz there's a bunch of redundancy here with AfterSetAttr.
>   // Maybe consolidate?
> 
>   if (aNamespaceID == kNameSpaceID_None) {
>+    if (aName == nsGkAtoms::_class) {
>+      mClassAttribute.Init();
>+      return;
>+    }
>+

Move further down but withing the same if (aNamespaceID == kNameSpaceID_None) { section please.

> 
> nsISMILAttr*
> nsSVGElement::GetAnimatedAttr(int32_t aNamespaceID, nsIAtom* aName)
> {
>   if (aNamespaceID == kNameSpaceID_None) {
>+    if (aName == nsGkAtoms::_class) {
>+      return mClassAttribute.ToSMILAttr(this);
>+    }
>+

Here too.

Comment 16

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3051733fec
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5a733e72673

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> Comment on attachment 694703 [details] [diff] [review]
> Part 1: Merge nsSVGStylableElement/nsSVGElement and
> nsIDOMSVGStylable/nsIDOMSVGElement
> 
> OK.  Watch out for tests that test for class and style NOT being present on
> some SVG elements!

Such as layout/reftests/svg/style-property-not-on-script-element-01.svg:

https://tbpl.mozilla.org/php/getParsedLog.php?id=18218203&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218232&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18219952&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18219847&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218712&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218249&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18219826&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218519&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218568&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218343&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218576&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218373&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218487&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218209&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218450&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18221402&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218727&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218522&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218496&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18221539&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18221495&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218758&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218470&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18218638&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/d81ab45d2a7d

Comment 18

[David Zbarsky (:dzbarsky)]

Thanks, I fixed the test.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b56b12b4a70
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc2abccc2adb

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b56b12b4a70
https://hg.mozilla.org/mozilla-central/rev/dc2abccc2adb

Comment 20

[Boris Zbarsky [:bzbarsky]]

Just realized: this patch should have removed the nsIDOMSVGElement quickstubs we already have.  David, want to do that?

Comment 21

[Florian Bender]

FYI, Part 2 [1] (among other changesets) caused a big spike (build bf26f61a0748 .. 0bb4773db082) on most AWSY tests. Is this expected?

[1] https://hg.mozilla.org/mozilla-central/rev/dc2abccc2adb

Comment 22

[Boris Zbarsky [:bzbarsky]]

It could be depending on what those tests are testing.

Have there been reductions since as subclasses have gotten converted?