Use mime type for image extension

Use mime type for image extension

This CL uses the mime type to determine the best image extension when saving
an image.

BUG=690355
Review-Url: https://codereview.chromium.org/2687873002
Cr-Commit-Position: refs/heads/master@{#449365}
Committed: https://chromium.googlesource.com/chromium/src/+/9efce7a0be559aa13c499f4400c2f2c3f66157c6

[gambard, 2017-02-09 12:19:26]

gambard@chromium.org changed reviewers:
+ marq@chromium.org, treib@chromium.org

[gambard, 2017-02-09 12:19:26]

PTAL.

[Marc Treib, 2017-02-09 12:34:45]

https://codereview.chromium.org/2687873002/diff/1/components/image_fetcher/im...
File components/image_fetcher/image_data_fetcher.cc (right):

https://codereview.chromium.org/2687873002/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher.cc:82: if
(source->GetResponseHeaders())
nit: braces please

https://codereview.chromium.org/2687873002/diff/1/components/image_fetcher/im...
File components/image_fetcher/image_data_fetcher.h (right):

https://codereview.chromium.org/2687873002/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher.h:31: const std::string&
mime_type)>;
Your other pending CL adds a response_code parameter here; this is another one.
It's not good that all clients have to be updated every time we add a new piece
of metadata here. How about wrapping all the metadata in a struct? Then existing
clients don't have to care about any added fields.

https://codereview.chromium.org/2687873002/diff/1/components/image_fetcher/im...
File components/image_fetcher/image_data_fetcher_unittest.cc (right):

https://codereview.chromium.org/2687873002/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher_unittest.cc:74: std::string
rawHeader =
raw_header

[gambard, 2017-02-09 13:44:12]

Thanks, PTAL!

https://codereview.chromium.org/2687873002/diff/1/components/image_fetcher/im...
File components/image_fetcher/image_data_fetcher.cc (right):

https://codereview.chromium.org/2687873002/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher.cc:82: if
(source->GetResponseHeaders())
On 2017/02/09 12:34:45, Marc Treib wrote:
> nit: braces please

Done.

https://codereview.chromium.org/2687873002/diff/1/components/image_fetcher/im...
File components/image_fetcher/image_data_fetcher.h (right):

https://codereview.chromium.org/2687873002/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher.h:31: const std::string&
mime_type)>;
On 2017/02/09 12:34:45, Marc Treib wrote:
> Your other pending CL adds a response_code parameter here; this is another
one.
> It's not good that all clients have to be updated every time we add a new
piece
> of metadata here. How about wrapping all the metadata in a struct? Then
existing
> clients don't have to care about any added fields.

Good idea!

https://codereview.chromium.org/2687873002/diff/1/components/image_fetcher/im...
File components/image_fetcher/image_data_fetcher_unittest.cc (right):

https://codereview.chromium.org/2687873002/diff/1/components/image_fetcher/im...
components/image_fetcher/image_data_fetcher_unittest.cc:74: std::string
rawHeader =
On 2017/02/09 12:34:45, Marc Treib wrote:
> raw_header

Done.

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
File components/image_fetcher/image_data_fetcher.h (right):

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
components/image_fetcher/image_data_fetcher.h:27: struct RequestMetadata {
I don't know if this should be inside the class?

[Marc Treib, 2017-02-09 13:49:48]

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
File components/image_fetcher/image_data_fetcher.cc (right):

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
components/image_fetcher/image_data_fetcher.cc:83:
source->GetResponseHeaders()->GetMimeType(&mime_type);
You could read directly into &metadata.mime_type

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
File components/image_fetcher/image_data_fetcher.h (right):

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
components/image_fetcher/image_data_fetcher.h:27: struct RequestMetadata {
On 2017/02/09 13:44:12, gambard wrote:
> I don't know if this should be inside the class?

That's mostly a matter of taste IMO. I tend to slightly prefer the nested class
approach, but I don't feel very strongly.

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
components/image_fetcher/image_data_fetcher.h:30: bool operator==(const
RequestMetadata& rhs) const {
Are the equality operators required?

[sdefresne, 2017-02-09 13:55:28]

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
File components/image_fetcher/image_data_fetcher.h (right):

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
components/image_fetcher/image_data_fetcher.h:27: struct RequestMetadata {
On 2017/02/09 13:49:48, Marc Treib wrote:
> On 2017/02/09 13:44:12, gambard wrote:
> > I don't know if this should be inside the class?
> 
> That's mostly a matter of taste IMO. I tend to slightly prefer the nested
class
> approach, but I don't feel very strongly.

drive-by: having the class separate make it easier to refactor code or break
dependencies as it allows you to 1. forward-declare it (it is not possible to
forward-declare nested classes outside of the outer class declaration), 2. move
it to a separate file (allowing you to just include the header for that class if
you do not care about the outer class).

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
components/image_fetcher/image_data_fetcher.h:30: bool operator==(const
RequestMetadata& rhs) const {
On 2017/02/09 13:49:48, Marc Treib wrote:
> Are the equality operators required?

Even if they are required, they generally do not need to be methods but can be
free functions instead:

struct RequestMetaData {
  ...
};

bool operator==(const RequestMetaData& lhs, const RequestMetadata& rhs) {
  return ...
}

[marq (ping after 24h), 2017-02-09 14:31:30]

ios/ LGTM

https://codereview.chromium.org/2687873002/diff/40001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2687873002/diff/40001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:3077:
fileExtension:(NSString*)fileExtension {
withFileExtension: reads better, I think.

https://codereview.chromium.org/2687873002/diff/40001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:3114: -
(void)saveImage:(NSData*)data fileExtension:(NSString*)fileExtension {
withFileExtension:

[gambard, 2017-02-09 15:10:32]

Thanks, PTAL.

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
File components/image_fetcher/image_data_fetcher.cc (right):

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
components/image_fetcher/image_data_fetcher.cc:83:
source->GetResponseHeaders()->GetMimeType(&mime_type);
On 2017/02/09 13:49:47, Marc Treib wrote:
> You could read directly into &metadata.mime_type

Done.

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
File components/image_fetcher/image_data_fetcher.h (right):

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
components/image_fetcher/image_data_fetcher.h:27: struct RequestMetadata {
On 2017/02/09 13:55:28, sdefresne wrote:
> On 2017/02/09 13:49:48, Marc Treib wrote:
> > On 2017/02/09 13:44:12, gambard wrote:
> > > I don't know if this should be inside the class?
> > 
> > That's mostly a matter of taste IMO. I tend to slightly prefer the nested
> class
> > approach, but I don't feel very strongly.
> 
> drive-by: having the class separate make it easier to refactor code or break
> dependencies as it allows you to 1. forward-declare it (it is not possible to
> forward-declare nested classes outside of the outer class declaration), 2.
move
> it to a separate file (allowing you to just include the header for that class
if
> you do not care about the outer class).

Ok, I think I will let it outside the class as it is also used by the
IOSImageDataWrapper.

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
components/image_fetcher/image_data_fetcher.h:27: struct RequestMetadata {
On 2017/02/09 13:55:28, sdefresne wrote:
> On 2017/02/09 13:49:48, Marc Treib wrote:
> > On 2017/02/09 13:44:12, gambard wrote:
> > > I don't know if this should be inside the class?
> > 
> > That's mostly a matter of taste IMO. I tend to slightly prefer the nested
> class
> > approach, but I don't feel very strongly.
> 
> drive-by: having the class separate make it easier to refactor code or break
> dependencies as it allows you to 1. forward-declare it (it is not possible to
> forward-declare nested classes outside of the outer class declaration), 2.
move
> it to a separate file (allowing you to just include the header for that class
if
> you do not care about the outer class).

Actually, I have moved it to its own class.

https://codereview.chromium.org/2687873002/diff/40001/components/image_fetche...
components/image_fetcher/image_data_fetcher.h:30: bool operator==(const
RequestMetadata& rhs) const {
On 2017/02/09 13:55:28, sdefresne wrote:
> On 2017/02/09 13:49:48, Marc Treib wrote:
> > Are the equality operators required?
> 
> Even if they are required, they generally do not need to be methods but can be
> free functions instead:
> 
> struct RequestMetaData {
>   ...
> };
> 
> bool operator==(const RequestMetaData& lhs, const RequestMetadata& rhs) {
>   return ...
> }

They are mainly here for testing (verifying that the received metadata is the
expected one).

https://codereview.chromium.org/2687873002/diff/40001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2687873002/diff/40001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:3077:
fileExtension:(NSString*)fileExtension {
On 2017/02/09 14:31:30, marq wrote:
> withFileExtension: reads better, I think.

Done.

https://codereview.chromium.org/2687873002/diff/40001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:3114: -
(void)saveImage:(NSData*)data fileExtension:(NSString*)fileExtension {
On 2017/02/09 14:31:30, marq wrote:
> withFileExtension:

Done.

[Marc Treib, 2017-02-09 15:31:15]

components/image_fetcher LGTM
(I assume you'll land this one first, and then update
https://codereview.chromium.org/2677993002/ accordingly?)

[gambard, 2017-02-09 15:31:59]

On 2017/02/09 15:31:15, Marc Treib wrote:
> components/image_fetcher LGTM
> (I assume you'll land this one first, and then update
> https://codereview.chromium.org/2677993002/ accordingly?)

Yes, thanks!

[gambard, 2017-02-09 15:32:09]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-02-09 15:32:10]

The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2687873002/#ps60001
(title: "Move RequestData to its own file")

[commit-bot: I haz the power, 2017-02-09 15:32:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 16:56:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 16:56:52]

Failed to apply patch for ios/chrome/browser/ui/browser_view_controller.mm:
While running git apply --index -p1;
  error: patch failed: ios/chrome/browser/ui/browser_view_controller.mm:678
  error: ios/chrome/browser/ui/browser_view_controller.mm: patch does not apply

Patch:       ios/chrome/browser/ui/browser_view_controller.mm
Index: ios/chrome/browser/ui/browser_view_controller.mm
diff --git a/ios/chrome/browser/ui/browser_view_controller.mm
b/ios/chrome/browser/ui/browser_view_controller.mm
index
09f2a6ac706186d7b750a0c322e6e123bd18fcf0..a952a7f997d6124eb5e2ef809b4b6571926b5403
100644
--- a/ios/chrome/browser/ui/browser_view_controller.mm
+++ b/ios/chrome/browser/ui/browser_view_controller.mm
@@ -16,6 +16,7 @@
 
 #include "base/base64.h"
 #include "base/command_line.h"
+#include "base/files/file_path.h"
 #include "base/format_macros.h"
 #include "base/i18n/rtl.h"
 #include "base/ios/block_types.h"
@@ -184,6 +185,7 @@
 #include "ios/web/public/web_thread.h"
 #import "ios/web/web_state/ui/crw_web_controller.h"
 #import "net/base/mac/url_conversions.h"
+#include "net/base/mime_util.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/ssl/ssl_info.h"
 #include "net/url_request/url_request_context_getter.h"
@@ -678,11 +680,12 @@ NSString* const kNativeControllerTemporaryKey =
@"NativeControllerTemporaryKey";
 // Returns Tab that corresponds to the given |webState|.
 - (Tab*)tabForWebState:(web::WebState*)webState;
 // Saves the image or display error message, based on privacy settings.
-- (void)managePermissionAndSaveImage:(NSData*)data;
+- (void)managePermissionAndSaveImage:(NSData*)data
+                   withFileExtension:(NSString*)fileExtension;
 // Saves the image. In order to keep the metadata of the image, the image is
 // saved as a temporary file on disk then saved in photos.
 // This should be called on FILE thread.
-- (void)saveImage:(NSData*)data;
+- (void)saveImage:(NSData*)data withFileExtension:(NSString*)fileExtension;
 // Called when Chrome has been denied access to the photos or videos and the
 // user can change it.
 // Shows a privacy alert on the main queue, allowing the user to go to Chrome's
@@ -2992,7 +2995,8 @@ class BrowserBookmarkModelBridge : public
bookmarks::BookmarkModelObserver {
   DCHECK(url.is_valid());
   base::WeakNSObject<BrowserViewController> weakSelf(self);
   const GURL image_source_url = url;
-  image_fetcher::IOSImageDataFetcherCallback callback = ^(NSData* data) {
+  image_fetcher::IOSImageDataFetcherCallback callback = ^(
+      NSData* data, const image_fetcher::RequestMetadata& metadata) {
     DCHECK(data);
     dispatch_async(dispatch_get_main_queue(), ^{
       [weakSelf searchByImageData:data atURL:image_source_url];
@@ -3048,17 +3052,29 @@ class BrowserBookmarkModelBridge : public
bookmarks::BookmarkModelObserver {
               referrer:(const web::Referrer&)referrer {
   DCHECK(url.is_valid());
 
-  image_fetcher::IOSImageDataFetcherCallback callback = ^(NSData* data) {
+  image_fetcher::IOSImageDataFetcherCallback callback = ^(
+      NSData* data, const image_fetcher::RequestMetadata& metadata) {
     DCHECK(data);
 
-    [self managePermissionAndSaveImage:data];
+    base::FilePath::StringType extension;
+
+    bool extensionSuccess =
+        net::GetPreferredExtensionForMimeType(metadata.mime_type, &extension);
+    if (!extensionSuccess || extension.length() == 0) {
+      extension = "png";
+    }
+
+    NSString* fileExtension =
+        [@"." stringByAppendingString:base::SysUTF8ToNSString(extension)];
+    [self managePermissionAndSaveImage:data withFileExtension:fileExtension];
   };
   _imageFetcher->FetchImageDataWebpDecoded(
       url, callback, web::ReferrerHeaderValueForNavigation(url, referrer),
       web::PolicyForNavigation(url, referrer));
 }
 
-- (void)managePermissionAndSaveImage:(NSData*)data {
+- (void)managePermissionAndSaveImage:(NSData*)data
+                   withFileExtension:(NSString*)fileExtension {
   switch ([PHPhotoLibrary authorizationStatus]) {
     // User was never asked for permission to access photos.
     case PHAuthorizationStatusNotDetermined:
@@ -3066,7 +3082,8 @@ class BrowserBookmarkModelBridge : public
bookmarks::BookmarkModelObserver {
         // Call -saveImage again to check if chrome needs to display an error
or
         // saves the image.
         if (status != PHAuthorizationStatusNotDetermined)
-          [self managePermissionAndSaveImage:data];
+          [self managePermissionAndSaveImage:data
+                           withFileExtension:fileExtension];
       }];
       break;
 
@@ -3086,18 +3103,18 @@ class BrowserBookmarkModelBridge : public
bookmarks::BookmarkModelObserver {
 
     // The application has permission to access the photos.
     default: {
-      web::WebThread::PostTask(web::WebThread::FILE, FROM_HERE,
-                               base::BindBlock(^{
-                                 [self saveImage:data];
-                               }));
+      web::WebThread::PostTask(
+          web::WebThread::FILE, FROM_HERE, base::BindBlock(^{
+            [self saveImage:data withFileExtension:fileExtension];
+          }));
       break;
     }
   }
 }
 
-- (void)saveImage:(NSData*)data {
+- (void)saveImage:(NSData*)data withFileExtension:(NSString*)fileExtension {
   NSString* fileName = [[[NSProcessInfo processInfo] globallyUniqueString]
-      stringByAppendingString:@".png"];
+      stringByAppendingString:fileExtension];
   NSURL* fileURL =
       [NSURL fileURLWithPath:[NSTemporaryDirectory()
                                  stringByAppendingPathComponent:fileName]];

[gambard, 2017-02-09 17:04:26]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-02-09 17:04:26]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2687873002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-09 17:05:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 18:53:23]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486659866256120,
"parent_rev": "83392e9f4312a231f60c5d2b8f0bfa4f80aa92cd", "commit_rev":
"9efce7a0be559aa13c499f4400c2f2c3f66157c6"}

[commit-bot: I haz the power, 2017-02-09 18:53:57]

Description was changed from

==========
Use mime type for image extension

This CL uses the mime type to determine the best image extension when saving
an image.

BUG=690355
==========

to

==========
Use mime type for image extension

This CL uses the mime type to determine the best image extension when saving
an image.

BUG=690355

Review-Url: https://codereview.chromium.org/2687873002
Cr-Commit-Position: refs/heads/master@{#449365}
Committed:
https://chromium.googlesource.com/chromium/src/+/9efce7a0be559aa13c499f4400c2...
==========

[commit-bot: I haz the power, 2017-02-09 18:53:59]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9efce7a0be559aa13c499f4400c2...

[Olivier, 2017-02-10 08:54:42]

olivierrobin@chromium.org changed reviewers:
+ olivierrobin@chromium.org

[Olivier, 2017-02-10 08:54:43]

https://codereview.chromium.org/2687873002/diff/80001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2687873002/diff/80001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:3116:
stringByAppendingString:fileExtension];
nit, and not important for this CL:
Whenever manipulating path, try to use path specific methods. 
Here, prefer [NSString stringByAppendingPathExtension:]