Bug 184191 – Drop MainFrame class

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Drop MainFrame class and merge contents into Page since there is a 1:1 relationship between the Page and the MainFrame.

This is ground work for introducing LocalFrame / RemoteFrame concepts.

[Chris Dumez]

Created attachment 336894 [details]
WIP patch

[EWS Watchlist]

Attachment 336894 [details] did not pass style-queue:


ERROR: Source/WebCore/page/Page.cpp:245:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 336896 [details]
WIP patch

[EWS Watchlist]

Attachment 336896 [details] did not pass style-queue:


ERROR: Source/WebCore/page/Page.cpp:245:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 336896 [details]
WIP patch

Attachment 336896 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7153144

New failing tests:
fast/loader/unload-reload.html

[EWS Watchlist]

Created attachment 336902 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 336896 [details]
WIP patch

Attachment 336896 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7153270

New failing tests:
fast/loader/unload-mutation-crash.html
fast/events/before-unload-remove-itself.html
fast/events/before-unload-remove-itself-async-delegate.html

[EWS Watchlist]

Created attachment 336904 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 336928 [details]
WIP Patch

[EWS Watchlist]

Attachment 336928 [details] did not pass style-queue:


ERROR: Source/WebCore/page/Page.cpp:244:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 142 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 336929 [details]
Patch

[EWS Watchlist]

Attachment 336929 [details] did not pass style-queue:


ERROR: Source/WebCore/page/Page.cpp:244:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 147 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 336934 [details]
Patch

[EWS Watchlist]

Attachment 336934 [details] did not pass style-queue:


ERROR: Source/WebCore/page/Page.cpp:244:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 147 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 336934 [details]
Patch

Attachment 336934 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7161461

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 336938 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Chris Dumez]

Created attachment 336942 [details]
Patch

[EWS Watchlist]

Attachment 336942 [details] did not pass style-queue:


ERROR: Source/WebCore/page/Page.cpp:244:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 147 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

I wish we would rename Page.

[Darin Adler]

I originally created MainFrame because I hoped to move state from Page to Frame to fix object lifetime problems. The MainFrame lives longer than the Page and is guaranteed to live longer than any of its subframes too. Whereas the Page can be destroyed and Frame objects will still be left behind since they are reference counted and it is not.

But it’s OK with me if you want to get rid of it. I guess we never made substantial progress on using it they way we had intended.

[Chris Dumez]

The issue is that I need to add support for remote frames and remote windows (when they are actually in another process). So a page’s main frame can either be local or remote. My plan was to rename Frame to LocalFrame and introduce a new RemoteFrame class, both would subclass a Frame base class.

But right now, MainFrame is a subclass of Frame (soon to be LocalFrame). This is an issue given that a MainFrame can be remote when you window.open() another origin.

[Darin Adler]

Your plan is OK with me. I wanted you to know why I created MainFrame. I am a little surprised that we want multiple classes of Frame itself. Since Frame is a constellation of classes such as FrameLoader, it seems that making Frame itself an abstract base class isn’t so powerful -- much of the behavior we want to make different is on those other objects, not the Frame object itself. It would be bad to expand Frame itself too much and make it a "god" class again. We have been working to cut it down for quite a while.

[Chris Dumez]

(In reply to Darin Adler from comment #22)
> Your plan is OK with me. I wanted you to know why I created MainFrame. I am
> a little surprised that we want multiple classes of Frame itself. Since
> Frame is a constellation of classes such as FrameLoader, it seems that
> making Frame itself an abstract base class isn’t so powerful -- much of the
> behavior we want to make different is on those other objects, not the Frame
> object itself. It would be bad to expand Frame itself too much and make it a
> "god" class again. We have been working to cut it down for quite a while.

I do believe I will need the concept of RemoteFrame for 2 reasons:
- I need to hang my RemoteDOMWindow off something. I plan to reuse the Page but swap its main Frame to be a RemoteFrame when navigated cross origin. Then the RemoteFrame would own the RemoteDOMWindow (I do not think we need a Document when a Frame / Window is remote)
- a newly opened window has an opener and while window.opener returns a DOMWindow, an opener is really a Frame. The reason I say that is that window.opener will start returning a different DOMWindow when the opener Frame is navigated. Therefore, when a new Window is opened cross origin and has an opener, I believe I will need FrameLoader::opener to be a RemoteFrame. And we will keep this Frame’s Window with the remote process.

[WebKit Commit Bot]

Comment on attachment 336942 [details]
Patch

Clearing flags on attachment: 336942

Committed r230211: <https://trac.webkit.org/changeset/230211>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39146741>

[Darin Adler]

Instead of the names Frame, RemoteFrame, and LocalFrame, I suggest you consider names like PossiblyRemoteFrame, RemoteFrame, and Frame.

[Chris Dumez]

(In reply to Darin Adler from comment #27)
> Instead of the names Frame, RemoteFrame, and LocalFrame, I suggest you
> consider names like PossiblyRemoteFrame, RemoteFrame, and Frame.

Although short-term, we'll hopefully not have too many PossiblyRemoteFrames, I think that in the longer term, most frames will be possibly remote. This is why I suggested Frame, because it is shorter and I expect in the future, most of them our code will use Frame. PossiblyRemoteFrame seems very long.