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.
Created attachment 336894 [details] WIP patch
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.
Created attachment 336896 [details] WIP patch
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.
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
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
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
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
Created attachment 336928 [details] WIP Patch
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.
Created attachment 336929 [details] Patch
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.
Created attachment 336934 [details] Patch
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.
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
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
Created attachment 336942 [details] Patch
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.
I wish we would rename Page.
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.
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.
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.
(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.
Comment on attachment 336942 [details] Patch Clearing flags on attachment: 336942 Committed r230211: <https://trac.webkit.org/changeset/230211>
All reviewed patches have been landed. Closing bug.
<rdar://problem/39146741>
Instead of the names Frame, RemoteFrame, and LocalFrame, I suggest you consider names like PossiblyRemoteFrame, RemoteFrame, and Frame.
(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.