Change FrameTree to return Frames instead of LocalFrames.

Change FrameTree to return Frames instead of LocalFrames.

This allows RemoteFrames to be added to the FrameTree.

BUG=346764
R=dcheng@chromium.org
TBR=japhet@chromium.org,eseidel@chromium.org

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

[kenrb, 2014-06-03 21:20:38]

I still need a Source/web owner, but Daniel can you PTAL?

[dcheng, 2014-06-04 18:06:36]

https://codereview.chromium.org/317493002/diff/20001/Source/core/page/FrameTr...
File Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/317493002/diff/20001/Source/core/page/FrameTr...
Source/core/page/FrameTree.cpp:73: // FIXME: Temporary hack to stage converting
locations that really should be Frame.
Delete FIXMEs?

https://codereview.chromium.org/317493002/diff/20001/Source/core/page/FrameTr...
Source/core/page/FrameTree.cpp:394: if (!frame->isLocalFrame())
I don't see a reason this shouldn't work for remote frames. I would do
frame->isLocal() ? toLocalFrame(frame)->view() : 0 below, etc.

https://codereview.chromium.org/317493002/diff/20001/Source/modules/geolocati...
File Source/modules/geolocation/testing/InternalsGeolocation.cpp (right):

https://codereview.chromium.org/317493002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/testing/InternalsGeolocation.cpp:50: for (Frame*
childFrame = document->page()->mainFrame(); childFrame; childFrame =
childFrame->tree().nextSibling())
I think you can just delete the for statement altogether and only keep line 51.

https://codereview.chromium.org/317493002/diff/20001/Source/web/SpellCheckerC...
File Source/web/SpellCheckerClientImpl.cpp (right):

https://codereview.chromium.org/317493002/diff/20001/Source/web/SpellCheckerC...
Source/web/SpellCheckerClientImpl.cpp:103: break;
groby, is it intentional that we break out of the loop as soon as we encounter a
non-null document?

https://codereview.chromium.org/317493002/diff/20001/Source/web/WebLocalFrame...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/317493002/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:660: if (!frame() ||
!frame()->tree().traversePreviousWithWrap(wrap)->isLocalFrame())
Put a FIXME here--this should move to WebFrame anyway and lose the LocalFrame
dependency, once we fix fromFrame().

https://codereview.chromium.org/317493002/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/317493002/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2460: return
WebLocalFrameImpl::fromFrame(toLocalFrame(frame));
Ditto--add a FIXME here. Either this should only deal with WebLocalFrames or it
should move to WebFrame.

[dcheng, 2014-06-04 18:06:56]

+groby for real for my spellcheck question

[dcheng, 2014-06-04 18:12:28]

https://codereview.chromium.org/317493002/diff/20001/Source/web/SpellCheckerC...
File Source/web/SpellCheckerClientImpl.cpp (right):

https://codereview.chromium.org/317493002/diff/20001/Source/web/SpellCheckerC...
Source/web/SpellCheckerClientImpl.cpp:103: break;
On 2014/06/04 18:06:37, dcheng wrote:
> groby, is it intentional that we break out of the loop as soon as we encounter
a
> non-null document?

I mean *null* document, sorry for the confusion.

[groby-ooo-7-16, 2014-06-04 18:28:13]

https://codereview.chromium.org/317493002/diff/20001/Source/web/SpellCheckerC...
File Source/web/SpellCheckerClientImpl.cpp (right):

https://codereview.chromium.org/317493002/diff/20001/Source/web/SpellCheckerC...
Source/web/SpellCheckerClientImpl.cpp:103: break;
On 2014/06/04 18:06:37, dcheng wrote:
> groby, is it intentional that we break out of the loop as soon as we encounter
a
> non-null document?

I'm not 100% certain - This was written by pstanek, so let me loop him in. In
general, in what case would we have contiguous text with a non-document frame in
between?

[dcheng, 2014-06-04 20:08:29]

https://codereview.chromium.org/317493002/diff/20001/Source/web/SpellCheckerC...
File Source/web/SpellCheckerClientImpl.cpp (right):

https://codereview.chromium.org/317493002/diff/20001/Source/web/SpellCheckerC...
Source/web/SpellCheckerClientImpl.cpp:103: break;
On 2014/06/04 18:28:14, groby wrote:
> On 2014/06/04 18:06:37, dcheng wrote:
> > groby, is it intentional that we break out of the loop as soon as we
encounter
> a
> > non-null document?
> 
> I'm not 100% certain - This was written by pstanek, so let me loop him in. In
> general, in what case would we have contiguous text with a non-document frame
in
> between?

In general, I don't think this is a normal condition to encounter. I guess you
might see this if this function got invoked in the middle of a frame getting
inserted into DOM, or during frame detach... but that doesn't seem like
something that should really be happening here. I think it's probably OK to just
remove the check...

[kenrb, 2014-06-04 20:17:21]

On 2014/06/04 20:08:29, dcheng wrote:
>
https://codereview.chromium.org/317493002/diff/20001/Source/web/SpellCheckerC...
> File Source/web/SpellCheckerClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/317493002/diff/20001/Source/web/SpellCheckerC...
> Source/web/SpellCheckerClientImpl.cpp:103: break;
> On 2014/06/04 18:28:14, groby wrote:
> > On 2014/06/04 18:06:37, dcheng wrote:
> > > groby, is it intentional that we break out of the loop as soon as we
> encounter
> > a
> > > non-null document?
> > 
> > I'm not 100% certain - This was written by pstanek, so let me loop him in.
In
> > general, in what case would we have contiguous text with a non-document
frame
> in
> > between?
> 
> In general, I don't think this is a normal condition to encounter. I guess you
> might see this if this function got invoked in the middle of a frame getting
> inserted into DOM, or during frame detach... but that doesn't seem like
> something that should really be happening here. I think it's probably OK to
just
> remove the check...

This was my suspicion also, but I chose to be conservative and preserve existing
functionality. I can try removing it and see if any tests break.

[kenrb, 2014-06-04 20:34:46]

Looks like I still have to deal with some red try bots, but comments addressed.
Any suggestions for reviewers for the non-core bits?

https://codereview.chromium.org/317493002/diff/20001/Source/core/page/FrameTr...
File Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/317493002/diff/20001/Source/core/page/FrameTr...
Source/core/page/FrameTree.cpp:73: // FIXME: Temporary hack to stage converting
locations that really should be Frame.
On 2014/06/04 18:06:37, dcheng wrote:
> Delete FIXMEs?

Done.

https://codereview.chromium.org/317493002/diff/20001/Source/core/page/FrameTr...
Source/core/page/FrameTree.cpp:394: if (!frame->isLocalFrame())
On 2014/06/04 18:06:37, dcheng wrote:
> I don't see a reason this shouldn't work for remote frames. I would do
> frame->isLocal() ? toLocalFrame(frame)->view() : 0 below, etc.

Changed.

https://codereview.chromium.org/317493002/diff/20001/Source/modules/geolocati...
File Source/modules/geolocation/testing/InternalsGeolocation.cpp (right):

https://codereview.chromium.org/317493002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/testing/InternalsGeolocation.cpp:50: for (Frame*
childFrame = document->page()->mainFrame(); childFrame; childFrame =
childFrame->tree().nextSibling())
On 2014/06/04 18:06:37, dcheng wrote:
> I think you can just delete the for statement altogether and only keep line
51.

This landed less than a day ago. I put a question on the code review about it.

https://codereview.chromium.org/317493002/diff/20001/Source/web/WebLocalFrame...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/317493002/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:660: if (!frame() ||
!frame()->tree().traversePreviousWithWrap(wrap)->isLocalFrame())
On 2014/06/04 18:06:37, dcheng wrote:
> Put a FIXME here--this should move to WebFrame anyway and lose the LocalFrame
> dependency, once we fix fromFrame().

Ok, I figured this was going to change pretty soon but wasn't sure exactly what
you had in mind to do..

https://codereview.chromium.org/317493002/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/317493002/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2460: return
WebLocalFrameImpl::fromFrame(toLocalFrame(frame));
On 2014/06/04 18:06:37, dcheng wrote:
> Ditto--add a FIXME here. Either this should only deal with WebLocalFrames or
it
> should move to WebFrame.

Done.

[pstanek, 2014-06-05 08:39:32]

On 2014/06/04 18:28:13, groby wrote:
>
https://codereview.chromium.org/317493002/diff/20001/Source/web/SpellCheckerC...
> File Source/web/SpellCheckerClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/317493002/diff/20001/Source/web/SpellCheckerC...
> Source/web/SpellCheckerClientImpl.cpp:103: break;
> On 2014/06/04 18:06:37, dcheng wrote:
> > groby, is it intentional that we break out of the loop as soon as we
encounter
> a
> > non-null document?
> 
> I'm not 100% certain - This was written by pstanek, so let me loop him in. In
> general, in what case would we have contiguous text with a non-document frame
in
> between?

I don't recall anything which forced me to add this null check - looks like
defensive programming. I'd say this may safely be removed.

[kenrb, 2014-06-06 17:34:05]

+ Eric, to review Source/modules please?
+ Nate, to review Source/web and Source/bindings please?

[kenrb, 2014-06-09 20:51:22]

Daniel, when you have some time, can you please take another pass at review?

[dcheng, 2014-06-09 21:12:32]

https://codereview.chromium.org/317493002/diff/130001/Source/core/page/FrameT...
File Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/317493002/diff/130001/Source/core/page/FrameT...
Source/core/page/FrameTree.cpp:54: toLocalFrame(child)->setView(nullptr);
Do we need to clear the view for RemoteFrames as well here?

[kenrb, 2014-06-10 16:55:29]

Any other thoughts? I think I can TBR Eric and Nate if you are okay with this.

https://codereview.chromium.org/317493002/diff/130001/Source/core/page/FrameT...
File Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/317493002/diff/130001/Source/core/page/FrameT...
Source/core/page/FrameTree.cpp:54: toLocalFrame(child)->setView(nullptr);
On 2014/06/09 21:12:32, dcheng wrote:
> Do we need to clear the view for RemoteFrames as well here?

I hadn't really thought about RemoteFrameView lifetime since we don't create one
yet, but it makes sense that it should match up here. Changed.

[dcheng, 2014-06-10 17:26:03]

lgtm

https://codereview.chromium.org/317493002/diff/150001/Source/core/page/FrameT...
File Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/317493002/diff/150001/Source/core/page/FrameT...
Source/core/page/FrameTree.cpp:57: if (child->isRemoteFrame())
else if

[kenrb, 2014-06-10 19:54:47]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2014-06-10 19:55:39]

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

[commit-bot: I haz the power, 2014-06-10 21:10:27]

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

[commit-bot: I haz the power, 2014-06-10 21:57:29]

Change committed as 175921