Skip to content

Commit

Permalink
Bug 1829225 - Make BeforeSetAttr take the parsed nsAttrValue. r=smaug
Browse files Browse the repository at this point in the history 
ParseAttribute ideally would be const (see bug 1829138), but the SVG and
SMIL code is rather messy. Still, now that BeforeSetAttr can't really
fail, swapping the order of ParseAttribute and BeforeSetAttr shouldn't
really change behavior.

Sorry for the extra `virtual` keyword removal and such. I had to do this
one by hand unlike the dependent bugs, and I went a bit drive-by, lmk if
you want me to split those changes.

Differential Revision: https://phabricator.services.mozilla.com/D176086
  • Loading branch information
@emilio
emilio committed Apr 21, 2023
1 parent d516ab3 commit 25b79c7
Show file tree
Hide file tree
Showing 38 changed files with 300 additions and 382 deletions.
40 changes: 23 additions & 17 deletions dom/base/Element.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2469,14 +2469,17 @@ nsresult Element::SetAttr(int32_t aNamespaceID, nsAtom* aName, nsAtom* aPrefix,

uint8_t modType;
bool hasListeners;
nsAttrValueOrString value(aValue);
nsAttrValue oldValue;
bool oldValueSet;

if (OnlyNotifySameValueSet(aNamespaceID, aName, aPrefix, value, aNotify,
oldValue, &modType, &hasListeners, &oldValueSet)) {
OnAttrSetButNotChanged(aNamespaceID, aName, value, aNotify);
return NS_OK;
{
const nsAttrValueOrString value(aValue);
if (OnlyNotifySameValueSet(aNamespaceID, aName, aPrefix, value, aNotify,
oldValue, &modType, &hasListeners,
&oldValueSet)) {
OnAttrSetButNotChanged(aNamespaceID, aName, value, aNotify);
return NS_OK;
}
}

// Hold a script blocker while calling ParseAttribute since that can call
Expand All @@ -2489,15 +2492,15 @@ nsresult Element::SetAttr(int32_t aNamespaceID, nsAtom* aName, nsAtom* aPrefix,
modType);
}

BeforeSetAttr(aNamespaceID, aName, &value, aNotify);

nsAttrValue attrValue;
if (!ParseAttribute(aNamespaceID, aName, aValue, aSubjectPrincipal,
attrValue)) {
attrValue.SetTo(aValue);
}

PreIdMaybeChange(aNamespaceID, aName, &value);
BeforeSetAttr(aNamespaceID, aName, &attrValue, aNotify);

PreIdMaybeChange(aNamespaceID, aName, &attrValue);

return SetAttrAndNotify(aNamespaceID, aName, aPrefix,
oldValueSet ? &oldValue : nullptr, attrValue,
Expand All @@ -2516,14 +2519,17 @@ nsresult Element::SetParsedAttr(int32_t aNamespaceID, nsAtom* aName,

uint8_t modType;
bool hasListeners;
nsAttrValueOrString value(aParsedValue);
nsAttrValue oldValue;
bool oldValueSet;

if (OnlyNotifySameValueSet(aNamespaceID, aName, aPrefix, value, aNotify,
oldValue, &modType, &hasListeners, &oldValueSet)) {
OnAttrSetButNotChanged(aNamespaceID, aName, value, aNotify);
return NS_OK;
{
const nsAttrValueOrString value(aParsedValue);
if (OnlyNotifySameValueSet(aNamespaceID, aName, aPrefix, value, aNotify,
oldValue, &modType, &hasListeners,
&oldValueSet)) {
OnAttrSetButNotChanged(aNamespaceID, aName, value, aNotify);
return NS_OK;
}
}

Document* document = GetComposedDoc();
Expand All @@ -2534,9 +2540,9 @@ nsresult Element::SetParsedAttr(int32_t aNamespaceID, nsAtom* aName,
modType);
}

BeforeSetAttr(aNamespaceID, aName, &value, aNotify);
BeforeSetAttr(aNamespaceID, aName, &aParsedValue, aNotify);

PreIdMaybeChange(aNamespaceID, aName, &value);
PreIdMaybeChange(aNamespaceID, aName, &aParsedValue);

return SetAttrAndNotify(aNamespaceID, aName, aPrefix,
oldValueSet ? &oldValue : nullptr, aParsedValue,
Expand Down Expand Up @@ -2731,7 +2737,7 @@ bool Element::SetAndSwapMappedAttribute(nsAtom* aName, nsAttrValue& aValue,
}

void Element::BeforeSetAttr(int32_t aNamespaceID, nsAtom* aName,
const nsAttrValueOrString* aValue, bool aNotify) {
const nsAttrValue* aValue, bool aNotify) {
if (aNamespaceID == kNameSpaceID_None) {
if (aName == nsGkAtoms::_class && aValue) {
// Note: This flag is asymmetrical. It is never unset and isn't exact.
Expand Down Expand Up @@ -2775,7 +2781,7 @@ void Element::AfterSetAttr(int32_t aNamespaceID, nsAtom* aName,
}

void Element::PreIdMaybeChange(int32_t aNamespaceID, nsAtom* aName,
const nsAttrValueOrString* aValue) {
const nsAttrValue* aValue) {
if (aNamespaceID != kNameSpaceID_None || aName != nsGkAtoms::id) {
return;
}
Expand Down
4 changes: 2 additions & 2 deletions dom/base/Element.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1928,7 +1928,7 @@ class Element : public FragmentOrElement {
* @param aNotify Whether we plan to notify document observers.
*/
virtual void BeforeSetAttr(int32_t aNamespaceID, nsAtom* aName,
const nsAttrValueOrString* aValue, bool aNotify);
const nsAttrValue* aValue, bool aNotify);

/**
* Hook that is called by Element::SetAttr to allow subclasses to
Expand Down Expand Up @@ -1974,7 +1974,7 @@ class Element : public FragmentOrElement {
* @param aValue the new id value. Will be null if the id is being unset.
*/
void PreIdMaybeChange(int32_t aNamespaceID, nsAtom* aName,
const nsAttrValueOrString* aValue);
const nsAttrValue* aValue);

/**
* This function shall be called just after the id attribute changes. It will
Expand Down
2 changes: 0 additions & 2 deletions dom/base/MutationObservers.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,6 @@ class MutationObservers {
* @param aNameSpaceID Namespace of changing attribute
* @param aAttribute Local-name of changing attribute
* @param aModType Type of change (add/change/removal)
* @param aNewValue The parsed new value, but only if BeforeSetAttr
* preparsed it!!!
* @see nsIMutationObserver::AttributeWillChange
*/
static void NotifyAttributeWillChange(mozilla::dom::Element* aElement,
Expand Down
2 changes: 0 additions & 2 deletions dom/base/nsIMutationObserver.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,6 @@ class nsIMutationObserver
* @param aModType Whether or not the attribute will be added, changed, or
* removed. The constants are defined in
* MutationEvent.webidl.
* @param aNewValue The new value, IF it has been preparsed by
* BeforeSetAttr, otherwise null.
*
* @note Callers of this method might not hold a strong reference to the
* observer. The observer is responsible for making sure it stays
Expand Down
12 changes: 4 additions & 8 deletions dom/base/nsStyledElement.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,10 @@ bool nsStyledElement::ParseAttribute(int32_t aNamespaceID, nsAtom* aAttribute,
}

void nsStyledElement::BeforeSetAttr(int32_t aNamespaceID, nsAtom* aName,
const nsAttrValueOrString* aValue,
bool aNotify) {
if (aNamespaceID == kNameSpaceID_None) {
if (aName == nsGkAtoms::style) {
if (aValue) {
SetMayHaveStyle();
}
}
const nsAttrValue* aValue, bool aNotify) {
if (aNamespaceID == kNameSpaceID_None && aName == nsGkAtoms::style &&
aValue) {
SetMayHaveStyle();
}

return nsStyledElementBase::BeforeSetAttr(aNamespaceID, aName, aValue,
Expand Down
13 changes: 6 additions & 7 deletions dom/base/nsStyledElement.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,10 @@ class nsStyledElement : public nsStyledElementBase {
nsIPrincipal* aMaybeScriptedPrincipal,
nsAttrValue& aResult, bool aForceInDataDoc);

virtual bool ParseAttribute(int32_t aNamespaceID, nsAtom* aAttribute,
const nsAString& aValue,
nsIPrincipal* aMaybeScriptedPrincipal,
nsAttrValue& aResult) override;
bool ParseAttribute(int32_t aNamespaceID, nsAtom* aAttribute,
const nsAString& aValue,
nsIPrincipal* aMaybeScriptedPrincipal,
nsAttrValue& aResult) override;

friend class mozilla::dom::Element;

Expand All @@ -91,9 +91,8 @@ class nsStyledElement : public nsStyledElementBase {
*/
nsresult ReparseStyleAttribute(bool aForceInDataDoc);

virtual void BeforeSetAttr(int32_t aNamespaceID, nsAtom* aName,
const nsAttrValueOrString* aValue,
bool aNotify) override;
void BeforeSetAttr(int32_t aNamespaceID, nsAtom* aName,
const nsAttrValue* aValue, bool aNotify) override;
};

NS_DEFINE_STATIC_IID_ACCESSOR(nsStyledElement, NS_STYLED_ELEMENT_IID)
Expand Down
10 changes: 3 additions & 7 deletions dom/html/HTMLAnchorElement.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,14 +187,10 @@ already_AddRefed<nsIURI> HTMLAnchorElement::GetHrefURI() const {
}

void HTMLAnchorElement::BeforeSetAttr(int32_t aNamespaceID, nsAtom* aName,
const nsAttrValueOrString* aValue,
bool aNotify) {
if (aNamespaceID == kNameSpaceID_None) {
if (aName == nsGkAtoms::href) {
CancelDNSPrefetch(*this);
}
const nsAttrValue* aValue, bool aNotify) {
if (aNamespaceID == kNameSpaceID_None && aName == nsGkAtoms::href) {
CancelDNSPrefetch(*this);
}

return nsGenericHTMLElement::BeforeSetAttr(aNamespaceID, aName, aValue,
aNotify);
}
Expand Down
34 changes: 15 additions & 19 deletions dom/html/HTMLAnchorElement.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,19 +39,19 @@ class HTMLAnchorElement final : public nsGenericHTMLElement,

NS_IMPL_FROMNODE_HTML_WITH_TAG(HTMLAnchorElement, a);

virtual int32_t TabIndexDefault() override;
virtual bool Draggable() const override;
int32_t TabIndexDefault() override;
bool Draggable() const override;

// Element
virtual bool IsInteractiveHTMLContent() const override;
bool IsInteractiveHTMLContent() const override;

// DOM memory reporter participant
NS_DECL_ADDSIZEOFEXCLUDINGTHIS

virtual nsresult BindToTree(BindContext&, nsINode& aParent) override;
virtual void UnbindFromTree(bool aNullParent = true) override;
virtual bool IsHTMLFocusable(bool aWithMouse, bool* aIsFocusable,
int32_t* aTabIndex) override;
nsresult BindToTree(BindContext&, nsINode& aParent) override;
void UnbindFromTree(bool aNullParent = true) override;
bool IsHTMLFocusable(bool aWithMouse, bool* aIsFocusable,
int32_t* aTabIndex) override;

void GetEventTargetParent(EventChainPreVisitor& aVisitor) override;
MOZ_CAN_RUN_SCRIPT
Expand All @@ -60,18 +60,15 @@ class HTMLAnchorElement final : public nsGenericHTMLElement,
void GetLinkTarget(nsAString& aTarget) override;
already_AddRefed<nsIURI> GetHrefURI() const override;

virtual void BeforeSetAttr(int32_t aNamespaceID, nsAtom* aName,
const nsAttrValueOrString* aValue,
bool aNotify) override;
virtual void AfterSetAttr(int32_t aNamespaceID, nsAtom* aName,
const nsAttrValue* aValue,
const nsAttrValue* aOldValue,
nsIPrincipal* aSubjectPrincipal,
bool aNotify) override;
void BeforeSetAttr(int32_t aNamespaceID, nsAtom* aName,
const nsAttrValue* aValue, bool aNotify) override;
void AfterSetAttr(int32_t aNamespaceID, nsAtom* aName,
const nsAttrValue* aValue, const nsAttrValue* aOldValue,
nsIPrincipal* aSubjectPrincipal, bool aNotify) override;

virtual nsresult Clone(dom::NodeInfo*, nsINode** aResult) const override;
nsresult Clone(dom::NodeInfo*, nsINode** aResult) const override;

virtual ElementState IntrinsicState() const override;
ElementState IntrinsicState() const override;

// WebIDL API

Expand Down Expand Up @@ -198,8 +195,7 @@ class HTMLAnchorElement final : public nsGenericHTMLElement,
protected:
virtual ~HTMLAnchorElement();

virtual JSObject* WrapNode(JSContext* aCx,
JS::Handle<JSObject*> aGivenProto) override;
JSObject* WrapNode(JSContext*, JS::Handle<JSObject*> aGivenProto) override;
RefPtr<nsDOMTokenList> mRelList;
};

Expand Down
3 changes: 1 addition & 2 deletions dom/html/HTMLButtonElement.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -336,8 +336,7 @@ void HTMLButtonElement::DoneCreatingElement() {
}

void HTMLButtonElement::BeforeSetAttr(int32_t aNameSpaceID, nsAtom* aName,
const nsAttrValueOrString* aValue,
bool aNotify) {
const nsAttrValue* aValue, bool aNotify) {
if (aNotify && aName == nsGkAtoms::disabled &&
aNameSpaceID == kNameSpaceID_None) {
mDisabledChanged = true;
Expand Down
46 changes: 21 additions & 25 deletions dom/html/HTMLButtonElement.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ class HTMLButtonElement final : public nsGenericHTMLFormControlElementWithState,
// nsISupports
NS_DECL_ISUPPORTS_INHERITED

virtual int32_t TabIndexDefault() override;
int32_t TabIndexDefault() override;

NS_IMPL_FROMNODE_HTML_WITH_TAG(HTMLButtonElement, button)

// Element
virtual bool IsInteractiveHTMLContent() const override { return true; }
bool IsInteractiveHTMLContent() const override { return true; }

// nsGenericHTMLFormElement
void SaveState() override;
Expand All @@ -47,49 +47,45 @@ class HTMLButtonElement final : public nsGenericHTMLFormControlElementWithState,
NS_IMETHOD Reset() override;
NS_IMETHOD SubmitNamesValues(FormData* aFormData) override;

virtual void FieldSetDisabledChanged(bool aNotify) override;
void FieldSetDisabledChanged(bool aNotify) override;

// EventTarget
void GetEventTargetParent(EventChainPreVisitor& aVisitor) override;
MOZ_CAN_RUN_SCRIPT_BOUNDARY
virtual nsresult PostHandleEvent(EventChainPostVisitor& aVisitor) override;
nsresult PostHandleEvent(EventChainPostVisitor& aVisitor) override;

// nsINode
virtual nsresult Clone(dom::NodeInfo*, nsINode** aResult) const override;
virtual JSObject* WrapNode(JSContext* aCx,
JS::Handle<JSObject*> aGivenProto) override;
nsresult Clone(dom::NodeInfo*, nsINode** aResult) const override;
JSObject* WrapNode(JSContext*, JS::Handle<JSObject*> aGivenProto) override;

// nsIContent
virtual nsresult BindToTree(BindContext&, nsINode& aParent) override;
virtual void UnbindFromTree(bool aNullParent = true) override;
virtual void DoneCreatingElement() override;
nsresult BindToTree(BindContext&, nsINode& aParent) override;
void UnbindFromTree(bool aNullParent = true) override;
void DoneCreatingElement() override;

void UpdateBarredFromConstraintValidation();
// Element
ElementState IntrinsicState() const override;
/**
* Called when an attribute is about to be changed
*/
virtual void BeforeSetAttr(int32_t aNameSpaceID, nsAtom* aName,
const nsAttrValueOrString* aValue,
bool aNotify) override;
void BeforeSetAttr(int32_t aNameSpaceID, nsAtom* aName,
const nsAttrValue* aValue, bool aNotify) override;
/**
* Called when an attribute has just been changed
*/
virtual void AfterSetAttr(int32_t aNamespaceID, nsAtom* aName,
const nsAttrValue* aValue,
const nsAttrValue* aOldValue,
nsIPrincipal* aSubjectPrincipal,
bool aNotify) override;
virtual bool ParseAttribute(int32_t aNamespaceID, nsAtom* aAttribute,
const nsAString& aValue,
nsIPrincipal* aMaybeScriptedPrincipal,
nsAttrValue& aResult) override;
void AfterSetAttr(int32_t aNamespaceID, nsAtom* aName,
const nsAttrValue* aValue, const nsAttrValue* aOldValue,
nsIPrincipal* aSubjectPrincipal, bool aNotify) override;
bool ParseAttribute(int32_t aNamespaceID, nsAtom* aAttribute,
const nsAString& aValue,
nsIPrincipal* aMaybeScriptedPrincipal,
nsAttrValue& aResult) override;

// nsGenericHTMLElement
virtual bool IsHTMLFocusable(bool aWithMouse, bool* aIsFocusable,
int32_t* aTabIndex) override;
virtual bool IsDisabledForEvents(WidgetEvent* aEvent) override;
bool IsHTMLFocusable(bool aWithMouse, bool* aIsFocusable,
int32_t* aTabIndex) override;
bool IsDisabledForEvents(WidgetEvent* aEvent) override;

// WebIDL
bool Disabled() const { return GetBoolAttr(nsGkAtoms::disabled); }
Expand Down
3 changes: 1 addition & 2 deletions dom/html/HTMLFormElement.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,7 @@ NS_IMPL_ELEMENT_CLONE(HTMLFormElement)
nsIHTMLCollection* HTMLFormElement::Elements() { return mControls; }

void HTMLFormElement::BeforeSetAttr(int32_t aNamespaceID, nsAtom* aName,
const nsAttrValueOrString* aValue,
bool aNotify) {
const nsAttrValue* aValue, bool aNotify) {
if (aNamespaceID == kNameSpaceID_None) {
if (aName == nsGkAtoms::action || aName == nsGkAtoms::target) {
// Don't forget we've notified the password manager already if the
Expand Down
Loading

0 comments on commit 25b79c7

Please sign in to comment.