Bug 95274 – REGRESSION: Not sending NPP_SetWindow is causing Flash to not throttle itself

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

REGRESSION:  Flash no longer sends throttle events

Many optimizations we've made recently in WebKit2 and the PluginProcess were to minimize the number of setWindow calls we make to the plug-in.

Flash uses an empty clip rect vs. a non-empty clip rect to decide when to send throttle events to the plug-in.

Whenever the clip rect *would* change between empty and non-empty we need to call setWindow.

A simple test plug-in to monitor throttle events can be found at http://inflagrantedelicto.memoryspiral.com/2012/06/as3-quickie-monitor-throttle-events/

In radar as <rdar://problem/12133021>

[Brady Eidson]

I plan to explore regression tests tomorrow A.M. but - for now - attaching the proposed fix.

[Brady Eidson]

Created attachment 161106 [details]
Proposed fix (no ChangeLog or tests yet)

Asking for review to vet the approach, even though it's known to be missing a ChangeLog and regression tests.

[Darin Adler]

Comment on attachment 161106 [details]
Proposed fix (no ChangeLog or tests yet)

View in context: https://bugs.webkit.org/attachment.cgi?id=161106&action=review

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.h:149
> +    void callSetWindow(bool visible = true);

No real reason to put this behavior into the same function and have the awkward construction of a function with a boolean with a default value. There could just be a new function that calls NPP_SetWindow making things invisible. There’s no logic shared between these two cases. Both could be called by separate names in NetscapePlugin::windowVisibilityChanged and it’d probably read more clearly that way.

> Source/WebKit2/WebProcess/Plugins/Netscape/mac/NetscapePluginMac.mm:917
> +void NetscapePlugin::windowVisibilityChanged(bool visible)

Seems a little strange that platformGeometryDidChange and platformVisibilityDidChange are not being touched here.

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:793
> +    // FIXME: This clip rect isn't correct.
> +    // But it is still important distinguish between empty and non-empty rects so we can notify the plug-in when
> +    // it becomes invisible.
> +    IntRect clipRect = clipRectInWindowCoordinates();
> +    if (!clipRect.isEmpty())
> +        clipRect = boundsRect();

I don’t see the part of the code that responds to this in geometryDidChange.

If only the boolean of whether the rect is empty is correct, then can we change the argument to be a boolean for clarity rather than passing a known-to-be-incorrect rect?

[Brady Eidson]

(In reply to comment #3)
> (From update of attachment 161106 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161106&action=review
> 
> > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.h:149
> > +    void callSetWindow(bool visible = true);
> 
> No real reason to put this behavior into the same function and have the awkward construction of a function with a boolean with a default value. There could just be a new function that calls NPP_SetWindow making things invisible. There’s no logic shared between these two cases. Both could be called by separate names in NetscapePlugin::windowVisibilityChanged and it’d probably read more clearly that way.

I think this is a great suggestion.

> > Source/WebKit2/WebProcess/Plugins/Netscape/mac/NetscapePluginMac.mm:917
> > +void NetscapePlugin::windowVisibilityChanged(bool visible)
> 
> Seems a little strange that platformGeometryDidChange and platformVisibilityDidChange are not being touched here.

platformGeometryDidChange() is for platform specific code that needs to run when the geometry changes.  But the change to call setWindow() at the right times is a cross-platform concern.

platformVisibilityDidChange() is about platform specific code that runs when visibilityDidChange() is called...  but it's important to not confuse "visibility changed" and "window visibility changed"

Window visibility is whether or not the window hosting the view is minimized or goes to a background tab, and that is the important case to capture here.

"visibility" is visibility in DOM API terms - such as "visibility = hidden" - and is not relevant to this patch.

> > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:793
> > +    // FIXME: This clip rect isn't correct.
> > +    // But it is still important distinguish between empty and non-empty rects so we can notify the plug-in when
> > +    // it becomes invisible.
> > +    IntRect clipRect = clipRectInWindowCoordinates();
> > +    if (!clipRect.isEmpty())
> > +        clipRect = boundsRect();
> 
> I don’t see the part of the code that responds to this in geometryDidChange.

The code that responds to this is in geometryDidChange is:

    if (pluginSize == m_pluginSize && m_clipRect == clipRect && m_pluginToRootViewTransform == pluginToRootViewTransform) {
        // Nothing to do.
        return;
    }

Currently, we always pass the incorrect clip rect of {0, 0, pluginWidth, pluginHeight}

With this patch we will pass two possible rects.  Either that incorrect clip rect if *any* of the plugin is visible, or a correct and empty clip rect if the plugin is entirely obscured.

Switching between the empty rect and the non-empty rect is what triggers the appropriate branch in geometryDidChange.

> If only the boolean of whether the rect is empty is correct, then can we change the argument to be a boolean for clarity rather than passing a known-to-be-incorrect rect?

It is true - as the FIXME expresses - that right now we're just flipping between an empty and non-empty clip rect.  But as the original FIXME suggests we do actually desire to always pass the correct clip rect and - in fact - plug-ins can be made more efficient if we pass the correct one along.  Simon and Anders both agree on this point.

I will file a followup bug to actually track that work.

[Brady Eidson]

I've made some changes based on Darin's feedback and am getting some tests up and running, so removing r?

Testing the scrolling behavior is easy... testing the window minimizing/background tabbing behavior is requiring quite a bit of harness infrastructute.

[Brady Eidson]

Created attachment 161289 [details]
Patch with test

[Sam Weinig]

Comment on attachment 161289 [details]
Patch with test

View in context: https://bugs.webkit.org/attachment.cgi?id=161289&action=review

> LayoutTests/platform/mac-wk2/plugins/npp-setwindow-called-on-scroll.html:41
> +	if (msg == windowWasSetExpectedResults[windowWasSetCount])
> +		testPassed("NPP_SetWindow called with expected parameters");
> +	else
> +		testFailed("NPP_SetWindow called with: " + msg + " but we expected " + windowWasSetExpectedResults[windowWasSetCount] + "... Maybe bug 95362 has been fixed?");
> +
> +	++windowWasSetCount;	
> +	if (windowWasSetCommands[windowWasSetCount])
> +		eval(windowWasSetCommands[windowWasSetCount]);
> +}

Tabs?

> LayoutTests/platform/mac-wk2/plugins/npp-setwindow-called-on-scroll.html:46
> +	if (window.testRunner)
> +		testRunner.notifyDone();

More tabs?

> Tools/DumpRenderTree/TestNetscapePlugIn/Tests/LogNPPSetWindow.cpp:2
> + * Copyright (C) 2011 Apple Inc. All rights reserved.

It's 2012 buddy.

[Brady Eidson]

http://trac.webkit.org/changeset/127047

[Darin Adler]

Comment on attachment 161289 [details]
Patch with test

View in context: https://bugs.webkit.org/attachment.cgi?id=161289&action=review

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:789
> +    // But it is still important distinguish between empty and non-empty rects so we can notify the plug-in when it becomes invisible.

missing word here: "important distinguish" should be "important to distinguish".