Bug 98417

Summary: After r130344, OpaqueJSString() creates an empty string which should be a null string
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, ggaren, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 98300    
Attachments:
Description Flags
Patch
none
Patch sam: review+

Michael Saboff
Reported 2012-10-04 09:16:03 PDT
The change for https://bugs.webkit.org/show_bug.cgi?id=98300 in changeset r130344: <http://trac.webkit.org/changeset/130344>, changes the default OpaqueJSString constructor to have an empty string. Before the change this was a null string. The new default constructor initializes m_string with m_string = emptyString(); this should be eliminated.
Attachments
Patch (1.23 KB, patch)
2012-10-04 10:09 PDT, Michael Saboff 		no flags
DetailsFormatted DiffDiff
Patch (1.28 KB, patch)
2012-10-08 10:19 PDT, Michael Saboff 		sam: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Michael Saboff
Comment 1 2012-10-04 10:09:37 PDT
Created attachment 167118 [details] Patch
WebKit Review Bot
Comment 2 2012-10-04 11:49:32 PDT
Comment on attachment 167118 [details] Patch Clearing flags on attachment: 167118 Committed r130413: <http://trac.webkit.org/changeset/130413>
WebKit Review Bot
Comment 3 2012-10-04 11:49:35 PDT
All reviewed patches have been landed. Closing bug.
Alexey Proskuryakov
Comment 4 2012-10-04 17:13:45 PDT
Just realized that this should have had an API test.
Michael Saboff
Comment 5 2012-10-04 17:41:50 PDT
There is a second case where we turn a null string into an empty string. That is when OpaqueJSString(String string) is called and string is null. If we fail the if (!string.isNull()) check, that is the string is null, we should return 0.
Geoffrey Garen
Comment 6 2012-10-07 11:32:41 PDT
> Just realized that this should have had an API test. Yes, please add an API test for both identified cases.
Michael Saboff
Comment 7 2012-10-08 10:19:10 PDT
Created attachment 167554 [details] Patch Working on API tests, although there isn't a direct way to test the API. Looking for an indirect method for both checks.
Michael Saboff
Comment 8 2012-10-08 11:46:13 PDT
Committed r130664: <http://trac.webkit.org/changeset/130664>
Note You need to log in before you can comment on or make changes to this bug.