[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