Bug 37729 - [Qt] QScriptValuePrivate class needs some cleanup.
Summary: [Qt] QScriptValuePrivate class needs some cleanup.
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 36594
  Show dependency treegraph
 
Reported: 2010-04-16 14:00 PDT by Jędrzej Nowacki
Modified: 2010-04-29 01:15 PDT (History)
4 users (show)

See Also:


Attachments
Fix v1 (10.37 KB, patch)
2010-04-16 14:05 PDT, Jędrzej Nowacki
kenneth: review-
kenneth: commit-queue-
Details | Formatted Diff | Diff
Fix v2 (11.13 KB, patch)
2010-04-20 10:43 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-04-16 14:00:40 PDT
QtScript implementation is placed in a private classes. Private classes should use only other private classes to avoid unnecessary conversions. QScriptValuePrivate breaks this rule, which is wrong. QScriptValuePrivate constructor shouldn't take QScriptEngine pointer as a parameter.
Comment 1 Jędrzej Nowacki 2010-04-16 14:05:50 PDT
Created attachment 53562 [details]
Fix v1
Comment 2 Kenneth Rohde Christiansen 2010-04-19 19:45:57 PDT
Comment on attachment 53562 [details]
Fix v1

> diff --git a/JavaScriptCore/ChangeLog b/JavaScriptCore/ChangeLog
> index 12327e4..d5621a3 100644
> --- a/JavaScriptCore/ChangeLog
> +++ b/JavaScriptCore/ChangeLog
> @@ -1,3 +1,20 @@
> +2010-04-16  Jedrzej Nowacki  <jedrzej.nowacki@nokia.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        QtScript implementation is placed in a private classes. 

a classes? Wrong English

> +        use only other private classes. QScriptValuePrivate breaks this rule, which
> +        is wrong. 

No need to say it is wrong, you already made that clear. Remove dedundant information.

> QScriptValuePrivate constructor shouldn't take QScriptEngine pointer
> +        as a parameter.

Is this another comment? or are you fixing a second thing?


> +    if (engine)
> +        d_ptr = new QScriptValuePrivate(QScriptEnginePrivate::get(engine).data(), value);
> +    else
> +        d_ptr = new QScriptValuePrivate(value);
>  }

I do not understand this get method? It gets the private of the engine? I guess you use a kind of smart pointer inside it, couldn't the method to the data() internally in that method. The code is not easy readable.
Comment 3 Jędrzej Nowacki 2010-04-20 10:43:51 PDT
Created attachment 53844 [details]
Fix v2

(In reply to comment #2)
Changelog changed.

> I do not understand this get method? It gets the private of the engine? 
Yes and the class do not have to be a friend, In general the get method is used for conversion between a public and a private class. In QtScript I decided to separete a public implementation and a private implementation. Public implementation is used only for converting to a private one. All real work is done in a private implementation. The design adventage is that we can inline nearby all methods in private imlementation and we do not waste time for converting values inside private implementation. If you want I can describe it in detail.

> I guess
> you use a kind of smart pointer inside it, couldn't the method to the data()
> internally in that method. The code is not easy readable.
You are right. It is done.
Comment 4 WebKit Commit Bot 2010-04-29 01:14:59 PDT
Comment on attachment 53844 [details]
Fix v2

Clearing flags on attachment: 53844

Committed r58483: <http://trac.webkit.org/changeset/58483>
Comment 5 WebKit Commit Bot 2010-04-29 01:15:05 PDT
All reviewed patches have been landed.  Closing bug.