Bug 10230 – SVGDOMImplementation should die (and be rolled into DOMImplementation)

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

SVGDOMImplementation should die (and be rolled into DOMImplementation)

Having a separate DOMImplementation for SVG is a legacy from KDOM and should be removed.

This will allow hasFeature to work properly in WebKit for SVG.

[Rob Buis]

Created attachment 9856 [details]
First attempt

Let me know what you think. I assume no testcase is needed.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 9856 [details]
First attempt

Instead of upper.startsWith("HTTP://WWW.W3.ORG/TR/SVG11/FEATURE#") I'd like to see feature.startsWith(xxx, false).

The svgFeatureSet set should be a case insensitive set intsead of being a case sensitive HashSet.

But if we're going to use all uppercase it should not include "dom.svg.ALL" in mixed case!

I'd prefer not to use "upper()" at all.

Also, don't use braces around single lines in this code:

+    if (namespaceURI == SVGNames::svgNamespaceURI) {
+        doc = new SVGDocument(this, 0);
+    }

[Maciej Stachowiak]

Comment on attachment 9856 [details]
First attempt

r=me

[Maciej Stachowiak]

Comment on attachment 9856 [details]
First attempt

Ach, nevermind, given Darin's comments this should probably be r- so you can consider them. Looks good in general though.

[Rob Buis]

Created attachment 9871 [details]
improved patch

This patch should address the issues.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 9871 [details]
improved patch

I believe darin was refering to using HashSet<StringImpl, CaseInsensitiveHash> instead of HashSet<String>

That would avoid any need to upper/lower the strings.

The rest of hasFeature could be converted to using a set if you really felt adventurous.

Unless this changes already existing layout tests, a test case is needed.

Infact, ideally only features that we currently support should be present in that set.  The ideal test case would check hasFeature and isSupported for every possible value, and print out the value name and YES/NO.  Then when we add features to WebKit we would turn them on (uncomment them) in DOMImplementation.cpp, and that test case would be affected.

I'll leave this for darin/mjs to have a final opinion on.  I think the code is pretty much ready to land, but it would be good to have a test case, and good to only "turn on" hasFeature support for features we actually have fully (or mostly) implemented.

[Darin Adler]

Comment on attachment 9871 [details]
improved patch

Yes, Eric is right.

I suggest that svgFeatureSet be HashSet<String, CaseInsensitiveHash>. You should not need to call lower or upper at all.

The patch looks good -- but I think it would be better to use a case-insensitive hash.

[Darin Adler]

And yes, I think we should add a test for this, unless we already have one!

[Rob Buis]

Created attachment 9894 [details]
Now with testcase

Now I added a testcase, based on fast/dom/features.html. I also noticed some typos in the
features as well as some missing ones, they are now added and all tested using the testcase.
Note that I disabled those features that I think we do not support atm, but someone else
may have a different opinion about it, so probably someone will need to go through every
single feature, look it up in the spec and make a decision based on that. So far I have only
done that quickly, not in-depth.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 9894 [details]
Now with testcase

This looks fabulous.

two comments:

1. this comment is backwards:
+// TODO: features need to be commented out when we implement them

things should be "uncommented" once we implement them.

also I would probably have used FALSE and true or some more-easily distinguisable pair of vaues in the test case.

Looks great!
r=me.

[Darin Adler]

Comment on attachment 9894 [details]
Now with testcase

I believe the changes here to StringHash are wrong! Please do not land them.

Why are these relevant to this patch?

[Eric Seidel (no email)]

(In reply to comment #11)
> (From update of attachment 9894 [details] [edit])
> I believe the changes here to StringHash are wrong! Please do not land them.
> 
> Why are these relevant to this patch?
> 

Rob wanted a way to have a HashMap<String, CaseInsensitiveHash> instead of HashMap<StringImpl, CaseInsensitiveHash>

Could you provide a bit more detail about what's wrong with the hash additions?

[Darin Adler]

Comment on attachment 9894 [details]
Now with testcase

(In reply to comment #12)
> Could you provide a bit more detail about what's wrong with the hash additions?

This change:

     template<> struct StrHash<WebCore::StringImpl*> {
+        static unsigned hash(const WebCore::String& key) { return key.impl()->hash(); }
         static unsigned hash(const WebCore::StringImpl* key) { return key->hash(); }

should be unnecessary. That's a string hash for StringImpl* and it doesn't make sense to also add a hash to it for String, nor should it be needed.

The two equal function overloads inside CaseInsensitiveHash should be OK. It's reasonable to try to make CaseInsensitiveHash work for both String and StringImpl

But this one is definitely wrong:

+        static const WebCore::String& deletedValue() {
+            static WebCore::String null;
+            return null;
+        }

It's going to create a separate global variable for each translation unit that it's used in. It's also going to make empty values and deleted values be the same, since emptyValueIsZero causes a String with all values 0 (any null string) be the empty value. The hash code goes completely awry if empty values and deleted values are the same.

[Darin Adler]

(In reply to comment #13)
> It's going to create a separate global variable for each translation unit that
> it's used in. It's also going to make empty values and deleted values be the
> same, since emptyValueIsZero causes a String with all values 0 (any null
> string) be the empty value. The hash code goes completely awry if empty values
> and deleted values are the same.
I think that there may be a way to get this to work using HashKeyStorageTraits to make the String-based hash table be implemented by using a hash table that uses StringImpl* -- I don't know exactly how yet, though.

[Eric Seidel (no email)]

A simpler fix would be for rob to just use something like this:

svgFeatures->add(new StringImpl("Structure"));
svgFeatures is a long-lived global anyway.

[Rob Buis]

Created attachment 9912 [details]
Improved patch

With the new patch I hope to address these problems with the old code:
- whole hash table was leaked before
- 1.0 and 1.1 feature were mixed, so a substring from 1.1 could be appended to the 1.0 prefix, for example org.w3c.Script. Now I use two seperate hash tables, no mixing possible.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 9912 [details]
Improved patch

Ideally, I'd like to see the test cover some poorly-formed feature strings too, and test the case insensitivity more directly. It's good as-is, but could be even better.

+#if SVG_SUPPORT

Should use #ifdef SVG_SUPPORT as in the upcoming patch to fix compilation without SVG while using -Wundef.

You don't really need the initialized boolean. Can just check is the HashSet is empty using isEmpty.

But those are nitpicks, this looks fine now.

r=me

[Eric Seidel (no email)]

Hum... looks like the case-insensitivity was lost somewhere along the way:

http://www.din.or.jp/~hagi3/JavaScript/JSTips/Mozilla/Samples/hasFeature.htm

We might have to re-open this... unless I'm reading things wrong.

[Rob Buis]

Hi Eric

(In reply to comment #18)
> Hum... looks like the case-insensitivity was lost somewhere along the way:
> 
> http://www.din.or.jp/~hagi3/JavaScript/JSTips/Mozilla/Samples/hasFeature.htm

You are right...

> We might have to re-open this... unless I'm reading things wrong.

Nope, not reading things wrong. I looked at it for a while until I realized
it is caused by what I thought was a mostly harmless optimizing change, not
using static for the temp AtomicString. Ofcourse this looked more efficient but
probably messes up because of refcounting problems, sorry! Anyway I tested with static, which
you okayed in the first place, and that works. Let me know what I should do to commit the fix
of the fix, whether we should do review again etc.
Cheers,

Rob.

[Rob Buis]

Created attachment 9964 [details]
Fix for case-insensitive lookup

[David Kilzer (:ddkilzer)]

Reopening for Attachment 9964 [details].

Rob, it would really be most helpful if you simply opened new bugs for issues like this.  Thanks!

[David Kilzer (:ddkilzer)]

Comment on attachment 9912 [details]
Improved patch

Clearing Darin's review for Attachment 9912 [details] since it's already been landed, and Attachment 9964 [details] was added with an r? flag.

[Eric Seidel (no email)]

(In reply to comment #21)
> Reopening for Attachment 9964 [details] [edit].
> 
> Rob, it would really be most helpful if you simply opened new bugs for issues
> like this.  Thanks!
> 

So that Rob's fine reputation not be needlessly besmirched: It should be known that I encouraged rob to just attach the patch to this bug. ;)

[Darin Adler]

Comment on attachment 9964 [details]
Fix for case-insensitive lookup

This looks fine. But there are more-efficient alternatives:

For example, you could make this:

    static void addString(HashSet<StringImpl*, CaseInsensitiveHash>& set, const char* string)
    {
        StringImpl* s = new StringImpl(string);
        s->ref();
        add(set, s);
    }

Then you wouldn't need local variables at all:

    #define SVGFEATURE(feature) addString(svgFeatures, #feature);

And in fact, invoking this is simple enough that you don't even need to use a macro! And you don't need SVGFEATURE2.
Also, isSVG10Feature and isSVG11Feature should both be marked static.

Feel free to land what you have, or please post a new patch based on my suggestions.

[Rob Buis]

Created attachment 9987 [details]
Improved patch

This new patch follows Darin's nice suggestions. I verified that the tests still are ok,
including that site that thoroughly tests hasFeature.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 9987 [details]
Improved patch

r=me

[David Kilzer (:ddkilzer)]

(In reply to comment #23)
> So that Rob's fine reputation not be needlessly besmirched: It should be known
> that I encouraged rob to just attach the patch to this bug. ;)

Sorry if I offended anyone.  :(

If you're going to post new patches on bugs that have already been marked as RESOLVED, you just need to remember to reopen the bug, and clear any r+ flags on patches that have already been applied.

[Rob Buis]

(In reply to comment #27)
> (In reply to comment #23)
> > So that Rob's fine reputation not be needlessly besmirched: It should be known
> > that I encouraged rob to just attach the patch to this bug. ;)
> 
> Sorry if I offended anyone.  :(

No offense taken :)

> If you're going to post new patches on bugs that have already been marked as
> RESOLVED, you just need to remember to reopen the bug, and clear any r+ flags
> on patches that have already been applied.

Will try to do that from now on.
Cheers,

Rob.