[webkit-reviews] review denied: [Bug 45770] When a live frame is moved between pages, some plug-in DOM methods cease to function : [Attachment 77913] updated patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jan 6 12:06:54 PST 2011


Dmitry Titov <dimich at chromium.org> has denied Jenn Braithwaite
<jennb at chromium.org>'s request for review:
Bug 45770: When a live frame is moved between pages, some plug-in DOM methods
cease to function
https://bugs.webkit.org/show_bug.cgi?id=45770

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

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=77913&action=review

Few comments (r- to add description to the test)

> LayoutTests/fast/frames/iframe-reparenting-plugins.html:55
> +<body onload="test()">

It would be nice to have a short text description of the test here - what it
does, what constitutes the failure and success.
Also, why we always expect to find a plugin there.

> WebCore/page/Frame.h:72
> +    class FrameObserver {

If this is only used to get a callback on frame destruction, it could be more
specific and be called FrameDestructionObserver, with frameDestroyed() as a
name of its callback function. So that it doesn't get an assortment of various
callbacks in the future.

> WebCore/plugins/DOMMimeType.cpp:71
> +    if (!m_frame ||
!m_frame->loader()->subframeLoader()->allowPlugins(NotAboutToInstantiatePlugin)
)

How important is the change in behavior here? Lets say I have a page on
apple.com embedding an iframe from YouTube. Could it be possible that Flash is
disabled for apple.com but not YouTube, and while old behavior would disable
Flash in that combination (because it could use settings for apple.com), the
new one would not.

> WebCore/plugins/PluginData.cpp:38
> +    m_page = 0; // Page only needed by initPlugins if
USE(PLATFORM_STRATEGIES).

Is it possible to only have m_page if USE(PLATFORM_STRATEGIES)?


More information about the webkit-reviews mailing list