Add a basic DOMWindow base class and use it in WindowProxy.

Add a basic DOMWindow base class and use it in WindowProxy.

This is the first step towards providing RemoteFrames with a
DOMWindow so things like postMessage() on a remote frame will work.

This patch also moves Frame::setDOMWindow with a LocalDOMWindow to
LocalFrame, since it's only called on a LocalFrame. The
corresponding member has also been moved to LocalFrame, since it
was never set to a non-null value for RemoteFrames anyway.

BUG=425623
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184244

[dcheng, 2014-10-21 22:26:58]

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

[Nate Chapin, 2014-10-21 22:44:31]

lgtm

[dcheng, 2014-10-21 23:22:55]

https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/Frame....
Source/core/frame/Frame.h:63: virtual LocalDOMWindow* domWindow() const = 0;
I meant to include this as a comment, but I'm not sure if making this virtual
will have a perf impact (it was non-virtual before, and inlined too, it looks
like)

[haraken, 2014-10-22 00:59:13]

haraken@chromium.org changed reviewers:
+ yukishiino@chromium.or

[haraken, 2014-10-22 00:59:14]

Just to confirm: Are you planning to make RemoteFrame inherit from DOMWindow in
the future?

https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
File Source/core/frame/DOMWindow.h (right):

https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
Source/core/frame/DOMWindow.h:9: class DOMWindow {

I'm curious why DOMWindow can't inherit from ScriptWrappableBase?

[dcheng, 2014-10-22 01:01:47]

RemoteFrame itself probably won't inherit from DOMWindow, but we'll add a
RemoteDOMWindow that will.

https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
File Source/core/frame/DOMWindow.h (right):

https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
Source/core/frame/DOMWindow.h:9: class DOMWindow {
On 2014/10/22 00:59:14, haraken wrote:
> 
> I'm curious why DOMWindow can't inherit from ScriptWrappableBase?

LocalDOMWindow is an EventTarget (and I don't think it makes sense for
RemoteDOMWindow to be an EventTarget -- I don't think you can register an event
listener on a cross-origin DOMWindow). If this was also a ScriptWrappable, then
you would have to explicitly say you wanted to use the ScriptWrappableBase from
EventTargetWithInlineData or the ScriptWrappableBase from DOMWindow.

[haraken, 2014-10-22 01:16:56]

On 2014/10/22 01:01:47, dcheng wrote:
> RemoteFrame itself probably won't inherit from DOMWindow, but we'll add a
> RemoteDOMWindow that will.

Sorry, I meant RemoteDOMWindow :)

>
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> File Source/core/frame/DOMWindow.h (right):
> 
>
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> Source/core/frame/DOMWindow.h:9: class DOMWindow {
> On 2014/10/22 00:59:14, haraken wrote:
> > 
> > I'm curious why DOMWindow can't inherit from ScriptWrappableBase?
> 
> LocalDOMWindow is an EventTarget (and I don't think it makes sense for
> RemoteDOMWindow to be an EventTarget -- I don't think you can register an
event
> listener on a cross-origin DOMWindow). If this was also a ScriptWrappable,
then
> you would have to explicitly say you wanted to use the ScriptWrappableBase
from
> EventTargetWithInlineData or the ScriptWrappableBase from DOMWindow.

Given that LocalDOMWindow is an EventTarget (which inherits from
ScriptWrappableBase), LocalDOMWindow should already be able to use
toScriptWrappableBase(). Why do we need to add toScriptWrappableBase() to
DOMWindow then?

The idea is:

- We want to make all DOM objects (i.e., objects exposed to JavaScript) inherit
from ScriptWrappable(Base).
- toScriptWrappableBase() should be defined only in ScriptWrappableBase.

[haraken, 2014-10-22 01:22:06]

On 2014/10/22 01:16:56, haraken wrote:
> On 2014/10/22 01:01:47, dcheng wrote:
> > RemoteFrame itself probably won't inherit from DOMWindow, but we'll add a
> > RemoteDOMWindow that will.
> 
> Sorry, I meant RemoteDOMWindow :)
> 
> >
>
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> > File Source/core/frame/DOMWindow.h (right):
> > 
> >
>
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> > Source/core/frame/DOMWindow.h:9: class DOMWindow {
> > On 2014/10/22 00:59:14, haraken wrote:
> > > 
> > > I'm curious why DOMWindow can't inherit from ScriptWrappableBase?
> > 
> > LocalDOMWindow is an EventTarget (and I don't think it makes sense for
> > RemoteDOMWindow to be an EventTarget -- I don't think you can register an
> event
> > listener on a cross-origin DOMWindow). If this was also a ScriptWrappable,
> then
> > you would have to explicitly say you wanted to use the ScriptWrappableBase
> from
> > EventTargetWithInlineData or the ScriptWrappableBase from DOMWindow.
> 
> Given that LocalDOMWindow is an EventTarget (which inherits from
> ScriptWrappableBase), LocalDOMWindow should already be able to use
> toScriptWrappableBase(). Why do we need to add toScriptWrappableBase() to
> DOMWindow then?
> 
> The idea is:
> 
> - We want to make all DOM objects (i.e., objects exposed to JavaScript)
inherit
> from ScriptWrappable(Base).
> - toScriptWrappableBase() should be defined only in ScriptWrappableBase.

Ah, I'm getting to understand the situation...

Conceptually we should have the following class inheritance:

class EventTarget { };  // Don't inherit from ScriptWrappable
class DOMWindow : public ScriptWrappable { };
class LocalDOMWindow : public DOMWindow, public EventTarget { };
class RemoteDOMWindow : public DOMWindow { };

The complexity comes from the fact that currently EventTarget inherits from
ScriptWrappable. This is conceptually not right but we do this in order not to
increase sizeof(Node)...

class Node : public EventTarget, public ScriptWrappable { };

^^^ This will increase sizeof(Node) because both EventTarget and ScriptWrappable
have vtables.

[dcheng, 2014-10-22 01:32:55]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2014-10-22 01:33:03]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2014-10-22 01:36:53]

On 2014/10/22 at 01:22:06, haraken wrote:
> On 2014/10/22 01:16:56, haraken wrote:
> > On 2014/10/22 01:01:47, dcheng wrote:
> > > RemoteFrame itself probably won't inherit from DOMWindow, but we'll add a
> > > RemoteDOMWindow that will.
> > 
> > Sorry, I meant RemoteDOMWindow :)
> > 
> > >
> >
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> > > File Source/core/frame/DOMWindow.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> > > Source/core/frame/DOMWindow.h:9: class DOMWindow {
> > > On 2014/10/22 00:59:14, haraken wrote:
> > > > 
> > > > I'm curious why DOMWindow can't inherit from ScriptWrappableBase?
> > > 
> > > LocalDOMWindow is an EventTarget (and I don't think it makes sense for
> > > RemoteDOMWindow to be an EventTarget -- I don't think you can register an
> > event
> > > listener on a cross-origin DOMWindow). If this was also a ScriptWrappable,
> > then
> > > you would have to explicitly say you wanted to use the ScriptWrappableBase
> > from
> > > EventTargetWithInlineData or the ScriptWrappableBase from DOMWindow.
> > 
> > Given that LocalDOMWindow is an EventTarget (which inherits from
> > ScriptWrappableBase), LocalDOMWindow should already be able to use
> > toScriptWrappableBase(). Why do we need to add toScriptWrappableBase() to
> > DOMWindow then?
> > 
> > The idea is:
> > 
> > - We want to make all DOM objects (i.e., objects exposed to JavaScript)
inherit
> > from ScriptWrappable(Base).
> > - toScriptWrappableBase() should be defined only in ScriptWrappableBase.
> 
> Ah, I'm getting to understand the situation...
> 
> Conceptually we should have the following class inheritance:
> 
> class EventTarget { };  // Don't inherit from ScriptWrappable
> class DOMWindow : public ScriptWrappable { };
> class LocalDOMWindow : public DOMWindow, public EventTarget { };
> class RemoteDOMWindow : public DOMWindow { };
> 
> The complexity comes from the fact that currently EventTarget inherits from
ScriptWrappable. This is conceptually not right but we do this in order not to
increase sizeof(Node)...
> 
> class Node : public EventTarget, public ScriptWrappable { };
> 
> ^^^ This will increase sizeof(Node) because both EventTarget and
ScriptWrappable have vtables.

Yeah, this is the reason I had to take this approach. Otherwise, we end up with
the following situation:

class DOMWindow : public ScriptWrappable { };
class EventTarget : public ScriptWrappable { };
class LocalDOMWindow : public DOMWindow, public EventTarget { };

Now if you have:
LocalDOMWindow* w;
then this will fail to compile:
w->wrapperTypeInfo();
because the compiler doesn't know which base class to go through to get to
ScriptWrappable. Instead, I would have to write:
static_cast<DOMWindow*>(w)->wrapperTypeInfo();

[haraken, 2014-10-22 01:42:25]

On 2014/10/22 01:36:53, dcheng wrote:
> On 2014/10/22 at 01:22:06, haraken wrote:
> > On 2014/10/22 01:16:56, haraken wrote:
> > > On 2014/10/22 01:01:47, dcheng wrote:
> > > > RemoteFrame itself probably won't inherit from DOMWindow, but we'll add
a
> > > > RemoteDOMWindow that will.
> > > 
> > > Sorry, I meant RemoteDOMWindow :)
> > > 
> > > >
> > >
>
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> > > > File Source/core/frame/DOMWindow.h (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> > > > Source/core/frame/DOMWindow.h:9: class DOMWindow {
> > > > On 2014/10/22 00:59:14, haraken wrote:
> > > > > 
> > > > > I'm curious why DOMWindow can't inherit from ScriptWrappableBase?
> > > > 
> > > > LocalDOMWindow is an EventTarget (and I don't think it makes sense for
> > > > RemoteDOMWindow to be an EventTarget -- I don't think you can register
an
> > > event
> > > > listener on a cross-origin DOMWindow). If this was also a
ScriptWrappable,
> > > then
> > > > you would have to explicitly say you wanted to use the
ScriptWrappableBase
> > > from
> > > > EventTargetWithInlineData or the ScriptWrappableBase from DOMWindow.
> > > 
> > > Given that LocalDOMWindow is an EventTarget (which inherits from
> > > ScriptWrappableBase), LocalDOMWindow should already be able to use
> > > toScriptWrappableBase(). Why do we need to add toScriptWrappableBase() to
> > > DOMWindow then?
> > > 
> > > The idea is:
> > > 
> > > - We want to make all DOM objects (i.e., objects exposed to JavaScript)
> inherit
> > > from ScriptWrappable(Base).
> > > - toScriptWrappableBase() should be defined only in ScriptWrappableBase.
> > 
> > Ah, I'm getting to understand the situation...
> > 
> > Conceptually we should have the following class inheritance:
> > 
> > class EventTarget { };  // Don't inherit from ScriptWrappable
> > class DOMWindow : public ScriptWrappable { };
> > class LocalDOMWindow : public DOMWindow, public EventTarget { };
> > class RemoteDOMWindow : public DOMWindow { };
> > 
> > The complexity comes from the fact that currently EventTarget inherits from
> ScriptWrappable. This is conceptually not right but we do this in order not to
> increase sizeof(Node)...
> > 
> > class Node : public EventTarget, public ScriptWrappable { };
> > 
> > ^^^ This will increase sizeof(Node) because both EventTarget and
> ScriptWrappable have vtables.
> 
> Yeah, this is the reason I had to take this approach. Otherwise, we end up
with
> the following situation:
> 
> class DOMWindow : public ScriptWrappable { };
> class EventTarget : public ScriptWrappable { };
> class LocalDOMWindow : public DOMWindow, public EventTarget { };
> 
> Now if you have:
> LocalDOMWindow* w;
> then this will fail to compile:
> w->wrapperTypeInfo();
> because the compiler doesn't know which base class to go through to get to
> ScriptWrappable. Instead, I would have to write:
> static_cast<DOMWindow*>(w)->wrapperTypeInfo();

Thanks, I understand what you're doing :)

shiino-san: Do you have any idea on this?

[Yuki, 2014-10-22 04:23:38]

On 2014/10/22 01:42:25, haraken wrote:
> On 2014/10/22 01:36:53, dcheng wrote:
> > On 2014/10/22 at 01:22:06, haraken wrote:
> > > On 2014/10/22 01:16:56, haraken wrote:
> > > > On 2014/10/22 01:01:47, dcheng wrote:
> > > > > RemoteFrame itself probably won't inherit from DOMWindow, but we'll
add
> a
> > > > > RemoteDOMWindow that will.
> > > > 
> > > > Sorry, I meant RemoteDOMWindow :)
> > > > 
> > > > >
> > > >
> >
>
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> > > > > File Source/core/frame/DOMWindow.h (right):
> > > > > 
> > > > >
> > > >
> >
>
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> > > > > Source/core/frame/DOMWindow.h:9: class DOMWindow {
> > > > > On 2014/10/22 00:59:14, haraken wrote:
> > > > > > 
> > > > > > I'm curious why DOMWindow can't inherit from ScriptWrappableBase?
> > > > > 
> > > > > LocalDOMWindow is an EventTarget (and I don't think it makes sense for
> > > > > RemoteDOMWindow to be an EventTarget -- I don't think you can register
> an
> > > > event
> > > > > listener on a cross-origin DOMWindow). If this was also a
> ScriptWrappable,
> > > > then
> > > > > you would have to explicitly say you wanted to use the
> ScriptWrappableBase
> > > > from
> > > > > EventTargetWithInlineData or the ScriptWrappableBase from DOMWindow.
> > > > 
> > > > Given that LocalDOMWindow is an EventTarget (which inherits from
> > > > ScriptWrappableBase), LocalDOMWindow should already be able to use
> > > > toScriptWrappableBase(). Why do we need to add toScriptWrappableBase()
to
> > > > DOMWindow then?
> > > > 
> > > > The idea is:
> > > > 
> > > > - We want to make all DOM objects (i.e., objects exposed to JavaScript)
> > inherit
> > > > from ScriptWrappable(Base).
> > > > - toScriptWrappableBase() should be defined only in ScriptWrappableBase.
> > > 
> > > Ah, I'm getting to understand the situation...
> > > 
> > > Conceptually we should have the following class inheritance:
> > > 
> > > class EventTarget { };  // Don't inherit from ScriptWrappable
> > > class DOMWindow : public ScriptWrappable { };
> > > class LocalDOMWindow : public DOMWindow, public EventTarget { };
> > > class RemoteDOMWindow : public DOMWindow { };
> > > 
> > > The complexity comes from the fact that currently EventTarget inherits
from
> > ScriptWrappable. This is conceptually not right but we do this in order not
to
> > increase sizeof(Node)...
> > > 
> > > class Node : public EventTarget, public ScriptWrappable { };
> > > 
> > > ^^^ This will increase sizeof(Node) because both EventTarget and
> > ScriptWrappable have vtables.
> > 
> > Yeah, this is the reason I had to take this approach. Otherwise, we end up
> with
> > the following situation:
> > 
> > class DOMWindow : public ScriptWrappable { };
> > class EventTarget : public ScriptWrappable { };
> > class LocalDOMWindow : public DOMWindow, public EventTarget { };
> > 
> > Now if you have:
> > LocalDOMWindow* w;
> > then this will fail to compile:
> > w->wrapperTypeInfo();
> > because the compiler doesn't know which base class to go through to get to
> > ScriptWrappable. Instead, I would have to write:
> > static_cast<DOMWindow*>(w)->wrapperTypeInfo();
> 
> Thanks, I understand what you're doing :)
> 
> shiino-san: Do you have any idea on this?

I understand that it's not ideal that EventTarget inherits from ScriptWrappable,
and that RemoteDOMWindow conceptually doesn't need to inherit from EventTarget,
however, we have a serious memory size issue about Node and we need this trick
to reduce the size of Node.

With these constraints, I think the simplest way is that DOMWindow inherits from
EventTarget even though it's not necessary.  Anyway we need tricky code here or
there, then, this is the simplest trick, I think.  We can write an excuse at
DOMWindow definition.

[dcheng, 2014-10-22 06:14:16]

On 2014/10/22 at 04:23:38, yukishiino wrote:
> On 2014/10/22 01:42:25, haraken wrote:
> > On 2014/10/22 01:36:53, dcheng wrote:
> > > On 2014/10/22 at 01:22:06, haraken wrote:
> > > > On 2014/10/22 01:16:56, haraken wrote:
> > > > > On 2014/10/22 01:01:47, dcheng wrote:
> > > > > > RemoteFrame itself probably won't inherit from DOMWindow, but we'll
add
> > a
> > > > > > RemoteDOMWindow that will.
> > > > > 
> > > > > Sorry, I meant RemoteDOMWindow :)
> > > > > 
> > > > > >
> > > > >
> > >
> >
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> > > > > > File Source/core/frame/DOMWindow.h (right):
> > > > > > 
> > > > > >
> > > > >
> > >
> >
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> > > > > > Source/core/frame/DOMWindow.h:9: class DOMWindow {
> > > > > > On 2014/10/22 00:59:14, haraken wrote:
> > > > > > > 
> > > > > > > I'm curious why DOMWindow can't inherit from ScriptWrappableBase?
> > > > > > 
> > > > > > LocalDOMWindow is an EventTarget (and I don't think it makes sense
for
> > > > > > RemoteDOMWindow to be an EventTarget -- I don't think you can
register
> > an
> > > > > event
> > > > > > listener on a cross-origin DOMWindow). If this was also a
> > ScriptWrappable,
> > > > > then
> > > > > > you would have to explicitly say you wanted to use the
> > ScriptWrappableBase
> > > > > from
> > > > > > EventTargetWithInlineData or the ScriptWrappableBase from DOMWindow.
> > > > > 
> > > > > Given that LocalDOMWindow is an EventTarget (which inherits from
> > > > > ScriptWrappableBase), LocalDOMWindow should already be able to use
> > > > > toScriptWrappableBase(). Why do we need to add toScriptWrappableBase()
to
> > > > > DOMWindow then?
> > > > > 
> > > > > The idea is:
> > > > > 
> > > > > - We want to make all DOM objects (i.e., objects exposed to
JavaScript)
> > > inherit
> > > > > from ScriptWrappable(Base).
> > > > > - toScriptWrappableBase() should be defined only in
ScriptWrappableBase.
> > > > 
> > > > Ah, I'm getting to understand the situation...
> > > > 
> > > > Conceptually we should have the following class inheritance:
> > > > 
> > > > class EventTarget { };  // Don't inherit from ScriptWrappable
> > > > class DOMWindow : public ScriptWrappable { };
> > > > class LocalDOMWindow : public DOMWindow, public EventTarget { };
> > > > class RemoteDOMWindow : public DOMWindow { };
> > > > 
> > > > The complexity comes from the fact that currently EventTarget inherits
from
> > > ScriptWrappable. This is conceptually not right but we do this in order
not to
> > > increase sizeof(Node)...
> > > > 
> > > > class Node : public EventTarget, public ScriptWrappable { };
> > > > 
> > > > ^^^ This will increase sizeof(Node) because both EventTarget and
> > > ScriptWrappable have vtables.
> > > 
> > > Yeah, this is the reason I had to take this approach. Otherwise, we end up
> > with
> > > the following situation:
> > > 
> > > class DOMWindow : public ScriptWrappable { };
> > > class EventTarget : public ScriptWrappable { };
> > > class LocalDOMWindow : public DOMWindow, public EventTarget { };
> > > 
> > > Now if you have:
> > > LocalDOMWindow* w;
> > > then this will fail to compile:
> > > w->wrapperTypeInfo();
> > > because the compiler doesn't know which base class to go through to get to
> > > ScriptWrappable. Instead, I would have to write:
> > > static_cast<DOMWindow*>(w)->wrapperTypeInfo();
> > 
> > Thanks, I understand what you're doing :)
> > 
> > shiino-san: Do you have any idea on this?
> 
> I understand that it's not ideal that EventTarget inherits from
ScriptWrappable, and that RemoteDOMWindow conceptually doesn't need to inherit
from EventTarget, however, we have a serious memory size issue about Node and we
need this trick to reduce the size of Node.
> 

I understand why this is important for Node, but I'm not sure why this would
matter for DOMWindow?

> With these constraints, I think the simplest way is that DOMWindow inherits
from EventTarget even though it's not necessary.  Anyway we need tricky code
here or there, then, this is the simplest trick, I think.  We can write an
excuse at DOMWindow definition.

EventTarget brings a lot of extra baggage that doesn't make sense in
RemoteDOMWindow. If I do this, RemoteDOMWindow:
- needs to trace into EventTarget.
- needs to override executionContext(), even though it never has one.
- exposes a bunch of methods that should not be called (without inheriting from
EventTarget, it would be a compile-time error instead, which is much better))

[Yuki, 2014-10-23 04:23:22]

yukishiino@chromium.org changed reviewers:
+ yukishiino@chromium.org
- yukishiino@chromium.or

[Yuki, 2014-10-23 04:45:18]

On 2014/10/22 06:14:16, dcheng wrote:
> On 2014/10/22 at 04:23:38, yukishiino wrote:
> > On 2014/10/22 01:42:25, haraken wrote:
> > > On 2014/10/22 01:36:53, dcheng wrote:
> > > > On 2014/10/22 at 01:22:06, haraken wrote:
> > > > > On 2014/10/22 01:16:56, haraken wrote:
> > > > > > On 2014/10/22 01:01:47, dcheng wrote:
> > > > > > > RemoteFrame itself probably won't inherit from DOMWindow, but
we'll
> add
> > > a
> > > > > > > RemoteDOMWindow that will.
> > > > > > 
> > > > > > Sorry, I meant RemoteDOMWindow :)
> > > > > > 
> > > > > > >
> > > > > >
> > > >
> > >
>
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> > > > > > > File Source/core/frame/DOMWindow.h (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > >
> > >
>
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> > > > > > > Source/core/frame/DOMWindow.h:9: class DOMWindow {
> > > > > > > On 2014/10/22 00:59:14, haraken wrote:
> > > > > > > > 
> > > > > > > > I'm curious why DOMWindow can't inherit from
ScriptWrappableBase?
> > > > > > > 
> > > > > > > LocalDOMWindow is an EventTarget (and I don't think it makes sense
> for
> > > > > > > RemoteDOMWindow to be an EventTarget -- I don't think you can
> register
> > > an
> > > > > > event
> > > > > > > listener on a cross-origin DOMWindow). If this was also a
> > > ScriptWrappable,
> > > > > > then
> > > > > > > you would have to explicitly say you wanted to use the
> > > ScriptWrappableBase
> > > > > > from
> > > > > > > EventTargetWithInlineData or the ScriptWrappableBase from
DOMWindow.
> > > > > > 
> > > > > > Given that LocalDOMWindow is an EventTarget (which inherits from
> > > > > > ScriptWrappableBase), LocalDOMWindow should already be able to use
> > > > > > toScriptWrappableBase(). Why do we need to add
toScriptWrappableBase()
> to
> > > > > > DOMWindow then?
> > > > > > 
> > > > > > The idea is:
> > > > > > 
> > > > > > - We want to make all DOM objects (i.e., objects exposed to
> JavaScript)
> > > > inherit
> > > > > > from ScriptWrappable(Base).
> > > > > > - toScriptWrappableBase() should be defined only in
> ScriptWrappableBase.
> > > > > 
> > > > > Ah, I'm getting to understand the situation...
> > > > > 
> > > > > Conceptually we should have the following class inheritance:
> > > > > 
> > > > > class EventTarget { };  // Don't inherit from ScriptWrappable
> > > > > class DOMWindow : public ScriptWrappable { };
> > > > > class LocalDOMWindow : public DOMWindow, public EventTarget { };
> > > > > class RemoteDOMWindow : public DOMWindow { };
> > > > > 
> > > > > The complexity comes from the fact that currently EventTarget inherits
> from
> > > > ScriptWrappable. This is conceptually not right but we do this in order
> not to
> > > > increase sizeof(Node)...
> > > > > 
> > > > > class Node : public EventTarget, public ScriptWrappable { };
> > > > > 
> > > > > ^^^ This will increase sizeof(Node) because both EventTarget and
> > > > ScriptWrappable have vtables.
> > > > 
> > > > Yeah, this is the reason I had to take this approach. Otherwise, we end
up
> > > with
> > > > the following situation:
> > > > 
> > > > class DOMWindow : public ScriptWrappable { };
> > > > class EventTarget : public ScriptWrappable { };
> > > > class LocalDOMWindow : public DOMWindow, public EventTarget { };
> > > > 
> > > > Now if you have:
> > > > LocalDOMWindow* w;
> > > > then this will fail to compile:
> > > > w->wrapperTypeInfo();
> > > > because the compiler doesn't know which base class to go through to get
to
> > > > ScriptWrappable. Instead, I would have to write:
> > > > static_cast<DOMWindow*>(w)->wrapperTypeInfo();
> > > 
> > > Thanks, I understand what you're doing :)
> > > 
> > > shiino-san: Do you have any idea on this?
> > 
> > I understand that it's not ideal that EventTarget inherits from
> ScriptWrappable, and that RemoteDOMWindow conceptually doesn't need to inherit
> from EventTarget, however, we have a serious memory size issue about Node and
we
> need this trick to reduce the size of Node.
> > 
> 
> I understand why this is important for Node, but I'm not sure why this would
> matter for DOMWindow?
> 
> > With these constraints, I think the simplest way is that DOMWindow inherits
> from EventTarget even though it's not necessary.  Anyway we need tricky code
> here or there, then, this is the simplest trick, I think.  We can write an
> excuse at DOMWindow definition.
> 
> EventTarget brings a lot of extra baggage that doesn't make sense in
> RemoteDOMWindow. If I do this, RemoteDOMWindow:
> - needs to trace into EventTarget.
> - needs to override executionContext(), even though it never has one.
> - exposes a bunch of methods that should not be called (without inheriting
from
> EventTarget, it would be a compile-time error instead, which is much better))

As long as DOMWindow itself does not need to be ScriptWrappable, your approach
makes sense.  lgtm with a nit.

[haraken, 2014-10-23 04:46:01]

On 2014/10/23 04:45:18, Yuki wrote:
> On 2014/10/22 06:14:16, dcheng wrote:
> > On 2014/10/22 at 04:23:38, yukishiino wrote:
> > > On 2014/10/22 01:42:25, haraken wrote:
> > > > On 2014/10/22 01:36:53, dcheng wrote:
> > > > > On 2014/10/22 at 01:22:06, haraken wrote:
> > > > > > On 2014/10/22 01:16:56, haraken wrote:
> > > > > > > On 2014/10/22 01:01:47, dcheng wrote:
> > > > > > > > RemoteFrame itself probably won't inherit from DOMWindow, but
> we'll
> > add
> > > > a
> > > > > > > > RemoteDOMWindow that will.
> > > > > > > 
> > > > > > > Sorry, I meant RemoteDOMWindow :)
> > > > > > > 
> > > > > > > >
> > > > > > >
> > > > >
> > > >
> >
>
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> > > > > > > > File Source/core/frame/DOMWindow.h (right):
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > >
> > > >
> >
>
https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/DOMWin...
> > > > > > > > Source/core/frame/DOMWindow.h:9: class DOMWindow {
> > > > > > > > On 2014/10/22 00:59:14, haraken wrote:
> > > > > > > > > 
> > > > > > > > > I'm curious why DOMWindow can't inherit from
> ScriptWrappableBase?
> > > > > > > > 
> > > > > > > > LocalDOMWindow is an EventTarget (and I don't think it makes
sense
> > for
> > > > > > > > RemoteDOMWindow to be an EventTarget -- I don't think you can
> > register
> > > > an
> > > > > > > event
> > > > > > > > listener on a cross-origin DOMWindow). If this was also a
> > > > ScriptWrappable,
> > > > > > > then
> > > > > > > > you would have to explicitly say you wanted to use the
> > > > ScriptWrappableBase
> > > > > > > from
> > > > > > > > EventTargetWithInlineData or the ScriptWrappableBase from
> DOMWindow.
> > > > > > > 
> > > > > > > Given that LocalDOMWindow is an EventTarget (which inherits from
> > > > > > > ScriptWrappableBase), LocalDOMWindow should already be able to use
> > > > > > > toScriptWrappableBase(). Why do we need to add
> toScriptWrappableBase()
> > to
> > > > > > > DOMWindow then?
> > > > > > > 
> > > > > > > The idea is:
> > > > > > > 
> > > > > > > - We want to make all DOM objects (i.e., objects exposed to
> > JavaScript)
> > > > > inherit
> > > > > > > from ScriptWrappable(Base).
> > > > > > > - toScriptWrappableBase() should be defined only in
> > ScriptWrappableBase.
> > > > > > 
> > > > > > Ah, I'm getting to understand the situation...
> > > > > > 
> > > > > > Conceptually we should have the following class inheritance:
> > > > > > 
> > > > > > class EventTarget { };  // Don't inherit from ScriptWrappable
> > > > > > class DOMWindow : public ScriptWrappable { };
> > > > > > class LocalDOMWindow : public DOMWindow, public EventTarget { };
> > > > > > class RemoteDOMWindow : public DOMWindow { };
> > > > > > 
> > > > > > The complexity comes from the fact that currently EventTarget
inherits
> > from
> > > > > ScriptWrappable. This is conceptually not right but we do this in
order
> > not to
> > > > > increase sizeof(Node)...
> > > > > > 
> > > > > > class Node : public EventTarget, public ScriptWrappable { };
> > > > > > 
> > > > > > ^^^ This will increase sizeof(Node) because both EventTarget and
> > > > > ScriptWrappable have vtables.
> > > > > 
> > > > > Yeah, this is the reason I had to take this approach. Otherwise, we
end
> up
> > > > with
> > > > > the following situation:
> > > > > 
> > > > > class DOMWindow : public ScriptWrappable { };
> > > > > class EventTarget : public ScriptWrappable { };
> > > > > class LocalDOMWindow : public DOMWindow, public EventTarget { };
> > > > > 
> > > > > Now if you have:
> > > > > LocalDOMWindow* w;
> > > > > then this will fail to compile:
> > > > > w->wrapperTypeInfo();
> > > > > because the compiler doesn't know which base class to go through to
get
> to
> > > > > ScriptWrappable. Instead, I would have to write:
> > > > > static_cast<DOMWindow*>(w)->wrapperTypeInfo();
> > > > 
> > > > Thanks, I understand what you're doing :)
> > > > 
> > > > shiino-san: Do you have any idea on this?
> > > 
> > > I understand that it's not ideal that EventTarget inherits from
> > ScriptWrappable, and that RemoteDOMWindow conceptually doesn't need to
inherit
> > from EventTarget, however, we have a serious memory size issue about Node
and
> we
> > need this trick to reduce the size of Node.
> > > 
> > 
> > I understand why this is important for Node, but I'm not sure why this would
> > matter for DOMWindow?
> > 
> > > With these constraints, I think the simplest way is that DOMWindow
inherits
> > from EventTarget even though it's not necessary.  Anyway we need tricky code
> > here or there, then, this is the simplest trick, I think.  We can write an
> > excuse at DOMWindow definition.
> > 
> > EventTarget brings a lot of extra baggage that doesn't make sense in
> > RemoteDOMWindow. If I do this, RemoteDOMWindow:
> > - needs to trace into EventTarget.
> > - needs to override executionContext(), even though it never has one.
> > - exposes a bunch of methods that should not be called (without inheriting
> from
> > EventTarget, it would be a compile-time error instead, which is much
better))
> 
> As long as DOMWindow itself does not need to be ScriptWrappable, your approach
> makes sense.  lgtm with a nit.

LGTM

[Yuki, 2014-10-23 06:03:34]

Sorry, I forgot to send these comments.

https://codereview.chromium.org/640803004/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/640803004/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/WindowProxy.cpp:294: DOMWindow* window =
m_frame->domWindow();
ScriptWrappable* scriptWrappable = m_frame->domWindow()->toScriptWrappable();

would make more sense and more effective?
I'm seeing too many window->toScriptWrappable().

https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/Frame....
Source/core/frame/Frame.h:63: virtual LocalDOMWindow* domWindow() const = 0;
IIUC, no one in this CL expects domWindow() returns LocalDOMWindow.  Who expects
LocalDOMWindow?  (I'm just curious.)

[haraken, 2014-10-23 06:08:38]

Also let's add a good comment about the situation on DOMWindow.h.

[dcheng, 2014-10-23 06:48:45]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2014-10-23 06:48:46]

I've also added a comment to DOMWindow::toScriptWrappable to describe why we
need a silly looking conversion method like this.

https://codereview.chromium.org/640803004/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/640803004/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/WindowProxy.cpp:294: DOMWindow* window =
m_frame->domWindow();
On 2014/10/23 06:03:34, Yuki wrote:
> ScriptWrappable* scriptWrappable = m_frame->domWindow()->toScriptWrappable();
> 
> would make more sense and more effective?
> I'm seeing too many window->toScriptWrappable().

Yeah, makes sense to alias it. Done.

https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/640803004/diff/40001/Source/core/frame/Frame....
Source/core/frame/Frame.h:63: virtual LocalDOMWindow* domWindow() const = 0;
On 2014/10/23 06:03:34, Yuki wrote:
> IIUC, no one in this CL expects domWindow() returns LocalDOMWindow.  Who
expects
> LocalDOMWindow?  (I'm just curious.)

A lot of places depend on this to return LocalDOMWindow.
LocalDOMWindow::parent(), top(), opener(), parts of WindowProxy, but also places
in Source/core that do something like
frame->domWindow()->someLocalDOMWindowMethod(). I'm actually working on changing
this in some followup CLs.

[commit-bot: I haz the power, 2014-10-23 06:49:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/640803004/60001

[commit-bot: I haz the power, 2014-10-23 08:40:29]

Committed patchset #4 (id:60001) as 184244