Do not handle a file as application/zip when the extension is not .zip.

Do not handle a file as application/zip when the extension is not .zip.

BUG=473604
TEST=manually tested

Committed: https://crrev.com/5fb4a1e8460df4c87aca8478f5606f6975536545
Cr-Commit-Position: refs/heads/master@{#325215}

[yawano, 2015-04-13 08:32:01]

yawano@chromium.org changed reviewers:
+ benwells@chromium.org

[yawano, 2015-04-13 08:32:02]

PTAL. Thank you!

[benwells, 2015-04-14 21:57:22]

https://codereview.chromium.org/1073003003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/file_handlers/mime_util.cc (right):

https://codereview.chromium.org/1073003003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/file_handlers/mime_util.cc:44:
local_path.Extension() != FILE_PATH_LITERAL(".zip"))
The logic of this flow is, I think:
- return the mime type from the extension, if it isn't empty, otherwise
- return the sniffed mime type from the contents, except if the extension is not
zip and the mime type is, in which case return the applicaiton/octet-stream

This doesn't seem that clean. In particular:
- if there are other valid application/zip extensions this now feels
inconsistent
- but, there shouldn't be any need to check the extension as .zip files
shouldn't end up being sniffed anyway

Instead of doing this you could add something to the handler of this
(OnSniffMimeTYpeForNativeLocalPathCompleted) which checks the returned
mime_type:
  if (*mime_type == "application/zip") {
    callback.Run("application/octet-stream");
    return;
  }

  callback.Run(*mime_type)

THat way you don't redundantly check the extension again.

But, maybe I'm missing some reason to do it this way...

https://codereview.chromium.org/1073003003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/file_handlers/mime_util.cc:45:
result->assign("application/octet-stream");
Why is the default application/octet-stream? Is that the default we always use?

[yawano, 2015-04-15 03:41:34]

Thank you for the review! PTAL.

https://codereview.chromium.org/1073003003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/file_handlers/mime_util.cc (right):

https://codereview.chromium.org/1073003003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/file_handlers/mime_util.cc:44:
local_path.Extension() != FILE_PATH_LITERAL(".zip"))
Moving to OnSniffMimeTypeForNativeLocalPathCompleted seems to be better. I moved
it.

https://codereview.chromium.org/1073003003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/file_handlers/mime_util.cc:45:
result->assign("application/octet-stream");
I think there is no default mime type. The default value of mime type is empty
string.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

The reason that we selected application/octet-stream is that generic mime type
should be less confusing.

[benwells, 2015-04-15 05:01:17]

https://codereview.chromium.org/1073003003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_handlers/mime_util.cc (right):

https://codereview.chromium.org/1073003003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/file_handlers/mime_util.cc:111:
scoped_ptr<std::string> sniffed_mime_type(new std::string);
Can we initialize this to "application/octet-stream" so the default is
consistent?

[yawano, 2015-04-15 06:34:02]

PTAL. Thank you!

https://codereview.chromium.org/1073003003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_handlers/mime_util.cc (right):

https://codereview.chromium.org/1073003003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/file_handlers/mime_util.cc:111:
scoped_ptr<std::string> sniffed_mime_type(new std::string);
On 2015/04/15 05:01:17, benwells wrote:
> Can we initialize this to "application/octet-stream" so the default is
> consistent?

Done.

[benwells, 2015-04-15 07:01:12]

lgtm

[benwells, 2015-04-15 07:01:13]

lgtm

lgtm

[yawano, 2015-04-15 07:02:39]

On 2015/04/15 07:01:13, benwells wrote:
> lgtm
> 
> lgtm

Thank you!

[yawano, 2015-04-15 07:02:43]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-04-15 07:03:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1073003003/60001

[commit-bot: I haz the power, 2015-04-15 09:59:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-15 09:59:58]

Patchset 4 (id:??) landed as
https://crrev.com/5fb4a1e8460df4c87aca8478f5606f6975536545
Cr-Commit-Position: refs/heads/master@{#325215}