Bug 133109 – [iOS] WKPDFView should have a page indicator

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

<rdar://problem/16383003>

[Tim Horton]

Created attachment 231754 [details]
first cut

First cut, need to see how to make this interact well with banners/etc.

[Ian Henderson]

Comment on attachment 231754 [details]
first cut

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

> Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:128
> +    [UIView commitAnimations];

We should use the more modern block-based animation API here.

> Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:146
> +        [UIView commitAnimations];

We should use the more modern block-based animation API here.  Something like:

void (^animations)() = ^{ [self setFrameOrigin:point]; };
if (animated)
    [UIView animateWithDuration:indicatorMoveDuration animations:animations];
else
    animations();

> Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:159
> +    [self setSize:size];

Could we do this layout in layoutSubviews instead, and use the sizeThatFits: method to determine the size instead of overriding sizeToFit?

> Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:181
> +    RetainPtr<CGPathRef> cornerPath = adoptCF(CGPathCreateWithRoundedRect(cornerImageBounds, indicatorCornerRadius, indicatorCornerRadius, nil));

We need to use UIBezierPath here so the rounded rect corners have continuous curvature.

[Benjamin Poulain]

Comment on attachment 231754 [details]
first cut

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:499
> +    WebCore::Color color;
> +    if (_customContentView)
> +        color = [_customContentView backgroundColor].CGColor;
> +    else
> +        color = _page->pageExtendedBackgroundColor();

This is very suspicious.
_updateScrollViewBackground is supposed to be called when we got new tiles and when the zoom factor changes.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1527
> +    if (_customContentView)

You don't need to check for nullity for obj-c types.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1528
> +        [_customContentView web_setObscuredInsets:obscuredInsets];

Oh boy :)

> Source/WebKit2/UIProcess/Cocoa/WKWebViewContentProvider.h:37
> +struct UIEdgeInsets;

Not sure why that works. You pass the insets by value. How does the compiler knows the size of the structure...?
Maybe the Obj-C ABI always pass structs by address?

> Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.h:33
> +@property (nonatomic, assign) unsigned currentPageNumber;
> +@property (nonatomic, assign) unsigned pageCount;

I don't think we put "assign" on non Obj-C types?

> Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:120
> +        _timer = [NSTimer scheduledTimerWithTimeInterval:fadeOutDelay target:self selector:@selector(hide) userInfo:nil repeats:NO];

Can you use adoptNS([[NSTimer alloc] init) to avoid the autorelease pool?

> Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:128
> +    [UIView beginAnimations:nil];
> +    [UIView setAnimationDuration:indicatorFadeDuration];
> +    self.alpha = 0;
> +    [UIView commitAnimations];

Can't you just use an animation block here?

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:127
> +    [_scrollView.superview insertSubview:_pageNumberIndicator.get() aboveSubview:self];

What the fuck?

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:150
> +        currentPage++;

++currentPage

[Tim Horton]

(In reply to comment #3)
> (From update of attachment 231754 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231754&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:499
> > +    WebCore::Color color;
> > +    if (_customContentView)
> > +        color = [_customContentView backgroundColor].CGColor;
> > +    else
> > +        color = _page->pageExtendedBackgroundColor();
> 
> This is very suspicious.
> _updateScrollViewBackground is supposed to be called when we got new tiles and when the zoom factor changes.

Not suspicious, just incomplete (as discussed).

> > Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:120
> > +        _timer = [NSTimer scheduledTimerWithTimeInterval:fadeOutDelay target:self selector:@selector(hide) userInfo:nil repeats:NO];
> 
> Can you use adoptNS([[NSTimer alloc] init) to avoid the autorelease pool?

I don’t think so. (see NSTimer.h)

> > Source/WebKit2/UIProcess/ios/WKPDFView.mm:127
> > +    [_scrollView.superview insertSubview:_pageNumberIndicator.get() aboveSubview:self];
> 
> What the fuck?

I’ll make this more explicit somehow.(In reply to comment #2)
> (From update of attachment 231754 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231754&action=review
> 
> > Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:128
> > +    [UIView commitAnimations];
> 
> We should use the more modern block-based animation API here.

Nice!

> > Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:159
> > +    [self setSize:size];
> 
> Could we do this layout in layoutSubviews instead, and use the sizeThatFits: method to determine the size instead of overriding sizeToFit?

I don’t think we need layoutSubviews, even. Much better!

> > Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:181
> > +    RetainPtr<CGPathRef> cornerPath = adoptCF(CGPathCreateWithRoundedRect(cornerImageBounds, indicatorCornerRadius, indicatorCornerRadius, nil));
> 
> We need to use UIBezierPath here so the rounded rect corners have continuous curvature.

OK.

[Tim Horton]

Created attachment 231812 [details]
take 2

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:127:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKPDFPageNumberIndicator.mm:137:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 10 files


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

[Tim Horton]

Created attachment 231833 [details]
webcore bits were missing

[Tim Horton]

Created attachment 231906 [details]
dan says no delegate

[Build Bot]

Comment on attachment 231906 [details]
dan says no delegate

Attachment 231906 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5878457263194112

New failing tests:
media/W3C/video/paused/paused_false_during_play.html

[Build Bot]

Created attachment 231911 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Anders Carlsson]

Comment on attachment 231906 [details]
dan says no delegate

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

Looks like the mac EWS bot is red.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1715
> +- (void)_setInsetForOverlaidAccessoryViews:(CGSize)offset

This method reads like it takes an array of views, maybe you can put "inset" at the end of the name?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1717
> +    [_customContentView web_setInsetForOverlaidAccessoryViews:offset];

Same here.

> Source/WebKit2/UIProcess/Cocoa/WKWebViewContentProvider.h:48
>  - (void)web_setContentProviderData:(NSData *)data suggestedFilename:(NSString *)filename;
>  - (void)web_setMinimumSize:(CGSize)size;
>  - (void)web_setScrollView:(UIScrollView *)scrollView;
> +- (void)web_setObscuredInsets:(UIEdgeInsets)insets;
> +- (void)web_setInsetForOverlaidAccessoryViews:(CGSize)size;
> +- (void)web_setFixedOverlayView:(UIView *)fixedOverlayView;

Why do we use web_ instead of wk_ here :(

> 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.

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

= nullptr.

[Tim Horton]

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

[Darin Adler]

Comment on attachment 231906 [details]
dan says no delegate

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?

[Darin Adler]

Heh, looks like it was a classic “review after patch was landed” situation.

[Tim Horton]

(In reply to comment #14)
> Heh, looks like it was a classic “review after patch was landed” situation.

Whoops, yes, I forgot to clear it after getting an IRC r+; sorry!

(In reply to comment #13)
> (From update of attachment 231906 [details])
> 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?

Is that something we're doing in WebKit code? I didn't realize that!

> > 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?

Yes, in this case, the new size does not depend on the old size at all.

> > Source/WebKit2/UIProcess/ios/WKPDFView.mm:80
> > +    [_pageNumberIndicator removeFromSuperview];
> 
> Is there no other shutdown code path besides dealloc?

Do you mean e.g. when the view is unparented? I suppose that's a good point, though at the moment we expect custom content providers to be totally owned by WKWebView and tossed when they're unparented.

[Anders Carlsson]

(In reply to comment #15)
> (In reply to comment #14)
> > Couldn’t we use ARC instead of RetainPtr?
> 
> Is that something we're doing in WebKit code? I didn't realize that!
> 

I don't think we should use ARC in WebKit code, here are a couple of reasons:

1. Since we need to support 32-bit on Mac, we can never fully adopt ARC until we drop 32-bit support which I don't think willl happen anytime soon.
2. I don't think we should use ARC at all unless we can use ARC everywhere.
3. With C++ smart pointers, such as RetainPtr and ObjCWeakPtr, ARC doesn't really give us a huge benefit.
4. ARC was mostly intended for applications, not frameworks.

[Darin Adler]

Comment on attachment 231906 [details]
dan says no delegate

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

>>> Source/WebKit2/UIProcess/ios/WKPDFView.mm:80
>>> +    [_pageNumberIndicator removeFromSuperview];
>> 
>> Is there no other shutdown code path besides dealloc?
> 
> Do you mean e.g. when the view is unparented? I suppose that's a good point, though at the moment we expect custom content providers to be totally owned by WKWebView and tossed when they're unparented.

With any reference counted object, even a view, it’s dangerous to assume nobody is holding onto a reference. Anything that’s done at dealloc time can happen at a surprising time a bit later than expected. I think that applies even here.

[Darin Adler]

(In reply to comment #16)
> 1. Since we need to support 32-bit on Mac, we can never fully adopt ARC until we drop 32-bit support which I don't think willl happen anytime soon.
> 2. I don't think we should use ARC at all unless we can use ARC everywhere.

I think that should have been your item 1. Because the item 1 above doesn’t seem to affect iOS-only code like this until I read your rule 2.

> 3. With C++ smart pointers, such as RetainPtr and ObjCWeakPtr, ARC doesn't really give us a huge benefit.

I think there’s still a substantial benefit. The smart pointers work if you remember to use them.

> 4. ARC was mostly intended for applications, not frameworks.

Ah, I did not know that.