Bug 34661 – REGRESSION (Safari 4.0-> Safari 4.0.4) NPP_SetWindow no longer sets a clipRect of (0,0,0,0) when it becomes hidden

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Kevin Decker]

When a Core Animation plug-in moves to a background tab or becomes minimized/hidden offscreen, it's supposed to provide the plug-in with a zero'd out clipRect. This is critically important for for CPU usage/power saving as the plug-in can throttle the timers in this case.

<rdar://problem/7614067>

[Kevin Decker]

Created attachment 48258 [details]
Proposed fix that properly clips in-process and out-of-process Core Animation plug-ins

[Mark Rowe (bdash)]

Comment on attachment 48258 [details]
Proposed fix that properly clips in-process and out-of-process Core Animation plug-ins

> @@ -351,6 +333,13 @@
>  
>          window.clipRect.bottom = window.clipRect.top;
>          window.clipRect.left = window.clipRect.right;
> +        
> +        // Core Animation plug-ins need to be updated (with a 0,0,0,0 clipRect) when
> +        // moved to a background tab. We don't do this for Core Graphics plug-ins as
> +        // older versions of Flash have historical WebKit-specific code that isn't
> +        // compatible with this behavior.

This comment would be clearer if the parenthesized clause were part of the sentence.

> +    // Core Animation plug-ins need to be updated (with a 0,0,0,0 clipRect) when
> +    // moved to a background tab. We don't do this for Core Graphics plug-ins as
> +    // older versions of Flash have historical WebKit-specific code that isn't
> +    // compatible with this behavior.    

Same here.

> +- (BOOL)superviewsHaveSuperviews
> +{
> +    NSView *contentView = [[self window] contentView];
> +    NSView *view;
> +    for (view = self; view != nil; view = [view superview]) { 
> +        if (view == contentView) {
> +            return YES;
> +        }
> +    }
> +    return NO;
> +}

You should clean up the style of this while you’re moving it -- move the declaration of view in to the loop, remove the explicit comparison with nil, and the extra braces.

> +- (BOOL)shouldClipOutPlugin
> +{
> +    NSWindow *window = [self window];
> +    return window == nil || [window isMiniaturized] || [NSApp isHidden] || ![self superviewsHaveSuperviews] || [self isHiddenOrHasHiddenAncestor];
> +}

Same here with the explicit comparison with nil.

r=me

[Darin Adler]

Comment on attachment 48258 [details]
Proposed fix that properly clips in-process and out-of-process Core Animation plug-ins

> +- (BOOL)superviewsHaveSuperviews
> +{
> +    NSView *contentView = [[self window] contentView];
> +    NSView *view;
> +    for (view = self; view != nil; view = [view superview]) { 
> +        if (view == contentView) {
> +            return YES;
> +        }
> +    }
> +    return NO;
> +}
> +
> +- (BOOL)shouldClipOutPlugin
> +{
> +    NSWindow *window = [self window];
> +    return window == nil || [window isMiniaturized] || [NSApp isHidden] || ![self superviewsHaveSuperviews] || [self isHiddenOrHasHiddenAncestor];

WebKit style for this is "!window" rather than "window == nil". But I know you just moved this.

Also, WebKit style does not put braces around things like "return YES".

Also, superviewsHaveSuperviews is a strangely named method you moved here. What the code implements is a check of whether this view is inside the contentView. It could be done more easily with -[NSView isDescendantOf:] -- we would not have to write a loop at all.

Patch seems fine, r=me too

[Kevin Decker]

Committed revision 54447.