Bug 11625 – Investigate possibility to share code between HTMLStyleElement and SVGStyleElement

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Rob Buis]

SVGStyleElement contains this comment:

 // FIXME: this code should be shared with HTMLStyleElement::childrenChanged()

The idea is to investigate how much can be shared and whether it is worth it.

[Rob Buis]

Created attachment 11547 [details]
First attempt

My first stab at this. It seems unfortunately MI or a helper class has to be used simply because one is a html and one is a svg class. Let me know if this is a good idea and worth it.
Cheers,

Rob.

[Rob Buis]

Created attachment 11550 [details]
Another option

It is possible to take it one step further and merge some of the code paths in Document::recalcStyleSelector, as this patch shows.
Cheers,

Rob.

[Rob Buis]

Landed in r17829.

[Rob Buis]

Oops, I meant to close 11635 instead of this one, reopening.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 11550 [details]
Another option

The Document.cpp change is great, but it seems like a separate fix to me -- it's independent of the inheritance change.

THe new base class seems OK. It's good not to have two entire copies of the code, even though we end up using multiple inheritance.

+    , StyleElement()

You should not need to explicitly invoke the base class's default constructor like this.

+//fprintf(stderr, "isLoading %d\n", m_loading);

Please don't land these.

Why all the changes from AtomicString to String? When possible, it's great to have functions take AtomicString parameters, in case you're already calling with an atomic string -- saves a hash table lookup. And it's great to have const AtomicString& return types instead of String in cases where that works -- much more efficient, but you can't always do it.

[Rob Buis]

Hi Darin,

(In reply to comment #5)
> (From update of attachment 11550 [details] [edit])
> The Document.cpp change is great, but it seems like a separate fix to me --
> it's independent of the inheritance change.

Right. At first I wanted to be able to cast both SVGStyle and HTMLStyle to a common base class and use that, and in the end not needing SVG_SUPPORT tests. I don't think that is doable anymore since svg needs slightly different behaviour here than html. Also it would require a dynamic_cast.

> THe new base class seems OK. It's good not to have two entire copies of the
> code, even though we end up using multiple inheritance.
> 
> +    , StyleElement()
> 
> You should not need to explicitly invoke the base class's default constructor
> like this.

Fixed.

> +//fprintf(stderr, "isLoading %d\n", m_loading);
> 
> Please don't land these.

Heh, I need to check better before submitting. Fixed.

> Why all the changes from AtomicString to String? When possible, it's great to
> have functions take AtomicString parameters, in case you're already calling
> with an atomic string -- saves a hash table lookup. And it's great to have
> const AtomicString& return types instead of String in cases where that works --
> much more efficient, but you can't always do it.

I tried it and I have an upcoming patch for it. Indeed the code seems much better that way.
Cheers,

Rob.

[Rob Buis]

Created attachment 11571 [details]
AtomicString improvements

This patch should address Darin's issues with the previous patch. Note the AtomicString changes in particular. I think something like tryGetAttribute(NS) should either be used everywhere or not at all, ie. it makes little sense IMHO to limit it to svg use only. Seeing that knowing up front the default value is not so common, I think we do not need tryGetAttribute(NS).
Cheers,

Rob.

[Darin Adler]

Comment on attachment 11571 [details]
AtomicString improvements

+            Element *e = static_cast<Element *>(n);

Should move the * next to Element.

+#include "Node.h"
+#include "Document.h"

No need to include both Document and Node, since Document is a Node.

+    String _type = type();

It's best to not use leading underscores in identifiers, since those are reserved for the compiler and operating system.

+class MappedAttribute;

This is a forward declaration for a class not used in the file; please leave it out.

+class StyleElement
+{

Where did the following code go?

-    if (attr->name() == typeAttr)
-        m_type = attr->value().domString().lower();

Again, please don't use underscores:

+    static const AtomicString _default("text/ecmascript");
+    static const AtomicString _xmlspace("xml:space");
+    static const AtomicString _default("all");

r=me

[Darin Adler]

Comment on attachment 11571 [details]
AtomicString improvements

 #include <SVGElement.h>
+#include <StyleElement.h>

Quotes here, not <>.

Landed in r18106.