We have lots of SetAttr implementations around: svg, html, xul, generic element. I went ahead and merged them all into a single implementation. This also happens to fix some other bugs we had lying about.

Summary of changes: 1) Introduce the following virtual methods on nsGenericElement: BeforeSetAttr -- called once we know we're changing an attr, before we parse the value. AfterSetAttr -- called after we set the newly parsed value. ParseAttribute -- parse a string into an nsAttrValue. GetEventListenerManagerForAttr -- gets the right ELM (the window's one in some XUL and HTML cases). GetAttrInfo -- gets the nsAttrName and nsAttrValue for a given namespace id and localname (needed for XUL, with its proto attrs). 2) Push SetAttrAndNotify up to nsGenericElement.h. Eliminate the other impls of this method. 3) Change nsGenericElement::AddScriptEventListener to take the event name atom, not the attr atom, and to use GetEventListenerManagerForAttr 4) Merge all 4 SetAttr impls into a single method in nsGenericElement. The execution order here is: Check whether we have a change, BeforeSetAttr, ParseAttribute, SetAttrAndNotify, AfterSetAttr. 5) Change the default attr storage from being strings for everything except XUL to be the same as XUL across the board -- atoms for short values, strings for longer ones. 6) Use atoms for namedItem in content lists, since most names are short. 7) Remove the AttrValueIs impls in nsXULElement, since using GetAttrInfo obviates the need for them. 8) Fix nsGenericHTMLFormElement to munge the hashtable correctly on attr removals too (that is, move all that code to BeforeSetAttr/AfterSetAttr) 9) Remove extra AfterSetAttr calls we had in nsHTMLInputElement. This patch is pretty hard to read; I'll attach the diff -w which is a little better, but if posting the files in their entirety would help, please tell me so.

Comment on attachment 195834 [details] [diff] [review] diff -w -- The XUL and HTML code is a lot more readable here An additional note: there is one case where we treat OOM in nsSVGElement::ParseAttribute as just failure to parse and try to store the attr as a string or atom instead. I think this is reasonable, given the lack of OOM handling in the rest of the attr-setting code, and this patch was already big enough without changing all the ParseAttribute impls. If people want that changed really badly, I'd like to do that in a followup bug.

Comment on attachment 195834 [details] [diff] [review] diff -w -- The XUL and HTML code is a lot more readable here Few comments. >+ /** >+ * Convert an attribute string value to attribute type based on the type of >+ * attribute. Called by SetAttr(). Note that at the moment we only do this >+ * for attributes in the null namespace (kNameSpaceID_None). >+ * >+ * @param aAttribute to attribute to convert >+ * @param aValue the string value to convert >+ * @param aResult the nsAttrValue [OUT] >+ * @return PR_TRUE if the parsing was successful, PR_FALSE otherwise >+ * @see nsGenericHTMLElement::SetAttr Is this right comment? "@see nsGenericHTMLElement::SetAttr" >- rv = mAttrsAndChildren.SetAndTakeAttr(aName, attrValue); >- NS_ENSURE_SUCCESS(rv, rv); >+ // XXXbz Perhaps we should push up the attribute mapping function >+ // stuff to nsGenericElement? >+ if (IsContentOfType(eHTML) && IsAttributeMapped(aName)) { >+ nsHTMLStyleSheet* sheet = document ? >+ document->GetAttributeStyleSheet() : nsnull; >+ rv = mAttrsAndChildren. >+ SetAndTakeMappedAttr(aName, aParsedValue, >+ NS_STATIC_CAST(nsGenericHTMLElement*, this), >+ sheet); I don't like this check, having element type specific code in nsGenericElement. Should we have PRBool SetMappedAttribute(name, value)? Though SVG uses mapped attributes in a bit different (wrong?) way. Not sure what is the best way to do this. Anyway because of this, r-. > >- if (aNamespaceID == kNameSpaceID_XMLEvents && aName == nsHTMLAtoms::_event && >- mNodeInfo->GetDocument()) { >+ if (aNamespaceID == kNameSpaceID_XMLEvents && >+ aName == nsHTMLAtoms::_event && mNodeInfo->GetDocument()) { > mNodeInfo->GetDocument()->AddXMLEventsContent(this); > } Eventually this could go to...hmm...nsGenericElement::BeforeSetAttr(). But that is a different bug, I think. >+ >+PRBool >+nsGenericElement::ParseAttribute(nsIAtom* aAttribute, >+ const nsAString& aValue, >+ nsAttrValue& aResult) >+{ >+ if (aAttribute == GetIDAttributeName() && !aValue.IsEmpty()) { >+ // Store id as an atom. id="" means that the element has no id, >+ // not that it has an emptystring as the id. >+ aResult.ParseAtom(aValue); >+ return PR_TRUE; >+ } I think we could handle also class attributes here. ...except SVG does something strange with |class|. Would have to check whether it works.

> Is this right comment? "@see nsGenericHTMLElement::SetAttr" That just needs to go away. Good catch. > I don't like this check, having element type specific code in > nsGenericElement. I don't either, but I couldn't think of a good way to do it, really... and yes, largely because what SVG does with mapped attrs is pretty wrong. I suppose we could do a SetMappedAttr here and only impl it in HTML... want me to? > except SVG does something strange with |class| Indeed. I'd really prefer to keep this patch from growing without bound... ;)

> 5) Change the default attr storage from being strings for everything except > XUL to be the same as XUL across the board -- atoms for short values, > strings for longer ones. I took this change out because of all the places that called GetStringValue() without checking the attr type. :(

Comment on attachment 196372 [details] [diff] [review] Same as -w >+nsGenericHTMLFormElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName, >+ const nsAString* aValue, PRBool aNotify) .... > > if (mForm && aName == nsHTMLAtoms::type) { >+ nsAutoString tmp; >+ > GetAttr(kNameSpaceID_None, nsHTMLAtoms::name, tmp); > > if (!tmp.IsEmpty()) { > mForm->AddElementToTable(thisControl, tmp); > } > > GetAttr(kNameSpaceID_None, nsHTMLAtoms::id, tmp); > > if (!tmp.IsEmpty()) { > mForm->AddElementToTable(thisControl, tmp); > } > > mForm->AddElement(thisControl); > } >- } > While you're here, could you change those ifs to be like if (NS_CONTENT_ATTR_HAS_VALUE == GetAttr(kNameSpaceID_None, nsHTMLAtoms::name, tmp)) ? Anyway, looks good to me. r=me. I hope this won't hurt Tp or Tdhtml too much.

> While you're here, could you change those ifs to be like That would be a change in behavior, which I would really rather not make unless we're sure we want it, and then in a separate patch... HTML returns NS_CONTENT_ATTR_HAS_VALUE even if the value is an empty string, as long as the attr is set.

(In reply to comment #10) > HTML returns > NS_CONTENT_ATTR_HAS_VALUE even if the value is an empty string, as long as the > attr is set. Ugh, it really does. Well then nm. r=me

Hey, now that we're frozen, can I get this out of my tree? ;)

Comment on attachment 197956 [details] [diff] [review] Same as diff -w Just some initial comments: >Index: content/base/src/nsGenericElement.h > //---------------------------------------- > > /** >- * Add a script event listener with the given attr name (like onclick) >+ * Add a script event listener with the given event name (like onclick) "like click"? >+ struct nsAttrInfo { >+ nsAttrInfo(const nsAttrName* aName, const nsAttrValue* aValue) : >+ mName(aName), mValue(aValue) {} >+ nsAttrInfo(const nsAttrInfo& aOther) : >+ mName(aOther.mName), mValue(aOther.mValue) {} >+ >+ const nsAttrName* mName; >+ const nsAttrValue* mValue; >+ >+ private: >+ nsAttrInfo() { NS_NOTREACHED("Don't call me!"); } This shouldn't be needed once you defined other ctors? >+ /** >+ * Convert an attribute string value to attribute type based on the type of >+ * attribute. Called by SetAttr(). Note that at the moment we only do this >+ * for attributes in the null namespace (kNameSpaceID_None). >+ * >+ * @param aAttribute the attribute to convert >+ * @param aValue the string value to convert >+ * @param aResult the nsAttrValue [OUT] >+ * @return PR_TRUE if the parsing was successful, PR_FALSE otherwise >+ */ >+ virtual PRBool ParseAttribute(nsIAtom* aAttribute, >+ const nsAString& aValue, >+ nsAttrValue& aResult); >+ >+ virtual PRBool SetMappedAttribute(nsIDocument* aDocument, >+ nsIAtom* aName, >+ nsAttrValue& aValue, >+ nsresult* aRetval); Do we really need this function? Couldn't you always call mAttrsAndChildren.SetAndTakeMappedAttr? >+ /** >+ * Hook that is called by nsGenericElement::SetAttr to allow subclasses to >+ * deal with attribute sets. This will only be called after we verify that >+ * we're actually doing an attr set and will be called before ParseAttribute >+ * and hence before we've set the new value. >+ * >+ * @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. >+ * // XXXbz we don't actually call this method when we're removing attrs yet. >+ * But we will eventually. I take it you want to do this in a separate patch? >+ * @param aNotify Whether we plan to notify document observers. >+ */ >+ virtual nsresult BeforeSetAttr(PRInt32 aNamespaceID, nsIAtom* aName, >+ const nsAString* aValue, PRBool aNotify); In general i'm a little bit concerned about the performance impact of these two additional virtual calls during parsing. It's also getting us back to a point where there are too many hooks for subclasses to hook into to react to attribute changes (ParseAttr, SetAttr, Before/AfterSetAttr) I had some idea like ParseAndReactToAttr but I never really figured out all details. >+ virtual nsAttrInfo GetAttrInfo(PRInt32 aNamespaceID, nsIAtom* aName) const; This seems hard to implement for XTF elements?

Missed one: >+ virtual PRBool ParseAttribute(nsIAtom* aAttribute, >+ const nsAString& aValue, >+ nsAttrValue& aResult); >+ Would it make the patch a lot bigger to add namespaceID here? We're going to have to do it sooner or later but it might make the patch harder to review...

> "like click"? No, definitely like onclick. Perhaps I should talk about an event handler name there, not event name. > This shouldn't be needed once you defined other ctors? Hmm... you might be right. OK. Will remove. > Couldn't you always call mAttrsAndChildren.SetAndTakeMappedAttr? That needs an nsGenericHTMLElement*. An initial version of this patch used a IsContentOfType check and explicit cast, but that's just as bad as having a virtual function here -- needs one virtual call... We could fix all that by pushing GetAttributeMappingFunction up to nsGenericElement, I suppose... But note that SVG doesn't actually want to set its attrs as mapped attrs, given what it does with them right now. > I take it you want to do this in a separate patch? Indeed. > It's also getting us back to a point where there are too many hooks for > subclasses to hook into to react to attribute changes (ParseAttr, SetAttr, > Before/AfterSetAttr) I did initially have ParseAttr and BeforeSetAttr in one function, but it seemed a little weird... Of course part of that was that BeforeSetAttr should really get the namespace, and ParseAttribute doesn't. More on that below. So if we fixed that, perhaps we could combine those into a single PrepareToSetAttr? > This seems hard to implement for XTF elements? True. At the same time, as long as we only use it in SetAttr and thereabouts, that's ok -- they only call up to the base class SetAttr if the base class is handling the attr. The only reason I need a separate method here at all instead of poking mAttrsAndChildren directly is the XUL proto attr stuff... That said, I'm open to suggestions on improving this! ;) > Would it make the patch a lot bigger to add namespaceID here? Yes. I'd have to touch every single nsHTML*Element impl, basically. I'm happy to fix this, but as a separate patch. At that point perhaps we could roll BeforeSetAttr into this method too, as I mentioned above?

So the alternative way of hooks that I thought of would be something like this: A ParseAttr like the one in this patch. A OnAttrChange(PRInt32 aNamespaceID, nsIAtom* aName, const nsAttrValue& aOldValue, const nsAttrValue& aNewValue, PRBool aNotify); On AttrChange would be called after ParseAttr but before the attribute is set in mAttrsAndChildren. This is to allow OnAttrChange to fail make us bail before anything in the element is changed. When UnsetAttr is called we call OnAttrChange with an empty nsAttrValue as aOldValue. We might need to add an |PRBool aRemove| argument if any OnAttrChange implementations need to differ between attributes being removed and attributes being set with emptyvalue as old value. But I think we should go with this patch for now anyway. First of all because I'm not sure that the above would be enough for all elements. Second because this patch is written and is a step in the right direction.

Actually, it might be better to call OnAttrChange *after* setting the attribute in mAttrsAndChildren since we can get the old value for free then. We'd just have to make sure to put the old value back in case OnAttrChange fails. Just documenting this stuff so we can investigate after this bug is fixed.

(In reply to comment #18) > When UnsetAttr is called we call OnAttrChange with an empty nsAttrValue as > aOldValue. We might need to add an |PRBool aRemove| argument if any > OnAttrChange implementations need to differ between attributes being removed > and attributes being set with emptyvalue as old value. The above is a little confused. I ment of course that we need |aRemove| if we need to differ between the attribute being removed and the attribute being set to empty value. |aOldValue| should be appropriatly set even during UnsetAttr calls. Anyhow, back to the our ordinary scheduled programming; this patch :)

Comment on attachment 197956 [details] [diff] [review] Same as diff -w I'm a little worried about the extra virtual calls in this patch, so keep an eye on tinderbox when you check in. I've commented on a few where I think a little bit of code duplication might be worth some saved perf. In general this is an awesome patch though. Nice to see all that code duplication die. And whereever we want to go eventually, this takes us much closer. >Index: content/base/src/nsGenericElement.cpp >+nsGenericElement::SetAttrAndNotify(PRInt32 aNamespaceID, ... >+ // XXXbz Perhaps we should push up the attribute mapping function >+ // stuff to nsGenericElement? IMHO we should. I bet XUL could be cleaned up with better attribute mapping code. SVG is a problem though since it wants to treat mapped attributes in a special way. > nsresult >+nsGenericElement::BeforeSetAttr(PRInt32 aNamespaceID, nsIAtom* aName, >+ const nsAString* aValue, PRBool aNotify) >+{ >+ return NS_OK; >+} >+ >+nsresult >+nsGenericElement::AfterSetAttr(PRInt32 aNamespaceID, nsIAtom* aName, >+ const nsAString* aValue, PRBool aNotify) >+{ >+ return NS_OK; >+} Inline these so they are optimized away when called by child classes? >@@ -3695,31 +3770,31 @@ PRBool > nsGenericElement::AttrValueIs(PRInt32 aNameSpaceID, > nsIAtom* aName, > const nsAString& aValue, > nsCaseTreatment aCaseSensitive) const > { > NS_ASSERTION(aName, "Must have attr name"); > NS_ASSERTION(aNameSpaceID != kNameSpaceID_Unknown, "Must have namespace"); > >- const nsAttrValue* val = mAttrsAndChildren.GetAttr(aName, aNameSpaceID); >+ const nsAttrValue* val = GetAttrInfo(aNameSpaceID, aName).mValue; > return val && val->Equals(aValue, aCaseSensitive); > } Would it be a good idea perf-wise to keep the AttrValueIs implementations as they are? I bet this will get called a lot so it might be worth a little extra code. >+nsresult >+nsGenericHTMLElement::GetEventListenerManagerForAttr(nsIEventListenerManager** aManager, >+ nsISupports** aTarget, >+ PRBool* aDefer) >+{ >+ // Attributes on the body and frameset tags get set on the global object >+ if (mNodeInfo->Equals(nsHTMLAtoms::body) || >+ mNodeInfo->Equals(nsHTMLAtoms::frameset)) { >+ nsIScriptGlobalObject *sgo; >+ >+ // If we have a document, and it has a script global, add the >+ // event listener on the global. If not, proceed as normal. >+ // XXXbz sXBL/XBL2 issue: should we instead use GetCurrentDoc() here, >+ // override BindToTree for those classes and munge event listeners there? >+ nsIDocument *document = GetOwnerDoc(); >+ nsresult rv = NS_OK; >+ if (document && (sgo = document->GetScriptGlobalObject())) { >+ nsCOMPtr<nsIDOMEventReceiver> receiver(do_QueryInterface(sgo)); >+ NS_ENSURE_TRUE(receiver, NS_ERROR_FAILURE); >+ >+ rv = receiver->GetListenerManager(aManager); >+ >+ NS_ADDREF(*aTarget = sgo); For sake of correctness you should only set aTarget if rv is a success value. Just in case someone calls this without using nsCOMPtrs. >+nsresult >+nsGenericHTMLFormElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName, >+ const nsAString* aValue, PRBool aNotify) >+{ >+ if (aNameSpaceID == kNameSpaceID_None) { >+ nsCOMPtr<nsIFormControl> thisControl; > >- if (!tmp.IsEmpty()) { >- mForm->AddElementToTable(thisControl, tmp); >+ QueryInterface(NS_GET_IID(nsIFormControl), getter_AddRefs(thisControl)); I don't really like direct calls to QI though it looks like this was a copy-n-paste job. Mind making it using do_QI(NS_STATIC_CAST(this))? >Index: content/xul/content/src/nsXULElement.h >+ /** >+ * Get the attr info for the given namespace ID and attribute name. >+ * The namespace ID must not be kNameSpaceID_Unknown and the name >+ * must not be null. >+ */ >+ virtual nsAttrInfo GetAttrInfo(PRInt32 aNamespaceID, nsIAtom* aName) const; >+ > const nsAttrValue* FindLocalOrProtoAttr(PRInt32 aNameSpaceID, >- nsIAtom *aName) const; >+ nsIAtom *aName) const { >+ return GetAttrInfo(aNameSpaceID, aName).mValue; >+ } This'll add virtuality to FindLocalOrProtoAttr... Maybe you could make this return nsXULElement::GetAttrInfo(aNameSpaceID, aName).mValue; since I doubt that we'll override in subclasses. (stupid c++ standard) Or maybe just leave it as it was since that'll avoid wasted call into nsAttrAndChildArray. >Index: content/xul/content/src/nsXULElement.cpp >+nsXULElement::GetEventListenerManagerForAttr(nsIEventListenerManager** aManager, ... > nsIContent *root = doc->GetRootContent(); >- nsCOMPtr<nsIContent> content(do_QueryInterface(NS_STATIC_CAST(nsIStyledContent*, this))); >- if ((!root || root == content) && !mNodeInfo->Equals(nsXULAtoms::overlay)) { >+ if ((!root || root == this) && !mNodeInfo->Equals(nsXULAtoms::overlay)) { > nsIScriptGlobalObject *global = doc->GetScriptGlobalObject(); > > nsCOMPtr<nsIDOMEventReceiver> receiver = do_QueryInterface(global); > if (! receiver) > return NS_ERROR_UNEXPECTED; > >- rv = receiver->GetListenerManager(getter_AddRefs(manager)); >- >- target = global; >- defer = PR_FALSE; >- } >- else { >- rv = GetListenerManager(getter_AddRefs(manager)); >+ nsresult rv = receiver->GetListenerManager(aManager); >+ NS_ADDREF(*aTarget = global); >+ *aDefer = PR_FALSE; >+ return rv; Again, you should probably only set aTarget on success. >+nsresult >+nsXULElement::AfterSetAttr(PRInt32 aNamespaceID, nsIAtom* aName, >+ const nsAString* aValue, PRBool aNotify) Namespace checks missing all over this function. I never thought i'd get to tell *you* that ;) >+PRBool >+nsXULElement::ParseAttribute(nsIAtom* aAttribute, >+ const nsAString& aValue, >+ nsAttrValue& aResult) ... >+ PRBool rv = nsGenericElement::ParseAttribute(aAttribute, aValue, aResult); Eep! Don't call this |rv|, major source of confusion. |parsed| or something sounds better, if you need the temporary at all. With that, r=me

(In reply to comment #21) > Would it be a good idea perf-wise to keep the AttrValueIs implementations as > they are? I bet this will get called a lot so it might be worth a little extra > code. Fair point. I undid this change. > This'll add virtuality to FindLocalOrProtoAttr... Maybe you could make this > > return nsXULElement::GetAttrInfo(aNameSpaceID, aName).mValue; Did that. That method has enough code in it that I don't think it's worth duplicating, since all the calls are now non-virtual. > Namespace checks missing all over this function. I never thought i'd get to > tell *you* that ;) Man. I must have been _tired_. ;) Fixed the rest of it. Updated patch coming up.

Comment on attachment 200957 [details] [diff] [review] Same as diff -w + nsresult SetAttrAndNotify(PRInt32 aNamespaceID, + nsIAtom* aName, + nsIAtom* aPrefix, + const nsAString& aOldValue, + nsAttrValue& aParsedValue, + PRBool aModification, + PRBool aFireMutation, + PRBool aNotify); Hmm, maybe it's time to change those three boolean arguments to a single flags argument soon? Maybe file a followup bug if you agree... - In nsGenericElement::SetAttrAndNotify(): - if (HasMutationListeners(this, NS_EVENT_BITS_MUTATION_ATTRMODIFIED)) { + if (aFireMutation) { Don't we still want to keep the mutation listener check optimization here? sr=jst

> Hmm, maybe it's time to change those three boolean arguments to a single flags > argument soon? Hmm.... Might not be a bad idea. ;) I'll file a followup on this. > Don't we still want to keep the mutation listener check optimization here? We do. The caller passes in the return value of HasMutationListeners() for aFireMutation. It's just that the caller needs to do HasMutationListeners() anyway to decide whether to grab the old value, so this is a way of not calling it twice...

I just noticed that this always sets |modification| to true in nsGenericElement::SetAttr

err.. make that 'false'. The 'always' part is what's wrong anyway.

Comment on attachment 201080 [details] [diff] [review] Doh. This should do the trick I checked this in for now to make sure that things don't break... But I still want you to check it out. ;)

The windows compiler seems to not be happy letting subclasses access protected structs declared in superclasses...

Comment on attachment 201080 [details] [diff] [review] Doh. This should do the trick sr=jst

Checked in, with various bustage fixes, etc. I'll work on filing followups tomorrow.

Filed bug 314286 as a followup.

This caused bug 314568