Closed Bug 1363481 Opened 7 years ago Closed 7 years ago

Add the old attribute value as a parameter to Element::AfterSetAttr

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bytesized, Assigned: bytesized)

References

Details

Attachments

(1 file)

In order to facilitate the movement of code with side-effects called by Element::SetAttr to Element::BeforeSetAttr and Element::AfterSetAttr, Element::AfterSetAttr should have access to the old value of the attribute. This includes information about whether there was previously a value set or not.
Attachment #8866953 - Flags: review?(bzbarsky)
Hmm. Looks like I'm hitting an awful lot of assertions. I will investigate this.
No longer blocks: 1357645
Comment on attachment 8866953 [details]
Bug 1363481 - Add the old attribute value as a parameter to Element::AfterSetAttr

https://reviewboard.mozilla.org/r/138564/#review143748

It would have been really nice (much easier to review) if the "propagate out whether we used to have a value" changes had been in a separate changeset from the "mass-change all the AfterSetAttr implementations" changes....

r=me with the nits below fixed.

::: dom/base/Element.h:627
(Diff revision 2)
>    bool MaybeCheckSameAttrVal(int32_t aNamespaceID, nsIAtom* aName,
>                               nsIAtom* aPrefix,
>                               const nsAttrValueOrString& aValue,
>                               bool aNotify, nsAttrValue& aOldValue,
> -                             uint8_t* aModType, bool* aHasListeners);
> +                             uint8_t* aModType, bool* aHasListeners,
> +                             bool* aOldValueSet);

Please document the new arg.

::: dom/base/Element.h:633
(Diff revision 2)
>  
>    bool OnlyNotifySameValueSet(int32_t aNamespaceID, nsIAtom* aName,
>                                nsIAtom* aPrefix,
>                                const nsAttrValueOrString& aValue,
>                                bool aNotify, nsAttrValue& aOldValue,
> -                              uint8_t* aModType, bool* aHasListeners);
> +                              uint8_t* aModType, bool* aHasListeners,

Document new arg (and ideally the others too).

::: dom/base/Element.h:1338
(Diff revision 2)
>     *                      in the cases where the actual old value (i.e.
>     *                      its current value) is !StoresOwnData() --- in which
>     *                      case the current value is probably already useless.
>     *                      If the current value is StoresOwnData() (or absent),
> -   *                      aOldValue will not be used.
> +   *                      aOldValue will not be used. aOldValue will be null
> +   *                      if no value was previously set

Or if we have no mutation listeners or aren't a custom element...

Basically, the common case is that aOldValue is null.

::: dom/base/Element.h:1399
(Diff revision 2)
>     *
>     * @param aName the name of the attribute
> -   * @param aValue the nsAttrValue to set
> +   * @param aValue the nsAttrValue to set. Will be swapped with the existing
> +   *               value of the attribute if the attribute already exists.
> +   * @param [out] aOldValueSet returns true if aValue now contains the previous
> +   *                           attribute value

Might be worth more clearly documenting that this will be true if the value was already set before this SetAndSwapMappedAttribute call, false otherwise.

::: dom/base/Element.h:1405
(Diff revision 2)
>     * @param [out] aRetval the nsresult status of the operation, if any.
>     * @return true if the setting was attempted, false otherwise.
>     */
> -  virtual bool SetMappedAttribute(nsIAtom* aName,
> +  virtual bool SetAndSwapMappedAttribute(nsIAtom* aName,
> -                                  nsAttrValue& aValue,
> +                                         nsAttrValue& aValue,
> +                                         bool& aOldValueSet,

Please use a pointer, not a reference, for an outparam.  

And maybe call this argument one aValueWasSet?

::: dom/base/Element.h:1434
(Diff revision 2)
>    }
>  
>    /**
>     * Hook that is called by Element::SetAttr to allow subclasses to
>     * deal with attribute sets.  This will only be called after we have called
> -   * SetAndTakeAttr and AttributeChanged (that is, after we have actually set
> +   * SetAndSwapAttr and AttributeChanged (that is, after we have actually set

It's called before AttributeChanged...  Looks like someone (likely me) forgot to update the comment when the ordering was changed.

::: dom/base/Element.h:1441
(Diff revision 2)
>     *
>     * @param aNamespaceID the namespace of the attr being set
>     * @param aName the localname of the attribute being set
>     * @param aValue the value it's being set to.  If null, the attr is being
>     *        removed.
> +   * @param aOldValue the value that the attribute was previously. If null,

s/was/had/

::: dom/base/Element.h:1442
(Diff revision 2)
>     * @param aNamespaceID the namespace of the attr being set
>     * @param aName the localname of the attribute being set
>     * @param aValue the value it's being set to.  If null, the attr is being
>     *        removed.
> +   * @param aOldValue the value that the attribute was previously. If null,
> +   *        the attr was not previously set. This attribute may not currently

"This argument may not have the correct value for SVG elements, or other cases in which the attr value doesn't store its own data" perhaps?

::: dom/base/Element.h:1449
(Diff revision 2)
>     * @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 AfterSetAttr(int32_t aNamespaceID, nsIAtom* aName,
> -                                const nsAttrValue* aValue, bool aNotify)
> +                                const nsAttrValue* aValue, 

Trailing whitespace.

::: dom/base/Element.cpp:2486
(Diff revision 2)
> +  NS_ENSURE_SUCCESS(rv, rv);
>  
>    // If the old value owns its own data, we know it is OK to keep using it.
> -  const nsAttrValue* oldValue =
> -      aParsedValue.StoresOwnData() ? &aParsedValue : &aOldValue;
> -
> +  // oldValue will be null if there was no previously set value
> +  const nsAttrValue* oldValue;
> +  if (aParsedValue.StoresOwnData()) {

This whole block of code could use better comments explaining what it's doing and why, e.g. setting oldValue = aOldValue is still safe if we used to not have the value (and hence should pass null for oldValue).

::: dom/base/nsAttrAndChildArray.h:95
(Diff revision 2)
>    const nsAttrValue* GetAttr(const nsAString& aName,
>                               nsCaseTreatment aCaseSensitive) const;
>    const nsAttrValue* AttrAt(uint32_t aPos) const;
>    // SetAndSwapAttr swaps the current attribute value with aValue.
> -  nsresult SetAndSwapAttr(nsIAtom* aLocalName, nsAttrValue& aValue);
> -  nsresult SetAndSwapAttr(mozilla::dom::NodeInfo* aName, nsAttrValue& aValue);
> +  nsresult SetAndSwapAttr(nsIAtom* aLocalName, nsAttrValue& aValue,
> +                          bool& aOldValueSet);

Please use a pointer for the outparam and document what it means.

It should probably be called aHadValue.

::: dom/base/nsAttrAndChildArray.h:97
(Diff revision 2)
>    const nsAttrValue* AttrAt(uint32_t aPos) const;
>    // SetAndSwapAttr swaps the current attribute value with aValue.
> -  nsresult SetAndSwapAttr(nsIAtom* aLocalName, nsAttrValue& aValue);
> -  nsresult SetAndSwapAttr(mozilla::dom::NodeInfo* aName, nsAttrValue& aValue);
> +  nsresult SetAndSwapAttr(nsIAtom* aLocalName, nsAttrValue& aValue,
> +                          bool& aOldValueSet);
> +  nsresult SetAndSwapAttr(mozilla::dom::NodeInfo* aName, nsAttrValue& aValue,
> +                          bool& aOldValueSet);

Same here.

::: dom/base/nsAttrAndChildArray.h:118
(Diff revision 2)
>    int32_t IndexOfAttr(nsIAtom* aLocalName, int32_t aNamespaceID = kNameSpaceID_None) const;
>  
> -  nsresult SetAndTakeMappedAttr(nsIAtom* aLocalName, nsAttrValue& aValue,
> +  nsresult SetAndSwapMappedAttr(nsIAtom* aLocalName, nsAttrValue& aValue,
>                                  nsMappedAttributeElement* aContent,
> -                                nsHTMLStyleSheet* aSheet);
> +                                nsHTMLStyleSheet* aSheet,
> +                                bool& aOldValueSet);

And here.

::: dom/base/nsMappedAttributeElement.h:45
(Diff revision 2)
>                                    mozilla::GenericSpecifiedValues* aGenericData);
>  
>    NS_IMETHOD WalkContentStyleRules(nsRuleWalker* aRuleWalker) override;
> -  virtual bool SetMappedAttribute(nsIAtom* aName,
> +  virtual bool SetAndSwapMappedAttribute(nsIAtom* aName,
> -                                  nsAttrValue& aValue,
> +                                         nsAttrValue& aValue,
> +                                         bool& aOldValueSet,

Pointer, please, and docs.  And maybe a better name.
Attachment #8866953 - Flags: review?(bzbarsky) → review+
Comment on attachment 8866953 [details]
Bug 1363481 - Add the old attribute value as a parameter to Element::AfterSetAttr

https://reviewboard.mozilla.org/r/138564/#review143748

Thank you for all your help on this. I will try to split reviews up better in the future.

> Pointer, please, and docs.  And maybe a better name.

Since this is an override of Element::SetAndSwapMappedAttribute, I will update the documentation there instead of duplicating it here.
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/74de5283b24d
Add the old attribute value as a parameter to Element::AfterSetAttr r=bz
https://hg.mozilla.org/mozilla-central/rev/74de5283b24d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Could you explain aCallAfterSetAttr param for SetAttrAndNotify? At least initially it feels rather weird and inconsistent and I have no idea when kDontCallAfterSetAttr is supposed to be passed.
Flags: needinfo?(ksteuber)
oh wait, that is even older stuff that I just don't remember.
Flags: needinfo?(ksteuber)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.