bindings: Simplifies WindowProxyManager and its relation to Frame.

bindings: Simplifies WindowProxyManager and its relation to Frame.

Both of LocalFrame and RemoteFrame have a WindowProxyManager.
We can move it into a base class Frame.

windowProxy->initializedIfNeeded() is moved into WindowProxyManager
from ScriptController and RemoteFrame.
WindowProxyManager::windowProxy(world) now always returns
an initialized global proxy.  It's safer than before, I think.
(But no behavioral change in this CL at all.)

Also the following renaming is done in order to emphasize that
the returned object may not be initialized.

  WindowProxyManagerBase
  => WindowProxyManager
  (Same naming as Frame, DOMWindow and WindowProxy)

  WindowProxyManager::mainWorldProxy
  => mainWorldProxyMaybeUninitialized

  WindowProxyManager::windowProxy
  => windowProxyMaybeUninitialized

By the way, we don't much use RemoteWindowProxyManager.
We can simply remove the class in a follow-up CL.

BUG=

Review-Url: https://codereview.chromium.org/2702273004
Cr-Commit-Position: refs/heads/master@{#457409}
Committed: https://chromium.googlesource.com/chromium/src/+/4136f45071af9b51dfb36598efa9ac705c8d1b0f

[Yuki, 2017-02-21 14:56:20]

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

[commit-bot: I haz the power, 2017-02-21 14:56:45]

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

[Yuki, 2017-02-21 15:12:39]

Description was changed from

==========
bindings: Simplifies WindowProxyManager and its relation to Frame.

BUG=
==========

to

==========
bindings: Simplifies WindowProxyManager and its relation to Frame.

Both of LocalFrame and RemoteFrame have a WindowProxyManager.
We can move it into a base class Frame.

windowProxy->initializedIfNeeded() is moved into WindowProxyManager
from ScriptController and RemoteFrame.
WindowProxyManager::windowProxy(world) now always returns
an initialized global proxy.  It's safer than before, I think.
(But no behavioral change in this CL at all.)

Also the following renaming is done in order to emphasize that
the returned object may not be initialized.

  WindowProxyManagerBase
  => WindowProxyManager
  (Same naming as Frame, DOMWindow and WindowProxy)

  WindowProxyManager::mainWorldProxy
  => mainWorldProxyMaybeUninitialized

  WindowProxyManager::windowProxy
  => windowProxyMaybeUninitialized

By the way, we don't much use RemoteWindowProxyManager.
We can simply remove the class in a follow-up CL.

BUG=
==========

[Yuki, 2017-02-21 15:17:11]

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

[Yuki, 2017-02-21 15:17:12]

Could you guys review this CL?

This CL should have no behavioral change (at least I meant so).

[commit-bot: I haz the power, 2017-02-21 16:07:46]

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

[commit-bot: I haz the power, 2017-02-21 16:07:46]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[haraken, 2017-02-22 01:03:49]

LGTM although I'd prefer splitting the CL into small pieces.

[dcheng, 2017-02-22 01:07:06]

https://codereview.chromium.org/2702273004/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h (right):

https://codereview.chromium.org/2702273004/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:86:
LocalWindowProxy* mainWorldProxyMaybeUninitialized() {
I would prefer not to expose this publicly if possible. My original approach was
to find the things that depended on this (there aren't that many) and add
explicit helpers for them.

https://codereview.chromium.org/2702273004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/Frame.h (right):

https://codereview.chromium.org/2702273004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/Frame.h:101: FrameOwner* owner() const {
return m_owner; }
Maybe split cleanup changes like this into a separate CL?

[haraken, 2017-02-22 03:05:49]

On 2017/02/22 01:07:06, dcheng wrote:
>
https://codereview.chromium.org/2702273004/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h (right):
> 
>
https://codereview.chromium.org/2702273004/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:86:
> LocalWindowProxy* mainWorldProxyMaybeUninitialized() {
> I would prefer not to expose this publicly if possible. My original approach
was
> to find the things that depended on this (there aren't that many) and add
> explicit helpers for them.
> 
>
https://codereview.chromium.org/2702273004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/Frame.h (right):
> 
>
https://codereview.chromium.org/2702273004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/Frame.h:101: FrameOwner* owner() const {
> return m_owner; }
> Maybe split cleanup changes like this into a separate CL?

Yeah, this CL is doing multiple things in one go. I'd prefer splitting the
trivial refactoring to a separate CL.

[Yuki, 2017-02-22 08:52:16]

Patchset #2 (id:20001) has been deleted

[Yuki, 2017-02-22 09:01:33]

https://codereview.chromium.org/2702273004/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h (right):

https://codereview.chromium.org/2702273004/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:86:
LocalWindowProxy* mainWorldProxyMaybeUninitialized() {
On 2017/02/22 01:07:06, dcheng wrote:
> I would prefer not to expose this publicly if possible. My original approach
was
> to find the things that depended on this (there aren't that many) and add
> explicit helpers for them.

Hmm, I added WindowProxy::isInitialized() and changed ScriptController to use
it.  See PS2.  This way, we don't need to expose ...MaybeUninitialized, however,
now the decision making is moved from WindowProxy to ScriptController about
whether we can skip task X on WindowProxy.  I'm on the fence which is better,
and I'm fine with either way.

https://codereview.chromium.org/2702273004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/Frame.h (right):

https://codereview.chromium.org/2702273004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/Frame.h:101: FrameOwner* owner() const {
return m_owner; }
On 2017/02/22 01:07:06, dcheng wrote:
> Maybe split cleanup changes like this into a separate CL?

Agreed.  Reverted these part.

[Yuki, 2017-02-22 09:02:18]

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

[commit-bot: I haz the power, 2017-02-22 09:03:38]

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, 2017-02-22 10:10:24]

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

[commit-bot: I haz the power, 2017-02-22 10:10:25]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dcheng, 2017-02-23 19:02:36]

https://codereview.chromium.org/2702273004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp (right):

https://codereview.chromium.org/2702273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp:84:
mainWorldProxy()->updateSecurityOrigin(securityOrigin);
Hmm... updateSecurityOrigin() also checks this, so it looks a bit unusual.

Maybe m_isolatedWorlds and m_mainWorldProxy should just be protected members so
they can be accessed directly?

https://codereview.chromium.org/2702273004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h (right):

https://codereview.chromium.org/2702273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:36: Frame*
frame() const { return m_frame; }
I would personally prefer not to expose this publicly, but I'm not sure if
there's a good way to do that without friends (this is why I tried to use
inheritance in an earlier patch)

https://codereview.chromium.org/2702273004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Frame.h (right):

https://codereview.chromium.org/2702273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Frame.h:105: WindowProxy*
windowProxy(DOMWrapperWorld&);
For grouping, maybe this should be placed with getWindowProxyManager()?

[Yuki, 2017-03-09 10:20:59]

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

[commit-bot: I haz the power, 2017-03-09 10:21:17]

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, 2017-03-09 11:17:43]

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

[commit-bot: I haz the power, 2017-03-09 11:17:44]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Yuki, 2017-03-09 14:55:09]

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

[commit-bot: I haz the power, 2017-03-09 14:55:20]

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

[Yuki, 2017-03-09 14:58:53]

Sorry for leaving this CL for a long time.  I'm finally back.

Could you take another look?  (You'd need time to remember this CL, sorry for
that pain.)

Addressed dcheng's comments in PS4, and made more changes in PS5 in order to
keep the exactly same behaviors as before.

https://codereview.chromium.org/2702273004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp (right):

https://codereview.chromium.org/2702273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp:84:
mainWorldProxy()->updateSecurityOrigin(securityOrigin);
On 2017/02/23 19:02:36, dcheng wrote:
> Hmm... updateSecurityOrigin() also checks this, so it looks a bit unusual.
> 
> Maybe m_isolatedWorlds and m_mainWorldProxy should just be protected members
so
> they can be accessed directly?

Done.

https://codereview.chromium.org/2702273004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h (right):

https://codereview.chromium.org/2702273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:36: Frame*
frame() const { return m_frame; }
On 2017/02/23 19:02:36, dcheng wrote:
> I would personally prefer not to expose this publicly, but I'm not sure if
> there's a good way to do that without friends (this is why I tried to use
> inheritance in an earlier patch)

Since this is only used by ScriptController, I removed this.  LocalFrame passes
|this| to ScriptController when creating it.

https://codereview.chromium.org/2702273004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Frame.h (right):

https://codereview.chromium.org/2702273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Frame.h:105: WindowProxy*
windowProxy(DOMWrapperWorld&);
On 2017/02/23 19:02:36, dcheng wrote:
> For grouping, maybe this should be placed with getWindowProxyManager()?

Done.

[commit-bot: I haz the power, 2017-03-09 15:43:56]

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

[commit-bot: I haz the power, 2017-03-09 15:43:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2017-03-10 09:52:13]

lgtm with nits

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

https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptController.h:70: static
ScriptController* create(LocalFrame* frame,
Nit: pass the arguments by reference

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

https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:38: return
!m_windowProxy->contextIfInitialized().IsEmpty();
Can we move this to LocalWindowProxyManager, since it's only used for local
frames?

https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:46: WindowProxy*
windowProxy(DOMWrapperWorld& world) {
It's not necessary to do it now, but maybe there is possibility for cleanup here
in the future, since there are two ways to get to the main world proxy (and they
behave identically now)

[haraken, 2017-03-10 12:44:12]

Mostly looks good with one question.

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

https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp:61: :
m_isolate(v8::Isolate::GetCurrent()),

toIsolate(frame).

Avoid using v8::Isolate::GetCurrent().

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

https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:855:
static_cast<LocalWindowProxyManager*>(getWindowProxyManager()))),

Hmm, it looks a bit redundant that LocalFrame has m_script and
m_windowProxyManager. Is there any way to refactor code to drop one of the
pointers?

[Yuki, 2017-03-10 14:29:41]

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

[commit-bot: I haz the power, 2017-03-10 14:29:53]

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

[Yuki, 2017-03-10 14:41:06]

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

[commit-bot: I haz the power, 2017-03-10 14:41:22]

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

[Yuki, 2017-03-10 14:41:25]

Patchset #6 (id:120001) has been deleted

[Yuki, 2017-03-10 14:56:12]

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

[commit-bot: I haz the power, 2017-03-10 14:56:21]

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

[Yuki, 2017-03-10 15:20:14]

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

[commit-bot: I haz the power, 2017-03-10 15:20:27]

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

[Yuki, 2017-03-10 15:21:51]

Addressed review comments.
Also removed (Local)WindowProxyManager::isMainWorldContextInitialized because it
turned out that it doesn't help much in use cases in ScriptController.cpp. 
Changed the code to directly use mainWorldProxyMaybeUninitialized().  It's close
to the original code.

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

https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptController.h:70: static
ScriptController* create(LocalFrame* frame,
On 2017/03/10 09:52:12, dcheng wrote:
> Nit: pass the arguments by reference

Done.

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

https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp:61: :
m_isolate(v8::Isolate::GetCurrent()),
On 2017/03/10 12:44:12, haraken wrote:
> 
> toIsolate(frame).
> 
> Avoid using v8::Isolate::GetCurrent().

blink::toIsolate(frame) is returning the frame's WindowProxyManager's isolate,
i.e. this value itself.  So we cannot use it.

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

https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:38: return
!m_windowProxy->contextIfInitialized().IsEmpty();
On 2017/03/10 09:52:13, dcheng wrote:
> Can we move this to LocalWindowProxyManager, since it's only used for local
> frames?

Done.

https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:46: WindowProxy*
windowProxy(DOMWrapperWorld& world) {
On 2017/03/10 09:52:13, dcheng wrote:
> It's not necessary to do it now, but maybe there is possibility for cleanup
here
> in the future, since there are two ways to get to the main world proxy (and
they
> behave identically now)

Acknowledged.

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

https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:855:
static_cast<LocalWindowProxyManager*>(getWindowProxyManager()))),
On 2017/03/10 12:44:12, haraken wrote:
> 
> Hmm, it looks a bit redundant that LocalFrame has m_script and
> m_windowProxyManager. Is there any way to refactor code to drop one of the
> pointers?

ScriptController::ScriptController(LocalFrame& frame)
    : m_frame(&frame) {}
ScriptController::getWindowProxyManager() {
  return m_frame->getWindowProxyManager();
}

Then, we don't need to directly pass a WindowProxyManager to ScriptController. 
I'm not sure if this is a good idea.

[commit-bot: I haz the power, 2017-03-10 16:47:10]

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

[commit-bot: I haz the power, 2017-03-10 16:47:12]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[haraken, 2017-03-11 19:35:25]

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

https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp:61: :
m_isolate(v8::Isolate::GetCurrent()),
On 2017/03/10 15:21:50, Yuki wrote:
> On 2017/03/10 12:44:12, haraken wrote:
> > 
> > toIsolate(frame).
> > 
> > Avoid using v8::Isolate::GetCurrent().
> 
> blink::toIsolate(frame) is returning the frame's WindowProxyManager's isolate,
> i.e. this value itself.  So we cannot use it.

Can we explicitly pass in the Isolate* parameter?

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

https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:855:
static_cast<LocalWindowProxyManager*>(getWindowProxyManager()))),
On 2017/03/10 15:21:51, Yuki wrote:
> On 2017/03/10 12:44:12, haraken wrote:
> > 
> > Hmm, it looks a bit redundant that LocalFrame has m_script and
> > m_windowProxyManager. Is there any way to refactor code to drop one of the
> > pointers?
> 
> ScriptController::ScriptController(LocalFrame& frame)
>     : m_frame(&frame) {}
> ScriptController::getWindowProxyManager() {
>   return m_frame->getWindowProxyManager();
> }
> 
> Then, we don't need to directly pass a WindowProxyManager to ScriptController.

> I'm not sure if this is a good idea.

It's fine that ScriptController has m_windowProxyManager.

I'm wondering why LocalFrame needs to hold both m_script (on LocalFrame) and
m_windowProxyManager (on Frame).

[dcheng, 2017-03-12 07:59:21]

On 2017/03/11 19:35:25, haraken wrote:
>
https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp
(right):
> 
>
https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp:61: :
> m_isolate(v8::Isolate::GetCurrent()),
> On 2017/03/10 15:21:50, Yuki wrote:
> > On 2017/03/10 12:44:12, haraken wrote:
> > > 
> > > toIsolate(frame).
> > > 
> > > Avoid using v8::Isolate::GetCurrent().
> > 
> > blink::toIsolate(frame) is returning the frame's WindowProxyManager's
isolate,
> > i.e. this value itself.  So we cannot use it.
> 
> Can we explicitly pass in the Isolate* parameter?
> 
>
https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):
> 
>
https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/LocalFrame.cpp:855:
> static_cast<LocalWindowProxyManager*>(getWindowProxyManager()))),
> On 2017/03/10 15:21:51, Yuki wrote:
> > On 2017/03/10 12:44:12, haraken wrote:
> > > 
> > > Hmm, it looks a bit redundant that LocalFrame has m_script and
> > > m_windowProxyManager. Is there any way to refactor code to drop one of the
> > > pointers?
> > 
> > ScriptController::ScriptController(LocalFrame& frame)
> >     : m_frame(&frame) {}
> > ScriptController::getWindowProxyManager() {
> >   return m_frame->getWindowProxyManager();
> > }
> > 
> > Then, we don't need to directly pass a WindowProxyManager to
ScriptController.
> 
> > I'm not sure if this is a good idea.
> 
> It's fine that ScriptController has m_windowProxyManager.
> 
> I'm wondering why LocalFrame needs to hold both m_script (on LocalFrame) and
> m_windowProxyManager (on Frame).

It's basically a tradeoff to make Frame::windowProxy non-virtual, right? I'm
curious for the reasoning behind this as well (and if it matters for
performance).

Note that it would be possible to make it non-virtual, if ScriptController
inherited LocalWindowProxyManager (something like
https://codereview.chromium.org/2630693002/): this is actually closer to the
original design of ScriptController, which combined script running functionality
with window proxy management. The only reason that it lives in a separate
subclass atm is because it was factored out for OOPIF. Personally, I feel the
current design (where ScriptController has-a WindowProxyManager) is kind of
problematic: ScriptController does complicated things with window proxies
sometimes, and so I ended up adding the friend declaration as a backdoor...

[haraken, 2017-03-12 21:27:56]

On 2017/03/12 07:59:21, dcheng wrote:
> On 2017/03/11 19:35:25, haraken wrote:
> >
>
https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp:61: :
> > m_isolate(v8::Isolate::GetCurrent()),
> > On 2017/03/10 15:21:50, Yuki wrote:
> > > On 2017/03/10 12:44:12, haraken wrote:
> > > > 
> > > > toIsolate(frame).
> > > > 
> > > > Avoid using v8::Isolate::GetCurrent().
> > > 
> > > blink::toIsolate(frame) is returning the frame's WindowProxyManager's
> isolate,
> > > i.e. this value itself.  So we cannot use it.
> > 
> > Can we explicitly pass in the Isolate* parameter?
> > 
> >
>
https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/frame/LocalFrame.cpp:855:
> > static_cast<LocalWindowProxyManager*>(getWindowProxyManager()))),
> > On 2017/03/10 15:21:51, Yuki wrote:
> > > On 2017/03/10 12:44:12, haraken wrote:
> > > > 
> > > > Hmm, it looks a bit redundant that LocalFrame has m_script and
> > > > m_windowProxyManager. Is there any way to refactor code to drop one of
the
> > > > pointers?
> > > 
> > > ScriptController::ScriptController(LocalFrame& frame)
> > >     : m_frame(&frame) {}
> > > ScriptController::getWindowProxyManager() {
> > >   return m_frame->getWindowProxyManager();
> > > }
> > > 
> > > Then, we don't need to directly pass a WindowProxyManager to
> ScriptController.
> > 
> > > I'm not sure if this is a good idea.
> > 
> > It's fine that ScriptController has m_windowProxyManager.
> > 
> > I'm wondering why LocalFrame needs to hold both m_script (on LocalFrame) and
> > m_windowProxyManager (on Frame).
> 
> It's basically a tradeoff to make Frame::windowProxy non-virtual, right? I'm
> curious for the reasoning behind this as well (and if it matters for
> performance).
> 
> Note that it would be possible to make it non-virtual, if ScriptController
> inherited LocalWindowProxyManager (something like
> https://codereview.chromium.org/2630693002/): this is actually closer to the
> original design of ScriptController, which combined script running
functionality
> with window proxy management. The only reason that it lives in a separate
> subclass atm is because it was factored out for OOPIF. Personally, I feel the
> current design (where ScriptController has-a WindowProxyManager) is kind of
> problematic: ScriptController does complicated things with window proxies
> sometimes, and so I ended up adding the friend declaration as a backdoor...

Ah, now I understand why you wanted to make ScriptController inherit from
LocalWindowProxyManager.

It looks like this issue is kind of orthogonal to this CL. LGTM.

[Yuki, 2017-03-13 07:27:23]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2017-03-13 07:27:24]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2702273004/#ps180001
(title: "Removed isMainWorldContextInitialized().")

[commit-bot: I haz the power, 2017-03-13 07:27:33]

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

[Yuki, 2017-03-13 07:27:40]

The CQ bit was unchecked by yukishiino@chromium.org

[Yuki, 2017-03-13 08:23:50]

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

[commit-bot: I haz the power, 2017-03-13 08:24:00]

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

[Yuki, 2017-03-13 08:24:30]

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

https://codereview.chromium.org/2702273004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp:61: :
m_isolate(v8::Isolate::GetCurrent()),
On 2017/03/11 19:35:24, haraken wrote:
> On 2017/03/10 15:21:50, Yuki wrote:
> > On 2017/03/10 12:44:12, haraken wrote:
> > > 
> > > toIsolate(frame).
> > > 
> > > Avoid using v8::Isolate::GetCurrent().
> > 
> > blink::toIsolate(frame) is returning the frame's WindowProxyManager's
isolate,
> > i.e. this value itself.  So we cannot use it.
> 
> Can we explicitly pass in the Isolate* parameter?

Currently, the call sites don't have an Isolate.  So it's not easy.  Anyway, I
didn't change this point in this CL (the old implementation has been using
v8::Isolate::GetCurrent()), so I suppose that it should be addressed separately.

[commit-bot: I haz the power, 2017-03-13 09:46:13]

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

[commit-bot: I haz the power, 2017-03-13 09:46:15]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Yuki, 2017-03-15 14:01:57]

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

[commit-bot: I haz the power, 2017-03-15 14:02:19]

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

[Yuki, 2017-03-15 14:04:14]

It turned out that I need more change about
WindowProxyManager::createWindowProxyForFrame in PS11.

Could you guys take a (hopefully last) look at PS11?

[Yuki, 2017-03-15 14:06:54]

On 2017/03/15 14:04:14, Yuki wrote:
> It turned out that I need more change about
> WindowProxyManager::createWindowProxyForFrame in PS11.
> 
> Could you guys take a (hopefully last) look at PS11?

The reason is that this CL is instantiating WindowProxyManager during a
construction of LocalFrame, so we cannot use a virtual member function such as
LocalFrame::isLocalFrame.  Memory allocation for LocalFrame is already done, so
we can use its address, but not vtbl.

[commit-bot: I haz the power, 2017-03-15 15:58:52]

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

[commit-bot: I haz the power, 2017-03-15 15:58:53]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-03-16 08:43:16]

I'm going offline for the night, so please don't feel like you need to block on
me.

https://codereview.chromium.org/2702273004/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp (right):

https://codereview.chromium.org/2702273004/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp:57: //
RemoteFrame.
This makes me a little nervous: I remember hitting similar issues in the past
(see
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page...),
and I'm worried that LocalWindowProxy/RemoteWindowProxy might try to call
virtual functions on Frame in the future.

One alternative, which is a little less nice, is to make m_windowProxyManager a
protected member, and have the subclasses (which are final) set it directly in
the body of the constructor, when the vtables are completely initialized. WDYT?

[Yuki, 2017-03-16 12:41:21]

https://codereview.chromium.org/2702273004/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp (right):

https://codereview.chromium.org/2702273004/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp:57: //
RemoteFrame.
On 2017/03/16 08:43:16, dcheng wrote:
> This makes me a little nervous: I remember hitting similar issues in the past
> (see
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page...),
> and I'm worried that LocalWindowProxy/RemoteWindowProxy might try to call
> virtual functions on Frame in the future.
> 
> One alternative, which is a little less nice, is to make m_windowProxyManager
a
> protected member, and have the subclasses (which are final) set it directly in
> the body of the constructor, when the vtables are completely initialized.
WDYT?

I personally prefer making Frame::m_windowProxyManager const, also making
WindowProxyManager::m_frame const.  When I see "const" there, I'm very happy
knowing "oh, it's guaranteed to be the same for the whole lifetime.  no need to
worry about any change nor lifetime cycle."

So, if we're going to set it in the body of the constructor, I'd like to use
const_cast, but I'm not really happy with const_cast.

I agree that this is a little hacky and not super nice.  I'm on the fence about
what to do, but I'd slightly prefer this way (static_cast with m_frameType) to
const_cast.

[Yuki, 2017-03-16 12:43:05]

I'll commit this CL seeing no strong objection, but I'm happy to address any
issues on a follow-up CL.  So, please comment anything you have.

[Yuki, 2017-03-16 12:43:12]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2017-03-16 12:43:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2702273004/#ps240001
(title: "Fixed WindowProxyManager::createWindowProxy(ForFrame).")

[commit-bot: I haz the power, 2017-03-16 12:43:29]

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-03-16 12:47:10]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1489668192891750,
"parent_rev": "2e20ab7ecb1ffe0fc6ece1e5c57c14b8ce1ec01f", "commit_rev":
"4136f45071af9b51dfb36598efa9ac705c8d1b0f"}

[commit-bot: I haz the power, 2017-03-16 12:47:54]

Description was changed from

==========
bindings: Simplifies WindowProxyManager and its relation to Frame.

Both of LocalFrame and RemoteFrame have a WindowProxyManager.
We can move it into a base class Frame.

windowProxy->initializedIfNeeded() is moved into WindowProxyManager
from ScriptController and RemoteFrame.
WindowProxyManager::windowProxy(world) now always returns
an initialized global proxy.  It's safer than before, I think.
(But no behavioral change in this CL at all.)

Also the following renaming is done in order to emphasize that
the returned object may not be initialized.

  WindowProxyManagerBase
  => WindowProxyManager
  (Same naming as Frame, DOMWindow and WindowProxy)

  WindowProxyManager::mainWorldProxy
  => mainWorldProxyMaybeUninitialized

  WindowProxyManager::windowProxy
  => windowProxyMaybeUninitialized

By the way, we don't much use RemoteWindowProxyManager.
We can simply remove the class in a follow-up CL.

BUG=
==========

to

==========
bindings: Simplifies WindowProxyManager and its relation to Frame.

Both of LocalFrame and RemoteFrame have a WindowProxyManager.
We can move it into a base class Frame.

windowProxy->initializedIfNeeded() is moved into WindowProxyManager
from ScriptController and RemoteFrame.
WindowProxyManager::windowProxy(world) now always returns
an initialized global proxy.  It's safer than before, I think.
(But no behavioral change in this CL at all.)

Also the following renaming is done in order to emphasize that
the returned object may not be initialized.

  WindowProxyManagerBase
  => WindowProxyManager
  (Same naming as Frame, DOMWindow and WindowProxy)

  WindowProxyManager::mainWorldProxy
  => mainWorldProxyMaybeUninitialized

  WindowProxyManager::windowProxy
  => windowProxyMaybeUninitialized

By the way, we don't much use RemoteWindowProxyManager.
We can simply remove the class in a follow-up CL.

BUG=

Review-Url: https://codereview.chromium.org/2702273004
Cr-Commit-Position: refs/heads/master@{#457409}
Committed:
https://chromium.googlesource.com/chromium/src/+/4136f45071af9b51dfb36598efa9...
==========

[commit-bot: I haz the power, 2017-03-16 12:47:55]

Committed patchset #11 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/4136f45071af9b51dfb36598efa9...