Summary: | Add a way to efficiently thumbnail WKViews | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||||||||||
Component: | WebKit2 | Assignee: | Tim Horton <thorton> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | andersca, commit-queue, mitz, sam, simon.fraser | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Tim Horton
2014-02-14 11:16:57 PST
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.
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.
Created attachment 224247 [details]
rebase+style
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.
Created attachment 224251 [details]
fix ML build + offline review comments
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. Created attachment 224400 [details]
patch
Created attachment 224402 [details]
rebase
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.
Created attachment 224412 [details]
build fix
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? 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. |