Bug 98417 - After r130344, OpaqueJSString() creates an empty string which should be a null string
Summary: After r130344, OpaqueJSString() creates an empty string which should be a nul...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks: 98300
  Show dependency tree / graph
 
Reported: 2012-10-04 09:16 PDT by Michael Saboff
Modified: 2012-10-08 11:46 PDT (History)
3 users (show)

See Also:

Attachments
Patch (1.23 KB, patch)
2012-10-04 10:09 PDT, Michael Saboff 		no flags Details | Formatted Diff | Diff
Patch (1.28 KB, patch)
2012-10-08 10:19 PDT, Michael Saboff 		sam: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 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.
Comment 1 Michael Saboff 2012-10-04 10:09:37 PDT 
Created attachment 167118 [details]
Patch
Comment 2 WebKit Review Bot 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>
Comment 3 WebKit Review Bot 2012-10-04 11:49:35 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 4 Alexey Proskuryakov 2012-10-04 17:13:45 PDT 
Just realized that this should have had an API test.
Comment 5 Michael Saboff 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.
Comment 6 Geoffrey Garen 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.
Comment 7 Michael Saboff 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.
Comment 8 Michael Saboff 2012-10-08 11:46:13 PDT 
Committed r130664: <http://trac.webkit.org/changeset/130664>