Bug 146350 – Add support for image previews

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Beth Dakin]

Add support for image previews

rdar://problem/20640234

[Beth Dakin]

Created attachment 255647 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/WKImagePreviewViewController.mm:59:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/WKImagePreviewViewController.mm:83:  Extra space after (  [whitespace/parens] [2]
Total errors found: 2 in 7 files


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

[Tim Horton]

Comment on attachment 255647 [details]
Patch

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

> Source/WebKit2/UIProcess/WKImagePreviewViewController.h:32
> +@interface WKImagePreviewViewController : UIViewController {

no need for the curly braces

> Source/WebKit2/UIProcess/WKImagePreviewViewController.mm:53
> +    _imageView = [[UIImageView alloc] initWithFrame:CGRectZero];

This is leaking, no?

> Source/WebKit2/UIProcess/WKImagePreviewViewController.mm:54
> +    [_imageView setImage:[[UIImage alloc] initWithCGImage:_image.get()]];

This too, probably?

> Source/WebKit2/UIProcess/WKImagePreviewViewController.mm:56
> +    CGSize screenSize = [[[UIApplication sharedApplication] delegate] window].bounds.size;

More dots, less square brackets.

> Source/WebKit2/UIProcess/WKImagePreviewViewController.mm:70
> +    CGRect bounds = self.view.bounds;
> +    _imageView.frame = bounds;

No need for the temporary.

> Source/WebKit2/UIProcess/WKImagePreviewViewController.mm:73
> +static CGSize _scaleSizeWithinSize(CGSize source, CGSize destination)

Is this one of those GeometryUtilities (or whatever) things? If not, can it be?

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3146
> +        SFSafariViewController *previewViewController = [allocSFSafariViewControllerInstance() initWithURL:targetURL];

Seems likely that this leaks too?

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3157
> +        return [[WKImagePreviewViewController alloc] initWithCGImage:_positionInformation.image->makeCGImageCopy()];

And this?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2142
> +                            RefPtr<ShareableBitmap> sharedBitmap = ShareableBitmap::createShareable(IntSize(image->size()), ShareableBitmap::SupportsAlpha);

Please always null-check the result of ShareableBitmap::ctreateShareable.

[Beth Dakin]

Created attachment 255661 [details]
Patch

Patch that addresses Tim's feedback!

[Beth Dakin]

(In reply to comment #3)
> Comment on attachment 255647 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255647&action=review
> 
> > Source/WebKit2/UIProcess/WKImagePreviewViewController.h:32
> > +@interface WKImagePreviewViewController : UIViewController {
> 
> no need for the curly braces
> 
Fixed.

> > Source/WebKit2/UIProcess/WKImagePreviewViewController.mm:53
> > +    _imageView = [[UIImageView alloc] initWithFrame:CGRectZero];
> 
> This is leaking, no?
> 

Fixed.

> > Source/WebKit2/UIProcess/WKImagePreviewViewController.mm:54
> > +    [_imageView setImage:[[UIImage alloc] initWithCGImage:_image.get()]];
> 
> This too, probably?
> 

Fixed.

> > Source/WebKit2/UIProcess/WKImagePreviewViewController.mm:56
> > +    CGSize screenSize = [[[UIApplication sharedApplication] delegate] window].bounds.size;
> 
> More dots, less square brackets.
> 

Fixed.

> > Source/WebKit2/UIProcess/WKImagePreviewViewController.mm:70
> > +    CGRect bounds = self.view.bounds;
> > +    _imageView.frame = bounds;
> 
> No need for the temporary.
> 

Fixed.

> > Source/WebKit2/UIProcess/WKImagePreviewViewController.mm:73
> > +static CGSize _scaleSizeWithinSize(CGSize source, CGSize destination)
> 
> Is this one of those GeometryUtilities (or whatever) things? If not, can it
> be?
> 

Sort of, but this will be easier to explain in person.

> > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3146
> > +        SFSafariViewController *previewViewController = [allocSFSafariViewControllerInstance() initWithURL:targetURL];
> 
> Seems likely that this leaks too?
> 

Fixed.

> > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3157
> > +        return [[WKImagePreviewViewController alloc] initWithCGImage:_positionInformation.image->makeCGImageCopy()];
> 
> And this?
> 

Fixed.

> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2142
> > +                            RefPtr<ShareableBitmap> sharedBitmap = ShareableBitmap::createShareable(IntSize(image->size()), ShareableBitmap::SupportsAlpha);
> 
> Please always null-check the result of ShareableBitmap::ctreateShareable.

Fixed.

Thanks Tim!

[Tim Horton]

Comment on attachment 255661 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3139
> +        NSURL *targetURL = [NSURL URLWithString:_positionInformation.url];

This probably should use _web_URLWithWTFString?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2142
> +                            if (RefPtr<ShareableBitmap> sharedBitmap = ShareableBitmap::createShareable(IntSize(image->size()), ShareableBitmap::SupportsAlpha)) {

Wonder if we should have a cap too (screen size?).

Also should consider what should happen with animated images.

[Beth Dakin]

Thanks Tim!

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

Comment on attachment 255661 [details]
Patch

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

> Source/WebKit2/UIProcess/WKImagePreviewViewController.mm:57
> +    CGSize screenSize = UIApplication.sharedApplication.delegate.window.bounds.size;

-window is an optional method in the UIApplicationDelegate protocol. Invoking it without checking if the delegate responds to it first could lead to a crash.

Comment on attachment 255661 [details]
Patch

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

>> Source/WebKit2/UIProcess/WKImagePreviewViewController.mm:57
>> +    CGSize screenSize = UIApplication.sharedApplication.delegate.window.bounds.size;
> 
> -window is an optional method in the UIApplicationDelegate protocol. Invoking it without checking if the delegate responds to it first could lead to a crash.

Also, -delegate itself may return nil. And also, we normally don’t use dot notation to invoke class methods, so this should begin [UIApplication sharedApplication]. It’s also misleading to call something that is the size of a window screenSize. If we want the size of a screen we can use UIScreen API to get it.

[Beth Dakin]

(In reply to comment #9)
> Comment on attachment 255661 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255661&action=review
> 
> >> Source/WebKit2/UIProcess/WKImagePreviewViewController.mm:57
> >> +    CGSize screenSize = UIApplication.sharedApplication.delegate.window.bounds.size;
> > 
> > -window is an optional method in the UIApplicationDelegate protocol. Invoking it without checking if the delegate responds to it first could lead to a crash.
> 
> Also, -delegate itself may return nil. And also, we normally don’t use dot
> notation to invoke class methods, so this should begin [UIApplication
> sharedApplication]. It’s also misleading to call something that is the size
> of a window screenSize. If we want the size of a screen we can use UIScreen
> API to get it.

Thanks, Dan! http://trac.webkit.org/changeset/186019