Eliminate "temporary" and "uninitialized" isolated worlds

Eliminate "temporary" and "uninitialized" isolated worlds

Temporary worlds were mostly used by the TestRunner. I've updated all layout
tests to always pass a world ID > 0 (Chromium always passes such an ID).
ASSERTs have been added in many places to check this invariant.

The only remaining usage of these concepts is a hack in ScriptController.
After isolating that hack (which itself should eventually be removed), there
are no other uses of uninitializedWorldId to create temporary (or "weak")
isolated worlds.

R=abarth
BUG=230460
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148291

[abarth-chromium, 2013-04-12 02:14:10]

I think there's more code you ran remove, but this is a great start.  You're my
hero.

[abarth-chromium, 2013-04-12 02:14:13]

lgtm

[haraken, 2013-04-12 02:14:19]

Great improvement.

I'd like to add ASSERT(worldID > 0) for all exposed APIs. Does your patch cover
all APIs?

https://codereview.chromium.org/13954010/diff/1/Source/bindings/v8/DOMWrapper...
File Source/bindings/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/13954010/diff/1/Source/bindings/v8/DOMWrapper...
Source/bindings/v8/DOMWrapperWorld.cpp:154: WorldMap::AddResult result =
map.add(worldId, 0);
Shouldn't this be map.find()? And then you need to call map.add() after creating
a new world.

https://codereview.chromium.org/13954010/diff/1/Source/bindings/v8/DOMWrapper...
File Source/bindings/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/13954010/diff/1/Source/bindings/v8/DOMWrapper...
Source/bindings/v8/DOMWrapperWorld.h:124: DOMWrapperWorld*
existingWindowShellWorkaroundWorld();
Maybe deprecatedWindowShellWorld is a better name?

[adamk, 2013-04-12 15:44:04]

I believe this patch covers all APIs (there aren't that many of them). Chromium
calls WebFrame::executeScriptInIsolatedWorld in just two places, and I've
already made sure that they already meet this calling convention (they use a
monotonically increasing world ID).

https://codereview.chromium.org/13954010/diff/1/Source/bindings/v8/DOMWrapper...
File Source/bindings/v8/DOMWrapperWorld.cpp (right):

https://codereview.chromium.org/13954010/diff/1/Source/bindings/v8/DOMWrapper...
Source/bindings/v8/DOMWrapperWorld.cpp:154: WorldMap::AddResult result =
map.add(worldId, 0);
On 2013/04/12 02:14:19, haraken wrote:
> Shouldn't this be map.find()? And then you need to call map.add() after
creating
> a new world.

Odd though this code looks, this is the idiomatic STL/WTF way of adding or
updating a map with a single hash lookup.  The speed probably doesn't matter in
this case, but I'm very used to writing it at this point.

Note that add() only adds the new key/value pair if it's new.

https://codereview.chromium.org/13954010/diff/1/Source/bindings/v8/DOMWrapper...
File Source/bindings/v8/DOMWrapperWorld.h (right):

https://codereview.chromium.org/13954010/diff/1/Source/bindings/v8/DOMWrapper...
Source/bindings/v8/DOMWrapperWorld.h:124: DOMWrapperWorld*
existingWindowShellWorkaroundWorld();
On 2013/04/12 02:14:19, haraken wrote:
> Maybe deprecatedWindowShellWorld is a better name?

I actually think the nearly-nonsensical name is a feature here. The "Do not use
this anywhere else!!" warning clearly wasn't enough to keep dcarney from using
it for this existingWindowShellWorkaround hack; the more specific (and weird) we
keep the name, the less likely someone will try to use it for something.

[commit-bot: I haz the power, 2013-04-12 15:44:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/13954010/1

[adamk, 2013-04-12 15:47:43]

On 2013/04/12 02:14:10, abarth wrote:
> I think there's more code you ran remove, but this is a great start.  You're
my
> hero.

Yeah, there's more to be done around this area, but this got me far enough to
open the way for using RefPtr<DOMWrapperWorld> safely in more places.

[commit-bot: I haz the power, 2013-04-12 16:26:49]

Change committed as 148291