[webkit-reviews] review granted: [Bug 34382] When live iframe element is moved between pages, it still depends on old page. : [Attachment 47781] Updated patch

Mon Feb 1 16:56:55 PST 2010

David Levin <levin at chromium.org> has granted Dmitry Titov
<dimich at chromium.org>'s request for review:
Bug 34382: When live iframe element is moved between pages, it still depends on
old page.
https://bugs.webkit.org/show_bug.cgi?id=34382

Attachment 47781: Updated patch
https://bugs.webkit.org/attachment.cgi?id=47781&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Consider addressing the items below before landing.

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> +2010-01-30  Dmitry Titov  <dimich at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   When live iframe element is moved between pages, it still depends on
old page.

s/When live/When a live/
s/on old/on the old/

> diff --git a/LayoutTests/fast/frames/iframe-reparenting-new-page.html
b/LayoutTests/fast/frames/iframe-reparenting-new-page.html

It looks like the test could easily be in the script only form which seems
preferred when possible.

> +<html>
> +<script>
...

> +<body onload="test()">
> +The test verifies that loaded iframe may be passed between different pages
without stopping firing timer events. It used window.open to open 'page 1'
which has iframe in it. It then passes iframe to 'page 2' and closes window of
'page 1'. The test verifies that the timer in iframe that is initialized when
iframe is loaded continues firing after iframe is re-parented and original
window was closed. If test passes, you'll see a few log entries and then
"FINISH".

This test verifies that the loaded iframe may be passed between different pages
without stopping firing timer events. It used window.open to open 'page 1'
which has an iframe in it. It then passes the iframe to 'page 2' and closes the
window of 'page 1'. 

This test verifies that the timer in an iframe that is initialized when the
iframe is loaded continues firing after the iframe is re-parented and the
original window was closed. If test passes, you'll see a few log entries and
then "FINISH".

The second "This test verifies" seems like overkill. I would delete all the
text before that.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2010-01-30  Dmitry Titov  <dimich at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   When live iframe element is moved between pages, it still depends on
old page.

When a live... it still depends on the old page.

> +	   https://bugs.webkit.org/show_bug.cgi?id=34382
> +
> +	   Test: fast/frames/iframe-reparenting-new-page.html
> +
> +	   * html/HTMLFrameElementBase.cpp:
> +	   (WebCore::HTMLFrameElementBase::setName): Move the code setting the
frame name into a separate function.
> +	   (WebCore::HTMLFrameElementBase::setNameAndOpenURL):
> +	   (WebCore::HTMLFrameElementBase::updateLiveFrame): Update frame tree,
reset page in the contentFrame and re-set the name.
> +	   (WebCore::HTMLFrameElementBase::insertedIntoDocument): 
> +	   * html/HTMLFrameElementBase.h:
> +	   * page/Frame.cpp:
> +	   (WebCore::Frame::setPage):
> +	   * page/Frame.h: Add setPage method. It is only currently used when
alive iframe is moved between pages (so the existing Frame should get a new
Page)

Please add a period. Also, I think ChangeLog entries typically line wrap at
~80-90 columns (unlike nearly all other files).

> diff --git a/WebCore/html/HTMLFrameElementBase.cpp
b/WebCore/html/HTMLFrameElementBase.cpp

> +// Used when live frame is moved in DOM, potentially to another page.

Is the term "live frame" used elsewhere? What does it mean?

> diff --git a/WebCore/page/Frame.cpp b/WebCore/page/Frame.cpp
> +void Frame::setPage(Page* page)
> +{

Shouldn't this ASSERT(ownerElement) ?

> +    if (m_page == page)
> +	   return;
> +
> +    if (m_page)
> +	   m_page->decrementFrameCount();
> +
> +    m_page = page;
> +
> +    if (page)
> +	   page->incrementFrameCount();
> +}
> +