[webkit-reviews] review denied: [Bug 32848] Avoid reloading iframe on re-parenting between documents. : [Attachment 45416] Proposed patch, v2.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jan 22 12:32:11 PST 2010
David Levin <levin at chromium.org> has denied Dmitry Titov
<dimich at chromium.org>'s request for review:
Bug 32848: Avoid reloading iframe on re-parenting between documents.
https://bugs.webkit.org/show_bug.cgi?id=32848
Attachment 45416: Proposed patch, v2.
https://bugs.webkit.org/attachment.cgi?id=45416&action=review
------- Additional Comments from David Levin <levin at chromium.org>
So far I've focused on the WebCore code and not the test code but I'll look at
the test more soon. Just wanted to send this feedback along first.
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2009-12-22 Dmitry Titov <dimich at chromium.org>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Avoid reloading iframe on re-parenting between documents.
> + https://bugs.webkit.org/show_bug.cgi?id=32848
Nice to have a blank line here. (I always see that information like a title.)
> diff --git a/WebCore/dom/Document.cpp b/WebCore/dom/Document.cpp
> + if (source->hasTagName(iframeTag) && attached())
> +
static_cast<HTMLIFrameElement*>(source.get())->setKeepAliveOnRemovalFromTree(tr
ue);
Shouldn't this be:
if (source->hasTagName(iframeTag))
static_cast<HTMLIFrameElement*>(source.get())->setKeepAliveOnRemovalFromTree(at
tached());
This handles the case of a node being adopted by an attached frame and then
adopted by an unattached frame right after (maybe the test should be adjusted
to cover this case as well?).
> diff --git a/WebCore/html/HTMLFrameElementBase.cpp
b/WebCore/html/HTMLFrameElementBase.cpp
> + if (!keepAliveOnRemovalFromTree())
> +
queuePostAttachCallback(&HTMLFrameElementBase::setNameAndOpenURLCallback,
this);
> + setKeepAliveOnRemovalFromTree(false);
Why does this only get set to false inside of this if?
> diff --git a/WebCore/html/HTMLFrameOwnerElement.cpp
b/WebCore/html/HTMLFrameOwnerElement.cpp
> +void HTMLFrameOwnerElement::setKeepAliveOnRemovalFromTree(bool value)
> +{
> + m_keepAliveOnRemovalFromTree = value;
> +
> + // There is a possibility that JS will do document.adoptNode() on this
element but will not insert it into the tree.
> + // Check asynchronously and unload the frame if needed.
> + if (value)
> + m_unloadTimer.startOneShot(0);
m_unloadTimer -- the name bothers me because it really is about checking to see
if the Frame got attached.
> + else
> + m_unloadTimer.stop();
> +}
> +
> +void HTMLFrameOwnerElement::detachTimerFired(Timer<HTMLFrameOwnerElement>*)
> +{
> + if (attached())
> + return;
> + m_keepAliveOnRemovalFromTree = false;
It seems like this should be before the "if" to reset the value of
m_keepAliveOnRemovalFromTree.
> + willRemove();
> +}
> +
> HTMLFrameOwnerElement::~HTMLFrameOwnerElement()
> {
> if (m_contentFrame)
> diff --git a/WebCore/html/HTMLFrameOwnerElement.h
b/WebCore/html/HTMLFrameOwnerElement.h
> SandboxFlags sandboxFlags() const { return m_sandboxFlags; }
> -
> + void setKeepAliveOnRemovalFromTree(bool);
> +
> protected:
> HTMLFrameOwnerElement(const QualifiedName& tagName, Document*);
>
> void setSandboxFlags(SandboxFlags);
> -
> +
> + bool keepAliveOnRemovalFromTree() const { return
m_keepAliveOnRemovalFromTree; }
I've started considering how method names read when prefixed by the class name.
HTMLFrameOwnerElement keepAliveOnRemovalFromTree is ok, but
HTMLFrameOwnerElement remainsAliveWhenRemovedFromTree reads much better imo.
> Frame* m_contentFrame;
> SandboxFlags m_sandboxFlags;
> + Timer<HTMLFrameOwnerElement> m_unloadTimer;
> + bool m_keepAliveOnRemovalFromTree;
Consider moving the m_keepAliveOnRemovalFromTree to be right after the
m_sandboxFlags (which is an int to help avoid possible padding holes on 64bit).
Actually I'd recommend moving this stuff into
WebCore/html/HTMLFrameElementBase.cpp if possible (and moving the bool by other
bool's in there).
More information about the webkit-reviews
mailing list