Bug 106790 – [V8] Remove a --es5_readonly flag from V8 initialization and address duplicated code

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

[Peter Rybin]

Group common code from 2 similar V8 initialize methods.

[Peter Rybin]

Created attachment 182579 [details]
Patch

[Kentaro Hara]

Comment on attachment 182579 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Initializer.cpp:-128
> -    // FIXME: Remove the following 2 lines when V8 default has changed.
> -    const char es5ReadonlyFlag[] = "--es5_readonly";
> -    v8::V8::SetFlagsFromString(es5ReadonlyFlag, sizeof(es5ReadonlyFlag));

What's the rationale for removing the lines? Has V8 default has changed?

[Peter Rybin]

> > Source/WebCore/bindings/v8/V8Initializer.cpp:-128
> > -    // FIXME: Remove the following 2 lines when V8 default has changed.
> > -    const char es5ReadonlyFlag[] = "--es5_readonly";
> > -    v8::V8::SetFlagsFromString(es5ReadonlyFlag, sizeof(es5ReadonlyFlag));
> 
> What's the rationale for removing the lines? Has V8 default has changed?

Exactly.
See http://code.google.com/p/v8/source/detail?r=12415

[Kentaro Hara]

Comment on attachment 182579 [details]
Patch

OK.

- Please wait for arv's comment before landing.

- Please add the V8 change URL to your ChangeLog.

- Change the bug title to "[V8] Remove a --es5_readonly flag from V8 initialization", because that is the main point of this change.

[Peter Rybin]

Created attachment 182584 [details]
Patch

[Kentaro Hara]

Comment on attachment 182584 [details]
Patch

OK, thanks. Let's wait for arv.

[Peter Rybin]

> - Please add the V8 change URL to your ChangeLog.
Done

> - Change the bug title to "[V8] Remove a --es5_readonly flag from V8 initialization", because that is the main point of this change.
Done (but this wasn't main intent though :)

[Peter Rybin]

(In reply to comment #6)
> (From update of attachment 182584 [details])
> OK, thanks. Let's wait for arv.

arv is not in office until 22rd according to his mail robot.
Is there anything we could do without waiting until 22rd? Is there any particular concern you meant?
Is it ok to ask my colleagues to review this instead? yurys, vsevik, loislo or pfeldmand probably could do it.

[Kentaro Hara]

Comment on attachment 182584 [details]
Patch

I asked Andreas (the author of the V8 patch) if it's safe to remove the flag from V8 binding. He is saying OK.

[WebKit Review Bot]

Comment on attachment 182584 [details]
Patch

Clearing flags on attachment: 182584

Committed r139990: <http://trac.webkit.org/changeset/139990>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Erik Arvidsson]

Thanks