WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
93990
Delete Frame::domWindow() and Frame::existingDOMWindow()
https://bugs.webkit.org/show_bug.cgi?id=93990
Summary
Delete Frame::domWindow() and Frame::existingDOMWindow()
WebKit Review Bot
Reported
2012-08-14 10:15:41 PDT
Delete Frame::domWindow() and Frame::existingDOMWindow() Requested by abarth on #webkit.
Attachments
Patch
(85.70 KB, patch)
2012-08-14 13:45 PDT
,
Adam Barth
no flags
Details
Formatted Diff
Diff
Patch
(87.70 KB, patch)
2012-08-14 14:43 PDT
,
Adam Barth
eric
: review+
Details
Formatted Diff
Diff
Patch for landing
(87.66 KB, patch)
2012-08-14 15:23 PDT
,
Adam Barth
no flags
Details
Formatted Diff
Diff
For landing once EWS cycle green
(87.63 KB, patch)
2012-08-14 15:24 PDT
,
Adam Barth
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Adam Barth
Comment 1
2012-08-14 13:45:09 PDT
Created
attachment 158406
[details]
Patch
Build Bot
Comment 2
2012-08-14 14:02:09 PDT
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 3
2012-08-14 14:03:21 PDT
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 4
2012-08-14 14:08:45 PDT
Comment on
attachment 158406
[details]
Patch You borks it.
Adam Barth
Comment 5
2012-08-14 14:09:23 PDT
Yeah, I need to fight the EWS for a bit.
Adam Barth
Comment 6
2012-08-14 14:43:30 PDT
Created
attachment 158418
[details]
Patch
Eric Seidel (no email)
Comment 7
2012-08-14 14:55:49 PDT
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 8
2012-08-14 15:02:04 PDT
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
Comment 9
2012-08-14 15:23:01 PDT
Created
attachment 158422
[details]
Patch for landing
Adam Barth
Comment 10
2012-08-14 15:24:13 PDT
Created
attachment 158423
[details]
For landing once EWS cycle green
Adam Barth
Comment 11
2012-08-14 15:34:09 PDT
Committed
r125615
: <
http://trac.webkit.org/changeset/125615
>
Darin Adler
Comment 12
2012-08-14 17:04:50 PDT
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 13
2012-08-14 17:35:38 PDT
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug