1423492 - Move side effects from SVG attribute unsetting to BeforeSetAttr / AfterSetAttr, and devirtualize UnsetAttr

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Emilio Cobos Álvarez (:emilio)]

Also from bug 1423167:

> There are a few more overrides of UnsetAttr, all SVG, but again they should probably be able to transition to Before/AfterSett and then we can devirtualize here.  A separate bug on doing that, please?  nsSVGElement::UnsetAttr might get complicated...

This is similar to bug 1423490, just with UnsetAttr instead.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Stop overring UnsetAttr in SVGAnimationElement

MozReview-Commit-ID: Ih8QmVkoOFZ

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Stop overring UnsetAttr in SVGMPathElement

MozReview-Commit-ID: JdjEuESAsD9

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop overring UnsetAttr in SVGMarkerElement

This changes behavior some, by moving the code involved from "before we remove
the attribute" to AfterSetAttr.  But I think it should be OK, and SVG elements
can't sanely implement BeforeSetAttr right now...

MozReview-Commit-ID: KQGIDQOMGne

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Stop overring UnsetAttr in SVGElement

There are now no more overrides of UnsetAttr, so it can stop being virtual.

MozReview-Commit-ID: 4QzAKCkRfgs

Comment 5

[Boris Zbarsky [:bzbarsky]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cf3715de57ee796ee826f5d01bd26917b6dcf66

Comment 6

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8938612 [details] [diff] [review]
part 3.  Stop overring UnsetAttr in SVGMarkerElement

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

::: dom/svg/SVGMarkerElement.cpp
@@ +245,5 @@
>  {
> +  if (!aValue && aNamespaceID == kNameSpaceID_None &&
> +      aName == nsGkAtoms::orient) {
> +    // XXXbz Should this really be in BeforeSetAttr, not AfterSetAttr?
> +    // Preserving the old behavior for now....

The commit message seems right about a minor behavior change, but this comment seems like it is saying the opposite. Am I misunderstanding something?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop overring UnsetAttr in SVGMarkerElement

Oops, that comment predates me discovering that SVG things can't implement BeforeSetAttr... As the comment indicates, doing this in AfterSetAttr is arguably saner anyway

Comment 8

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8938610 [details] [diff] [review]
part 1.  Stop overring UnsetAttr in SVGAnimationElement

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

These all look good to me.

Comment 9

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8939630 [details] [diff] [review]
part 3.  Stop overring UnsetAttr in SVGMarkerElement

oops

Comment 10

[Boris Zbarsky [:bzbarsky]]

> oops

Actually, my oops.  I totally typed in the wrong username... :(

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0cb5b7abb37
part 1.  Stop overring UnsetAttr in SVGAnimationElement.  r=bytesized
https://hg.mozilla.org/integration/mozilla-inbound/rev/a24287fb46ab
part 2.  Stop overring UnsetAttr in SVGMPathElement.  r=bytesized
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccd05fb92628
part 3.  Stop overring UnsetAttr in SVGMarkerElement.  r=bytesized,qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4310a053726
part 4.  Stop overring UnsetAttr in SVGElement.  r=bytesized

Comment 12

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/f0cb5b7abb37
https://hg.mozilla.org/mozilla-central/rev/a24287fb46ab
https://hg.mozilla.org/mozilla-central/rev/ccd05fb92628
https://hg.mozilla.org/mozilla-central/rev/c4310a053726