Bug 94561 - [V8] Move compileScript() from V8Proxy to ScriptSourceCode
Summary: [V8] Move compileScript() from V8Proxy to ScriptSourceCode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks: 85330
  Show dependency treegraph
 
Reported: 2012-08-20 19:14 PDT by Kentaro Hara
Modified: 2012-08-21 16:51 PDT (History)
5 users (show)

See Also:


Attachments
Patch (9.20 KB, patch)
2012-08-20 19:17 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (16.52 KB, patch)
2012-08-21 04:09 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (16.50 KB, patch)
2012-08-21 07:31 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-08-20 19:14:16 PDT
To kill V8Proxy, we can move compileScript() from V8Proxy to ScriptController.
Comment 1 Kentaro Hara 2012-08-20 19:17:59 PDT
Created attachment 159589 [details]
Patch
Comment 2 Adam Barth 2012-08-20 23:03:43 PDT
Comment on attachment 159589 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159589&action=review

> Source/WebCore/bindings/v8/ScriptController.cpp:342
> +    const uint16_t* fileNameString = fromWebCoreString(fileName);

What is fromWebCoreString ?

> Source/WebCore/bindings/v8/ScriptController.h:91
> +    static v8::Handle<v8::Script> compileScript(v8::Handle<v8::String> code, const String& fileName, const TextPosition& scriptStartPosition, v8::ScriptData* = 0);

Should this really go on script controller?  I guess I would have expected this to be related to ScriptSourceCode....  I guess the types don't quite line up.
Comment 3 Kentaro Hara 2012-08-20 23:13:15 PDT
(In reply to comment #2)
> > Source/WebCore/bindings/v8/ScriptController.cpp:342
> > +    const uint16_t* fileNameString = fromWebCoreString(fileName);
> 
> What is fromWebCoreString ?

fromWebCoreString() retrieves uint16 from WebCore String. Maybe should we rename it to uint16OfWebCoreString()?

> > Source/WebCore/bindings/v8/ScriptController.h:91
> > +    static v8::Handle<v8::Script> compileScript(v8::Handle<v8::String> code, const String& fileName, const TextPosition& scriptStartPosition, v8::ScriptData* = 0);
> 
> Should this really go on script controller?  I guess I would have expected this to be related to ScriptSourceCode....  I guess the types don't quite line up.

Makes sense. I'll update the patch.
Comment 4 Adam Barth 2012-08-20 23:24:04 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > > Source/WebCore/bindings/v8/ScriptController.cpp:342
> > > +    const uint16_t* fileNameString = fromWebCoreString(fileName);
> > 
> > What is fromWebCoreString ?
> 
> fromWebCoreString() retrieves uint16 from WebCore String. Maybe should we rename it to uint16OfWebCoreString()?

    inline const uint16_t* fromWebCoreString(const String& str)
    {
        return reinterpret_cast<const uint16_t*>(str.characters());
    }

It might be clearer to just inline this function into all it's call sites.  Having the body of the function makes it clear that the memory is still owned by the string...

Up to you.
Comment 5 Kentaro Hara 2012-08-21 04:09:39 PDT
Created attachment 159650 [details]
Patch
Comment 6 Kentaro Hara 2012-08-21 07:31:11 PDT
Created attachment 159686 [details]
Patch
Comment 7 Adam Barth 2012-08-21 07:39:32 PDT
Comment on attachment 159686 [details]
Patch

Even better.
Comment 8 WebKit Review Bot 2012-08-21 16:51:17 PDT
Comment on attachment 159686 [details]
Patch

Clearing flags on attachment: 159686

Committed r126222: <http://trac.webkit.org/changeset/126222>
Comment 9 WebKit Review Bot 2012-08-21 16:51:21 PDT
All reviewed patches have been landed.  Closing bug.