1210812 - HTMLCanvasElement::SetAttr should be cleaned up

Status: NEW

(Core :: Graphics: Canvas2D, defect, P3)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

With the existence of BeforeSetAttr and AfterSetAttr most of the SetAttr method overrides should disappear.  HTMLCanvasElement::SetAttr and HTMLCanvasElement::UnsetAttr do some extra work, and we should investigate what can be moved to BeforeSetAttr or AfterSetAttr.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

One thing to keep in mind; base class (Element) calls AfterSetAttr only if the attribute is modified; in the case of canvas width or height, the spec (https://html.spec.whatwg.org/multipage/scripting.html#canvas) says we should do something even when the values are redundantly set to the same value.  There are web platform tests which will fail if this is not the case.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Use AfterSetAttr instead of SetAttr/UnsetAttr. Special case widht/height. r=smaug

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26368c95e06c

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8669006 [details] [diff] [review]
Use AfterSetAttr instead of SetAttr/UnsetAttr. Special case widht/height. r=smaug

>+  virtual bool MaybeCheckSameAttrVal(int32_t aNamespaceID, nsIAtom* aName,
>+                                     nsIAtom* aPrefix,
>+                                     const nsAttrValueOrString& aValue,
>+                                     bool aNotify, nsAttrValue& aOldValue,
>+                                     uint8_t* aModType, bool* aHasListeners);
Hmm, virtual call. I guess I can live with this. Hopefully doesn't show up too badly in profiles.
This is hot code in some microbenchmarks

>+  // The canvas spec says that we need to "update" widht
Haa, I'm not the only one who makes mistakes with 'width'! (not widht)


 and height even
>+  // if they are redundantly set to the same value already held.
>+  // https://html.spec.whatwg.org/multipage/scripting.html#canvas
>+  // We still call the base class to set all the out parameters.
>+  return Element::MaybeCheckSameAttrVal(aNamespaceID, aName, aPrefix, aValue,
>+                                         aNotify, aOldValue, aModType,
>+                                         aHasListeners) &&
>+    (aName != nsGkAtoms::width && aName != nsGkAtoms::height);
>+}
You can't change the meaning of MaybeCheckSameAttrVal. That would possibly break DOM MutationObserver
which rely on Element::OnlyNotifySameValueSet to work properly, and even more importantly, 
the change would make MaybeCheckSameAttrVal inconsistent.


Didn't the approach to use autoscriptblocker in HTMLCanvasElement::SetAttr work?

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Olli Pettay [:smaug] from comment #4)
>
> You can't change the meaning of MaybeCheckSameAttrVal. That would possibly break DOM MutationObserver
> which rely on Element::OnlyNotifySameValueSet to work properly, and even more importantly, 
> the change would make MaybeCheckSameAttrVal inconsistent.
> 

The whole point of the patch was to change the meaning and make it inconsistent :)

> Didn't the approach to use autoscriptblocker in HTMLCanvasElement::SetAttr
> work?

For reference, this is the script blocker approach: "One would need to override SetAttr after all, and before calling parent class' SetAttr push a scriptblocker on stack so that mutationevent will happen only once the scriptblocker goes out from the stack, not during calling parent's SetAttr."

Feels icky, but I'll give it a try and see what happens.