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
Summary: REGRESSION (Safari 4.0-> Safari 4.0.4) NPP_SetWindow no longer sets a clipRec...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P1 Critical
Assignee: Kevin Decker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-05 14:06 PST by Kevin Decker
Modified: 2010-02-05 14:56 PST (History)
0 users

See Also:


Attachments
Proposed fix that properly clips in-process and out-of-process Core Animation plug-ins (6.60 KB, patch)
2010-02-05 14:14 PST, Kevin Decker
mrowe: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Decker 2010-02-05 14:06:02 PST
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>
Comment 1 Kevin Decker 2010-02-05 14:14:58 PST
Created attachment 48258 [details]
Proposed fix that properly clips in-process and out-of-process Core Animation plug-ins
Comment 2 Mark Rowe (bdash) 2010-02-05 14:50:05 PST
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
Comment 3 Darin Adler 2010-02-05 14:50:41 PST
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
Comment 4 Kevin Decker 2010-02-05 14:56:24 PST
Committed revision 54447.