Closed Bug 232016 Opened 21 years ago Closed 20 years ago

SetHTMLAttribute must die

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sicking, Assigned: sicking)

References

Details

Attachments

(1 file)

First of all it adds unneccesary complexity in nsGenericHTMLElement to have to
deal with two types of attributes comming in (both unparsed strings and parsed
nsHTMLValues).

Second it bypasses the entire mechanism where elements can override SetAttr to
detect when attributes are changed and act accordingly. Which means that we're
bound to get bugs.

Third it twarts attempts at killing nsIHTMLContent, since it'd be a bit nasty to
move SetHTMLAttribute to nsIContent.

Forth, it relies on the caller to use the correct datatype, which means that the
code that desides datatype for a specific attribute is spread out.


Off with it's head!
Blocks: 166650
Status: NEW → ASSIGNED
Attached patch patch to fixSplinter Review
This patch kills SetHTMLAttribute compleatly. Most of the callers came from the
various map-attribute-to-property macros that we have in nsGenericHTMLElement.h
so I decided to include some cleanup for those too.

The cleanup is mainly making the functions that the macros generate smaller by
having them call into functions in nsGenericHTMLElement. There should not be a
perf-issue since this code is rarly called and the overhead is minimal.

I also looked through all the html-element classes and converted as many
functions as I could to use the macros to take advantage of the smaller
implementations as well as make future changes easier. This included moving
some code from nsHTMLFooElement::SetBar into nsHTMLFooElement::SetAttr which
will fix some obsure bugs where calling .setAttribute("bar", "...") didn't
behave the same as .bar = "...".
Assignee: general → bugmail
Attachment #141582 - Flags: superreview?(jst)
Attachment #141582 - Flags: review?(peterv)
Actually, the animation-resetting behavior was rather purposefully _not_ in
SetAttr, as I recall... at least I thought we had reasons for doing that....

At least, condition all that rigmarole on aNotify -- we don't want to be
resetting animations just 'cause we parsed an image.
Well, if you don't want it to reset on .setAttribute better speak up now or it's
going ;-)

But i did purposly include an aNotify-check for that very reason.
Comment on attachment 141582 [details] [diff] [review]
patch to fix

+void
+nsGenericHTMLElement::GetIntAttr(nsIAtom* aAttr, PRInt32 aDefault, PRInt32*
aResult)

Any reason not to just return the result in stead of using an out param?

+nsresult
+nsGenericHTMLElement::GetURIAttr(nsIAtom* aAttr, nsAString& aResult)

Looks like this always returns NS_OK, so maybe make it a void function?

sr=jst
Attachment #141582 - Flags: superreview?(jst) → superreview+
> +void
> +nsGenericHTMLElement::GetIntAttr(nsIAtom* aAttr, PRInt32 aDefault, PRInt32*
> aResult)
> 
> Any reason not to just return the result in stead of using an out param?

Yes, this way the code will be smaller in all the callers of GetIntAttr. Which
is the main reason for GetIntAttrs existance.

> +nsresult
> +nsGenericHTMLElement::GetURIAttr(nsIAtom* aAttr, nsAString& aResult)
> 
> Looks like this always returns NS_OK, so maybe make it a void function?

Will do since that's what i do in the GetXXXAttr. Though i do actually suspect
that the codesize would be reduced in all callers this way, might be something
to look into in the future.
Comment on attachment 141582 [details] [diff] [review]
patch to fix

> Index: content/html/content/src/nsGenericHTMLElement.cpp
> ===================================================================

> @@ -2098,14 +2046,39 @@ nsGenericHTMLElement::GetInlineStyleRule
>  }
>  
>  nsresult
>  nsGenericHTMLElement::SetInlineStyleRule(nsICSSStyleRule* aStyleRule,
>                                           PRBool aNotify)
>  {
> -  return SetHTMLAttribute(nsHTMLAtoms::style, nsHTMLValue(aStyleRule),
> -                          aNotify);
> +  PRBool hasListeners = PR_FALSE;
> +  PRBool modification = PR_FALSE;
> +  nsAutoString oldValueStr;
> +
> +  if (mDocument) {
> +    hasListeners = nsGenericElement::HasMutationListeners(this,
> +      NS_EVENT_BITS_MUTATION_ATTRMODIFIED);
> +  
> +    // We can't compare the stringvalues of the old and the new rules
> +    // since both will point to the same declaration and thus will be
> +    // the same.
> +    if (aNotify) {
> +      modification = !!mAttrsAndChildren.GetAttr(nsHTMLAtoms::style);

Maybe add a comment here (you proposed |We're always getting a new
nsIStyleCSSRule (or whatever it is) here so no need to check that we're getting
a new object|.

> +    }
> +    else if (hasListeners) {
> +      // save the old attribute so we can set up the mutation event properly
> +      modification = GetAttr(kNameSpaceID_None, nsHTMLAtoms::style,
> +                             oldValueStr) != NS_CONTENT_ATTR_NOT_THERE;
> +    }

You need to flip around the aNotify and hasListeners tests to set the
oldValueStr when hasListener and aNotify are true.

> Index: content/html/content/src/nsGenericHTMLElement.h
> ===================================================================

>  /**
>   * A macro to implement the getter and setter for a given content
>   * property that needs to return a URI in string form.  The method
>   * uses the generic GetAttr and SetAttr methods.  This macro is much
>   * like the NS_IMPL_STRING_ATTR macro, except we make sure the URI is
>   * absolute.
>   */
> -#define NS_IMPL_URI_ATTR_GETTER(_class, _method, _atom)             \
> -  NS_IMETHODIMP                                                     \
> -  _class::Get##_method(nsAString& aValue)                           \
> -  {                                                                 \
> -    return AttrToURI(nsHTMLAtoms::_atom, aValue);                   \
> -  }
> -#define NS_IMPL_URI_ATTR_SETTER(_class, _method, _atom)             \
> +#define NS_IMPL_URI_ATTR(_class, _method, _atom)                    \
> +  NS_IMPL_URI_ATTR_GETTER(_class, _method, _atom)                   \
>    NS_IMETHODIMP                                                     \
>    _class::Set##_method(const nsAString& aValue)                     \
>    {                                                                 \
>      return SetAttr(kNameSpaceID_None, nsHTMLAtoms::_atom, aValue,   \
>                     PR_TRUE);                                        \
>    }
> -#define NS_IMPL_URI_ATTR(_class, _method, _atom) \
> -  NS_IMPL_URI_ATTR_GETTER(_class, _method, _atom) \
> -  NS_IMPL_URI_ATTR_SETTER(_class, _method, _atom)
> +
> +#define NS_IMPL_URI_ATTR_GETTER(_class, _method, _atom)             \
> +  NS_IMETHODIMP                                                     \
> +  _class::Get##_method(nsAString& aValue)                           \
> +  {                                                                 \
> +    return GetURIAttr(nsHTMLAtoms::_atom, aValue);                  \
> +  }

No need to move this under NS_IMPL_URI_ATTR, is there?

> Index: content/html/content/src/nsHTMLInputElement.cpp
> ===================================================================

> +NS_IMPL_STRING_ATTR(nsHTMLInputElement, DefaultValue, defaultvalue)
> +NS_IMPL_BOOL_ATTR(nsHTMLInputElement, DefaultChecked, defaultchecked)

Hmm, you're using different atoms than we used to? defaultvalue needs to be
value and defaultchecked needs to be checked

> Index: content/html/content/src/nsHTMLLabelElement.cpp
> ===================================================================

> -NS_IMETHODIMP
> -nsHTMLLabelElement::SetHtmlFor(const nsAString& aValue)
> -{
> -  // trim leading and trailing whitespace 
> -  static char whitespace[] = " \r\n\t";
> -  nsAutoString value(aValue);
> -  value.Trim(whitespace, PR_TRUE, PR_TRUE);
> -  return nsGenericHTMLFormElement::SetAttr(kNameSpaceID_None,
> -                                           nsHTMLAtoms::_for, value, PR_TRUE);
> -}
> +NS_IMPL_STRING_ATTR(nsHTMLLabelElement, HtmlFor, _for)

We don't want to do the whitespace trimming?

> Index: content/html/content/src/nsHTMLOptionElement.cpp
> ===================================================================

> @@ -345,79 +353,16 @@ nsHTMLOptionElement::SetSelected(PRBool 
>      return SetSelectedInternal(aValue, PR_TRUE);
>    }
>  
>    return NS_OK;
>  }
>  
> -//NS_IMPL_BOOL_ATTR(nsHTMLOptionElement, DefaultSelected, defaultselected)
> -//NS_IMPL_INT_ATTR(nsHTMLOptionElement, Index, index)
> -//NS_IMPL_STRING_ATTR(nsHTMLOptionElement, Label, label)
> +NS_IMPL_BOOL_ATTR(nsHTMLOptionElement, DefaultSelected, defaultselected)

defaultselected should be selected.

> Index: content/html/document/src/nsPluginDocument.cpp
> ===================================================================

> @@ -179,31 +179,31 @@ nsPluginDocument::CreateSyntheticPluginD

>    // fill viewport and auto-reize

Correct reize while you're here.

> +  mPluginContent->SetAttr(kNameSpaceID_None, nsHTMLAtoms::src,
> +                          NS_ConvertUTF8toUCS2(src), PR_FALSE);

Make that NS_ConvertUTF8toUTF16.

> +  mPluginContent->SetAttr(kNameSpaceID_None, nsHTMLAtoms::type,
> +                          NS_ConvertUTF8toUCS2(mMimeType), PR_FALSE);

Make that NS_ConvertUTF8toUTF16.
Attachment #141582 - Flags: review?(peterv) → review+
> No need to move this under NS_IMPL_URI_ATTR, is there?

Removed the NS_IMPL_URI_ATTR_GETTER macro entierly, it wasn't used any more.

> Hmm, you're using different atoms than we used to? defaultvalue needs to be
> value and defaultchecked needs to be checked
> defaultselected should be selected.

Good catch!! Fixed.

> We don't want to do the whitespace trimming?

No, IE doesn't do it and there is no spec that says we should do it.

Fixed everything else
Checked in. Binsize savings ranged from 10k (win) to 20k (osx)
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: