Delete Frame::domWindow() and Frame::existingDOMWindow()

• Component: New Bugs
• Status: RESOLVED FIXED

[WebKit Review Bot]

Delete Frame::domWindow() and Frame::existingDOMWindow()
Requested by abarth on #webkit.

[Adam Barth]

Created attachment 158406 [details]
Patch

[Build Bot]

Comment on attachment 158406 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 158406 [details]
Patch

Attachment 158406 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13493789

[Eric Seidel (no email)]

Comment on attachment 158406 [details]
Patch

You borks it.

[Adam Barth]

Yeah, I need to fight the EWS for a bit.

[Adam Barth]

Created attachment 158418 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 158418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158418&action=review

Looks reasonable.

> Source/WebCore/bindings/v8/custom/V8HTMLFrameSetElementCustom.cpp:59
> +        return v8::Undefined();

I think you mean v8Undefined. :)

> Source/WebCore/bindings/v8/custom/V8HTMLFrameSetElementCustom.cpp:61
> +    if (!document->frame())
> +        return v8::Undefined(); // FIXME: How could Frame be null here?

Seems like this would be possible from JS, no? Grab the content document of a frame, then disconnect the frameset from the document (removeChild it), and then try and grab a named property on teh frameset?

> Source/WebCore/svg/SVGDocumentExtensions.cpp:199
> +    if (document->frame())

do you wnat to check frame here?

> Source/WebCore/xml/XSLStyleSheetLibxslt.cpp:148
> +    if (ownerDocument()->frame())

Do you want to check frame here?

[Adam Barth]

Comment on attachment 158418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158418&action=review

>> Source/WebCore/bindings/v8/custom/V8HTMLFrameSetElementCustom.cpp:59
>> +        return v8::Undefined();
> 
> I think you mean v8Undefined. :)

Shoot me now.

>> Source/WebCore/bindings/v8/custom/V8HTMLFrameSetElementCustom.cpp:61
>> +        return v8::Undefined(); // FIXME: How could Frame be null here?
> 
> Seems like this would be possible from JS, no? Grab the content document of a frame, then disconnect the frameset from the document (removeChild it), and then try and grab a named property on teh frameset?

Yes, you're right.

>> Source/WebCore/svg/SVGDocumentExtensions.cpp:199
>> +    if (document->frame())
> 
> do you wnat to check frame here?

I think checking document->domWindow() for 0 is equivalent, but I wanted to make this change as mechanical as possible.

>> Source/WebCore/xml/XSLStyleSheetLibxslt.cpp:148
>> +    if (ownerDocument()->frame())
> 
> Do you want to check frame here?

ditto

[Adam Barth]

Created attachment 158422 [details]
Patch for landing

[Adam Barth]

Created attachment 158423 [details]
For landing once EWS cycle green

[Adam Barth]

Committed r125615: <http://trac.webkit.org/changeset/125615>

[Darin Adler]

Comment on attachment 158423 [details]
For landing once EWS cycle green

View in context: https://bugs.webkit.org/attachment.cgi?id=158423&action=review

> Source/WebCore/bindings/js/JSEventListener.cpp:-95
> -        // The window must still be active in its frame. See <https://bugs.webkit.org/show_bug.cgi?id=21921>.
> -        // FIXME: A better fix for this may be to change DOMWindow::frame() to not return a frame the detached window used to be in.

What made this comment obsolete?

> Source/WebCore/loader/DocumentLoader.cpp:425
> -    if (DOMWindow* window = m_frame->existingDOMWindow())
> +    if (DOMWindow* window = m_frame->document()->domWindow())

Can domWindow ever return null? Why null check?

> Source/WebCore/loader/FrameLoader.cpp:467
> -        if (DOMWindow* window = m_frame->existingDOMWindow()) {
> +        if (DOMWindow* window = m_frame->document()->domWindow()) {

Can domWindow ever return null? Why null check?

> Source/WebCore/loader/FrameLoader.cpp:1905
> -        if (DOMWindow* window = m_frame->existingDOMWindow()) {
> +        if (DOMWindow* window = m_frame->document()->domWindow()) {

Can domWindow ever return null? Why null check?

> Source/WebCore/loader/FrameLoader.cpp:2697
> -    DOMWindow* domWindow = m_frame->existingDOMWindow();
> +    DOMWindow* domWindow = m_frame->document()->domWindow();

Can domWindow ever return null? Why null check?

> Source/WebCore/page/Frame.h:-133
> -        DOMWindow* existingDOMWindow() { return domWindow(); }

Did we lose an optimization at some point earlier in this work where we’d avoid creating a DOMWindow in some cases?

[Adam Barth]

Comment on attachment 158423 [details]
For landing once EWS cycle green

View in context: https://bugs.webkit.org/attachment.cgi?id=158423&action=review

>> Source/WebCore/bindings/js/JSEventListener.cpp:-95
>> -        // FIXME: A better fix for this may be to change DOMWindow::frame() to not return a frame the detached window used to be in.
> 
> What made this comment obsolete?

This FIXME is redundant with the FIXME comment above DOMWindow::isCurrentlyDisplayedInFrame.  Now that this code calls isCurrentlyDisplayedInFrame, I thought we didn't need the FIXME comment here.

>> Source/WebCore/loader/DocumentLoader.cpp:425
>> +    if (DOMWindow* window = m_frame->document()->domWindow())
> 
> Can domWindow ever return null? Why null check?

Document::domWindow() can return 0 for document that were never active in a Frame (e.g., a Document created by XMLHttpRequest).  In this instance, we're getting the Document from a Frame, so it's obviously active in that frame.  I've filed Bug 94052 about removing these extra null checks.

>> Source/WebCore/page/Frame.h:-133
>> -        DOMWindow* existingDOMWindow() { return domWindow(); }
> 
> Did we lose an optimization at some point earlier in this work where we’d avoid creating a DOMWindow in some cases?

I don't think so.  As far as I can tell, every active Document ended up with a DOMWindow.  For example, FrameLoader::clear ends up creating one.  It's possible that we could have avoided creating the DOMWindow before we had a split window.  I'm not as familiar with how things worked then.

DOMWindow is actually a pretty light object.  It's just a bunch of 0 RefPtrs that get filled in lazily.