Uploading an animated GIF from the photo library uploads a JPEG version of the image. To reproduce: 1. Go to <http://i.imgur.com/I8HyAQD.gif> 2. Long-press on the image, and select “Save Image” 3. Go to <http://imgur.com> and tap “upload” then “device” 4. Select the animated GIF from step 1, and select “start upload” Image uploaded is a single frame JPEG. <rdar://problem/19760877>
Created attachment 254100 [details] Patch
Attachment 254100 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:838: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 254100 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254100&action=review Overall looks good to me. I had some small comments / questions. Ultimately needs a WK2 owner. > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:786 > + // FIXME: Should we get the photo asset and get the actual filename for the photo instead of > + // naming each of the individual uploads image.jpg? This won't work for photos taken with > + // the camera, but would work for photos picked from the library. See <rdar://problem/12353008>. Can we do this now in the asset URL path? > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:792 > +static NSString *uploadImageNameFromUTI(NSString *uti) This seems easy to trip up, if there is any UTI photo library supports that we don't support. We should be able to go from a UTI directly to a file extension with UTTypeCopyPreferredTagWithClass: https://developer.apple.com/library/ios/documentation/MobileCoreServices/Reference/UTTypeRef/#//apple_ref/c/func/UTTypeCopyPreferredTagWithClass Something like: CFStringRef extension = UTTypeCopyPreferredTagWithClass((CFStringRef)uti, kUTTagClassFilenameExtension); > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:829 > + PHFetchResult *result = [getPHAssetClass() fetchAssetsWithALAssetURLs:@[assetURL] options:nil]; > + if (!result.count) { > + LOG_ERROR("WKFileUploadPanel: Failed to fetch asset with URL %@", assetURL); > + [self _uploadItemForJPEGRepresentationOfImage:image successBlock:successBlock failureBlock:failureBlock]; > + return; > + } Should this fetch be done on the background queue as well, could it take a while? > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:846 > + PHImageManager *manager = (PHImageManager *)[getPHImageManagerClass() defaultManager]; > + [manager requestImageDataForAsset:result[0] options:options.get() resultHandler:^(NSData *imageData, NSString *dataUTI, UIImageOrientation, NSDictionary *) { > + if (!imageData) { > + LOG_ERROR("WKFileUploadPanel: Failed to request image data for asset with URL %@", assetURL); > + [self _uploadItemForJPEGRepresentationOfImage:image successBlock:successBlock failureBlock:failureBlock]; > + return; > + } > + NSString *imageName = uploadImageNameFromUTI(dataUTI); > + [self _uploadItemForImageData:imageData originalImage:image imageName:imageName successBlock:successBlock failureBlock:failureBlock]; > + }]; In this case we don't do compressions (probably necessary). Do images of these types retain geolocation metadata? The JPEG case ends up not including the metadata. > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:888 > + // If we have an asset URL, try to upload the native image. > + NSURL *referenceURL = [info objectForKey:UIImagePickerControllerReferenceURL]; > + if (referenceURL) { > + [self _uploadItemForImage:originalImage withAssetURL:referenceURL successBlock:successBlock failureBlock:failureBlock]; > return; > } As from the previous comment. Does this referenceURL contain a better name that we can use instead of "image.ext"? For instance, maybe we can get the actual name "IMG_3373.PNG"? > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:891 > + // Photos taken with the camera will not have an asset URL. Fall back to a JPEG representation. > + [self _uploadItemForJPEGRepresentationOfImage:originalImage successBlock:successBlock failureBlock:failureBlock]; Ultimately it sounds like we will still need a generic name.
(In reply to comment #3) > Comment on attachment 254100 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254100&action=review > > Overall looks good to me. I had some small comments / questions. Ultimately > needs a WK2 owner. > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:786 > > + // FIXME: Should we get the photo asset and get the actual filename for the photo instead of > > + // naming each of the individual uploads image.jpg? This won't work for photos taken with > > + // the camera, but would work for photos picked from the library. > > See <rdar://problem/12353008>. Can we do this now in the asset URL path? There’s SPI for this, but no API. I’ll look into this. > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:792 > > +static NSString *uploadImageNameFromUTI(NSString *uti) > > This seems easy to trip up, if there is any UTI photo library supports that > we don't support. We should be able to go from a UTI directly to a file > extension with UTTypeCopyPreferredTagWithClass: > https://developer.apple.com/library/ios/documentation/MobileCoreServices/ > Reference/UTTypeRef/#//apple_ref/c/func/UTTypeCopyPreferredTagWithClass > > Something like: > > CFStringRef extension = > UTTypeCopyPreferredTagWithClass((CFStringRef)uti, > kUTTagClassFilenameExtension); Fixed. > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:829 > > + PHFetchResult *result = [getPHAssetClass() fetchAssetsWithALAssetURLs:@[assetURL] options:nil]; > > + if (!result.count) { > > + LOG_ERROR("WKFileUploadPanel: Failed to fetch asset with URL %@", assetURL); > > + [self _uploadItemForJPEGRepresentationOfImage:image successBlock:successBlock failureBlock:failureBlock]; > > + return; > > + } > > Should this fetch be done on the background queue as well, could it take a > while? I don’t think so, but there’s no particular reason not to move it into the block as well. Moved. > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:846 > > + PHImageManager *manager = (PHImageManager *)[getPHImageManagerClass() defaultManager]; > > + [manager requestImageDataForAsset:result[0] options:options.get() resultHandler:^(NSData *imageData, NSString *dataUTI, UIImageOrientation, NSDictionary *) { > > + if (!imageData) { > > + LOG_ERROR("WKFileUploadPanel: Failed to request image data for asset with URL %@", assetURL); > > + [self _uploadItemForJPEGRepresentationOfImage:image successBlock:successBlock failureBlock:failureBlock]; > > + return; > > + } > > + NSString *imageName = uploadImageNameFromUTI(dataUTI); > > + [self _uploadItemForImageData:imageData originalImage:image imageName:imageName successBlock:successBlock failureBlock:failureBlock]; > > + }]; > > In this case we don't do compressions (probably necessary). Do images of > these types retain geolocation metadata? The JPEG case ends up not including > the metadata. Yes, all Exif data for the image is retained including GPS data. > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:888 > > + // If we have an asset URL, try to upload the native image. > > + NSURL *referenceURL = [info objectForKey:UIImagePickerControllerReferenceURL]; > > + if (referenceURL) { > > + [self _uploadItemForImage:originalImage withAssetURL:referenceURL successBlock:successBlock failureBlock:failureBlock]; > > return; > > } > > As from the previous comment. Does this referenceURL contain a better name > that we can use instead of "image.ext"? For instance, maybe we can get the > actual name "IMG_3373.PNG”? No, it contains only the file extension and a UUID. > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:891 > > + // Photos taken with the camera will not have an asset URL. Fall back to a JPEG representation. > > + [self _uploadItemForJPEGRepresentationOfImage:originalImage successBlock:successBlock failureBlock:failureBlock]; > > Ultimately it sounds like we will still need a generic name. Yeah. I’m not sure that IMG_0011 is any less generic than “image” though. Thanks for looking at this!
Comment on attachment 254100 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254100&action=review r=me given that Joe has also looked at it. > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:65 > +SOFT_LINK_CONSTANT(Photos, PHImageRequestOptionsVersionCurrent, NSString *); > +SOFT_LINK_CONSTANT(Photos, PHImageRequestOptionsResizeModeNone, NSString *); V comes after R. > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:789 > + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ > + // FIXME: Different compression for different devices? > + // FIXME: Different compression for different UIImage sizes? > + // FIXME: Should EXIF data be maintained? > + const CGFloat compression = 0.8; > + NSData *jpeg = UIImageJPEGRepresentation(image, compression); > + if (!jpeg) { > + LOG_ERROR("WKFileUploadPanel: Failed to create JPEG representation for image"); > + failureBlock(); > + return; > + } > + > + // FIXME: Should we get the photo asset and get the actual filename for the photo instead of > + // naming each of the individual uploads image.jpg? This won't work for photos taken with > + // the camera, but would work for photos picked from the library. > + NSString * const kUploadImageName = @"image.jpg"; > + [self _uploadItemForImageData:jpeg originalImage:image imageName:kUploadImageName successBlock:successBlock failureBlock:failureBlock]; > + }); You should use a C++ lambda instead of a block, since the lambda requires you to list the variables you capture. Not at all new to this code, and I'm not asking you to fix this right now, but I don't like that it's the responsibility of the caller of -_processMediaInfoDictionaries:successBlock:failureBlock: to dispatch back to the main thread in their success/failure blocks. I think the fact that we dispatch to a global concurrent queue should be kept an implementation detail, and the API should make the guarantee that blocks are called on the main thread. > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:847 > + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ > + RetainPtr<PHImageRequestOptions> options = adoptNS([allocPHImageRequestOptionsInstance() init]); > + [options setVersion:PHImageRequestOptionsVersionCurrent]; > + [options setSynchronous:YES]; > + [options setResizeMode:PHImageRequestOptionsResizeModeNone]; > + > + PHImageManager *manager = (PHImageManager *)[getPHImageManagerClass() defaultManager]; > + [manager requestImageDataForAsset:result[0] options:options.get() resultHandler:^(NSData *imageData, NSString *dataUTI, UIImageOrientation, NSDictionary *) { > + if (!imageData) { > + LOG_ERROR("WKFileUploadPanel: Failed to request image data for asset with URL %@", assetURL); > + [self _uploadItemForJPEGRepresentationOfImage:image successBlock:successBlock failureBlock:failureBlock]; > + return; > + } > + NSString *imageName = uploadImageNameFromUTI(dataUTI); > + [self _uploadItemForImageData:imageData originalImage:image imageName:imageName successBlock:successBlock failureBlock:failureBlock]; > + }]; > + }); Same comment about C++ lambdas instead of blocks, both for the block passed to dispatch_async() and the one passed to -requestImageDataForAsset:options:resultHandler:.
(In reply to comment #5) > Comment on attachment 254100 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254100&action=review > > r=me given that Joe has also looked at it. > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:65 > > +SOFT_LINK_CONSTANT(Photos, PHImageRequestOptionsVersionCurrent, NSString *); > > +SOFT_LINK_CONSTANT(Photos, PHImageRequestOptionsResizeModeNone, NSString *); > > V comes after R. Fixed. > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:789 > > + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ > > + // FIXME: Different compression for different devices? > > + // FIXME: Different compression for different UIImage sizes? > > + // FIXME: Should EXIF data be maintained? > > + const CGFloat compression = 0.8; > > + NSData *jpeg = UIImageJPEGRepresentation(image, compression); > > + if (!jpeg) { > > + LOG_ERROR("WKFileUploadPanel: Failed to create JPEG representation for image"); > > + failureBlock(); > > + return; > > + } > > + > > + // FIXME: Should we get the photo asset and get the actual filename for the photo instead of > > + // naming each of the individual uploads image.jpg? This won't work for photos taken with > > + // the camera, but would work for photos picked from the library. > > + NSString * const kUploadImageName = @"image.jpg"; > > + [self _uploadItemForImageData:jpeg originalImage:image imageName:kUploadImageName successBlock:successBlock failureBlock:failureBlock]; > > + }); > > You should use a C++ lambda instead of a block, since the lambda requires > you to list the variables you capture. Any and I discussed this in person. I’m going to use blocks here, because the implicit behavior of having Obj-C types retained when captured is useful and makes the code more readable. > > Not at all new to this code, and I'm not asking you to fix this right now, > but I don't like that it's the responsibility of the caller of > -_processMediaInfoDictionaries:successBlock:failureBlock: to dispatch back > to the main thread in their success/failure blocks. I think the fact that we > dispatch to a global concurrent queue should be kept an implementation > detail, and the API should make the guarantee that blocks are called on the > main thread. I agree. I’ll file a bug. > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:847 > > + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ > > + RetainPtr<PHImageRequestOptions> options = adoptNS([allocPHImageRequestOptionsInstance() init]); > > + [options setVersion:PHImageRequestOptionsVersionCurrent]; > > + [options setSynchronous:YES]; > > + [options setResizeMode:PHImageRequestOptionsResizeModeNone]; > > + > > + PHImageManager *manager = (PHImageManager *)[getPHImageManagerClass() defaultManager]; > > + [manager requestImageDataForAsset:result[0] options:options.get() resultHandler:^(NSData *imageData, NSString *dataUTI, UIImageOrientation, NSDictionary *) { > > + if (!imageData) { > > + LOG_ERROR("WKFileUploadPanel: Failed to request image data for asset with URL %@", assetURL); > > + [self _uploadItemForJPEGRepresentationOfImage:image successBlock:successBlock failureBlock:failureBlock]; > > + return; > > + } > > + NSString *imageName = uploadImageNameFromUTI(dataUTI); > > + [self _uploadItemForImageData:imageData originalImage:image imageName:imageName successBlock:successBlock failureBlock:failureBlock]; > > + }]; > > + }); > > Same comment about C++ lambdas instead of blocks, both for the block passed > to dispatch_async() and the one passed to > -requestImageDataForAsset:options:resultHandler:. Same comment. Thanks for the review! Landed in <http://trac.webkit.org/changeset/185241>.