Refactor away IsolatedWorld
Created attachment 158339 [details] Patch
Comment on attachment 158339 [details] Patch killing patch - some test code made it in
Created attachment 158389 [details] Patch
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).
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.
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()?
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.
Comment on attachment 158389 [details] Patch Clearing flags on attachment: 158389 Committed r125717: <http://trac.webkit.org/changeset/125717>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 94272
*** Bug 94318 has been marked as a duplicate of this bug. ***
Created attachment 159059 [details] Patch
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.
(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.
(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?
Comment on attachment 159059 [details] Patch Clearing flags on attachment: 159059 Committed r125884: <http://trac.webkit.org/changeset/125884>
Re-opened since this is blocked by 94523
Created attachment 160180 [details] Patch
(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.
(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.
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 "?".
(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.
(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.
Comment on attachment 160180 [details] Patch OK
Comment on attachment 160180 [details] Patch Clearing flags on attachment: 160180 Committed r126566: <http://trac.webkit.org/changeset/126566>