Closed Bug 823394 Opened 7 years ago Closed 7 years ago

Convert SVGElement to WebIDL bindings

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(3 files)

No description provided.
Attachment #694717 - Flags: review?(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?
(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.
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 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
Attachment #694703 - Flags: review?(bzbarsky) → review+
(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 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....
Attachment #694717 - Flags: review?(bzbarsky) → review-
> Isn't there a difference when testing SVGElement.getPresentationAttribute?

Yes.  OK, let's leave it and file a followup bug on removing.
Oh, one more thing.  It looks like I was wrong about whether you need the NEW_BINDING bits.  See bug 821606 comment 9.
(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.
Attached patch Part 2 interdiffSplinter Review
Attachment #695104 - Flags: review?(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 on attachment 695104 [details] [diff] [review]
Part 2 interdiff

r=me modulo the xmlspace thing
Attachment #695104 - Flags: review?(bzbarsky) → review+
Blocks: 824229
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.
Blocks: 824327
(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
https://hg.mozilla.org/mozilla-central/rev/3b56b12b4a70
https://hg.mozilla.org/mozilla-central/rev/dc2abccc2adb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Just realized: this patch should have removed the nsIDOMSVGElement quickstubs we already have.  David, want to do that?
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
It could be depending on what those tests are testing.

Have there been reductions since as subclasses have gotten converted?
You need to log in before you can comment on or make changes to this bug.