Bug 36600 - [Qt] QScriptValue::strictlyEquals is broken
Summary: [Qt] QScriptValue::strictlyEquals is broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jędrzej Nowacki
URL:
Keywords: Qt, QtTriaged
Depends on: 38987
Blocks: 31863
  Show dependency treegraph
 
Reported: 2010-03-25 07:44 PDT by Jędrzej Nowacki
Modified: 2010-06-02 18:20 PDT (History)
5 users (show)

See Also:


Attachments
Fix v1 (42.61 KB, patch)
2010-05-12 08:41 PDT, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff
Fix v1.01 (42.62 KB, patch)
2010-05-25 08:40 PDT, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jędrzej Nowacki 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...)
Comment 1 Jędrzej Nowacki 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.
Comment 2 Jędrzej Nowacki 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.
Comment 3 Kenneth Rohde Christiansen 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?
Comment 4 Jędrzej Nowacki 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.
Comment 5 Jędrzej Nowacki 2010-05-14 06:04:12 PDT
Comment on attachment 55853 [details]
Fix v1

c- as 38987 has not been accepted yet.
Comment 6 Jędrzej Nowacki 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).
Comment 7 WebKit Review Bot 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.
Comment 8 Jędrzej Nowacki 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
Comment 9 Eric Seidel (no email) 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.
Comment 10 WebKit Commit Bot 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>
Comment 11 WebKit Commit Bot 2010-06-02 18:20:17 PDT
All reviewed patches have been landed.  Closing bug.