Bug 113003

Summary: Remove 2 bad branches from StringHash::equal() and CaseFoldingHash::equal()
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, eric, esprehn+autocc, ojan.autocc, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch eric: review+

Description Benjamin Poulain 2013-03-21 22:12:42 PDT
Remove 2 bad branches from StringHash::equal() and CaseFoldingHash::equal()
Comment 1 Benjamin Poulain 2013-03-21 22:35:27 PDT
Created attachment 194443 [details]
Patch
Comment 2 Benjamin Poulain 2013-03-22 15:30:57 PDT
Created attachment 194645 [details]
Patch
Comment 3 Benjamin Poulain 2013-03-22 15:33:37 PDT
*** Bug 111892 has been marked as a duplicate of this bug. ***
Comment 4 Eric Seidel (no email) 2013-03-22 16:26:00 PDT
Comment on attachment 194645 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194645&action=review

LGTM!

> Source/WebCore/html/parser/HTMLTreeBuilderSimulator.cpp:98
> +    return equalIgnoringCase(token.data().asStringImpl(), SVGNames::foreignObjectTag.localName().impl());

These are known to never be null, btw.
Comment 5 Benjamin Poulain 2013-03-22 16:30:20 PDT
> > Source/WebCore/html/parser/HTMLTreeBuilderSimulator.cpp:98
> > +    return equalIgnoringCase(token.data().asStringImpl(), SVGNames::foreignObjectTag.localName().impl());
> 
> These are known to never be null, btw.

I was not sure so I took the safe option :).
I will change to the non-null form before landing.

Thank you for the review.
Comment 6 Eric Seidel (no email) 2013-03-22 16:31:58 PDT
I believe the HTMLParser basically never has any null strings ever. :)  Everything is empty at most.  In the case of a tag name, the parser will tread <> as a characters in a character token, not as a tag with empty name.

Thanks for fixing!
Comment 7 Benjamin Poulain 2013-03-22 18:38:08 PDT
Committed r146702: <http://trac.webkit.org/changeset/146702>
Comment 8 Ryosuke Niwa 2013-03-22 19:44:13 PDT
This patch broke Windows build:

http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/46465/steps/compile-webkit/logs/stdio

7>WebKitSystemInterface.lib(WebKitSystemInterface.obj) : MSIL .netmodule or module compiled with /GL found; restarting link with /LTCG; add /LTCG to the link command line to improve linker performance
7>   Creating library C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\lib\WebKit.lib and object C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\lib\WebKit.exp
7>WebKit.exp : error LNK2001: unresolved external symbol "bool __cdecl WTF::equalIgnoringCase(class WTF::StringImpl *,unsigned char const *)" (?equalIgnoringCase@WTF@@YA_NPAVStringImpl@1@PBE@Z)
7>C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin\WebKit.dll : fatal error LNK1120: 1 unresolved externals
Comment 9 Benjamin Poulain 2013-03-22 20:32:57 PDT
(In reply to comment #8)
> This patch broke Windows build:

I am on it.
Comment 11 Benjamin Poulain 2013-03-22 20:39:02 PDT
(In reply to comment #10)
> Fixed in http://trac.webkit.org/changeset/146705 and http://trac.webkit.org/changeset/146710.

Thanks for fixing this.