Bug 93971 – [V8] Refactor away IsolatedWorld

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dan Carney]

Refactor away IsolatedWorld

[Dan Carney]

Created attachment 158339 [details]
Patch

[Dan Carney]

Comment on attachment 158339 [details]
Patch

killing patch - some test code made it in

[Dan Carney]

Created attachment 158389 [details]
Patch

[Adam Barth]

Comment on attachment 158389 [details]
Patch

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

> Source/WebCore/bindings/v8/DOMWrapperWorld.h:45
> +    static PassRefPtr<DOMWrapperWorld> create(int worldId = mainWorldId) { return adoptRef(new DOMWrapperWorld(worldId)); }

Can you make createMainWorld be a separate function?  It's a bit too tempting to call create() and duplicate the main world.

> Source/WebCore/bindings/v8/DOMWrapperWorld.h:49
> +        if (m_worldId != mainWorldId)
> +            isolatedWorldCount--;

I would just make this unconditional and have a domWrapperWorldCount.  There's always going to be a main world, so we know how to interpet the value.

> Source/WebCore/bindings/v8/DOMWrapperWorld.h:57
> +

Please remove this blank line.

> Source/WebCore/bindings/v8/DOMWrapperWorld.h:58
> +    DOMWrapperWorld(int worldId): m_worldId(worldId)

This should be split onto two lines with the : as the first character of the second line (indented four spaces).

[Dan Carney]

Comment on attachment 158389 [details]
Patch

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

>> Source/WebCore/bindings/v8/DOMWrapperWorld.h:45
>> +    static PassRefPtr<DOMWrapperWorld> create(int worldId = mainWorldId) { return adoptRef(new DOMWrapperWorld(worldId)); }
> 
> Can you make createMainWorld be a separate function?  It's a bit too tempting to call create() and duplicate the main world.

This is transitional.  There are 2 places in the code where DOMWrapperWorld() was being called, as opposed to IsolatedWorld().  This logic replicates that.  I think those are only called in tests, but I'm not certain.

>> Source/WebCore/bindings/v8/DOMWrapperWorld.h:49
>> +            isolatedWorldCount--;
> 
> I would just make this unconditional and have a domWrapperWorldCount.  There's always going to be a main world, so we know how to interpet the value.

Same issue as above.

[Dan Carney]

Comment on attachment 158389 [details]
Patch

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

>>> Source/WebCore/bindings/v8/DOMWrapperWorld.h:45
>>> +    static PassRefPtr<DOMWrapperWorld> create(int worldId = mainWorldId) { return adoptRef(new DOMWrapperWorld(worldId)); }
>> 
>> Can you make createMainWorld be a separate function?  It's a bit too tempting to call create() and duplicate the main world.
> 
> This is transitional.  There are 2 places in the code where DOMWrapperWorld() was being called, as opposed to IsolatedWorld().  This logic replicates that.  I think those are only called in tests, but I'm not certain.

The problematic calls are in WebViewImpl  http://code.google.com/p/chromium/source/search?q=DOMWrapperWorld%3A%3Acreate&origq=DOMWrapperWorld%3A%3Acreate&btnG=Search+Trunk
Do you happen to know if I can replace them with a call to mainThreadNormalWorld()?

[Adam Barth]

Comment on attachment 158389 [details]
Patch

Ok.  It's fine to move in small steps like this.  We shouldn't be afraid to break things though.  That's what the tests are for.

[Adam Barth]

Comment on attachment 158389 [details]
Patch

Ok.  It's fine to move in small steps like this.  We shouldn't be afraid to break things though.  That's what the tests are for.

[WebKit Review Bot]

Comment on attachment 158389 [details]
Patch

Clearing flags on attachment: 158389

Committed r125717: <http://trac.webkit.org/changeset/125717>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 94272

[Kentaro Hara]

*** Bug 94318 has been marked as a duplicate of this bug. ***

[Dan Carney]

Created attachment 159059 [details]
Patch

[Kentaro Hara]

Comment on attachment 159059 [details]
Patch

Looks OK.

By the way, do you have a plan to rename DOMWrapperWorld to IsolatedWorld? (You can do it in a follow-up patch.) A bunch of method names and variable names are using "isolatedWorld". In particular, some Chromium API names (e.g. evaluateInIsolatedWorld()) are using "IsolatedWorld", and it is difficult to rename Chromium APIs. So I think that it would be reasonable to rename DOMWrapperWorld to IsolatedWorld.

[Dan Carney]

(In reply to comment #14)
> (From update of attachment 159059 [details])
> Looks OK.
> 
> By the way, do you have a plan to rename DOMWrapperWorld to IsolatedWorld? (You can do it in a follow-up patch.) A bunch of method names and variable names are using "isolatedWorld". In particular, some Chromium API names (e.g. evaluateInIsolatedWorld()) are using "IsolatedWorld", and it is difficult to rename Chromium APIs. So I think that it would be reasonable to rename DOMWrapperWorld to IsolatedWorld.

I have some follow up patches, but they just rename the constructors to createIsolatedWorld and createMainWorld.  I wasn't planning to rename back to IsolatedWorld as we are trying to match the JSC api a little more.

[Kentaro Hara]

(In reply to comment #15)
> I have some follow up patches, but they just rename the constructors to createIsolatedWorld and createMainWorld.  I wasn't planning to rename back to IsolatedWorld as we are trying to match the JSC api a little more.

It looks like that JSC binding is sometimes using "IsolatedWorld" and "DOMWrapperWorld" interchangeably. What is the difference between IsolatedWorld and DOMWrapperWorld conceptually?

[WebKit Review Bot]

Comment on attachment 159059 [details]
Patch

Clearing flags on attachment: 159059

Committed r125884: <http://trac.webkit.org/changeset/125884>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 94523

[Dan Carney]

Created attachment 160180 [details]
Patch

[Dan Carney]

(In reply to comment #16)
> (In reply to comment #15)
> > I have some follow up patches, but they just rename the constructors to createIsolatedWorld and createMainWorld.  I wasn't planning to rename back to IsolatedWorld as we are trying to match the JSC api a little more.
> 
> It looks like that JSC binding is sometimes using "IsolatedWorld" and "DOMWrapperWorld" interchangeably. What is the difference between IsolatedWorld and DOMWrapperWorld conceptually?

No difference.  That's why I'm removing the distinction.  The main world is the one the user sees in the inspector, but it's ultimately just an isolated world, too.

[Dan Carney]

(In reply to comment #20)
> Created an attachment (id=160180) [details]
> Patch

Attempt #3.  This version works around an issue where WebViewImpl needs a throwaway main world that it doesn't use but needs it before the v8 isolate is set, causing a crash.

[Adam Barth]

Comment on attachment 160180 [details]
Patch

It looks like you've got all green bubbles.  If you'd like this patch reviewed, please set the Review flag to "?".

[Dan Carney]

(In reply to comment #23)
> (From update of attachment 160180 [details])
> It looks like you've got all green bubbles.  If you'd like this patch reviewed, please set the Review flag to "?".

Yeah, I have to run this through the unit tests and browser tests locally in debug mode to be sure this is good.  It's very easy to trigger assertions by getting this slightly wrong.  I'll flap the flag shortly.

[Kentaro Hara]

(In reply to comment #21)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > I have some follow up patches, but they just rename the constructors to createIsolatedWorld and createMainWorld.  I wasn't planning to rename back to IsolatedWorld as we are trying to match the JSC api a little more.
> > 
> > It looks like that JSC binding is sometimes using "IsolatedWorld" and "DOMWrapperWorld" interchangeably. What is the difference between IsolatedWorld and DOMWrapperWorld conceptually?
> 
> No difference.  That's why I'm removing the distinction.  The main world is the one the user sees in the inspector, but it's ultimately just an isolated world, too.

Then you might want to rename DOMWrapperWorld to IsolatedWorld in a follow-up patch (in both JSC and V8). Having two names for one concept would be confusing.

[Kentaro Hara]

Comment on attachment 160180 [details]
Patch

OK

[WebKit Review Bot]

Comment on attachment 160180 [details]
Patch

Clearing flags on attachment: 160180

Committed r126566: <http://trac.webkit.org/changeset/126566>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.