[webkit-reviews] review requested: [Bug 33269] Improve HTMLElement::tagPriority() : [Attachment 45999] Proposed patch (rev.2)

[bugzilla-daemon at webkit.org]

TAMURA, Kent <tkent at chromium.org> has asked  for review:
Bug 33269: Improve HTMLElement::tagPriority()
https://bugs.webkit.org/show_bug.cgi?id=33269

Attachment 45999: Proposed patch (rev.2)
https://bugs.webkit.org/attachment.cgi?id=45999&action=review

------- Additional Comments from TAMURA, Kent <tkent at chromium.org>
(In reply to comment #3)
> (From update of attachment 45992 [details])
> > +	 static const bool emptyValueIsZero = false;
> > +	 static const bool needsDestruction = false;
> > +	 static int emptyValue() { return 1; }
> > +	 static void constructDeletedValue(int& slot) { slot = -1; }
> > +	 static bool isDeletedValue(int value) { return value == -1; }
> 
> There's no need for you to override needsDestruction, constructDeletedValue,
or
> isDeletedValue. You can just use the inherited versions of those from the
base
> class.

ok, I removed them.

> I think it would be best to put the code to initialize the map into a
separate
> function, initializeTagPriorityMap, that takes the map as an argument. That
> will make the tagPriorityMap function easier to read and even ever so
slightly
> more efficient since it will be smaller with better code locality.

Done.

> Is there a guarantee that localName().impl() is never 0?

I think so for the current code though there is no ASSERTION for it.
However, 0 makes no problems because the HashMap key is AtomicStringImpl*. The
generic PtrHash is used and tagPriorityMap.get(0) simply returns emptyValue().