Add Frame and RemoteFrame classes.

Add Frame and RemoteFrame classes.

Creates RemoteFrame, which will represent in WebCore frames
that are rendered outside of Blink, and Frame, a base class
for both LocalFrame and RemoteFrame. The current division of
members and methods between Frame and LocalFrame is not
final, but provides a starting point for us to start
converting LocalFrame to Frame in some parts of Blink.

BUG=346764
R=eseidel@chromium.org

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

[kenrb, 2014-02-27 23:44:57]

Eric, PTAL?

[eseidel, 2014-02-27 23:52:38]

Can we include RemoteFrame in this patch and move its bits there?  Otherwise we
just are creating a super-class w/o a clear picture of what needs to be there?

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

https://codereview.chromium.org/183713002/diff/30001/Source/core/frame/Frame....
Source/core/frame/Frame.h:29: class FrameInit : public RefCounted<FrameInit> {
I'm confused.  Doesn't FrameInit only make sense for LocalFrames?  Do
RemoteFrames need clients, etc too?

https://codereview.chromium.org/183713002/diff/30001/Source/core/frame/Frame....
Source/core/frame/Frame.h:95: // ======== All public functions below this point
are candidates to move out of Frame into another class. ========
I'm not sure Settings is going anywhere.  This is just an accessor which bounces
through host() anyway?

https://codereview.chromium.org/183713002/diff/30001/Source/core/frame/Frame....
Source/core/frame/Frame.h:109: RefPtr<DOMWindow> m_domWindow;
RemoteFrames don't have a DOMWindow, do they?

https://codereview.chromium.org/183713002/diff/30001/Source/core/frame/Frame....
Source/core/frame/Frame.h:118: blink::WebLayer* m_remotePlatformLayer;
This moves onto remote frame right?

[kenrb, 2014-02-28 16:32:38]

Okay RemoteFrame added.

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

https://codereview.chromium.org/183713002/diff/30001/Source/core/frame/Frame....
Source/core/frame/Frame.h:29: class FrameInit : public RefCounted<FrameInit> {
On 2014/02/27 23:52:38, eseidel wrote:
> I'm confused.  Doesn't FrameInit only make sense for LocalFrames?  Do
> RemoteFrames need clients, etc too?

We need to track the owner element and I think the FrameHost. We shouldn't need
the FrameLoaderClient but that might be useful as an intermediate step if we
start using RemoteFrame before we work out its proper creation code path.

https://codereview.chromium.org/183713002/diff/30001/Source/core/frame/Frame....
Source/core/frame/Frame.h:95: // ======== All public functions below this point
are candidates to move out of Frame into another class. ========
On 2014/02/27 23:52:38, eseidel wrote:
> I'm not sure Settings is going anywhere.  This is just an accessor which
bounces
> through host() anyway?

I was just copying it from the section it was originally in on Frame. I have no
opinion, and can move it above the comment if you want.

https://codereview.chromium.org/183713002/diff/30001/Source/core/frame/Frame....
Source/core/frame/Frame.h:109: RefPtr<DOMWindow> m_domWindow;
On 2014/02/27 23:52:38, eseidel wrote:
> RemoteFrames don't have a DOMWindow, do they?

Scripts can still interact with an OOP iframe's window object. I believe the
current plan is to have a special DOMWindow for RemoteFrame, but to use the real
one until that is written.

https://codereview.chromium.org/183713002/diff/30001/Source/core/frame/Frame....
Source/core/frame/Frame.h:118: blink::WebLayer* m_remotePlatformLayer;
On 2014/02/27 23:52:38, eseidel wrote:
> This moves onto remote frame right?

Yes, when we get to the point where RemoteFrame can be instantiated. We don't
want to break --site-per-process. I put a comment about that above the accessor.

[kenrb, 2014-02-28 17:02:21]

On 2014/02/27 23:52:38, eseidel wrote:
> Can we include RemoteFrame in this patch and move its bits there?  Otherwise
we
> just are creating a super-class w/o a clear picture of what needs to be there?
> 

I just realized that I didn't really answer the question in my last reply. I've
added RemoteFrame but it is mostly empty. Since we don't want to break
--site-per-process, we need LocalFrame to act as a RemoteFrame until all the
places that need to deal with a RemoteFrame understand the difference between
Frame and LocalFrame. The next phase convert pointers from LocalFrame to Frame
in any places where a RemoteFrame might need to be attached, and sprinkle
toLocalFrame(frame()) liberally over the entire code base. WebFrameProxy should
be finished by then, and we can add the path to convert WebFrameImpl+LocalFrame
into WebFrameProxy+RemoteFrame. Then we should be able to (mostly) put all the
members and methods in their correct places.

Does that plan sound okay?

[kenrb, 2014-03-03 19:22:19]

Eric: ping?

[eseidel, 2014-03-04 01:32:59]

https://codereview.chromium.org/183713002/diff/30002/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/183713002/diff/30002/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
We can't re-license existing code.  Please copy the original license block from
Frame.cpp.

https://codereview.chromium.org/183713002/diff/30002/Source/core/frame/Remote...
File Source/core/frame/RemoteFrame.h (right):

https://codereview.chromium.org/183713002/diff/30002/Source/core/frame/Remote...
Source/core/frame/RemoteFrame.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
New code it's OK to use the new block for, thank you.

[eseidel, 2014-03-04 01:36:21]

I don't think we want to sprinkle Frame* or toLocalFrame() liberarlly over the
code base.  Most places only care about LocalFrame.  Very few can have either
type of Frame.

We should operate under the assumption that most blinkers don't care about OOPI
and shouldn't need to. :)

Overall this change looks fine.  It looks like we're moving too much to Frame
from LocalFrame, and so it would be nice to // FIXME: next to functions which
really should move back after this intermediate period.

How long until we can use RemoteFrame and make this separation more clean?  I
would encourage you to be sure to loop abarth and haraken in on any DOMWindow
changes needed for RemoteFrame.

[eseidel, 2014-03-04 01:37:22]

Sorry for the delay.  With the license updates and ideally comments explaining
what (if anything) is stayngin in Frame vs. moving back up into LocalFrame vs.
moving to RemoteFrame, then lgtm.

[eseidel, 2014-03-04 01:38:10]

Given that we're considering dcheng as a reviewer, I think we should have him do
unofficial reviews of all OOPI changes and then someone else can always provide
the official rubber stampl

[kenrb, 2014-03-04 14:58:26]

On 2014/03/04 01:36:21, eseidel wrote:
> I don't think we want to sprinkle Frame* or toLocalFrame() liberarlly over the
> code base.  Most places only care about LocalFrame.  Very few can have either
> type of Frame.
> 

I have already started working on this. Most of the places I am thinking about
shouldn't be contentious (HTMLFrameOwnerElement and the FrameTree stand out). A
few are questionable (FocusController I think needs to handle both types of
Frames), and a few should only be LocalFrame but will require some work to
untangle -- DOMWindow will need to handle RemoteFrame until there is a
RemoteDOMWindow class or similar, Document *won't* need to know about
RemoteFrames but we will break the current implementation of OOPIF input event
routing. We are also creating a RemoteFrameView and rendering code will need to
know about that.

This can be discussed on a case by case basis as we get things working.

> Overall this change looks fine.  It looks like we're moving too much to Frame
> from LocalFrame, and so it would be nice to // FIXME: next to functions which
> really should move back after this intermediate period.
> 
> How long until we can use RemoteFrame and make this separation more clean?  I
> would encourage you to be sure to loop abarth and haraken in on any DOMWindow
> changes needed for RemoteFrame.

Possibly except for DOMWindow, I would hope to have everything in the right
place by the end of March.

[eseidel, 2014-03-04 17:59:26]

As you're aware, incomplete refactorings take a loan out on the code base, which
the entire community is held liable for. :)  So hopefully we'll complete this
refactoring as soon as possible (repay the loan).  End of march sounds good.

thanks.  lgtm.

[dcheng, 2014-03-04 19:03:02]

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

https://codereview.chromium.org/183713002/diff/90001/Source/core/frame/Frame....
Source/core/frame/Frame.h:124: double devicePixelRatio() const;
I don't see an actual implementation for this function. Why is this on Frame
anyway?

[kenrb, 2014-03-04 19:36:50]

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

https://codereview.chromium.org/183713002/diff/90001/Source/core/frame/Frame....
Source/core/frame/Frame.h:124: double devicePixelRatio() const;
On 2014/03/04 19:03:03, dcheng wrote:
> I don't see an actual implementation for this function. Why is this on Frame
> anyway?

Good catch, it was an oversight. Removed.

[kenrb, 2014-03-04 19:37:05]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2014-03-04 19:37:27]

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

[commit-bot: I haz the power, 2014-03-05 17:54:33]

Change committed as 168501