Remove WindowProxy::isGlobalInitialized(). (DO NOT COMMIT YET)

Remove WindowProxy::isGlobalInitialized().

The distinction is confusing, and all the places that use it can just
use isContextInitialized() instead.

BUG=527190

[dcheng, 2016-10-04 06:58:51]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 06:59:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2016-10-04 07:32:38]

dcheng@chromium.org changed reviewers:
+ haraken@chromium.org, kouhei@chromium.org

[dcheng, 2016-10-04 07:32:38]

Note that this CL doesn't work yet and shouldn't be committed.

I'm currently working on landing lightweight globals for remote frames, using
v8::Context::NewRemoteContext. However, this requires adding more 'is
initialized' checks to WindowProxy: we need a new one to tell if the global
proxy is initialized and attached.

This seems confusing to me, since we will need three different kinds of
initialization checks:
- isContextInitialized: returns true iff there is a v8::Context created and
initialized successfully. Can only be true for local frames.
- isGlobalInitialized: returns true iff we ever successfully created and
initialized a context (full v8::Context or lightweight remote context) for this
WindowProxy. Can be true for either local or remote frames, but will also still
return true if the global is detached.
- isGlobalProxyInitialized: returns true iff the global proxy is valid and
currently attached. Can return true for local or remote frames.

I'd like to get rid of isGlobalInitialized. At the time this code was written,
it looks like we were worried about not updating the document property on a
detached global. However, if the global is detached, then we must run
WindowProxy::initialize() before we can reuse the global, which will update the
cached document. Thus, script will never be able to see a stale cached value of
document. Does this sound reasonable?

The side-effect of this change is that we will need to fix
mainWorldScriptContext() to initialize the v8::Context on demand: currently, it
relies on the fact that the context is almost always implicitly initialized when
calling installNewDocument().

(Also, I think there is a small problem here for remote frames: remote frame
never update the document property. While the cached document wrapper is
inaccessible, I think it will retain the old Document and waste memory. I will
fix that in a separate patch)

[dcheng, 2016-10-04 07:33:31]

On 2016/10/04 07:32:38, dcheng wrote:
> Note that this CL doesn't work yet and shouldn't be committed.
> 
> I'm currently working on landing lightweight globals for remote frames, using
> v8::Context::NewRemoteContext. However, this requires adding more 'is
> initialized' checks to WindowProxy: we need a new one to tell if the global
> proxy is initialized and attached.
> 
> This seems confusing to me, since we will need three different kinds of
> initialization checks:
> - isContextInitialized: returns true iff there is a v8::Context created and
> initialized successfully. Can only be true for local frames.
> - isGlobalInitialized: returns true iff we ever successfully created and
> initialized a context (full v8::Context or lightweight remote context) for
this
> WindowProxy. Can be true for either local or remote frames, but will also
still
> return true if the global is detached.
> - isGlobalProxyInitialized: returns true iff the global proxy is valid and
> currently attached. Can return true for local or remote frames.
> 
> I'd like to get rid of isGlobalInitialized. At the time this code was written,
> it looks like we were worried about not updating the document property on a
> detached global. However, if the global is detached, then we must run
> WindowProxy::initialize() before we can reuse the global, which will update
the
> cached document. Thus, script will never be able to see a stale cached value
of
> document. Does this sound reasonable?
> 
> The side-effect of this change is that we will need to fix
> mainWorldScriptContext() to initialize the v8::Context on demand: currently,
it
> relies on the fact that the context is almost always implicitly initialized
when
> calling installNewDocument().

To clarify, I haven't implemented this last part yet. But if we feel this is the
right direction to go, it is easy to add.

> 
> (Also, I think there is a small problem here for remote frames: remote frame
> never update the document property. While the cached document wrapper is
> inaccessible, I think it will retain the old Document and waste memory. I will
> fix that in a separate patch)

[commit-bot: I haz the power, 2016-10-04 07:46:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 07:46:12]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[haraken, 2016-10-04 11:20:07]

haraken@chromium.org changed reviewers:
+ jochen@chromium.org

[haraken, 2016-10-04 11:20:08]

+jochen

I think that this change is safe.

> This seems confusing to me, since we will need three different kinds of
> initialization checks:
> - isContextInitialized: returns true iff there is a v8::Context created and
> initialized successfully. Can only be true for local frames.
> - isGlobalInitialized: returns true iff we ever successfully created and
> initialized a context (full v8::Context or lightweight remote context) for
this
> WindowProxy. Can be true for either local or remote frames, but will also
still
> return true if the global is detached.
> - isGlobalProxyInitialized: returns true iff the global proxy is valid and
> currently attached. Can return true for local or remote frames.

Maybe would it be better to introduce an enum to maintain the lifecycle?

enum GlobalObjectLifecycle {
  Detached,
  RemoteGlobalAttached,  // light-weight proxy is attached
  LocalGlobalAttached,  // actual global object is attached
};

[dcheng, 2016-10-07 08:16:52]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 08:17:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 09:09:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-07 09:09:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dcheng, 2016-10-14 22:03:20]

So the CL as-is is not practical to land.

kouhei@, do you remember why this optimization is important, if we're
initializing JS anyway most of the time? Is it for documents that can never have
script enabled?

[kouhei (in TOK), 2016-10-17 01:27:09]

On 2016/10/14 22:03:20, dcheng wrote:
> So the CL as-is is not practical to land.
> 
> kouhei@, do you remember why this optimization is important, if we're
> initializing JS anyway most of the time? Is it for documents that can never
have
> script enabled?

This is particularly effective on SVG images. They are handled in Blink like web
pages, but rarely trigger javascript.

[dcheng, 2016-11-08 07:10:12]

Description was changed from

==========
Remove WindowProxy::isGlobalInitialized().

The distinction is confusing, and all the places that use it can just
use isContextInitialized() instead.

BUG=527190
==========

to

==========
Remove WindowProxy::isGlobalInitialized().

The distinction is confusing, and all the places that use it can just
use isContextInitialized() instead.

BUG=527190
==========