236873 - Share class for more elements

Status: RESOLVED FIXED

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

Description

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

I was thinking of adding the following elements into nsHTMLSharedLeafElement:

directory, dl, legend, menu, quote, nsHTMLUnknownElement

The new class will be called nsHTMLSharedElement.

Additionally I plan on letting <object>, <applet> and <embed> move into a class
called:

nsHTMLSharedObjectElement

Comment 1

[Brendan Eich [:brendan]]

I've cvs-copied files as follows:

ls -l nsHTMLSharedLeafElement.cpp,v
-r--r--r--    1 cvsuser  mozsrc      40386 Mar  3 18:06
nsHTMLSharedLeafElement.cpp,v
[root@megalon src]# cp -p !$ nsHTMLSharedElement.cpp,v
cp -p nsHTMLSharedLeafElement.cpp,v nsHTMLSharedElement.cpp,v
[root@megalon src]# ls -l !$
ls -l nsHTMLSharedElement.cpp,v
-r--r--r--    1 cvsuser  mozsrc      40386 Mar  3 18:06 nsHTMLSharedElement.cpp,v
[root@megalon src]# cp -p nsHTMLObjectElement.cpp,v nsHTMLSharedObjectElement.cpp,v
[root@megalon src]# ls -l !$
ls -l nsHTMLSharedObjectElement.cpp,v
-r--r--r--    1 cvsuser  mozsrc      80505 Mar  3 18:06
nsHTMLSharedObjectElement.cpp,v

cvs remove the old and commit to the new at will!

/be

Comment 2

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

Is there a reason we don't just use nsHTMLSpanElement for unknown elements?
AFAICT the only difference is that the classinfo object will be HTMLSpanElement
vs. HTMLUnknownElement. Does that really matter?

Same goes for WBR (which already lives in nsHTMLSharedLeafElement)

Comment 3

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

Attachment: merge elements v1

This patch merges:
<menu>, <dir>, <q> and <basefont> into nsHTMLSharedElement.
<ul> and <dl> into nsHTMLOListElement (and renames it nsHTMLSharedListElement)
<ins> into nsHTMLDelElement (and renames it nsHTMLModElement)

I also moved the nsHTMLUnknownElement class into nsHTMLSpanElement.cpp. That
way it can inherit nsHTMLSpanElement and reuse the bulk of the code from that
class.

Comment 4

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

Comment on attachment 143460 [details] [diff] [review]
merge elements v1

Not sure if you want a separate reviewer for this? The patch is fairly large,
but it's all codeshuffeling.

Comment 5

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

Oh, <basefont> had some funky code for GetSize. However that didn't replicate
what IE did anyway so I just removed it and used the standard macro to implement
the function.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 143460 [details] [diff] [review]
merge elements v1

r+sr=jst

Comment 7

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

Comment on attachment 143460 [details] [diff] [review]
merge elements v1

requesting approval.

This patch is somewhat risky since it touches HTML-code. However the worst
thing that should happen is regressing visual rendering of lists or some of the
more obsucre elements like <q>, <menu> and <dir>

I'd really like to get this patch into beta in order to get good testing on it
before a release.

The patch looks big (3400 lines), but the bulk of it (2000 lines) is just
removing files that are no longer used, and the rest of it is just moving
existing code between files. No new code was written.

I'm currently doing as much testing as I can and so far have not found any
regressions caused by this patch (I did find a regression caused bug 232706
though, i'm filing a separate bug on that).

Comment 8

[Johnny Stenback (:jst)]

How much code size benefits would we get from this? IOW, is it worth it for
beta, or should we delay this one until 1.8a?

Comment 9

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

Attachment: testcase 1

tests rendering

Comment 10

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

Attachment: testcase 2

tests dom

I've ran both these tests without any regressions.
The first test makes sure that attribute-to-style mapping is correct, as well
as that we end up applying the same stylerules (the latter should not be
affected by this patch, but just in case).

The second test will make sure that parsing still is correct of the attributes,
and that the classinfo/QI is correct for all elements.

This should cover pretty much all changes that I can think of.

Comment 11

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

The filesize save is just under 42k, so it's not remarkably big, but at this
point i think the risk of regressions are pretty low.

Comment 12

[chris hofmann]

Comment on attachment 143460 [details] [diff] [review]
merge elements v1

think this is a better fit for 1.8a..

Comment 13

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

Checked in. However I need to rename a couple of more files so i'm keeping this
bug open for that.

Comment 14

[Boris Zbarsky [:bzbarsky]]

That patch caused (debug-only) crash bug 241058

Comment 15

[Peter Van der Beken [:peterv]]

Stealing this from Jonas while I merge applet/embed/object into nsHTMLSharedObjectElement.

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: SharedObject v1

This is the approach I originally had. Object is a form element though, so the shared class needs to selectively forward to nsGenericHTMLElement or nsGenericHTMLFormElement which is messy. I'll attach a different patch that just merges applet and embed.

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: SharedObject v2

This one just merges applet and embed into SharedObject. Object is still separate.

Comment 18

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

Comment on attachment 221819 [details] [diff] [review]
SharedObject v2

>Index: content/html/content/src/nsHTMLObjectElement.cpp
>+  void GetTypeAttrValue(nsCString &aValue) const
>+  {
>+    nsAutoString type;
>+    GetAttr(kNameSpaceID_None, nsGkAtoms::type, type);
>+
>+    CopyUTF16toUTF8(type, aValue);
>+  }

This function doesn't seem very useful. If we ever want to merge Object into SharedObject this part will be easy enough to deal with then. I don't care much though so feel free to go either way.


>Index: content/html/content/src/nsHTMLSharedObjectElement.cpp

>+  nsIAtom *URIAttrName() const
>+  {
>+    if (mNodeInfo->Equals(nsGkAtoms::applet)) {
>+      return nsGkAtoms::code;
>+    }
>+
>+    return nsGkAtoms::src;
>+  }

Nit: You can use ?: syntax here, that'd probably make the function more inlineable too.

>+nsHTMLSharedObjectElement::StartObjectLoad(PRBool aNotify)
...
>+      nsCAutoString type;
>+      GetTypeAttrValue(type);
>+      LoadObject(nsnull, aNotify, type);
>+    }
>+    return;
>   }
>+
>+  nsCAutoString type;
>+  GetTypeAttrValue(type);
>+  LoadObject(uri, aNotify, type);
> }

You could move the GetTypeAttrValue call to the top of the function. The one codepath that doesn't call doesn't seem common enough to optimize for.

Comment 19

[Peter Van der Beken [:peterv]]

Jonas: now that class and embed are merged I'm closing this bug. I think any other changes should get their own bug (not sure what you meant in comment 13).

Comment 20

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

Sounds great, i can't remember that comment either.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

I think this caused bug 338834.