<rdar://problem/16383003>
Created attachment 231754 [details] first cut First cut, need to see how to make this interact well with banners/etc.
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.
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
(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.
Created attachment 231812 [details] take 2
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.
Created attachment 231833 [details] webcore bits were missing
Created attachment 231906 [details] dan says no delegate
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
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
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.
http://trac.webkit.org/changeset/169290
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?
Heh, looks like it was a classic “review after patch was landed” situation.
(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.
(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.
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.
(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.