Devirtualize Frame::domWindow().

Devirtualize Frame::domWindow().

Originally, this was a virtual, but it meant that LocalFrame::domWindow
could not return a LocalDOMWindow, due to the circular dependencies
between the LocalFrame and LocalDOMWindow header. As a workaround,
LocalFrame exposed a special localDOMWindow getter. However, this leads
to code that reads very LocalFrame-centric. Since the ultimate goal of
OOPIF integration is to integrate seamlessly, it would be nice to
remove the "local" prefix from the getter.

Since this function doesn't need to be virtual, just de-virtualize it
and have the getters of derived classes return the appropriate type.

BUG=none

Committed: https://crrev.com/fd3cea4f35b5b831cef689f0720f13d8f22ba103
Cr-Commit-Position: refs/heads/master@{#437887}

[dcheng, 2016-12-12 08:14:18]

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

[commit-bot: I haz the power, 2016-12-12 08:14:30]

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

[dcheng, 2016-12-12 08:16:11]

Description was changed from

==========
Devirtualize Frame::domWindow().

Originally, this was a virtual, but it meant that LocalFrame::domWindow
could not return a LocalDOMWindow, due to the circular dependencies
between the LocalFrame and LocalDOMWindow header.

Since this function doesn't need to be virtual, just de-virtualize it
and have the getters of derived classes return the appropriate type.

BUG=none
==========

to

==========
Devirtualize Frame::domWindow().

Originally, this was a virtual, but it meant that LocalFrame::domWindow
could not return a LocalDOMWindow, due to the circular dependencies
between the LocalFrame and LocalDOMWindow header. As a workaround,
LocalFrame exposed a special localDOMWindow getter. However, this leads
to code that reads very LocalFrame-centric. Since the ultimate goal of
OOPIF integration is to integrate seamlessly, it would be nice to
remove the "local" prefix from the getter.

Since this function doesn't need to be virtual, just de-virtualize it
and have the getters of derived classes return the appropriate type.

BUG=none
==========

[dcheng, 2016-12-12 08:16:11]

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

[dcheng, 2016-12-12 08:18:31]

Note: I also want to make similar de-vritualization transformations for
DOMWindow::frame() in followups as well as fix some of the random issues around
null-pointer checking (most code should never need to handle the case where
there is a LocalFrame but no LocalDOMWindow).

[haraken, 2016-12-12 09:54:59]

https://codereview.chromium.org/2562323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2562323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:413: // pointer?

I want to guarantee the following fact:

- When a context is detached, frame()->domWindow(), domWindow()->document(),
document()->frame() and frame()->document() start returning nullptr.

To that goal, I tried to clear domWindow()->document() when the context gets
detached (https://codereview.chromium.org/2393133005) but found that it needs to
fix a lot of call sites that are assuming that domWindow()->document() doesn't
return null after the detachment...

https://codereview.chromium.org/2562323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2562323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:524: m_domWindow =
domWindow;

LocalFrame::m_domWindow is reset here. Where is RemoteFrame::m_domWindow reset?
Don't we need to reset in RemoteFrame::setView()?

[commit-bot: I haz the power, 2016-12-12 10:09:52]

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

[commit-bot: I haz the power, 2016-12-12 10:09:53]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-12-12 10:21:39]

https://codereview.chromium.org/2562323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2562323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:413: // pointer?
On 2016/12/12 09:54:59, haraken wrote:
> 
> I want to guarantee the following fact:
> 
> - When a context is detached, frame()->domWindow(), domWindow()->document(),
> document()->frame() and frame()->document() start returning nullptr.
> 
> To that goal, I tried to clear domWindow()->document() when the context gets
> detached (https://codereview.chromium.org/2393133005) but found that it needs
to
> fix a lot of call sites that are assuming that domWindow()->document() doesn't
> return null after the detachment...
> 

Yes, that's basically not possible today: because JS can access properties of a
detached window, we can't clear any of the pointers when a frame is detached.

If we ever want to change this invariant, we'll have to revisit this, but until
then, I think it would be better if code doesn't null-check unnecessarily,
because it makes the invariants harder to understand.

https://codereview.chromium.org/2562323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2562323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:524: m_domWindow =
domWindow;
On 2016/12/12 09:54:59, haraken wrote:
> 
> LocalFrame::m_domWindow is reset here. Where is RemoteFrame::m_domWindow
reset?
> Don't we need to reset in RemoteFrame::setView()?

LocalDOMWindow is swapped on navigation, but RemoteDOMWindow is not: none of the
properties exposed by RemoteDOMWindow are affected by navigation.  We might want
to change this to address the inconsistency at some point, but at the time, it
didn't seem to add much value.

[dcheng, 2016-12-12 10:32:23]

https://codereview.chromium.org/2562323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2562323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:413: // pointer?
On 2016/12/12 10:21:39, dcheng wrote:
> On 2016/12/12 09:54:59, haraken wrote:
> > 
> > I want to guarantee the following fact:
> > 
> > - When a context is detached, frame()->domWindow(), domWindow()->document(),
> > document()->frame() and frame()->document() start returning nullptr.
> > 
> > To that goal, I tried to clear domWindow()->document() when the context gets
> > detached (https://codereview.chromium.org/2393133005) but found that it
needs
> to
> > fix a lot of call sites that are assuming that domWindow()->document()
doesn't
> > return null after the detachment...
> > 
> 
> Yes, that's basically not possible today: because JS can access properties of
a
> detached window, we can't clear any of the pointers when a frame is detached.
> 
> If we ever want to change this invariant, we'll have to revisit this, but
until
> then, I think it would be better if code doesn't null-check unnecessarily,
> because it makes the invariants harder to understand.

ALso, in a followup CL, I want to explore removing m_frame or m_domWindow, so
it's easier to keep these two in sync. That would help resolve the TODO below as
well. Probaby I would want to remove m_frame, since we clear
m_domWindow->frame() on detach already.

[haraken, 2016-12-12 11:12:23]

On 2016/12/12 10:32:23, dcheng wrote:
>
https://codereview.chromium.org/2562323002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2562323002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Document.cpp:413: // pointer?
> On 2016/12/12 10:21:39, dcheng wrote:
> > On 2016/12/12 09:54:59, haraken wrote:
> > > 
> > > I want to guarantee the following fact:
> > > 
> > > - When a context is detached, frame()->domWindow(),
domWindow()->document(),
> > > document()->frame() and frame()->document() start returning nullptr.
> > > 
> > > To that goal, I tried to clear domWindow()->document() when the context
gets
> > > detached (https://codereview.chromium.org/2393133005) but found that it
> needs
> > to
> > > fix a lot of call sites that are assuming that domWindow()->document()
> doesn't
> > > return null after the detachment...
> > > 
> > 
> > Yes, that's basically not possible today: because JS can access properties
of
> a
> > detached window, we can't clear any of the pointers when a frame is
detached.
> > 
> > If we ever want to change this invariant, we'll have to revisit this, but
> until
> > then, I think it would be better if code doesn't null-check unnecessarily,
> > because it makes the invariants harder to understand.
> 
> ALso, in a followup CL, I want to explore removing m_frame or m_domWindow, so
> it's easier to keep these two in sync. That would help resolve the TODO below
as
> well. Probaby I would want to remove m_frame, since we clear
> m_domWindow->frame() on detach already.

Sounds great to me. LGTM.

[dcheng, 2016-12-12 16:01:44]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-12-12 16:02:01]

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

[commit-bot: I haz the power, 2016-12-12 16:33:52]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1481558504476710, "parent_rev":
"295e1d5bee0cd5285825c03d941ead7bd04405e7", "commit_rev":
"bf103ff84b2db210671a797b98e9503de4fdf743"}

[commit-bot: I haz the power, 2016-12-12 16:34:22]

Description was changed from

==========
Devirtualize Frame::domWindow().

Originally, this was a virtual, but it meant that LocalFrame::domWindow
could not return a LocalDOMWindow, due to the circular dependencies
between the LocalFrame and LocalDOMWindow header. As a workaround,
LocalFrame exposed a special localDOMWindow getter. However, this leads
to code that reads very LocalFrame-centric. Since the ultimate goal of
OOPIF integration is to integrate seamlessly, it would be nice to
remove the "local" prefix from the getter.

Since this function doesn't need to be virtual, just de-virtualize it
and have the getters of derived classes return the appropriate type.

BUG=none
==========

to

==========
Devirtualize Frame::domWindow().

Originally, this was a virtual, but it meant that LocalFrame::domWindow
could not return a LocalDOMWindow, due to the circular dependencies
between the LocalFrame and LocalDOMWindow header. As a workaround,
LocalFrame exposed a special localDOMWindow getter. However, this leads
to code that reads very LocalFrame-centric. Since the ultimate goal of
OOPIF integration is to integrate seamlessly, it would be nice to
remove the "local" prefix from the getter.

Since this function doesn't need to be virtual, just de-virtualize it
and have the getters of derived classes return the appropriate type.

BUG=none

Review-Url: https://codereview.chromium.org/2562323002
==========

[commit-bot: I haz the power, 2016-12-12 16:34:23]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-12 16:37:36]

Description was changed from

==========
Devirtualize Frame::domWindow().

Originally, this was a virtual, but it meant that LocalFrame::domWindow
could not return a LocalDOMWindow, due to the circular dependencies
between the LocalFrame and LocalDOMWindow header. As a workaround,
LocalFrame exposed a special localDOMWindow getter. However, this leads
to code that reads very LocalFrame-centric. Since the ultimate goal of
OOPIF integration is to integrate seamlessly, it would be nice to
remove the "local" prefix from the getter.

Since this function doesn't need to be virtual, just de-virtualize it
and have the getters of derived classes return the appropriate type.

BUG=none

Review-Url: https://codereview.chromium.org/2562323002
==========

to

==========
Devirtualize Frame::domWindow().

Originally, this was a virtual, but it meant that LocalFrame::domWindow
could not return a LocalDOMWindow, due to the circular dependencies
between the LocalFrame and LocalDOMWindow header. As a workaround,
LocalFrame exposed a special localDOMWindow getter. However, this leads
to code that reads very LocalFrame-centric. Since the ultimate goal of
OOPIF integration is to integrate seamlessly, it would be nice to
remove the "local" prefix from the getter.

Since this function doesn't need to be virtual, just de-virtualize it
and have the getters of derived classes return the appropriate type.

BUG=none

Committed: https://crrev.com/fd3cea4f35b5b831cef689f0720f13d8f22ba103
Cr-Commit-Position: refs/heads/master@{#437887}
==========

[commit-bot: I haz the power, 2016-12-12 16:37:37]

Patchset 1 (id:??) landed as
https://crrev.com/fd3cea4f35b5b831cef689f0720f13d8f22ba103
Cr-Commit-Position: refs/heads/master@{#437887}

[haraken, 2016-12-13 07:22:23]

https://codereview.chromium.org/2562323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2562323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:413: // pointer?
On 2016/12/12 10:32:23, dcheng wrote:
> On 2016/12/12 10:21:39, dcheng wrote:
> > On 2016/12/12 09:54:59, haraken wrote:
> > > 
> > > I want to guarantee the following fact:
> > > 
> > > - When a context is detached, frame()->domWindow(),
domWindow()->document(),
> > > document()->frame() and frame()->document() start returning nullptr.
> > > 
> > > To that goal, I tried to clear domWindow()->document() when the context
gets
> > > detached (https://codereview.chromium.org/2393133005) but found that it
> needs
> > to
> > > fix a lot of call sites that are assuming that domWindow()->document()
> doesn't
> > > return null after the detachment...
> > > 
> > 
> > Yes, that's basically not possible today: because JS can access properties
of
> a
> > detached window, we can't clear any of the pointers when a frame is
detached.
> > 
> > If we ever want to change this invariant, we'll have to revisit this, but
> until
> > then, I think it would be better if code doesn't null-check unnecessarily,
> > because it makes the invariants harder to understand.
> 
> ALso, in a followup CL, I want to explore removing m_frame or m_domWindow, so
> it's easier to keep these two in sync. That would help resolve the TODO below
as
> well. Probaby I would want to remove m_frame, since we clear
> m_domWindow->frame() on detach already.

You're right that e.g., frame()->domWindow() should not be cleared when the
context is detached. However, document()->frame() is cleared when the context is
detached.

I think we shouldn't break the connection between Frame and Window on the
context detachment. However, I think we should break the connection between
Document and Frame and the connection between Document and Window.

That's confusing anyway :)