Bug 656197 - Push state updates further out across beforesetattr/aftersetattr

* Remove the generic attr preparsing mechanism from BeforeSetAttr and just preparse class attributes directly in the one place that needs to do it
* Move calls to BeforeSetAttr to after AttributeWillChange
* Remove UpdateState calls in BeforeSetAttr
* Move calls to AfterSetAttr to before UpdateState when manipulating attributes
* Remove UpdateState calls from AfterSetAttr, since they are no longer needed there

Tag #1375
This commit is contained in:
Matt A. Tobin 2020-04-16 19:59:10 -04:00 • committed by Roy Tam
commit 7a3f9be509
27 changed files with 84 additions and 117 deletions

View file

@ -2461,6 +2461,9 @@ Element::SetAttr(int32_t aNamespaceID, nsIAtom* aName,
uint8_t modType;
bool hasListeners;
// We don't want to spend time preparsing class attributes if the value is not
// changing, so just init our nsAttrValueOrString with aValue for the
// OnlyNotifySameValueSet call.
nsAttrValueOrString value(aValue);
nsAttrValue oldValue;
@ -2469,25 +2472,30 @@ Element::SetAttr(int32_t aNamespaceID, nsIAtom* aName,
return NS_OK;
}
nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify);
NS_ENSURE_SUCCESS(rv, rv);
nsAttrValue* preparsedAttrValue = value.GetStoredAttrValue();
nsAttrValue attrValue;
nsAttrValue* preparsedAttrValue;
if (aNamespaceID == kNameSpaceID_None && aName == nsGkAtoms::_class) {
attrValue.ParseAtomArray(aValue);
value.ResetToAttrValue(attrValue);
preparsedAttrValue = &attrValue;
} else {
preparsedAttrValue = nullptr;
}
if (aNotify) {
nsNodeUtils::AttributeWillChange(this, aNamespaceID, aName, modType,
preparsedAttrValue);
}
nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify);
NS_ENSURE_SUCCESS(rv, rv);
// Hold a script blocker while calling ParseAttribute since that can call
// out to id-observers
nsAutoScriptBlocker scriptBlocker;
nsAttrValue attrValue;
if (preparsedAttrValue) {
attrValue.SwapValueWith(*preparsedAttrValue);
}
// Even the value was pre-parsed in BeforeSetAttr, we still need to call
// ParseAttribute because it can have side effects.
// Even the value was pre-parsed, we still need to call ParseAttribute because
// it can have side effects.
if (!ParseAttribute(aNamespaceID, aName, aValue, attrValue)) {
attrValue.SetTo(aValue);
}
@ -2523,14 +2531,14 @@ Element::SetParsedAttr(int32_t aNamespaceID, nsIAtom* aName,
return NS_OK;
}
nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify);
NS_ENSURE_SUCCESS(rv, rv);
if (aNotify) {
nsNodeUtils::AttributeWillChange(this, aNamespaceID, aName, modType,
&aParsedValue);
}
nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify);
NS_ENSURE_SUCCESS(rv, rv);
return SetAttrAndNotify(aNamespaceID, aName, aPrefix, oldValue,
aParsedValue, modType, hasListeners, aNotify,
kCallAfterSetAttr);
@ -2599,8 +2607,6 @@ Element::SetAttrAndNotify(int32_t aNamespaceID,
}
}
UpdateState(aNotify);
if (CustomElementRegistry::IsCustomElementEnabled()) {
if (CustomElementData* data = GetCustomElementData()) {
if (CustomElementDefinition* definition =
@ -2639,6 +2645,8 @@ Element::SetAttrAndNotify(int32_t aNamespaceID,
}
}
UpdateState(aNotify);
if (aNotify) {
// Don't pass aOldValue to AttributeChanged since it may not be reliable.
// Callers only compute aOldValue under certain conditions which may not
@ -2674,25 +2682,6 @@ Element::SetAttrAndNotify(int32_t aNamespaceID,
return NS_OK;
}
nsresult
Element::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
nsAttrValueOrString* aValue, bool aNotify)
{
if (aNamespaceID == kNameSpaceID_None) {
if (aName == nsGkAtoms::_class) {
// aValue->GetAttrValue will only be non-null here when this is called
// via Element::SetParsedAttr. This shouldn't happen for "class", but
// this will handle it.
if (aValue && !aValue->GetAttrValue()) {
nsAttrValue attr;
attr.ParseAtomArray(aValue->String());
aValue->TakeParsedValue(attr);
}
}
}
return NS_OK;
}
bool
Element::ParseAttribute(int32_t aNamespaceID,
nsIAtom* aAttribute,
@ -2809,9 +2798,6 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
return NS_OK;
}
nsresult rv = BeforeSetAttr(aNameSpaceID, aName, nullptr, aNotify);
NS_ENSURE_SUCCESS(rv, rv);
nsIDocument *document = GetComposedDoc();
mozAutoDocUpdate updateBatch(document, UPDATE_CONTENT_MODEL, aNotify);
@ -2821,6 +2807,9 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nullptr);
}
nsresult rv = BeforeSetAttr(aNameSpaceID, aName, nullptr, aNotify);
NS_ENSURE_SUCCESS(rv, rv);
bool hasMutationListeners = aNotify &&
nsContentUtils::HasMutationListeners(this,
NS_EVENT_BITS_MUTATION_ATTRMODIFIED,
@ -2869,8 +2858,6 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
}
}
UpdateState(aNotify);
if (CustomElementRegistry::IsCustomElementEnabled()) {
if (CustomElementData* data = GetCustomElementData()) {
if (CustomElementDefinition* definition =
@ -2897,6 +2884,11 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
}
}
rv = AfterSetAttr(aNameSpaceID, aName, nullptr, aNotify);
NS_ENSURE_SUCCESS(rv, rv);
UpdateState(aNotify);
if (aNotify) {
// We can always pass oldValue here since there is no new value which could
// have corrupted it.
@ -2904,9 +2896,6 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsIDOMMutationEvent::REMOVAL, &oldValue);
}
rv = AfterSetAttr(aNameSpaceID, aName, nullptr, aNotify);
NS_ENSURE_SUCCESS(rv, rv);
if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::dir) {
OnSetDirAttr(this, nullptr, hadValidDir, hadDirAuto, aNotify);
}

View file

@ -1336,16 +1336,17 @@ protected:
* @param aName the localname of the attribute being set
* @param aValue the value it's being set to represented as either a string or
* a parsed nsAttrValue. Alternatively, if the attr is being removed it
* will be null. BeforeSetAttr is allowed to modify aValue by parsing
* the string to an nsAttrValue (to avoid having to reparse it in
* ParseAttribute).
* will be null.
* @param aNotify Whether we plan to notify document observers.
*/
// Note that this is inlined so that when subclasses call it it gets
// inlined. Those calls don't go through a vtable.
virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
bool aNotify);
const nsAttrValueOrString* aValue,
bool aNotify)
{
return NS_OK;
}
/**
* Hook that is called by Element::SetAttr to allow subclasses to

View file

@ -49,20 +49,13 @@ public:
, mCheapString(nullptr)
{ }
void TakeParsedValue(nsAttrValue& aValue)
void ResetToAttrValue(const nsAttrValue& aValue)
{
mStoredAttrValue.SwapValueWith(aValue);
mAttrValue = &mStoredAttrValue;
mAttrValue = &aValue;
mStringPtr = nullptr;
// No need to touch mCheapString here. If we need to use it, we will reset
// it to the rigthe value anyway.
}
/**
* If TakeParsedValue has been called, returns the value that it set.
*/
nsAttrValue* GetStoredAttrValue()
{
return mAttrValue == &mStoredAttrValue ? &mStoredAttrValue : nullptr;
}
const nsAttrValue* GetAttrValue() { return mAttrValue; }
/**
* Returns a reference to the string value of the contents of this object.
@ -89,7 +82,6 @@ protected:
const nsAttrValue* mAttrValue;
mutable const nsAString* mStringPtr;
mutable nsCheapString mCheapString;
nsAttrValue mStoredAttrValue;
};
#endif // nsAttrValueOrString_h___

View file

@ -423,7 +423,7 @@ HTMLButtonElement::DoneCreatingElement()
nsresult
HTMLButtonElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify)
{
if (aNotify && aName == nsGkAtoms::disabled &&
@ -448,7 +448,6 @@ HTMLButtonElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
if (aName == nsGkAtoms::type || aName == nsGkAtoms::disabled) {
UpdateBarredFromConstraintValidation();
UpdateState(aNotify);
}
}

View file

@ -81,7 +81,7 @@ public:
* Called when an attribute is about to be changed
*/
virtual nsresult BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify) override;
/**
* Called when an attribute has just been changed

View file

@ -45,7 +45,7 @@ HTMLDetailsElement::GetAttributeChangeHint(const nsIAtom* aAttribute,
nsresult
HTMLDetailsElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue, bool aNotify)
const nsAttrValueOrString* aValue, bool aNotify)
{
if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::open) {
bool setOpen = aValue != nullptr;

View file

@ -38,7 +38,8 @@ public:
int32_t aModType) const override;
nsresult BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue, bool aNotify) override;
const nsAttrValueOrString* aValue,
bool aNotify) override;
// HTMLDetailsElement WebIDL
bool Open() const { return GetBoolAttr(nsGkAtoms::open); }

View file

@ -369,7 +369,7 @@ HTMLImageElement::GetAttributeMappingFunction() const
nsresult
HTMLImageElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify)
{

View file

@ -344,7 +344,7 @@ protected:
void UpdateFormOwner();
virtual nsresult BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify) override;
virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,

View file

@ -1237,7 +1237,7 @@ HTMLInputElement::Clone(mozilla::dom::NodeInfo* aNodeInfo, nsINode** aResult) co
nsresult
HTMLInputElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify)
{
if (aNameSpaceID == kNameSpaceID_None) {
@ -1452,8 +1452,6 @@ HTMLInputElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
// Clear the cached @autocomplete attribute state.
mAutocompleteAttrState = nsContentUtils::eAutocompleteAttrState_Unknown;
}
UpdateState(aNotify);
}
return nsGenericHTMLFormElementWithState::AfterSetAttr(aNameSpaceID, aName,

View file

@ -958,7 +958,7 @@ protected:
* Called when an attribute is about to be changed
*/
virtual nsresult BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify) override;
/**
* Called when an attribute has just been changed

View file

@ -333,7 +333,7 @@ HTMLLinkElement::UpdateImport()
nsresult
HTMLLinkElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue, bool aNotify)
const nsAttrValueOrString* aValue, bool aNotify)
{
if (aNameSpaceID == kNameSpaceID_None &&
(aName == nsGkAtoms::href || aName == nsGkAtoms::rel)) {

View file

@ -62,7 +62,7 @@ public:
virtual void UnbindFromTree(bool aDeep = true,
bool aNullParent = true) override;
virtual nsresult BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify) override;
virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
const nsAttrValue* aValue,

View file

@ -181,7 +181,7 @@ HTMLOptionElement::GetAttributeChangeHint(const nsIAtom* aAttribute,
nsresult
HTMLOptionElement::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify)
{
nsresult rv = nsGenericHTMLElement::BeforeSetAttr(aNamespaceID, aName,

View file

@ -47,7 +47,7 @@ public:
int32_t aModType) const override;
virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify) override;
virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
const nsAttrValue* aValue, bool aNotify) override;

View file

@ -1269,7 +1269,7 @@ HTMLSelectElement::UnbindFromTree(bool aDeep, bool aNullParent)
nsresult
HTMLSelectElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify)
{
if (aNotify && aName == nsGkAtoms::disabled &&
@ -1294,8 +1294,6 @@ HTMLSelectElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
// Clear the cached @autocomplete attribute state
mAutocompleteAttrState = nsContentUtils::eAutocompleteAttrState_Unknown;
}
UpdateState(aNotify);
}
return nsGenericHTMLFormElementWithState::AfterSetAttr(aNameSpaceID, aName,

View file

@ -375,7 +375,7 @@ public:
bool aCompileEventHandlers) override;
virtual void UnbindFromTree(bool aDeep, bool aNullParent) override;
virtual nsresult BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify) override;
virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
const nsAttrValue* aValue, bool aNotify) override;

View file

@ -986,7 +986,7 @@ HTMLTableElement::UnbindFromTree(bool aDeep, bool aNullParent)
nsresult
HTMLTableElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify)
{
if (aName == nsGkAtoms::cellpadding && aNameSpaceID == kNameSpaceID_None) {

View file

@ -190,7 +190,7 @@ public:
* Called when an attribute is about to be changed
*/
virtual nsresult BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify) override;
/**
* Called when an attribute has just been changed

View file

@ -1286,7 +1286,7 @@ HTMLTextAreaElement::UnbindFromTree(bool aDeep, bool aNullParent)
nsresult
HTMLTextAreaElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify)
{
if (aNotify && aName == nsGkAtoms::disabled &&
@ -1364,8 +1364,6 @@ HTMLTextAreaElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
} else if (aName == nsGkAtoms::minlength) {
UpdateTooShortValidityState();
}
UpdateState(aNotify);
}
return nsGenericHTMLFormElementWithState::AfterSetAttr(aNameSpaceID, aName, aValue,

View file

@ -144,7 +144,7 @@ public:
* Called when an attribute is about to be changed
*/
virtual nsresult BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify) override;
// nsIMutationObserver

View file

@ -1962,7 +1962,7 @@ nsGenericHTMLFormElement::UnbindFromTree(bool aDeep, bool aNullParent)
nsresult
nsGenericHTMLFormElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify)
{
if (aNameSpaceID == kNameSpaceID_None) {
@ -1995,13 +1995,6 @@ nsGenericHTMLFormElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
}
mForm->RemoveElement(this, false);
// Removing the element from the form can make it not be the default
// control anymore. Go ahead and notify on that change, though we might
// end up readding and becoming the default control again in
// AfterSetAttr.
// FIXME: Bug 656197
UpdateState(aNotify);
}
if (aName == nsGkAtoms::form) {
@ -2051,12 +2044,6 @@ nsGenericHTMLFormElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
}
mForm->AddElement(this, false, aNotify);
// Adding the element to the form can make it be the default control .
// Go ahead and notify on that change.
// Note: no need to notify on CanBeDisabled(), since type attr
// changes can't affect that.
UpdateState(aNotify);
}
if (aName == nsGkAtoms::form) {

View file

@ -1265,7 +1265,7 @@ protected:
virtual ~nsGenericHTMLFormElement();
virtual nsresult BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify) override;
virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,

View file

@ -1441,27 +1441,16 @@ nsSVGElement::WillChangeValue(nsIAtom* aName)
// We need an empty attr value:
// a) to pass to BeforeSetAttr when GetParsedAttr returns nullptr
// b) to store the old value in the case we have mutation listeners
// We can use the same value for both purposes since (a) happens before (b).
//
// We can use the same value for both purposes, because if GetParsedAttr
// returns non-null its return value is what will get passed to BeforeSetAttr,
// not matter what our mutation listener situation is.
//
// Also, we should be careful to always return this value to benefit from
// return value optimization.
nsAttrValue emptyOrOldAttrValue;
const nsAttrValue* attrValue = GetParsedAttr(aName);
// This is not strictly correct--the attribute value parameter for
// BeforeSetAttr should reflect the value that *will* be set but that implies
// allocating, e.g. an extra nsSVGLength2, and isn't necessary at the moment
// since no SVG elements overload BeforeSetAttr. For now we just pass the
// current value.
nsAttrValueOrString attrStringOrValue(attrValue ? *attrValue
: emptyOrOldAttrValue);
DebugOnly<nsresult> rv =
BeforeSetAttr(kNameSpaceID_None, aName, &attrStringOrValue,
kNotifyDocumentObservers);
// SVG elements aren't expected to overload BeforeSetAttr in such a way that
// it may fail. So long as this is the case we don't need to check and pass on
// the return value which simplifies the calling code significantly.
MOZ_ASSERT(NS_SUCCEEDED(rv), "Unexpected failure from BeforeSetAttr");
// We only need to set the old value if we have listeners since otherwise it
// isn't used.
if (attrValue &&
@ -1477,6 +1466,21 @@ nsSVGElement::WillChangeValue(nsIAtom* aName)
nsNodeUtils::AttributeWillChange(this, kNameSpaceID_None, aName, modType,
nullptr);
// This is not strictly correct--the attribute value parameter for
// BeforeSetAttr should reflect the value that *will* be set but that implies
// allocating, e.g. an extra nsSVGLength2, and isn't necessary at the moment
// since no SVG elements overload BeforeSetAttr. For now we just pass the
// current value.
nsAttrValueOrString attrStringOrValue(attrValue ? *attrValue
: emptyOrOldAttrValue);
DebugOnly<nsresult> rv =
BeforeSetAttr(kNameSpaceID_None, aName, &attrStringOrValue,
kNotifyDocumentObservers);
// SVG elements aren't expected to overload BeforeSetAttr in such a way that
// it may fail. So long as this is the case we don't need to check and pass on
// the return value which simplifies the calling code significantly.
MOZ_ASSERT(NS_SUCCEEDED(rv), "Unexpected failure from BeforeSetAttr");
return emptyOrOldAttrValue;
}

View file

@ -329,7 +329,7 @@ protected:
// BeforeSetAttr since it would involve allocating extra SVG value types.
// See the comment in nsSVGElement::WillChangeValue.
virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify) override final
{
return nsSVGElementBase::BeforeSetAttr(aNamespaceID, aName, aValue, aNotify);

View file

@ -1007,7 +1007,7 @@ nsXULElement::UnregisterAccessKey(const nsAString& aOldValue)
nsresult
nsXULElement::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
nsAttrValueOrString* aValue, bool aNotify)
const nsAttrValueOrString* aValue, bool aNotify)
{
if (aNamespaceID == kNameSpaceID_None && aName == nsGkAtoms::accesskey &&
IsInUncomposedDoc()) {

View file

@ -624,7 +624,7 @@ protected:
nsresult MakeHeavyweight(nsXULPrototypeElement* aPrototype);
virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName,
nsAttrValueOrString* aValue,
const nsAttrValueOrString* aValue,
bool aNotify) override;
virtual nsresult AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
const nsAttrValue* aValue, bool aNotify) override;