Bug 36600 – [Qt] QScriptValue::strictlyEquals is broken

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Jędrzej Nowacki]

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...)

[Jędrzej Nowacki]

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]

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 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]

(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 on attachment 55853 [details]
Fix v1

c- as 38987 has not been accepted yet.

[Jędrzej Nowacki]

Created attachment 57022 [details]
Fix v1.01

Patch was rebased against trunk (right now it should apply correctly).

[WebKit Review Bot]

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]

(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 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 on attachment 57022 [details]
Fix v1.01

Clearing flags on attachment: 57022

Committed r60585: <http://trac.webkit.org/changeset/60585>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.