Closed Bug 838501 Opened 10 years ago Closed 10 years ago

Refactor SVGElementFactory


(Core :: SVG, defect)

Not set





(Reporter: longsonr, Assigned: longsonr)




(1 file, 4 obsolete files)

Attached patch patch based on how html does it (obsolete) — Splinter Review
See and for how html does it.

The patch is based on that.
Attachment #710552 - Attachment is patch: true
Attachment #710552 - Flags: review?(dholbert)
Assignee: nobody → longsonr
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/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

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)

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)

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.
Attached patch address review comments (obsolete) — Splinter Review
Attachment #710552 - Attachment is obsolete: true
Attachment #710552 - Flags: review?(dholbert)
Attachment #710960 - Flags: review?(dholbert)
Comment on attachment 710960 [details] [diff] [review]
address review comments

>+#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
>+  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 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."

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));
>+  }
Attached patch simplify and correct (obsolete) — Splinter Review
Attachment #710960 - Attachment is obsolete: true
Attachment #710960 - Flags: review?(dholbert)
Attachment #711178 - Flags: review?(dholbert)
Exellent catch on the nullptr = 0 issue BTW
Comment on attachment 711178 [details] [diff] [review]
simplify and correct


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:

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
Attachment #711178 - Flags: review?(dholbert) → review+
Attached file simplify and correct (obsolete) —
Attachment #711178 - Attachment is obsolete: true
Attachment #711191 - Flags: review?(dholbert)
Attachment #711191 - Attachment is obsolete: true
Attachment #711191 - Attachment is patch: false
Attachment #711191 - Flags: review?(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 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
Attachment #711788 - Flags: review?(dholbert) → review+
I had another thought after I landed it that

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

might be even better?
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.
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Depends on: 842272
You need to log in before you can comment on or make changes to this bug.