[webkit-reviews] review denied: [Bug 33269] Improve HTMLElement::tagPriority() : [Attachment 45992] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 33269: Improve HTMLElement::tagPriority()
https://bugs.webkit.org/show_bug.cgi?id=33269

Attachment 45992: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=45992&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +struct Empty1IntHashTraits : HashTraits<int> {

This seems a slightly heavyweight wait of making this use a map. I wonder if
there's some other clever approach that avoids custom hash traits.

One thing we could do is this:

    1) Increase all tag priorities by 1 in all the code.
    2) Use find and return a tag priority of 2 if we don't find anything,
instead of using get.

I guess your way is probably better.

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

> +    DEFINE_STATIC_LOCAL(TagPriorityMap, tagPriorityMap, ());
> +    if (tagPriorityMap.isEmpty()) {
> +	   tagPriorityMap.add(wbrTag.localName().impl(), 0);
> +
> +	   tagPriorityMap.add(addressTag.localName().impl(), 3);
> +	   tagPriorityMap.add(ddTag.localName().impl(), 3);
> +	   tagPriorityMap.add(dtTag.localName().impl(), 3);
> +	   tagPriorityMap.add(noscriptTag.localName().impl(), 3);
> +	   tagPriorityMap.add(rpTag.localName().impl(), 3);
> +	   tagPriorityMap.add(rtTag.localName().impl(), 3);
> +
> +	   // 5 is same as <div>'s priority.
> +	   tagPriorityMap.add(articleTag.localName().impl(), 5);
> +	   tagPriorityMap.add(asideTag.localName().impl(), 5);
> +	   tagPriorityMap.add(centerTag.localName().impl(), 5);
> +	   tagPriorityMap.add(footerTag.localName().impl(), 5);
> +	   tagPriorityMap.add(headerTag.localName().impl(), 5);
> +	   tagPriorityMap.add(nobrTag.localName().impl(), 5);
> +	   tagPriorityMap.add(rubyTag.localName().impl(), 5);
> +	   tagPriorityMap.add(navTag.localName().impl(), 5);
> +	   tagPriorityMap.add(sectionTag.localName().impl(), 5);
> +
> +	   tagPriorityMap.add(noembedTag.localName().impl(), 10);
> +	   tagPriorityMap.add(noframesTag.localName().impl(), 10);
> +
> +	   // Return 1 for other tags. It's same as <span>.
> +	   // This way custom tag name elements will behave like inline spans.
> +    }
>  
> -    // Same values as <span>.  This way custom tag name elements will behave
like inline spans.
> -    return 1;
> +    return tagPriorityMap.get(localName().impl());
>  }

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.

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

review- so you can at least take out the unneeded overrides