Bug 94561

Summary:

[V8] Move compileScript() from V8Proxy to ScriptSourceCode

Product:

WebKit

Reporter:

Kentaro Hara <haraken>

Component:

WebCore JavaScript

Assignee:

Kentaro Hara <haraken>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, gyuyoung.kim, japhet, rakuco, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

85330

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Kentaro Hara

Reported 2012-08-20 19:14:16 PDT

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

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Kentaro Hara

Comment 1 2012-08-20 19:17:59 PDT

Created attachment 159589 [details] Patch

Adam Barth

Comment 2 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.

Kentaro Hara

Comment 3 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.

Adam Barth

Comment 4 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.

Kentaro Hara

Comment 5 2012-08-21 04:09:39 PDT

Created attachment 159650 [details] Patch

Kentaro Hara

Comment 6 2012-08-21 07:31:11 PDT

Created attachment 159686 [details] Patch

Adam Barth

Comment 7 2012-08-21 07:39:32 PDT

Comment on attachment 159686 [details] Patch Even better.

WebKit Review Bot

Comment 8 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>

WebKit Review Bot

Comment 9 2012-08-21 16:51:21 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.