[Webkit-unassigned] [Bug 34382] When live iframe element is moved between pages, it still depends on old page.

Mon Feb 1 16:56:57 PST 2010

https://bugs.webkit.org/show_bug.cgi?id=34382

David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #47781|review?                     |review+
               Flag|                            |

--- Comment #4 from David Levin <levin at chromium.org>  2010-02-01 16:56:56 PST ---
(From update of attachment 47781)
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();
> +}
> +

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.