Refactor WindowProxy into Local and Remote subclasses.

Refactor WindowProxy into Local and Remote subclasses.

WindowProxy for remote frames will soon use remote contexts to avoid
the overhead of instantiating a full v8::Context. As preparation,
separate the logic for WindowProxy into LocalFrame and RemoteFrame
versions to make it obvious what's used in each path.

To avoid adding virtual calls to the fast path, common members such
as the lifecycle state and the global proxy have been moved to the
base class. When the WindowProxy is already initialized, this avoids
the cost of any virtual calls.

Future CLs will:
- Convert RemoteWindowProxy to use v8::Context::NewRemoteContext
- Merge LocalWindowProxyManager back into ScriptController
- Clean up the interface for transferring global objects
- Clean up the layering between WindowProxy and its subclasses

BUG=527190
Review-Url: https://codereview.chromium.org/2620313002
Cr-Commit-Position: refs/heads/master@{#442948}
Committed: https://chromium.googlesource.com/chromium/src/+/690c3cf203b94623e5adbece7d6b4afc3501cd94

[dcheng, 2017-01-11 08:04:30]

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

[commit-bot: I haz the power, 2017-01-11 08:04:51]

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

[dcheng, 2017-01-11 08:08:34]

Patchset #3 (id:40001) has been deleted

[dcheng, 2017-01-11 08:10:47]

Description was changed from

==========
Refactor WindowProxy into Local and Remote subclasses.

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

to

==========
Refactor WindowProxy into Local and Remote subclasses.

WindowProxy for remote frames will soon use remote contexts to avoid
the overhead of instantiating a full v8::Context. As preparation,
separate the logic for WindowProxy into LocalFrame and RemoteFrame
versions to make it obvious what's used in each path.

Future CLs will:
- Convert RemoteWindowProxy to use v8::Context::NewRemoteContext
- Merge LocalWindowProxyManager back into ScriptController
- Clean up the interface for transferring global objects

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

[dcheng, 2017-01-11 08:10:48]

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

[dcheng, 2017-01-11 08:12:54]

Patchset #3 (id:60001) has been deleted

[dcheng, 2017-01-11 08:18:51]

Description was changed from

==========
Refactor WindowProxy into Local and Remote subclasses.

WindowProxy for remote frames will soon use remote contexts to avoid
the overhead of instantiating a full v8::Context. As preparation,
separate the logic for WindowProxy into LocalFrame and RemoteFrame
versions to make it obvious what's used in each path.

Future CLs will:
- Convert RemoteWindowProxy to use v8::Context::NewRemoteContext
- Merge LocalWindowProxyManager back into ScriptController
- Clean up the interface for transferring global objects

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

to

==========
Refactor WindowProxy into Local and Remote subclasses.

WindowProxy for remote frames will soon use remote contexts to avoid
the overhead of instantiating a full v8::Context. As preparation,
separate the logic for WindowProxy into LocalFrame and RemoteFrame
versions to make it obvious what's used in each path.

Future CLs will:
- Convert RemoteWindowProxy to use v8::Context::NewRemoteContext
- Merge LocalWindowProxyManager back into ScriptController
- Clean up the interface for transferring global objects
- Clean up the layering between WindowProxy and its subclasses

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

[dcheng, 2017-01-11 08:28:06]

Description was changed from

==========
Refactor WindowProxy into Local and Remote subclasses.

WindowProxy for remote frames will soon use remote contexts to avoid
the overhead of instantiating a full v8::Context. As preparation,
separate the logic for WindowProxy into LocalFrame and RemoteFrame
versions to make it obvious what's used in each path.

Future CLs will:
- Convert RemoteWindowProxy to use v8::Context::NewRemoteContext
- Merge LocalWindowProxyManager back into ScriptController
- Clean up the interface for transferring global objects
- Clean up the layering between WindowProxy and its subclasses

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

to

==========
Refactor WindowProxy into Local and Remote subclasses.

WindowProxy for remote frames will soon use remote contexts to avoid
the overhead of instantiating a full v8::Context. As preparation,
separate the logic for WindowProxy into LocalFrame and RemoteFrame
versions to make it obvious what's used in each path.

To avoid adding virtual calls to the fast path, common members such
as the lifecycle state and the global proxy have been moved to the
base class. When the WindowProxy is already initialized, this avoids
the cost of any virtual calls.

Future CLs will:
- Convert RemoteWindowProxy to use v8::Context::NewRemoteContext
- Merge LocalWindowProxyManager back into ScriptController
- Clean up the interface for transferring global objects
- Clean up the layering between WindowProxy and its subclasses

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

[dcheng, 2017-01-11 08:35:13]

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

[dcheng, 2017-01-11 08:35:16]

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:135:
frame()->loader().dispatchDidClearWindowObjectInMainWorld();
Moved from initializeIfNeeded(), since this logic only applies to local frames.

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (left):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:2: * Copyright
(C) 2009 Google Inc. All rights reserved.
I was unable to get the diffbase on Rietveld to work correctly: this is forked
from WindowProxy.cpp, but for some reason, Rietveld refuses to detect that,
though depot_tools handles it correctly...

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:117: // Remote
frames always require a full canAccess() check.
Moving this here allows us to cleanup the logic in setSecurityToken(). All the
other work in WindowProxy after createContext() is pretty much a no-op. Going in
order:

- updateDocument() calls
updateActivityLogger()+updateDocumentProperty()+updateSecurityOrigin()
  - updateActivityLogger(): the installed activity logger isn't used by anything
available via a remote frame
  - updateDocumentProperty(): returns early if the frame is remote
  - updateSecurityOrigin(): calls setSecurityToken() which eventually uses the
default security token (which is what we do here)

- Checks CSP to enable eval, but we can't eval in a remote context.

- Invokes a few FrameLoaderClient / inspector callbacks that only work for local
frames.

- Installs conditional features, which we don't have on remote contexts.

[commit-bot: I haz the power, 2017-01-11 08:35:35]

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

[Yuki, 2017-01-11 09:05:23]

Is this CL missing LocalWindowProxyManager.{cpp,h}?  Do you have
RemoteWindowProxyManager, too?

[Yuki, 2017-01-11 09:06:07]

On 2017/01/11 09:05:23, Yuki wrote:
> Is this CL missing LocalWindowProxyManager.{cpp,h}?  Do you have
> RemoteWindowProxyManager, too?

Oops, I found them in WindowProxyManager.{cpp,h}.

[Yuki, 2017-01-11 10:09:24]

Mostly nits only.

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.h (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.h:55:
~LocalWindowProxy() override;
nit: "= default" should work.  WindowProxy::~WindowProxy already has DCHECK.

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:121: void
RemoteWindowProxy::createContext() {
Can we unify the implementation between local and remote frames?

Say,
  LocalWindowProxy::createContext() {
    v8::ExcensionConfiguration extensionConfiguration = ...;
    WindowProxy::createContextInternal(&extensionConfiguration);
  }
  RemoteWindowProxy::createContext() {
    WindowProxy::createContextInternal(nullptr);
  }

Does this make sense?

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.h (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.h:49:
~RemoteWindowProxy() override;
nit: "= default" should work, or we don't need this declaration entirely.

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.h:55: void
disposeContext(GlobalDetachmentBehavior);
nit: Add override;

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.h (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptController.h:82:
LocalWindowProxy* windowProxy(DOMWrapperWorld&);
nit: I don't fully understand why you made RefPtr<DOMWrapperWorld> in
WindowProxy, but if it's better, why did you leave this a reference?

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:83: WindowProxy(Frame&,
v8::Isolate*, RefPtr<DOMWrapperWorld>);
nit: Could you put v8::Isolate* first as it's our convention?

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:102: protected:
Could you unify two protected: sections into one?

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:50: protected:
nit: Could you unify two protected: sections into one?
IIRC, our coding style recommends one public:, protected:, and private: section
per class.

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:56: class
WindowProxyManager : public WindowProxyManagerBase {
nit: I guess "WindowProxyManager" is not a good name for this template class. 
This template class looks a kind of hacks to make the following implementations
easy, and doesn't have meaningful role as other classes have.  I personally
don't want to give this very nice and general name to this class.

My understanding is:
  WindowProxyManagerBase = the unified root class
  LocalWindowProxyManager = local version
  RemoteWindowProxyManager = remote version
and we don't need to expose this class to anyone.

What about WindowProxyManager{Internal,ImplHelper} or something like that?

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.h:239: WindowProxyManagerBase*
getWindowProxyManager() const override;
nit: Why not LocalWindowProxyManager?

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RemoteFrame.h (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RemoteFrame.h:71: WindowProxyManagerBase*
getWindowProxyManager() const override;
nit: Why not RemoteWindowProxyManager?

[commit-bot: I haz the power, 2017-01-11 10:30:39]

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

[commit-bot: I haz the power, 2017-01-11 10:30:40]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-01-11 10:34:46]

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

[commit-bot: I haz the power, 2017-01-11 10:35:10]

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

[dcheng, 2017-01-11 10:35:29]

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.h (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.h:55:
~LocalWindowProxy() override;
On 2017/01/11 10:09:24, Yuki wrote:
> nit: "= default" should work.  WindowProxy::~WindowProxy already has DCHECK.

Removed, since that's equivalent to = default.

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.cpp:121: void
RemoteWindowProxy::createContext() {
On 2017/01/11 10:09:24, Yuki wrote:
> Can we unify the implementation between local and remote frames?
> 
> Say,
>   LocalWindowProxy::createContext() {
>     v8::ExcensionConfiguration extensionConfiguration = ...;
>     WindowProxy::createContextInternal(&extensionConfiguration);
>   }
>   RemoteWindowProxy::createContext() {
>     WindowProxy::createContextInternal(nullptr);
>   }
> 
> Does this make sense?

I intentionally split this, because RemoteWindowProxy::createContext() will look
very different soon -- it won't create ScriptState or v8::Context at all, since
it will be using v8::Context::NewRemoteContext. Thus, I didn't want to implement
this logic in a common base class because I'll have to move it in a followup
anyway.

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.h (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.h:49:
~RemoteWindowProxy() override;
On 2017/01/11 10:09:24, Yuki wrote:
> nit: "= default" should work, or we don't need this declaration entirely.

Done.

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/RemoteWindowProxy.h:55: void
disposeContext(GlobalDetachmentBehavior);
On 2017/01/11 10:09:24, Yuki wrote:
> nit: Add override;

Done.

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.h (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptController.h:82:
LocalWindowProxy* windowProxy(DOMWrapperWorld&);
On 2017/01/11 10:09:24, Yuki wrote:
> nit: I don't fully understand why you made RefPtr<DOMWrapperWorld> in
> WindowProxy, but if it's better, why did you leave this a reference?

WindowProxy takes a reference when we construct it, so to indicate that, the
constructor / create factory functions take a RefPtr.

Logically, this function is for looking up window proxies -- while it might
sometimes ref them when it needs to construct a new one, that is an internal
implementation detail. Thus, this takes a DOMWrapperWorld& instead of a
RefPtr<DOMWrapperWorld>.

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.h (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:83: WindowProxy(Frame&,
v8::Isolate*, RefPtr<DOMWrapperWorld>);
On 2017/01/11 10:09:24, Yuki wrote:
> nit: Could you put v8::Isolate* first as it's our convention?

Done, here and elsewhere.

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.h:102: protected:
On 2017/01/11 10:09:24, Yuki wrote:
> Could you unify two protected: sections into one?

This is a bit messy right now. However, the reason it's split into multiple
sections is to keep destruction order the same. Changing it /probably/ won't
break anything, but it's not certain. Thus, I want to do the cleanups in
followups.

In the future cleanups, all these variables are going to move into the private
section, with one exception:
- m_scriptState will be LocalWindowProxy only. Thus, I don't add a protected
getter for it since I'll have to change it back in the followup CL.
- m_world will become private once we don't need to expose world() as a getter,
by doing some cleanup in WindowProxyManager.
- m_globalProxy will stay protected and move into the above section.
- m_lifecycle will become private, and have a getter. There will be a special
way to mark this as initialized so we don't need to directly set it in the
subclasses.

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:50: protected:
On 2017/01/11 10:09:24, Yuki wrote:
> nit: Could you unify two protected: sections into one?
> IIRC, our coding style recommends one public:, protected:, and private:
section
> per class.

Made this private and added protected getters for LocalWindowProxyManager.

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:56: class
WindowProxyManager : public WindowProxyManagerBase {
On 2017/01/11 10:09:24, Yuki wrote:
> nit: I guess "WindowProxyManager" is not a good name for this template class. 
> This template class looks a kind of hacks to make the following
implementations
> easy, and doesn't have meaningful role as other classes have.  I personally
> don't want to give this very nice and general name to this class.
> 
> My understanding is:
>   WindowProxyManagerBase = the unified root class
>   LocalWindowProxyManager = local version
>   RemoteWindowProxyManager = remote version
> and we don't need to expose this class to anyone.
> 
> What about WindowProxyManager{Internal,ImplHelper} or something like that?

Renamed it ImplHelper.

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.h:239: WindowProxyManagerBase*
getWindowProxyManager() const override;
On 2017/01/11 10:09:24, Yuki wrote:
> nit: Why not LocalWindowProxyManager?

This is an override. I can't include WindowProxyManager.h here, due to include
cycles. Since it doesn't need to be covariant, I just return the base type for
simplicity.

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RemoteFrame.h (right):

https://codereview.chromium.org/2620313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RemoteFrame.h:71: WindowProxyManagerBase*
getWindowProxyManager() const override;
On 2017/01/11 10:09:24, Yuki wrote:
> nit: Why not RemoteWindowProxyManager?

Ditto.

[Yuki, 2017-01-11 11:20:09]

LGTM.

[haraken, 2017-01-11 12:33:16]

LGTM

https://codereview.chromium.org/2620313002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h (right):

https://codereview.chromium.org/2620313002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:56: class
WindowProxyManagerImplHelper : public WindowProxyManagerBase {

Why do we need the ImplHelper? i.e., Can we reorganize the class hierarchy like
this?

class WindowProxyManager { };
class LocalWindowProxyManager : public WindowProxyManager { };
class RemoteWindowProxyManager : public WindowProxyManager { };

https://codereview.chromium.org/2620313002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:89:
LocalWindowProxyManager(LocalFrame& frame)

Add explicit.

https://codereview.chromium.org/2620313002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:101:
RemoteWindowProxyManager(RemoteFrame& frame)

Add explicit.

[haraken, 2017-01-11 12:34:36]

On 2017/01/11 12:33:16, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/2620313002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h (right):
> 
>
https://codereview.chromium.org/2620313002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:56: class
> WindowProxyManagerImplHelper : public WindowProxyManagerBase {
> 
> Why do we need the ImplHelper? i.e., Can we reorganize the class hierarchy
like
> this?
> 
> class WindowProxyManager { };
> class LocalWindowProxyManager : public WindowProxyManager { };
> class RemoteWindowProxyManager : public WindowProxyManager { };

ah, I noticed you've discussed it in the above.

> 
>
https://codereview.chromium.org/2620313002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:89:
> LocalWindowProxyManager(LocalFrame& frame)
> 
> Add explicit.
> 
>
https://codereview.chromium.org/2620313002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:101:
> RemoteWindowProxyManager(RemoteFrame& frame)
> 
> Add explicit.

[commit-bot: I haz the power, 2017-01-11 13:48:21]

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

[commit-bot: I haz the power, 2017-01-11 13:48:22]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-01-11 16:12:40]

Thank you for the fast reviews!

https://codereview.chromium.org/2620313002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h (right):

https://codereview.chromium.org/2620313002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:56: class
WindowProxyManagerImplHelper : public WindowProxyManagerBase {
On 2017/01/11 12:33:16, haraken wrote:
> 
> Why do we need the ImplHelper? i.e., Can we reorganize the class hierarchy
like
> this?
> 
> class WindowProxyManager { };
> class LocalWindowProxyManager : public WindowProxyManager { };
> class RemoteWindowProxyManager : public WindowProxyManager { };

I split it into two portions so that we will have just one copy of the shared
functionality: usually, the linker should be able to merge the two copies, but
there's no guarantee that it will do so. In addition, it allows us to
out-of-line non-trivial functions. This is a pattern used by things like
RefCountedBase + RefCounted, etc as well.

https://codereview.chromium.org/2620313002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:89:
LocalWindowProxyManager(LocalFrame& frame)
On 2017/01/11 12:33:16, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/2620313002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:101:
RemoteWindowProxyManager(RemoteFrame& frame)
On 2017/01/11 12:33:16, haraken wrote:
> 
> Add explicit.

Done.

[dcheng, 2017-01-11 16:12:45]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-01-11 16:12:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2620313002/#ps140001
(title: "explicit")

[commit-bot: I haz the power, 2017-01-11 16:13:13]

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, 2017-01-11 17:47:32]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484151165992060,
"parent_rev": "0811dbe5577e389b35e8bc42ff9d19da756de89c", "commit_rev":
"690c3cf203b94623e5adbece7d6b4afc3501cd94"}

[commit-bot: I haz the power, 2017-01-11 17:48:21]

Description was changed from

==========
Refactor WindowProxy into Local and Remote subclasses.

WindowProxy for remote frames will soon use remote contexts to avoid
the overhead of instantiating a full v8::Context. As preparation,
separate the logic for WindowProxy into LocalFrame and RemoteFrame
versions to make it obvious what's used in each path.

To avoid adding virtual calls to the fast path, common members such
as the lifecycle state and the global proxy have been moved to the
base class. When the WindowProxy is already initialized, this avoids
the cost of any virtual calls.

Future CLs will:
- Convert RemoteWindowProxy to use v8::Context::NewRemoteContext
- Merge LocalWindowProxyManager back into ScriptController
- Clean up the interface for transferring global objects
- Clean up the layering between WindowProxy and its subclasses

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

to

==========
Refactor WindowProxy into Local and Remote subclasses.

WindowProxy for remote frames will soon use remote contexts to avoid
the overhead of instantiating a full v8::Context. As preparation,
separate the logic for WindowProxy into LocalFrame and RemoteFrame
versions to make it obvious what's used in each path.

To avoid adding virtual calls to the fast path, common members such
as the lifecycle state and the global proxy have been moved to the
base class. When the WindowProxy is already initialized, this avoids
the cost of any virtual calls.

Future CLs will:
- Convert RemoteWindowProxy to use v8::Context::NewRemoteContext
- Merge LocalWindowProxyManager back into ScriptController
- Clean up the interface for transferring global objects
- Clean up the layering between WindowProxy and its subclasses

BUG=527190

Review-Url: https://codereview.chromium.org/2620313002
Cr-Commit-Position: refs/heads/master@{#442948}
Committed:
https://chromium.googlesource.com/chromium/src/+/690c3cf203b94623e5adbece7d6b...
==========

[commit-bot: I haz the power, 2017-01-11 17:48:22]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/690c3cf203b94623e5adbece7d6b...