[webkit-reviews] review granted: [Bug 133109] [iOS] WKPDFView should have a page indicator : [Attachment 231906] dan says no delegate

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tim Horton <thorton at apple.com>'s
request for review:
Bug 133109: [iOS] WKPDFView should have a page indicator
https://bugs.webkit.org/show_bug.cgi?id=133109

Attachment 231906: dan says no delegate
https://bugs.webkit.org/attachment.cgi?id=231906&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=231906&action=review


> Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:55
> +    RetainPtr<UILabel> _label;
> +    RetainPtr<_UIBackdropView> _backdropView;
> +    RetainPtr<NSTimer> _timer;

Couldn’t we use ARC instead of RetainPtr?

> Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:75
> +    _label = adoptNS([[UILabel alloc] initWithFrame:CGRectZero]);

Isn’t this what [[UILabel alloc] init] does, too?

>> Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:122
>> +	    _timer = [NSTimer scheduledTimerWithTimeInterval:indicatorTimeout
target:self selector:@selector(hide) userInfo:nil repeats:NO];
> 
> Timer fired selectors should take a timer argument.

Could just use the block version to avoid that question.

>> Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:131
>> +	_timer.clear();
> 
> = nullptr.

= nil, you mean

> Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:154
> +- (CGSize)sizeThatFits:(CGSize)size
> +{
> +    CGSize labelSize = [_label sizeThatFits:[_label size]];
> +    labelSize.width += 2 * indicatorHorizontalPadding;
> +    labelSize.height += 2 * indicatorVerticalPadding;
> +    return labelSize;
> +}

The size argument is unused. Is that right?

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:80
> +    [_pageNumberIndicator removeFromSuperview];

Is there no other shutdown code path besides dealloc?