[webkit-reviews] review granted: [Bug 122313] FocusController::advanceFocus spends a lot of time in HTMLMapElement::imageElement : [Attachment 213335] Fixes the bug

[bugzilla-daemon at webkit.org]

Andreas Kling <akling at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 122313: FocusController::advanceFocus spends a lot of time in
HTMLMapElement::imageElement
https://bugs.webkit.org/show_bug.cgi?id=122313

Attachment 213335: Fixes the bug
https://bugs.webkit.org/attachment.cgi?id=213335&action=review

------- Additional Comments from Andreas Kling <akling at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=213335&action=review


r=me with some improvements.

> Source/WebCore/dom/Document.h:262
> +    void addImageElementByLowercasedUsemap(const AtomicString&,
HTMLImageElement*);
> +    void removeImageElementByLowercasedUsemap(const AtomicString&,
HTMLImageElement*);

HTMLImageElement* -> HTMLImageElement& since these don't accept null.

> Source/WebCore/html/HTMLImageElement.cpp:139
> +	   const AtomicString& usemap =
fastGetAttribute(HTMLNames::usemapAttr);

You already have the usemap attribute in the "value" argument variable here, no
need to search for it.

> Source/WebCore/html/HTMLImageElement.cpp:141
> +	       m_lowercasedUsemap =
usemap.string().substring(1).lower().impl();

Is the .impl() really needed?

> Source/WebCore/html/HTMLImageElement.cpp:324
> +    ASSERT(const_cast<AtomicStringImpl*>(&name)->lower() == &name);

This would look ~1% nicer as:
ASSERT(const_cast<AtomicStringImpl&>(name).lower() == &name);

> Source/WebCore/html/HTMLMapElement.cpp:82
> +    return document().imageElementByUsemap(m_name.lower());

You could make this even faster by caching a lowercase version of the map's
name in a member variable.
I don't know what your test case is, so I don't know if it'd be worth much.