Bug 41769 - [Qt] QScriptValue API should have a property flag accessor.
Summary: [Qt] QScriptValue API should have a property flag accessor.
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:
Blocks: 31863 41680
  Show dependency treegraph
 
Reported: 2010-07-07 07:17 PDT by Jędrzej Nowacki
Modified: 2010-07-09 02:21 PDT (History)
4 users (show)

See Also:


Attachments
Fix v1 (10.99 KB, patch)
2010-07-07 07:31 PDT, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff
fix v1.01 (11.06 KB, patch)
2010-07-08 01:27 PDT, Jędrzej Nowacki
hausmann: review+
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-07-07 07:17:42 PDT
QtScript should provide an API for checking a JS object's properties flags

see http://doc.trolltech.com/4.6/qscriptvalue.html#propertyFlags
Comment 1 Jędrzej Nowacki 2010-07-07 07:31:24 PDT
Created attachment 60733 [details]
Fix v1
Comment 2 Caio Marcelo de Oliveira Filho 2010-07-07 15:52:17 PDT
Jędrzej, I have a question: since the 'Object' property of the global object can be modified and propertyFlags() could be called after that, isn't "dangerous" to get the Object constructor at this point?

I had a similar situation for isArray(), and ended up extracting the Array constructor and prototype just after the global context was created in QScriptEnginePrivate (C++) ctor, storing them in member variables.

Maybe the same could be done to the Object constructor, or am I missing a difference between those two cases?
Comment 3 Jędrzej Nowacki 2010-07-08 00:49:31 PDT
Comment on attachment 60733 [details]
Fix v1

(In reply to comment #2)
> Jędrzej, I have a question: since the 'Object' property of the global object can be modified and propertyFlags() could be called after that, isn't "dangerous" to get the Object constructor at this point?
It is. There should be a "fixme" apparently it was lost somehow. Thanks for the review.

> I had a similar situation for isArray(), and ended up extracting the Array constructor and prototype just after the global context was created in QScriptEnginePrivate (C++) ctor, storing them in member variables.
Yes I think keeping a reference to the prototype and to the constructor is a good solution (and maybe only one as JSC doesn't keep original values).

The same issue is in the QSV::isError(), QSV::isRegExp() functions, I think it will be for an other too. Moreover the QtScript API permits to change global object and in the same time it use it a lot.

From architectural point of view I believe that we should create separate class/struct that represent Original Global Object, It should be created at QScriptEnginePrivate creation time and it should be private for the QSEP.

I will repost the patch with the fixme comment.
Comment 4 Jędrzej Nowacki 2010-07-08 01:20:29 PDT
(In reply to comment #3)
> From architectural point of view I believe that we should create separate class/struct that represent Original Global Object, It should be created at QScriptEnginePrivate creation time and it should be private for the QSEP.
I've created a bug(41839).
Comment 5 Jędrzej Nowacki 2010-07-08 01:27:13 PDT
Created attachment 60850 [details]
fix v1.01

Mostly the same patch, I have added the FIXME.
Comment 6 Kent Hansen 2010-07-09 00:57:18 PDT
(In reply to comment #5)
> Created an attachment (id=60850) [details]
> fix v1.01
> 
> Mostly the same patch, I have added the FIXME.

The way to make it "safe" would be to store a reference to the original getOwnPropertyDescriptor() function at engine construction time (storing the Object constructor is not enough).

The current approach is suboptimal anyway; as the patch says, there should be a C API for getting attributes. I've created https://bugs.webkit.org/show_bug.cgi?id=41937 for this.

I think the patch is fine for now. Let's do it "properly" when the C API arrives.
Comment 7 Simon Hausmann 2010-07-09 02:21:06 PDT
Committed r62921: <http://trac.webkit.org/changeset/62921>