Closed Bug 1423492 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: emilio, Assigned: bzbarsky)

References

Details

Attachments

(4 files, 1 obsolete file)

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.
Priority: -- → P2
Summary: Move side effects from SVG attribute setting to BeforeSetAttr / AfterSetAttr, and devirtualize UnsetAttr → Move side effects from SVG attribute unsetting to BeforeSetAttr / AfterSetAttr, and devirtualize UnsetAttr
Depends on: 1423490
MozReview-Commit-ID: Ih8QmVkoOFZ
Attachment #8938610 - Flags: review?(ksteuber)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
MozReview-Commit-ID: JdjEuESAsD9
Attachment #8938611 - Flags: review?(ksteuber)
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
Attachment #8938612 - Flags: review?(ksteuber)
There are now no more overrides of UnsetAttr, so it can stop being virtual.

MozReview-Commit-ID: 4QzAKCkRfgs
Attachment #8938613 - Flags: review?(ksteuber)
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?
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
Attachment #8939630 - Flags: review?(kyle)
Attachment #8938612 - Attachment is obsolete: true
Attachment #8938612 - Flags: review?(ksteuber)
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.
Attachment #8938610 - Flags: review?(ksteuber) → review+
Attachment #8938611 - Flags: review?(ksteuber) → review+
Attachment #8938613 - Flags: review?(ksteuber) → review+
Attachment #8939630 - Flags: review?(kyle) → review+
Comment on attachment 8939630 [details] [diff] [review]
part 3.  Stop overring UnsetAttr in SVGMarkerElement

oops
Attachment #8939630 - Flags: review+ → review?(kyle)
Attachment #8939630 - Flags: review?(kyle) → review+
> oops

Actually, my oops.  I totally typed in the wrong username... :(
Pushed by bzbarsky@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.