| Summary: | Add support for image previews | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Beth Dakin <bdakin> | ||||||
| Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | andersca, bdakin, commit-queue, enrica, mitz, sam, thorton, yongjun_zhang | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Beth Dakin
2015-06-26 10:47:09 PDT
Created attachment 255647 [details]
Patch
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 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. Created attachment 255661 [details]
Patch
Patch that addresses Tim's feedback!
(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 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. 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. (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 |