[webkit-reviews] review granted: [Bug 66784] HTMLImageElement: Don't cache "ismap" and "usemap" attributes. : [Attachment 105360] Proposed patch v2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <kling at webkit.org>'s
request for review:
Bug 66784: HTMLImageElement: Don't cache "ismap" and "usemap" attributes.
https://bugs.webkit.org/show_bug.cgi?id=66784

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=105360&action=review


>> Source/WebCore/html/HTMLImageElement.cpp:-137
>> -	    ismap = true;
> 
> I'm unsure whether it's preferred to still catch ismapAttr here, but do
nothing, rather than calling the base class.

I think it’s fine to leave out the code and thus call the base class. I don’t
know of any concrete benefits of not calling the base class.

>> Source/WebCore/html/HTMLImageElement.cpp:401
>> +	AtomicString usemap = fastGetAttribute(usemapAttr);
> 
> On second thought, this should probably be "const AtomicString& usemap = ..."


Yes, that would be slightly better.

> Source/WebCore/html/HTMLImageElement.cpp:403
> +    if (usemap.string()[0] == '#')
> +	   return false;

I think this could use a “why” comment.

> Source/WebCore/html/HTMLImageElement.cpp:405
> +    return
document()->completeURL(stripLeadingAndTrailingHTMLSpaces(usemap)).isEmpty();

I’m not sure we have to call completeURL just to check if it’s empty. I think
that the completeURL implementation guarantees that passed-in empty strings
result in an empty URL and passed-in non-empty strings result in a non-empty
URL.