838501 - Refactor SVGElementFactory

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: patch based on how html does it

See mxr.mozilla.org/mozilla-central/source/parser/htmlparser/public/nsHTMLTagList.h and http://mxr.mozilla.org/mozilla-central/source/parser/htmlparser/src/nsHTMLTags.cpp for how html does it.

The patch is based on that.

Comment 1

[Daniel Holbert [:dholbert]]

Comment on attachment 710552 [details] [diff] [review]
patch based on how html does it

This is just a partial review -- I tried building w/ this patch applied, and I get this build error (w/ GCC 4.7):
{
content/svg/content/src/SVGElementFactory.cpp:80:0:
content/svg/content/src/SVGTagList.h:21:1: error: pasting "::" and "a" does not give a valid preprocessing token
content/svg/content/src/SVGTagList.h:22:1: error: pasting "::" and "altGlyph" does not give a valid preprocessing token
[etc]
}

Looks like something needs fixing there -- I'll hold off on reviewing the rest of that until that's fixed.  Here are my review-nits from before I hit that build error, though:

>+typedef nsresult
>+  (*contentCreatorCallback)(nsIContent** aResult,
>+                            already_AddRefed<nsINodeInfo>,
>+                            FromParser aFromParser);

Nit: looks like the second param needs a name there, for consistency.

So, you probably should add " aNodeInfo" after ">" (or if you prefer, just drop the names of the other params.)

>+++ b/content/svg/content/src/SVGTagList.h
>+  This file contains the list of all SVG tags 

Nit: Add a period at the end of that sentence.

>+  All entries must be enclosed in the macro SVG_TAG which will have cruel
>+  and unusual things done to it

...and that one.

(I noticed that these were missing periods in the HTML version, too, so I filed bug 838715 to fix it over there as well. :))

>+SVG_TAG(rect, Rect)
>+SVG_FROM_PARSER_TAG(svg, SVG)

This line violates the documentation that I quoted above. ("All entries must be enclosed in the macro SVG_TAG")

Could you tweak the documentation to indicate that SVG_FROM_PARSER_TAG is also allowed, and briefly explain how it differs / what sorts of tags need it?

>+SVG_TAG(path, Path)
>+SVG_TAG(text, Text)
>+SVG_TAG(tspan, TSpan)
>+SVG_TAG(image, Image)
>+SVG_TAG(style, Style)
[etc]

The ordering of this list seems a bit arbitrary, aside from some obvious local groupings... Perhaps the list should be in alphabetical order, like the one in nsHTMLTagList.h?

That change doesn't necessarily need to happen here, though -- it looks like you might be preserving our pre-existing (arbitrary?) ordering from nsSVGElementFactory, and I suppose that's reasonable for a refactoring patch like this.

Comment 2

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 710960 [details] [diff] [review]
address review comments

>+void
>+SVGElementFactory::Init()
>+{
>+#define SVG_TAG(_tag, _classname) sTagAtomTable[eSVGTag_##_tag] = nsGkAtoms::_tag;
>+#define SVG_FROM_PARSER_TAG(_tag, _classname) sTagAtomTable[eSVGTag_##_tag] = nsGkAtoms::_tag;
>+#include "SVGTagList.h"
>+#undef SVG_TAG
>+#undef SVG_FROM_PARSER_TAG
>+
>+  gTagAtomTable = PL_NewHashTable(64, SVGTagsHashCodeAtom,

The magic-number there makes me a little queasy, but I suppose it's what we use in nsHTMLTags.cpp, too, so I won't object.

>+  void* tag = PL_HashTableLookupConst(gTagAtomTable, name);
>+  if (tag) {
>+    contentCreatorCallback cb = sContentCreatorCallbacks[NS_PTR_TO_INT32(tag)];
>+
>+    return cb(aResult, aNodeInfo, aFromParser);
>+  }

"tag" is a confusing name for that variable. 'name' is a tag, whereas 'tag' is actually a void*-encoded index -- technically it's an "enum SVGTag".

I think this would be clearer with s/tag/tagEnum/ (or encodedTagEnum).


ALSO: It looks like we're treating "tag == nsnull" to mean our entry wasn't found. But isn't nsnull an encoding of the (valid) SVGEnum value of 0, i.e. the enum value for our first SVG tag (depending on where our enum starts counting from)?

(maybe I'm missing something)

I'll bet this is why nsHTMLTags adds 1 to the enum values before inserting them into the hash table.  Maybe we need to do that as well?

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 710960 [details] [diff] [review]
address review comments

Just to follow up / expand on my last comment above -- the PL_HashTableLookup MDN page says "one cannot tell whether a NULL return value means the entry does not exist or the value of the entry is NULL. Keep this ambiguity in mind if you want to store NULL values in a hash table."

  https://developer.mozilla.org/en-US/docs/PL_HashTableLookup

and this is the chunk of code that I think runs afoul of this & stores NULL in the hash table, when i == 0:
>+  for (uint32_t i = 0; i < eSVGTag_Count; ++i) {
>+    PL_HashTableAdd(gTagAtomTable, sTagAtomTable[i], NS_INT32_TO_PTR(i));
>+  }
>+}

Comment 5

[Robert Longson [:longsonr]]

Attachment: simplify and correct

Comment 6

[Robert Longson [:longsonr]]

Exellent catch on the nullptr = 0 issue BTW

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 711178 [details] [diff] [review]
simplify and correct

Thanks!

So, in the new patch, it looks like the hash-table directly maps nsIAtom --> function-pointer, right?  (where the function pointer is stored as a void*)

I like the reduced indirection, but I think there's a problem with that, unfortunately... I'm pretty sure some compilers will complain about converting between a function pointer and a pointer to data (e.g. a void*) -- at least spam a compile warning, possibly even an error.  Some googling turns up these results, as background/reference on why this isn't kosher:
http://stackoverflow.com/questions/12358843/why-are-function-pointers-and-data-pointers-incompatible-in-c-c
http://stackoverflow.com/questions/5579835/c-function-pointer-casting-to-void-pointer

I suspect that's why the HTML version has the level of indirection -- it makes the hashtable store an index (which you can cast to/from void* just fine), and it uses *that* to index into an array of function-pointers, and it never has to do any evil function-pointer void* casts.

Even though it means an extra level of indirection (and the need to be careful about handling 0), I think it'd be better to go back to the array-of-function-pointers technique. :-/ 


SO: basically, r=me if you do the following (not that you *have* to do exactly this, but this is just what I'm pre-emptively r+'ing):
 a) Switch back to using an array of function pointers, and storing an (offset) index in the hash table.

 b) Create "#define TABLE_VALUE_OFFSET 1" or something like that, to represent the offset. Add it to index-values before inserting them into the hash table, and subtract it when retrieving them later.  (And add a brief comment above the #define explaining why it's there.)

 c) optional: rename the table to something clearer (e.g. gTagAtomToCreatorFunctionIdxTable -- though that's kind of huge, so maybe it sucks... you could also stick with current name if you like.)

 d) Add a comment above the table saying explicitly what its key & value represents, e.g.:
   // Hash table that maps nsIAtom* SVG tags to an offset index
   // within the array sContentCreatorCallbacks (offset by
   // TABLE_VALUE_OFFSET)

Comment 8

[Robert Longson [:longsonr]]

Attachment: simplify and correct

Comment 9

[Daniel Holbert [:dholbert]]

(though don't let the preemptive r+ keep you from requesting a final once-over if you'd like to. :) Basically, I'll be happy if it looks like attachment 710960 [details] [diff] [review] (your last patch w/ an array of function pointers) with comment 7 (b), (c)(optional) & (d) addressed, and it passes Try. :)

Comment 10

[Robert Longson [:longsonr]]

Attachment: address review comments

Try seems OK.

https://tbpl.mozilla.org/?tree=Try&rev=04c72dedbff6

Comment 11

[Robert Longson [:longsonr]]

With index checking...

https://tbpl.mozilla.org/?tree=Try&rev=61f190a52f71

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 711788 [details] [diff] [review]
address review comments

One last nit (that I hadn't thought of yet when we spoke on IRC) -- I'd rather we use something more severe than NS_ASSERTION to test the array-index-out-of-bounds.  MOZ_CRASH is probably appropriate here (for opt & debug builds), because it'll make us safely crash, instead of potentially invoking some arbitrary function-pointer.

This means we'll be adding a hopefully-unnecessary bounds check in runtime builds -- the perf impact of that should be negligible (especially compared to the hash table operations just before it), and I think it's worth the cost/benefit tradeoff.

So: I'd rather we replace this line (from the try push):
> NS_ASSERTION(index >= 0 && index < eSVGTag_Count, "index out of bounds");
...with something like...
> if (index < 0 || index >= eSVGTag_Count) {
>   NS_WARNING("About to index out of array bounds - crashing instead"); 
>   MOZ_CRASH();
> }

r=me with that

Comment 13

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d3212955deb

Comment 14

[Robert Longson [:longsonr]]

I had another thought after I landed it that

if (index < 0 || index >= ArrayLength(sContentCreatorCallbacks)) {

might be even better?

Comment 15

[Daniel Holbert [:dholbert]]

Sure, that sound good -- though you should use static_cast<size_t>(index) in the comparison against ArrayLength(), or else you'll get signed/unsigned comparison compiler warnings.

Alternately, to avoid that static_cast, it might be better to use NS_PTR_TO_UINT32 (instead of TO_INT32), and make index an unsigned value.  (We don't have the reverse -- UINT32_TO_PTR -- for some reason, presumably because the existing INT32_TO_PTR works just fine regardless of the signed-ness of its input.)

Regardless, if you switch to using ArrayLength(), it'd probably be good to also PR_STATIC_ASSERT that the array length is equal to eSVGTag_Count, to be sure we're putting the right number of things into the array.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d3212955deb