Bug 94561 – [V8] Move compileScript() from V8Proxy to ScriptSourceCode

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

To kill V8Proxy, we can move compileScript() from V8Proxy to ScriptController.

[Kentaro Hara]

Created attachment 159589 [details]
Patch

[Adam Barth]

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.

[Kentaro Hara]

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

[Adam Barth]

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

[Kentaro Hara]

Created attachment 159650 [details]
Patch

[Kentaro Hara]

Created attachment 159686 [details]
Patch

[Adam Barth]

Comment on attachment 159686 [details]
Patch

Even better.

[WebKit Review Bot]

Comment on attachment 159686 [details]
Patch

Clearing flags on attachment: 159686

Committed r126222: <http://trac.webkit.org/changeset/126222>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.