[webkit-reviews] review granted: [Bug 128831] Add a way to efficiently thumbnail WKViews : [Attachment 224412] build fix

[bugzilla-daemon at webkit.org]

mitz at webkit.org <mitz at webkit.org> has granted Tim Horton <thorton at apple.com>'s
request for review:
Bug 128831: Add a way to efficiently thumbnail WKViews
https://bugs.webkit.org/show_bug.cgi?id=128831

Attachment 224412: build fix
https://bugs.webkit.org/attachment.cgi?id=224412&action=review

------- Additional Comments from mitz at webkit.org <mitz at webkit.org>
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
> + at 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?