Add a layer of indirection between Frame and Page

Add a layer of indirection between Frame and Page

This adds a new class FrameHost whose job it is to
hold state which is shared between multiple Frames
such as Settings or Clients.

Right now FrameHost just redirects all calls to Page --
just enough to get Frame.cpp building using FrameHost
instead of Page.

In a follow-up change I will actually move ownership
of Settings, (some of PageClients) and various
members which Page holds onto just to be a central
place for all Frames to access.

Page and friends should be about holding onto page-level
logic and members for coordinating interaction between
frames.  FrameHost is about holding members and logic
which are simply shared between frames but do not
need to coordinate between those frames.

This provides the basic plumbing to allow me to remove
most of the remaining includes of page/ classes from
the rest of core.  Anything left over after that will
be actual layering violations and not just bad plumbing.

R=abarth@chromium.org, abarth
BUG=305811
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164117

[jam, 2013-12-13 23:20:29]

sweet: have you given thought about what will happen to the objects that
are created in WebViewImpl's constructor and associated with Page? Many of
these are backed by objects on the content side which are now per
content::RenderView and need to be per content::RenderFrame.


On Fri, Dec 13, 2013 at 3:08 PM, <eseidel@chromium.org> wrote:

> Reviewers: abarth,
>
> Description:
> Add a layer of indirection between Frame and Page
>
> This adds a new class FrameHost whose job it is to
> hold state which is shared between multiple Frames
> such as Settings or Clients.
>
> Right now FrameHost just redirects all calls to Page --
> just enough to get Frame.cpp building using FrameHost
> instead of Page.
>
> In a follow-up change I will actually move ownership
> of Settings, (some of PageClients) and various
> members which Page holds onto just to be a central
> place for all Frames to access.
>
> Page and friends should be about holding onto page-level
> logic and members for coordinating interaction between
> frames.  FrameHost is about holding members and logic
> which are simply shared between frames but do not
> need to coordinate between those frames.
>
> This provides the basic plumbing to allow me to remove
> most of the remaining includes of page/ classes from
> the rest of core.  Anything left over after that will
> be actual layering violations and not just bad plumbing.
>
> R=abarth
> BUG=305811
>
> Please review this at https://codereview.chromium.org/115293005/
>
> SVN Base: svn://svn.chromium.org/blink/trunk
>
> Affected files (+144, -102 lines):
>   M Source/core/core.gypi
>   M Source/core/frame/DOMWindow.h
>   M Source/core/frame/DOMWindow.cpp
>   M Source/core/frame/Frame.h
>   M Source/core/frame/Frame.cpp
>   M Source/core/frame/FrameDestructionObserver.h
>   M Source/core/frame/FrameDestructionObserver.cpp
>   A + Source/core/frame/FrameHost.h
>   A + Source/core/frame/FrameHost.cpp
>   M Source/core/inspector/InspectorOverlay.cpp
>   M Source/core/loader/FrameLoader.cpp
>   M Source/core/page/Page.h
>   M Source/core/page/Page.cpp
>   M Source/core/svg/graphics/SVGImage.cpp
>   M Source/core/testing/DummyPageHolder.cpp
>   M Source/web/WebFrameImpl.h
>   M Source/web/WebFrameImpl.cpp
>   M Source/web/WebPagePopupImpl.cpp
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium Site Isolation Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to site-isolation-reviews+unsubscribe@chromium.org.
> For more options, visit https://groups.google.com/a/
> chromium.org/groups/opt_out.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[eseidel, 2013-12-13 23:23:52]

Yeah, there are a lot of objects which will need to be split into per-frame
parts and page-global-coordination parts.

This just giving us a place to put hang the per-frame parts. :)

[jam, 2013-12-13 23:30:59]

On 2013/12/13 23:23:52, eseidel wrote:
> Yeah, there are a lot of objects which will need to be split into per-frame
> parts and page-global-coordination parts.
> 
> This just giving us a place to put hang the per-frame parts. :)

ok. I haven't traced in details how these objects that I mentioned are used, but
hopefully they all have a frame associated with them and they can all be
per-frame. I'm hoping we can avoid a per-page concept in content\renderer, and
have that only exist in the browser side.

[abarth-chromium, 2013-12-15 06:04:23]

lgtm

Neat

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

https://codereview.chromium.org/115293005/diff/1/Source/core/frame/Frame.cpp#...
Source/core/frame/Frame.cpp:661: // If the node has a renderer, this Frame must
be active and have a host.
Can we make this an ASSERT rather than a comment?  Perhaps:

ASSERT(document()->isActive()) ?

https://codereview.chromium.org/115293005/diff/1/Source/core/frame/Frame.cpp#...
Source/core/frame/Frame.cpp:690: // If the node has a renderer, this Frame must
be active and have a host.
ditto

https://codereview.chromium.org/115293005/diff/1/Source/core/frame/FrameHost.cpp
File Source/core/frame/FrameHost.cpp (right):

https://codereview.chromium.org/115293005/diff/1/Source/core/frame/FrameHost....
Source/core/frame/FrameHost.cpp:46: 
Extra blank line.  :)

[commit-bot: I haz the power, 2013-12-15 16:42:49]

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

[commit-bot: I haz the power, 2013-12-15 17:31:22]

Retried try job too often on linux_blink_rel for step(s)
blink_platform_unittests, webkit_lint, webkit_python_tests, webkit_tests,
webkit_unit_tests, wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-12-15 19:16:08]

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

[commit-bot: I haz the power, 2013-12-15 19:48:42]

Retried try job too often on linux_blink for step(s) blink_platform_unittests,
webkit_lint, webkit_python_tests, webkit_tests, webkit_unit_tests, wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[eseidel, 2013-12-18 20:51:13]

Updating now.

[commit-bot: I haz the power, 2013-12-18 21:16:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eseidel@chromium.org/115293005/40001

[eseidel, 2013-12-18 21:26:23]

Committed patchset #3 manually as r164117 (presubmit successful).