Convert page-level classes to handle RemoteFrames from FrameTree

Convert page-level classes to handle RemoteFrames from FrameTree

Several classes iterate through the FrameTree for some page-related
behavior. They now need to be able to handle the situation where some
nodes in the tree are RemoteFrames.

R=dcheng@chromium.org
BUG=346764
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175317

[kenrb, 2014-05-30 20:50:52]

Daniel: Do you feel you can review this CL adequately? It is the page-level
subset of the FrameTree changes.

[dcheng, 2014-05-30 21:02:57]

https://codereview.chromium.org/302213002/diff/1/Source/core/page/CreateWindo...
File Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/302213002/diff/1/Source/core/page/CreateWindo...
Source/core/page/CreateWindow.cpp:71: return openerFrame.tree().top();
Do we need to add a call to toLocalFrame() here?

https://codereview.chromium.org/302213002/diff/1/Source/core/page/EventHandle...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/302213002/diff/1/Source/core/page/EventHandle...
Source/core/page/EventHandler.cpp:1512: gestureIndicator = adoptPtr(new
UserGestureIndicator(m_frame->localFrameRoot()->eventHandler().m_lastMouseDownUserGestureToken.release()));
How come the diff doesn't include localFrameRoot?

[kenrb, 2014-05-30 21:43:27]

https://codereview.chromium.org/302213002/diff/1/Source/core/page/CreateWindo...
File Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/302213002/diff/1/Source/core/page/CreateWindo...
Source/core/page/CreateWindow.cpp:71: return openerFrame.tree().top();
On 2014/05/30 21:02:58, dcheng wrote:
> Do we need to add a call to toLocalFrame() here?

Yes, but there is a bit of a problem that I can't call toLocalFrame() directly
on a return value from FrameTree because the casting macros prevent trying to
cast LocalFrame* to LocalFrame*. There are only a few places like this, so I am
leaving them until the final CL where I actually change the return type.

https://codereview.chromium.org/302213002/diff/1/Source/core/page/EventHandle...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/302213002/diff/1/Source/core/page/EventHandle...
Source/core/page/EventHandler.cpp:1512: gestureIndicator = adoptPtr(new
UserGestureIndicator(m_frame->localFrameRoot()->eventHandler().m_lastMouseDownUserGestureToken.release()));
On 2014/05/30 21:02:58, dcheng wrote:
> How come the diff doesn't include localFrameRoot?

That was a previous CL that already landed. I missed this line by accident, and
am correcting it now.

[dcheng, 2014-05-30 21:57:09]

On 2014/05/30 21:43:27, kenrb wrote:
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/CreateWindo...
> File Source/core/page/CreateWindow.cpp (right):
> 
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/CreateWindo...
> Source/core/page/CreateWindow.cpp:71: return openerFrame.tree().top();
> On 2014/05/30 21:02:58, dcheng wrote:
> > Do we need to add a call to toLocalFrame() here?
> 
> Yes, but there is a bit of a problem that I can't call toLocalFrame() directly
> on a return value from FrameTree because the casting macros prevent trying to
> cast LocalFrame* to LocalFrame*. There are only a few places like this, so I
am
> leaving them until the final CL where I actually change the return type.
> 
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/EventHandle...
> File Source/core/page/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/EventHandle...
> Source/core/page/EventHandler.cpp:1512: gestureIndicator = adoptPtr(new
>
UserGestureIndicator(m_frame->localFrameRoot()->eventHandler().m_lastMouseDownUserGestureToken.release()));
> On 2014/05/30 21:02:58, dcheng wrote:
> > How come the diff doesn't include localFrameRoot?
> 
> That was a previous CL that already landed. I missed this line by accident,
and
> am correcting it now.

Ah. I'm a bit confused about localFrameRoot(). Given:

Local A1
  |
Remote B
  |
Local A2

Is A2::localFrameRoot() supposed to return A1 or A2? From the name and the
comment, I would expect A1, but from what I can tell, the code returns A2?

[kenrb, 2014-05-30 23:27:43]

On 2014/05/30 21:57:09, dcheng wrote:
> On 2014/05/30 21:43:27, kenrb wrote:
> >
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/CreateWindo...
> > File Source/core/page/CreateWindow.cpp (right):
> > 
> >
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/CreateWindo...
> > Source/core/page/CreateWindow.cpp:71: return openerFrame.tree().top();
> > On 2014/05/30 21:02:58, dcheng wrote:
> > > Do we need to add a call to toLocalFrame() here?
> > 
> > Yes, but there is a bit of a problem that I can't call toLocalFrame()
directly
> > on a return value from FrameTree because the casting macros prevent trying
to
> > cast LocalFrame* to LocalFrame*. There are only a few places like this, so I
> am
> > leaving them until the final CL where I actually change the return type.
> > 
> >
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/EventHandle...
> > File Source/core/page/EventHandler.cpp (right):
> > 
> >
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/EventHandle...
> > Source/core/page/EventHandler.cpp:1512: gestureIndicator = adoptPtr(new
> >
>
UserGestureIndicator(m_frame->localFrameRoot()->eventHandler().m_lastMouseDownUserGestureToken.release()));
> > On 2014/05/30 21:02:58, dcheng wrote:
> > > How come the diff doesn't include localFrameRoot?
> > 
> > That was a previous CL that already landed. I missed this line by accident,
> and
> > am correcting it now.
> 
> Ah. I'm a bit confused about localFrameRoot(). Given:
> 
> Local A1
>   |
> Remote B
>   |
> Local A2
> 
> Is A2::localFrameRoot() supposed to return A1 or A2? From the name and the
> comment, I would expect A1, but from what I can tell, the code returns A2?

It returns A2. It doesn't particularly matter, since neither will exhibit
correct behavior -- the intent is to have a universal token across all frames,
but that hasn't been implemented yet. I think using A2 is probably better
because if B is reparented to be in a different frame then we could lose track
of the token we were using.

[dcheng, 2014-05-30 23:28:49]

On 2014/05/30 23:27:43, kenrb wrote:
> On 2014/05/30 21:57:09, dcheng wrote:
> > On 2014/05/30 21:43:27, kenrb wrote:
> > >
> >
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/CreateWindo...
> > > File Source/core/page/CreateWindow.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/CreateWindo...
> > > Source/core/page/CreateWindow.cpp:71: return openerFrame.tree().top();
> > > On 2014/05/30 21:02:58, dcheng wrote:
> > > > Do we need to add a call to toLocalFrame() here?
> > > 
> > > Yes, but there is a bit of a problem that I can't call toLocalFrame()
> directly
> > > on a return value from FrameTree because the casting macros prevent trying
> to
> > > cast LocalFrame* to LocalFrame*. There are only a few places like this, so
I
> > am
> > > leaving them until the final CL where I actually change the return type.
> > > 
> > >
> >
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/EventHandle...
> > > File Source/core/page/EventHandler.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/EventHandle...
> > > Source/core/page/EventHandler.cpp:1512: gestureIndicator = adoptPtr(new
> > >
> >
>
UserGestureIndicator(m_frame->localFrameRoot()->eventHandler().m_lastMouseDownUserGestureToken.release()));
> > > On 2014/05/30 21:02:58, dcheng wrote:
> > > > How come the diff doesn't include localFrameRoot?
> > > 
> > > That was a previous CL that already landed. I missed this line by
accident,
> > and
> > > am correcting it now.
> > 
> > Ah. I'm a bit confused about localFrameRoot(). Given:
> > 
> > Local A1
> >   |
> > Remote B
> >   |
> > Local A2
> > 
> > Is A2::localFrameRoot() supposed to return A1 or A2? From the name and the
> > comment, I would expect A1, but from what I can tell, the code returns A2?
> 
> It returns A2. It doesn't particularly matter, since neither will exhibit
> correct behavior -- the intent is to have a universal token across all frames,
> but that hasn't been implemented yet. I think using A2 is probably better
> because if B is reparented to be in a different frame then we could lose track
> of the token we were using.

You can't reparent frames without the children getting blown away =)

[kenrb, 2014-05-30 23:29:51]

On 2014/05/30 23:28:49, dcheng wrote:
> On 2014/05/30 23:27:43, kenrb wrote:
> > On 2014/05/30 21:57:09, dcheng wrote:
> > > On 2014/05/30 21:43:27, kenrb wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/CreateWindo...
> > > > File Source/core/page/CreateWindow.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/CreateWindo...
> > > > Source/core/page/CreateWindow.cpp:71: return openerFrame.tree().top();
> > > > On 2014/05/30 21:02:58, dcheng wrote:
> > > > > Do we need to add a call to toLocalFrame() here?
> > > > 
> > > > Yes, but there is a bit of a problem that I can't call toLocalFrame()
> > directly
> > > > on a return value from FrameTree because the casting macros prevent
trying
> > to
> > > > cast LocalFrame* to LocalFrame*. There are only a few places like this,
so
> I
> > > am
> > > > leaving them until the final CL where I actually change the return type.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/EventHandle...
> > > > File Source/core/page/EventHandler.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/302213002/diff/1/Source/core/page/EventHandle...
> > > > Source/core/page/EventHandler.cpp:1512: gestureIndicator = adoptPtr(new
> > > >
> > >
> >
>
UserGestureIndicator(m_frame->localFrameRoot()->eventHandler().m_lastMouseDownUserGestureToken.release()));
> > > > On 2014/05/30 21:02:58, dcheng wrote:
> > > > > How come the diff doesn't include localFrameRoot?
> > > > 
> > > > That was a previous CL that already landed. I missed this line by
> accident,
> > > and
> > > > am correcting it now.
> > > 
> > > Ah. I'm a bit confused about localFrameRoot(). Given:
> > > 
> > > Local A1
> > >   |
> > > Remote B
> > >   |
> > > Local A2
> > > 
> > > Is A2::localFrameRoot() supposed to return A1 or A2? From the name and the
> > > comment, I would expect A1, but from what I can tell, the code returns A2?
> > 
> > It returns A2. It doesn't particularly matter, since neither will exhibit
> > correct behavior -- the intent is to have a universal token across all
frames,
> > but that hasn't been implemented yet. I think using A2 is probably better
> > because if B is reparented to be in a different frame then we could lose
track
> > of the token we were using.
> 
> You can't reparent frames without the children getting blown away =)

Ah, okay. It probably doesn't matter then.

[dcheng, 2014-06-02 18:40:34]

lgtm

[kenrb, 2014-06-02 18:43:59]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2014-06-02 18:44:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kenrb@chromium.org/302213002/20001

[commit-bot: I haz the power, 2014-06-02 18:45:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6710)

[kenrb, 2014-06-02 20:05:59]

The CQ bit was unchecked by kenrb@chromium.org

[kenrb, 2014-06-02 20:06:01]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2014-06-02 20:06:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kenrb@chromium.org/302213002/20001

[commit-bot: I haz the power, 2014-06-02 21:35:13]

Change committed as 175317