Bug 146350 - Add support for image previews
Summary: Add support for image previews
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-26 10:47 PDT by Beth Dakin
Modified: 2015-06-26 16:51 PDT (History)
8 users (show)

See Also:


Attachments
Patch (19.35 KB, patch)
2015-06-26 10:49 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (19.45 KB, patch)
2015-06-26 12:35 PDT, Beth Dakin
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2015-06-26 10:47:09 PDT
Add support for image previews

rdar://problem/20640234
Comment 1 Beth Dakin 2015-06-26 10:49:57 PDT
Created attachment 255647 [details]
Patch
Comment 2 WebKit Commit Bot 2015-06-26 10:52:25 PDT
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.
Comment 3 Tim Horton 2015-06-26 10:58:29 PDT
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.
Comment 4 Beth Dakin 2015-06-26 12:35:42 PDT
Created attachment 255661 [details]
Patch

Patch that addresses Tim's feedback!
Comment 5 Beth Dakin 2015-06-26 12:37:01 PDT
(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!
Comment 6 Tim Horton 2015-06-26 14:04:09 PDT
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.
Comment 7 Beth Dakin 2015-06-26 14:25:15 PDT
Thanks Tim!

http://trac.webkit.org/changeset/186008
Comment 8 mitz 2015-06-26 15:02:19 PDT
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 9 mitz 2015-06-26 15:46:33 PDT
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.
Comment 10 Beth Dakin 2015-06-26 16:51:51 PDT
(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