Bug 11625 - Investigate possibility to share code between HTMLStyleElement and SVGStyleElement
Summary: Investigate possibility to share code between HTMLStyleElement and SVGStyleEl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-11-17 02:05 PST by Rob Buis
Modified: 2006-12-09 14:07 PST (History)
0 users

See Also:


Attachments
First attempt (21.50 KB, patch)
2006-11-17 02:14 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Another option (24.02 KB, patch)
2006-11-17 03:06 PST, Rob Buis
no flags Details | Formatted Diff | Diff
AtomicString improvements (25.30 KB, patch)
2006-11-19 05:48 PST, Rob Buis
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2006-11-17 02:05:22 PST
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.
Comment 1 Rob Buis 2006-11-17 02:14:10 PST
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.
Comment 2 Rob Buis 2006-11-17 03:06:08 PST
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.
Comment 3 Rob Buis 2006-11-17 12:49:12 PST
Landed in r17829.
Comment 4 Rob Buis 2006-11-17 12:50:30 PST
Oops, I meant to close 11635 instead of this one, reopening.
Cheers,

Rob.
Comment 5 Darin Adler 2006-11-18 17:54:47 PST
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.
Comment 6 Rob Buis 2006-11-19 05:31:09 PST
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.
Comment 7 Rob Buis 2006-11-19 05:48:25 PST
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.
Comment 8 Darin Adler 2006-12-07 16:05:42 PST
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
Comment 9 Darin Adler 2006-12-07 16:07:02 PST
Comment on attachment 11571 [details]
AtomicString improvements

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

Quotes here, not <>.
Comment 10 mitz 2006-12-09 14:07:42 PST
Landed in r18106.