Implement ChromeOS mime type extension mappings.

Implement ChromeOS mime type extension mappings.

Previously ChromeOS used same implementation with linux which returns mime type by reading mimetype-extension database in the OS.
However it doesn't worked since ChromeOS doesn't have it.
This CL implements ChromeOS specific logic which returns mime type based on hard-coded list.

BUG=472017, 477913, 488378
TEST=net_unittests:MimeUtilTest.ExtensionTest

Committed: https://crrev.com/c8297369dc77b7f06027b3ca91bd4c9cc552a3cf
Cr-Commit-Position: refs/heads/master@{#330482}

[yawano, 2015-05-14 08:34:38]

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

[yawano, 2015-05-14 08:34:38]

PTAL. Thank you!

[eroman, 2015-05-14 23:06:19]

We need a better strategy for ChromeOS than simply adding more mappings to the
global tables.

For instance something like:

   * Ship /usr/share/mime on ChromeOS
   * Add a platform_mime_util_chromeos.cc (currently it is using
platform_mime_util_linux.cc I imagine)

Since I expect we are going to need to keep adding mappings to work around the
ChromeOS mimetype problem, it would at least be better to have a
platform_mime_util_chromeos.cc which contains those mappings, and explains why
they are necessary just on ChromeOS.

Long term it would be good to figure out if ChromeOS can just have
/usr/share/mime equivalent with relevant data.

[yawano, 2015-05-15 05:02:01]

I'll create a patch set to move this mapping to platform_mime_util_chromeos.cc.
While platform_mime_util_linux.cc is also used for ChromeOS, I think it does
nothing for ChromeOS since ChromeOS does not have the mime type mapping data in
it.

[eroman, 2015-05-15 05:04:21]

sounds good.

[yawano, 2015-05-15 08:11:51]

PTAL. Thank you!

https://codereview.chromium.org/1134393002/diff/20001/net/base/mime_util.cc
File net/base/mime_util.cc (left):

https://codereview.chromium.org/1134393002/diff/20001/net/base/mime_util.cc#o...
net/base/mime_util.cc:219: { "application/epub+zip", "epub" },
Since epub mime type is added for same reason (http://crrev.com/1061833002), I
move this to _chromeos.cc.

[eroman, 2015-05-15 09:05:33]

What is the testing strategy for this changelist?

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
File net/base/platform_mime_util_chromeos.cc (right):

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
net/base/platform_mime_util_chromeos.cc:18: }
Add comment:
  // namespace

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
net/base/platform_mime_util_chromeos.cc:20: // Since ChromeOS does not have
mimetype-extension database in it, we use
Please provide some extra details in this comment -- explain how on Linux it
uses xdg_mime and /usr/share/mime, but ChromeOS does not have /usr/share/mime
(and did you figure out *why* it does not have that?)

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
net/base/platform_mime_util_chromeos.cc:26:
net::FindMimeType(mimetype_extension_mapping,
no need for net:: prefix

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
net/base/platform_mime_util_chromeos.cc:27:
arraysize(mimetype_extension_mapping), ext.c_str());
It would be better if didn't have to take .c_str() of |ext|.
It is probably possible for |ext| to contain embedded \0 in which case using
.c_str() will not be correct. I guess we already have this problem in mime_util
then...

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
net/base/platform_mime_util_chromeos.cc:35: }
add comment:
// namespace net

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
File net/base/platform_mime_util_linux.cc (right):

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
net/base/platform_mime_util_linux.cc:25: #elif defined(OS_CHROMEOS)
Actually, I think we need to remove this file from the build when built with
chromeos. Otherwise we will end up with duplicate definitions for
PlatformMimeUtil

[yawano, 2015-05-15 09:19:50]

Thank you for the review!

Currently we don't have a test code for this CL. While we can test it manually
and indirectly on ChromeOS, there is no way to test this directly (i.e. there is
no way to check which mime type is actually returned).

It might be a best time to add platform_mime_util_unittest.cc. However, I have a
concern that writing a unittest which depends on 'platform' is a good choice or
not.

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
File net/base/platform_mime_util_linux.cc (right):

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
net/base/platform_mime_util_linux.cc:25: #elif defined(OS_CHROMEOS)
_linux.cc is included in chromeos build. Changing a gypi file not to include
_linux.cc but to include _chromeos.cc seems to be a big change for me.

How about to do a same thing with OS_ANDROID? i.e. Implement
chromeos::GetMimTypeFromExtension in mime_type_extension_chromeos.cc.

[yawano, 2015-05-15 10:05:09]

mime_util has mime_util_unittest.cc. I think this can be covered by the test
case (while we need to add test pairs to test this CL).

https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_util...

[eroman, 2015-05-18 01:00:08]

Adding the test to mime_util_unittest.cc sounds like a good plan. If the mapping
is not universal, then the test may need to be surrounded by a chromeos #ifdef.

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
File net/base/platform_mime_util_linux.cc (right):

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
net/base/platform_mime_util_linux.cc:25: #elif defined(OS_CHROMEOS)
On 2015/05/15 09:19:50, yawano wrote:
> _linux.cc is included in chromeos build. Changing a gypi file not to include
> _linux.cc but to include _chromeos.cc seems to be a big change for me.
> 
> How about to do a same thing with OS_ANDROID? i.e. Implement
> chromeos::GetMimTypeFromExtension in mime_type_extension_chromeos.cc.

Yes that sounds like a good approach (use the same strategy as Android)

[yawano, 2015-05-18 08:59:44]

PTAL. Thank you!

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
File net/base/platform_mime_util_chromeos.cc (right):

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
net/base/platform_mime_util_chromeos.cc:18: }
On 2015/05/15 09:05:33, eroman (sick slow) wrote:
> Add comment:
>   // namespace

Done.

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
net/base/platform_mime_util_chromeos.cc:20: // Since ChromeOS does not have
mimetype-extension database in it, we use
Done.

I asked this on ML but I couldn't figure out why. It seems that it just doesn't
include it for now. -
https://groups.google.com/a/chromium.org/d/msg/chromium-os-dev/mCgDTj8AJqs/LQ...

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
net/base/platform_mime_util_chromeos.cc:26:
net::FindMimeType(mimetype_extension_mapping,
On 2015/05/15 09:05:33, eroman (sick slow) wrote:
> no need for net:: prefix

Acknowledged.

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
net/base/platform_mime_util_chromeos.cc:27:
arraysize(mimetype_extension_mapping), ext.c_str());
I think an extension which contains null character should not be valid. Changed
to reject such input in GetMimeTypeFromExtensionHelper.

Note: In the current usage, FindMimeType don't get an input which contains null
character. base::FilePath cuts string not to contain null character. e.g. If
input is png\0css, base::FilePath is initialized as png.

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
net/base/platform_mime_util_chromeos.cc:35: }
On 2015/05/15 09:05:33, eroman (sick slow) wrote:
> add comment:
> // namespace net

Acknowledged.

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
File net/base/platform_mime_util_linux.cc (right):

https://codereview.chromium.org/1134393002/diff/20001/net/base/platform_mime_...
net/base/platform_mime_util_linux.cc:25: #elif defined(OS_CHROMEOS)
On 2015/05/18 01:00:08, eroman (sick slow) wrote:
> On 2015/05/15 09:19:50, yawano wrote:
> > _linux.cc is included in chromeos build. Changing a gypi file not to include
> > _linux.cc but to include _chromeos.cc seems to be a big change for me.
> > 
> > How about to do a same thing with OS_ANDROID? i.e. Implement
> > chromeos::GetMimTypeFromExtension in mime_type_extension_chromeos.cc.
> 
> Yes that sounds like a good approach (use the same strategy as Android)

Done.

[eroman, 2015-05-18 15:45:24]

Please update the changelist descriptive to be be more descriptive than "Add
text/calendar mime type.":

   * Explain that this is adding a solution specifically for ChromeOS, by not
using the linux platform mime util, and instead using a hardcoded list of
mappings

otherwise looks good!

https://codereview.chromium.org/1134393002/diff/160001/net/base/mime_extensio...
File net/base/mime_extension_chromeos.h (right):

https://codereview.chromium.org/1134393002/diff/160001/net/base/mime_extensio...
net/base/mime_extension_chromeos.h:12: namespace chromeos {
This should additionally be in the net namespace:

namespace net {
namespace chromeos {

...

https://codereview.chromium.org/1134393002/diff/160001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1134393002/diff/160001/net/base/mime_util.cc#...
net/base/mime_util.cc:32: const base::FilePath::CharType kStringTerminator =
FILE_PATH_LITERAL('\0');
nit: I think it would be shorter to just inline this constant where it is used.

In other words, no need to define it here.

https://codereview.chromium.org/1134393002/diff/160001/net/base/mime_util.cc#...
net/base/mime_util.cc:272: base::FilePath::StringType::size_type nul_pos =
ext.find(kStringTerminator);
Thanks for adding. I have also filed
https://code.google.com/p/chromium/issues/detail?id=488378 to address this
differently. Could you put a comment referring to crbug.com/488378, so this gets
cleaned up as part of that bug?

https://codereview.chromium.org/1134393002/diff/160001/net/base/mime_util.h
File net/base/mime_util.h (right):

https://codereview.chromium.org/1134393002/diff/160001/net/base/mime_util.h#n...
net/base/mime_util.h:174: // Find mime type of |ext| from |mapping|.
Find --> Finds
|mapping| --> |mappings|

[yawano, 2015-05-19 02:26:40]

Thank you for the review! PTAL.

https://codereview.chromium.org/1134393002/diff/160001/net/base/mime_extensio...
File net/base/mime_extension_chromeos.h (right):

https://codereview.chromium.org/1134393002/diff/160001/net/base/mime_extensio...
net/base/mime_extension_chromeos.h:12: namespace chromeos {
On 2015/05/18 15:45:24, eroman (sick slow) wrote:
> This should additionally be in the net namespace:
> 
> namespace net {
> namespace chromeos {
> 
> ...

Done.

https://codereview.chromium.org/1134393002/diff/160001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1134393002/diff/160001/net/base/mime_util.cc#...
net/base/mime_util.cc:32: const base::FilePath::CharType kStringTerminator =
FILE_PATH_LITERAL('\0');
On 2015/05/18 15:45:24, eroman (sick slow) wrote:
> nit: I think it would be shorter to just inline this constant where it is
used.
> 
> In other words, no need to define it here.

Done.

https://codereview.chromium.org/1134393002/diff/160001/net/base/mime_util.cc#...
net/base/mime_util.cc:272: base::FilePath::StringType::size_type nul_pos =
ext.find(kStringTerminator);
On 2015/05/18 15:45:24, eroman (sick slow) wrote:
> Thanks for adding. I have also filed
> https://code.google.com/p/chromium/issues/detail?id=488378 to address this
> differently. Could you put a comment referring to crbug.com/488378, so this
gets
> cleaned up as part of that bug?

Done.

https://codereview.chromium.org/1134393002/diff/160001/net/base/mime_util.h
File net/base/mime_util.h (right):

https://codereview.chromium.org/1134393002/diff/160001/net/base/mime_util.h#n...
net/base/mime_util.h:174: // Find mime type of |ext| from |mapping|.
On 2015/05/18 15:45:24, eroman (sick slow) wrote:
> Find --> Finds
> |mapping| --> |mappings|

Done.

[eroman, 2015-05-19 04:32:23]

lgtm

[yawano, 2015-05-19 04:58:20]

On 2015/05/19 04:32:23, eroman (sick slow) wrote:
> lgtm

Thank you!

[yawano, 2015-05-19 04:58:28]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-05-19 04:58:46]

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

[commit-bot: I haz the power, 2015-05-19 05:48:10]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-05-19 05:49:03]

Patchset 10 (id:??) landed as
https://crrev.com/c8297369dc77b7f06027b3ca91bd4c9cc552a3cf
Cr-Commit-Position: refs/heads/master@{#330482}