Bug 34575

Summary: [Qt] QScriptValue::isNumber() returns an incorrect value
Product: WebKit Reporter: Jędrzej Nowacki <jedrzej.nowacki>
Component: JavaScriptCoreAssignee: Jędrzej Nowacki <jedrzej.nowacki>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jedrzej.nowacki, webkit.review.bot
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 34533    
Bug Blocks: 31863    
Attachments:
Description Flags
Fix v1
none
Fix v2 none

Description Jędrzej Nowacki 2010-02-04 05:51:06 PST
For these values:
QScriptValue v1(0, 0.0);
QScriptValue v2(0.0);
QScriptValue v3(0, 0);
QScriptValue v4(0);
::isNumber() returns false which is incorrect.
Comment 1 Jędrzej Nowacki 2010-02-04 05:58:35 PST
Created attachment 48135 [details]
Fix v1
Comment 2 WebKit Review Bot 2010-02-04 06:03:15 PST
Attachment 48135 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Ignoring "JavaScriptCore/qt/api/qscriptvalue_p.h": this file is exempt from the style guide.
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.h:58:  isNumber_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.h:100:  isNumber_initData is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.h:101:  isNumber_makeData is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.h:102:  isNumber_test is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Eric Seidel (no email) 2010-02-04 15:34:23 PST
Comment on attachment 48135 [details]
Fix v1

So previously 0 and NaN were not "numbers"?  (assuming m_number is a float here).  Why?

WebKit style is not to name arguments when their names add no value:
 102     void isNumber_test(const char* expr, const QScriptValue& value);

"value" there adds no value.

I don't understand how this actually adds the test data:
 371     newRow(expr) << isNumber.contains(expr);
but I think I'm simply not familiar enough with Qt testing infrastructure.

Looks like you'll need a Qt reviewer here.
Comment 4 Jędrzej Nowacki 2010-02-09 02:49:28 PST
Created attachment 48399 [details]
Fix v2
Comment 5 Jędrzej Nowacki 2010-02-09 03:49:09 PST
(In reply to comment #3)
> (From update of attachment 48135 [details])
> So previously 0 and NaN were not "numbers"?  (assuming m_number is a float
> here).  Why?
m_number is double and it was casted automatically to bool. (bool)0.0 and (bool)NaN is false. 

> WebKit style is not to name arguments when their names add no value:
>  102     void isNumber_test(const char* expr, const QScriptValue& value);
> 
> "value" there adds no value.
fixed.

> I don't understand how this actually adds the test data:
>  371     newRow(expr) << isNumber.contains(expr);
> but I think I'm simply not familiar enough with Qt testing infrastructure.

Essentially, isNumber() method is called for each QScriptValue created in the initScriptValues() so it is about 100 different values. The line you mentioned provides expected results only which are passed to the isNumber_test() by QFETCH makro.
Comment 6 WebKit Commit Bot 2010-02-09 18:19:35 PST
Comment on attachment 48399 [details]
Fix v2

Clearing flags on attachment: 48399

Committed r54577: <http://trac.webkit.org/changeset/54577>
Comment 7 WebKit Commit Bot 2010-02-09 18:19:40 PST
All reviewed patches have been landed.  Closing bug.