Bug 128831 – Add a way to efficiently thumbnail WKViews

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

[Tim Horton]

We should provide a way to create a view from a WKView which can borrow the WKView's live contents and render them in a second view.

We can then provide optimizations, e.g. painting at a lower resolution, reusing existing tiles, etc. given knowledge that we're thumbnailing.

The thumbnail view will not forward events, and WKView will disable event handling while thumbnailed. Also, changes to the size of the thumbnail view will not affect the layout of the content.

<rdar://problem/15669655>

[Tim Horton]

Created attachment 224246 [details]
patch

an initial cut. currently we don't re-use existing tiles when bringing in unparented views, because we can't tell if they've been purged without parenting them, and don't want to cause a sudden surge in backing store if many thumbnail views are made from unparented views at once, but this can be revisited.

[WebKit Commit Bot]

Attachment 224246 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/mac/WKViewInternal.h:108:  Missing spaces around =  [whitespace/operators] [4]
ERROR: Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.h:41:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 224247 [details]
rebase+style

[WebKit Commit Bot]

Attachment 224247 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/mac/WKViewInternal.h:108:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 224251 [details]
fix ML build + offline review comments

[WebKit Commit Bot]

Attachment 224251 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/mac/WKViewInternal.h:108:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 224251 [details]
fix ML build + offline review comments

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

> Source/WebKit2/UIProcess/API/Cocoa/WKThumbnailView.h:30
> +#import <Cocoa/Cocoa.h>

We shouldn’t import the Core Data headers here. AppKit will do.

> Source/WebKit2/UIProcess/API/Cocoa/WKThumbnailView.h:31
> +#import <WebKit2/WKDeclarationSpecifiers.h>

I don’t think this is necessary anymore.

> Source/WebKit2/UIProcess/API/Cocoa/WKThumbnailView.h:35
> +WK_API_CLASS @interface WKThumbnailView : NSView

WK_API_CLASS goes on its own line before the @interface line.

[Tim Horton]

Created attachment 224400 [details]
patch

[Tim Horton]

Created attachment 224402 [details]
rebase

[WebKit Commit Bot]

Attachment 224402 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/mac/WKViewInternal.h:108:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 224412 [details]
build fix

[WebKit Commit Bot]

Attachment 224412 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/mac/WKViewInternal.h:108:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 224412 [details]
build fix

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

> Source/WebKit2/UIProcess/API/Cocoa/WKThumbnailView.h:38
> +- (void)setScale:(double)scale;

This should be a property so that it’s possible to get it, not just set it. It should be a CGFloat.

> Source/WebKit2/UIProcess/API/Cocoa/WKThumbnailView.mm:46
> +@implementation WKThumbnailView
> +{

The brace normally goes on the same line with the class name.

> Source/WebKit2/UIProcess/API/Cocoa/WKThumbnailView.mm:49
> +    double _thumbnailScale;

If scale were a property, you’d get a _scale ivar for free.

> Source/WebKit2/UIProcess/API/Cocoa/WKThumbnailView.mm:54
> +    bool _originalMayStartMediaWhenInWindow;
> +    bool _originalSourceViewIsInWindow;
> +
> +    bool _shouldApplyThumbnailScale;

Objective-C booleans are normally BOOL.

> Source/WebKit2/UIProcess/API/Cocoa/WKThumbnailView.mm:60
> +    self = [super initWithFrame:frame];
> +    if (self) {

Please use the usual early return pattern if (!(self = [super initWithFrame:frame]))\n    return nil;

> Source/WebKit2/UIProcess/API/Cocoa/WKThumbnailView.mm:114
> +    self.layer.sublayers = layer ? @[layer] : @[];

Please add spaces inside the @[]s.

> Source/WebKit2/UIProcess/API/Cocoa/WKThumbnailView.mm:122
> +    return [self.layer.sublayers objectAtIndex:0];

You can use -firstObject.

> Source/WebKit2/UIProcess/API/mac/PageClientImpl.mm:189
> +    NSView *activeView = m_wkView._thumbnailView ? m_wkView._thumbnailView : m_wkView;
> +    NSWindow *activeViewWindow = activeView.window;

I’d factor this out into a helper function.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:2562
> +    return hostView.layer.sublayers[0];

Can use -firstObject.

> Source/WebKit2/UIProcess/API/mac/WKViewInternal.h:32
>  #import <wtf/Vector.h>
> +#import <wtf/RetainPtr.h>

R < V

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:295
> +    , m_preThumbnailPageScale(1)

Terrible name.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4190
> +{

Should we ASSERT_ARG(destinationScale, destinationScale > 0) here? Also, I’m not sure what the word destination signifies here. Normally the parameter name in a setter should be the property being set or some abbreviation thereof.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4227
> +}

This whole business of special-casing 1 seems highly dubious to me.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:1100
> +    WebCore::IntPoint m_preThumbnailScrollPosition;

Also terrible name.

> Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:574
> +    [m_hostingLayer setSublayers:layer ? [NSArray arrayWithObject:layer] : [NSArray array]];

Can’t use array literals here?

[Tim Horton]

Comment on attachment 224412 [details]
build fix

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4191
> +    if (destinationScale == m_thumbnailScale)

Dan wants some fixmees here (or fixes) about what happens if the page navigates or does layout.

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4227
>> +}
> 
> This whole business of special-casing 1 seems highly dubious to me.

Yup, we've sorted out that this is unnecessary.

[Tim Horton]

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