Bug 40067 - [Qt] The QScriptValuePrivate::CSpecial is too generic.
Summary: [Qt] The QScriptValuePrivate::CSpecial is too generic.
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
Depends on:
Blocks: 31863
  Show dependency treegraph
 
Reported: 2010-06-02 09:26 PDT by Jędrzej Nowacki
Modified: 2010-06-03 22:57 PDT (History)
3 users (show)

See Also:


Attachments
Fix v1 (5.80 KB, patch)
2010-06-02 09:35 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-06-02 09:26:57 PDT
The QScriptValuePrivate::CSpecial should be divided into a separate states; CNull and CUndefined.
It would simplify the QScriptValue's design and implementation which should use the m_state member for distinguish between states not m_value.
Comment 1 Jędrzej Nowacki 2010-06-02 09:35:37 PDT
Created attachment 57663 [details]
Fix v1

No functional changes.
Comment 2 Kenneth Rohde Christiansen 2010-06-02 11:44:23 PDT
Comment on attachment 57663 [details]
Fix v1

JavaScriptCore/qt/api/qscriptvalue_p.h:54
 +      CUndefined -> QSVP is undefined, but a JSC engine hasn't been associated yet.
So does this not change the behaviour of QtScript?
Comment 3 Jędrzej Nowacki 2010-06-03 00:16:46 PDT
(In reply to comment #2)
> (From update of attachment 57663 [details])
> JavaScriptCore/qt/api/qscriptvalue_p.h:54
>  +      CUndefined -> QSVP is undefined, but a JSC engine hasn't been associated yet.
> So does this not change the behaviour of QtScript?

True, It is a small design fix :-) The CSpecial state was a bad idea. From the beginning CNull and CUndefined should be separated.

Patch removes conversions between double and enum.
> -    case CSpecial:
> -        return m_number == static_cast<int>(QScriptValue::NullValue);
> +    case CNull:
> +        return true;

Code is simpler, and there is no need to check other variable then m_state to find the real state of QScriptValuePrivate
> -    case CSpecial:
> -        return m_number == QScriptValue::NullValue ? QString::fromLatin1("null") : QString::fromLatin1("undefined");
> +    case CNull:
> +        return QString::fromLatin1("null");
> +    case CUndefined:
> +        return QString::fromLatin1("undefined");

I planed to change it in Feb. but the patch touches too many functions in the QSVP, so it was difficult to apply it without conflicts. This is the first time when a QSVP patches are not waiting in bugzilla's commit queue.
Comment 4 WebKit Commit Bot 2010-06-03 22:57:52 PDT
Comment on attachment 57663 [details]
Fix v1

Clearing flags on attachment: 57663

Committed r60655: <http://trac.webkit.org/changeset/60655>
Comment 5 WebKit Commit Bot 2010-06-03 22:57:58 PDT
All reviewed patches have been landed.  Closing bug.