RESOLVED FIXED 36600
[Qt] QScriptValue::strictlyEquals is broken 
https://bugs.webkit.org/show_bug.cgi?id=36600
Summary [Qt] QScriptValue::strictlyEquals is broken
Jędrzej Nowacki
Reported 2010-03-25 07:44:25 PDT
The QScriptValue::strictlyEquals returns always false in comparison between a QScriptValue object and a result of a code evaluation. QScriptEngine engine; QScriptValue global = engine.globalObject(); QScriptValue self = engine.evaluate("this"); QVERIFY(global.isObject()); //QVERIFY(self.isObject()); QVERIFY(engine.globalObject().strictlyEquals(self)); The last test fails, but if self.isObject is called (commented part) than it pass. In general more autotests should be added for all comparisons methods (equals, strictlyEquals...)
Attachments
Fix v1 (42.61 KB, patch)
2010-05-12 08:41 PDT, Jędrzej Nowacki 		no flags
DetailsFormatted DiffDiff
Fix v1.01 (42.62 KB, patch)
2010-05-25 08:40 PDT, Jędrzej Nowacki 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Jędrzej Nowacki
Comment 1 2010-05-12 08:38:37 PDT
The fix will be done in two steps; 1. reorganize the QScriptValue autotest suite (bug 38987) 2. fix the strictlyEquals method and add an autotest to cover it.
Jędrzej Nowacki
Comment 2 2010-05-12 08:41:57 PDT
Created attachment 55853 [details] Fix v1 The patch depends on 38987 and will not be clearly applied unless 38987 will be accepted.
Kenneth Rohde Christiansen
Comment 3 2010-05-13 07:42:17 PDT
Comment on attachment 55853 [details] Fix v1 I see code like 571 if (other->isJSBased()) { 572 assignEngine(other->engine()); 573 return JSValueIsStrictEqual(context(), value(), other->value()); 574 } in two-three places, maybe make it an inline method? or will this only be used here?
Jędrzej Nowacki
Comment 4 2010-05-14 01:06:50 PDT
(In reply to comment #3) > (From update of attachment 55853 [details]) > I see code like > > 571 if (other->isJSBased()) { > 572 assignEngine(other->engine()); > 573 return JSValueIsStrictEqual(context(), value(), other->value()); > 574 } > > in two-three places, maybe make it an inline method? or will this only be used here? Lets say only in 2,5 places, one is slightly different :-). The snippet will be used only in QScriptValuePrivate::strictlyEquals.
Jędrzej Nowacki
Comment 5 2010-05-14 06:04:12 PDT
Comment on attachment 55853 [details] Fix v1 c- as 38987 has not been accepted yet.
Jędrzej Nowacki
Comment 6 2010-05-25 08:40:25 PDT
Created attachment 57022 [details] Fix v1.01 Patch was rebased against trunk (right now it should apply correctly).
WebKit Review Bot
Comment 7 2010-05-25 08:42:42 PDT
Attachment 57022 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WARNING: File exempt from style guide. Skipping: "JavaScriptCore/qt/api/qscriptvalue_p.h" WARNING: File exempt from style guide. Skipping: "JavaScriptCore/qt/api/qscriptvalue.cpp" JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_comparison.cpp:24: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Jędrzej Nowacki
Comment 8 2010-05-26 07:20:01 PDT
(In reply to comment #7) > Attachment 57022 [details] did not pass style-queue: > If any of these errors are false positives, please file a bug against check-webkit-style. Bugs 35143, 35151
Eric Seidel (no email)
Comment 9 2010-06-02 02:26:58 PDT
Comment on attachment 55853 [details] Fix v1 Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 55853 [details] so that this bug does not appear in http://webkit.org/pending-commit.
WebKit Commit Bot
Comment 10 2010-06-02 18:20:10 PDT
Comment on attachment 57022 [details] Fix v1.01 Clearing flags on attachment: 57022 Committed r60585: <http://trac.webkit.org/changeset/60585>
WebKit Commit Bot
Comment 11 2010-06-02 18:20:17 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.