232480 - Make nsGenericHTMLElement inherit nsGenericContainerElement

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The summary pretty much says it all. Now that both nsGenericHTMLElement and
nsGenericContainerElement has a |nsAttrAndChildArray mAttrsAndChildren| member
this should be pretty easy.

I don't want to try to kill nsGenericContainerElement yet. First of all svg
inherits from nsGenericElement and has its own attribute-code. Second, it might
make it easier to fix bug 198533 if we keep a simpler nsGenericElement.

I have an untested tree that does most of this work, I should have a working
patch soon

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v1

This patch does the following.

Let nsGenericHTMLElement inherit nsGenericContainerElement. Removes a bunch of
child and attribute-handling in the process since nsGenericContainerElement
handles that for us.

Move the mAttrsAndChildren member to nsGenericElement so that svg can use that
for storing children. This means that all childhandling is done by
nsGenericElement. This shouldn't complicate making nsXULElement inherit
nsGenericElement since nsXULElement will have that member too. Unfortunatly i
had to leave the code in nsSVGElement that forwarded calls to nsGenericElement.
See bg 233111.

Kill the nsGenericHTMLContainer/LeafElement classes and make all the subclasses
inherit nsGenericHTMLElement instead. This generated a lot of seach-n-replace
which is the bulk of this patch.

Remove the first argument to CopyInnerTo, it wasn't used and didn't make sense.


Removed some leftover code in nsHTMLButtonElement that wasn't used.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v1 body -w

Body of previous attachment. I.e. everything except the search-n-replace and
remove first argument to CopyInnerTo

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v1 search-n-replace -w

The search-n-replace changes. Diffed with -w

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 140685 [details] [diff] [review]
Patch v1 body -w

These are very very cool changes! Just a few minor nits:

- In nsGenericElement.h:

-  // XXX This can probably be static
-  NS_IMETHOD CopyInnerTo(nsIContent* aSrcContent,
-			  nsGenericContainerElement* aDest,
+  // XXX We should remove the aSrcContent argument
+  NS_IMETHOD CopyInnerTo(nsGenericContainerElement* aDest,
			  PRBool aDeep);

Didn't you just remove the aSrcContent argument?

-nsresult
-nsGenericHTMLElement::FindForm(nsIDOMHTMLFormElement **aForm)
+already_AddRefed<nsIDOMHTMLFormElement>
+nsGenericHTMLElement::FindForm()
 {
   // XXX: Namespaces!!!

Looks like this comment could be removed now, we do the right thing now, it
seems...

sr=jst

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 140686 [details] [diff] [review]
Patch v1 search-n-replace -w

sr=jst

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 140685 [details] [diff] [review]
Patch v1 body -w

> Index: base/src/nsGenericElement.cpp
> ===================================================================

> +nsresult
> +nsGenericElement::GetLastChild(nsIDOMNode** aNode)
> +{
> +  PRUint32 count = mAttrsAndChildren.ChildCount();
> +  
> +  if (count) {

I prefer count > 0

> Index: base/src/nsGenericElement.h
> ===================================================================

> @@ -733,54 +734,16 @@ public:

> +  // XXX We should remove the aSrcContent argument
> +  NS_IMETHOD CopyInnerTo(nsGenericContainerElement* aDest,
>                           PRBool aDeep);

Heh, what aSrcContent argument?

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

> @@ -327,13 +326,32 @@ nsGenericHTMLElement::CopyInnerTo(nsICon

> +  if (aDeep) {
> +    PRInt32 i;
> +    PRInt32 count = mAttrsAndChildren.ChildCount();
> +    for (i = 0; i < count; i++) {

++i

> +already_AddRefed<nsIDOMHTMLFormElement>
> +nsGenericHTMLElement::FindForm()

>    while (content) {
>      // If the current ancestor is a form, return it as our form
>      if (content->IsContentOfType(nsIContent::eHTML) &&
>          content->GetNodeInfo()->Equals(nsHTMLAtoms::form)) {
> -      return CallQueryInterface(content, aForm);
> +      nsIDOMHTMLFormElement* form = nsnull;
> +      CallQueryInterface(content, &form);

No need to initialize form to nsnull.

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

> @@ -327,13 +286,13 @@ nsHTMLButtonElement::SetFocus(nsIPresCon
>  {
>    if (!aPresContext)
>      return;
>  
>    // first see if we are disabled or not. If disabled then do nothing.
>    nsAutoString disabled;
> -  if (NS_CONTENT_ATTR_HAS_VALUE == GetAttribute(kNameSpaceID_None,
> +  if (NS_CONTENT_ATTR_HAS_VALUE == GetAttr(kNameSpaceID_None,
>                                                  nsHTMLAtoms::disabled,
>                                                  disabled)) {

Doesn't this need to check if disabled is "true"?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> Doesn't this need to check if disabled is "true"?

No, the html-spec doesn't mention anything about the values "true" or "false"
for the disabled-attribute. In fact it's an empty attribute.

And the old code didn't either. GetAttribute would just set the aResult-string
to "true"/"false", but that argument is ignored by the one and only caller.

In fact, i should make the code use HasAttr rather then GetAttr

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Forward to the correct parentclass

gotta forward to nsGenericContainer element in nsGenericHTMLElement since
that's what we inherit now

Comment 9

[Boris Zbarsky [:bzbarsky]]

This is when we wish C++ had "super" or we had a nsGenericHTMLElementSuper
define.... ;)

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Agreed.

Anyhow, fix checked in. Saved us some 13k on btek, 12k on luna and 2k on windows.

It did add a lot of warnings though, patch for that comming up soon

Comment 11

[Johnny Stenback (:jst)]

Sweet!

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fix warnings

This fixes warnings that were introduced by this patch. To fix the bulk of them
i changed the signature of CopyInnerTo to exactly match that of the parentclass
so that we override the parents version rather then hide it.

I also fixed some code that caused warnings in subclasse of
nsGenericDOMDataNode. The problem was that the
NS_IMPL_NSIDOMNODE_USING_GENERIC_DOM_DATA macro forwarded a few nsIDOM3Node
methods, one of which happened to hide a function in the baseclass. However
there is no need to forward these methods since the subclasses doesn't
implement nsIDOM3Node.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 141119 [details] [diff] [review]
Fix warnings

sr=jst

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 141119 [details] [diff] [review]
Fix warnings

-  PRInt32 i;
+  PRInt32 index;

I forgot to mention that this will actually cause a warning on some systems
too, index is a libc function, so this will cause a warning about index
shadowing... We've got these all over, and I think we pretty generally ignore
them, but still! :-)

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

since the patch is about removing warnings i'll just name the variable 'idx'

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

checked in, thanks for reviews!