mediaview: Recognize more MIME types.

mediaview: Recognize more MIME types.

Android uses "audio/x-wav" MIME type to describe .wav files, while
Chromium knows "audio/wav" only. This patch adds hard-coded mapping
between MIME types and file name extensions taken from Android.

BUG=chromium:685330
TEST=unit_tests
TEST=.wav files recognized in media views

Review-Url: https://codereview.chromium.org/2656973003
Cr-Commit-Position: refs/heads/master@{#446633}
Committed: https://chromium.googlesource.com/chromium/src/+/588a513a3772d7cc466568f9007780aee8251003

[Shuhei Takahashi, 2017-01-27 06:20:44]

The CQ bit was checked by nya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 06:21:04]

Dry run: 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-01-27 06:50:19]

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

[commit-bot: I haz the power, 2017-01-27 06:50:20]

Dry run: This issue passed the CQ dry run.

[Shuhei Takahashi, 2017-01-27 07:02:21]

nya@chromium.org changed reviewers:
+ hidehiko@chromium.org

[Shuhei Takahashi, 2017-01-27 07:02:22]

hidehiko: PTAL

[hidehiko, 2017-01-27 07:39:45]

https://codereview.chromium.org/2656973003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right):

https://codereview.chromium.org/2656973003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:196:
extensions.insert(extensions.begin(), preferred_extension);
Just curious: Is there a case this is needed?

This means, GetExtensionsForMimeType() does not return
GetPreferredExtensionForMimeType(), which sounds slightly weird to me.
Comments help, I think.

https://codereview.chromium.org/2656973003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:207:
base::SplitStringPiece(entry.extensions, ",", base::TRIM_WHITESPACE,
As you assume base::FilePath::StringType is std::string (i.e. posix env), I
think

return base::SplitString(entry.extensions, ",", ...);

should work. Or, at L209,

extensions.emplace_back(FilePath::FromUTF8Unsafe(extension).value())

looks more correct (but inefficient) way?

https://codereview.chromium.org/2656973003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h (right):

https://codereview.chromium.org/2656973003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h:70:
std::vector<base::FilePath::StringType> GetExtensionsForMimeTypeForArc(
Optional: two For may be annoying? How about
- GetExtensionsForArcMimeType
- GetExtensionsForMineType (defined in arc namespace, so ForArc may be
redundant).

[Shuhei Takahashi, 2017-01-27 08:12:45]

The CQ bit was checked by nya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 08:12:56]

Dry run: 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-01-27 09:03:37]

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

[commit-bot: I haz the power, 2017-01-27 09:03:38]

Dry run: This issue passed the CQ dry run.

[Shuhei Takahashi, 2017-01-27 09:17:14]

PTAL

https://codereview.chromium.org/2656973003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right):

https://codereview.chromium.org/2656973003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:196:
extensions.insert(extensions.begin(), preferred_extension);
On 2017/01/27 07:39:44, hidehiko wrote:
> Just curious: Is there a case this is needed?
> 
> This means, GetExtensionsForMimeType() does not return
> GetPreferredExtensionForMimeType(), which sounds slightly weird to me.
> Comments help, I think.

I'm not sure if this happens, but I could not find any guarantee in mime_util.h.

https://codereview.chromium.org/2656973003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:207:
base::SplitStringPiece(entry.extensions, ",", base::TRIM_WHITESPACE,
On 2017/01/27 07:39:44, hidehiko wrote:
> As you assume base::FilePath::StringType is std::string (i.e. posix env), I
> think
> 
> return base::SplitString(entry.extensions, ",", ...);
> 
> should work. Or, at L209,
> 
> extensions.emplace_back(FilePath::FromUTF8Unsafe(extension).value())
> 
> looks more correct (but inefficient) way?
> 

Thanks, I'll use base::SplitString instead.

https://codereview.chromium.org/2656973003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h (right):

https://codereview.chromium.org/2656973003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h:70:
std::vector<base::FilePath::StringType> GetExtensionsForMimeTypeForArc(
On 2017/01/27 07:39:44, hidehiko wrote:
> Optional: two For may be annoying? How about
> - GetExtensionsForArcMimeType
> - GetExtensionsForMineType (defined in arc namespace, so ForArc may be
> redundant).

OK, let's call it GetExtensionsForArcMimeType.

(I did not choose GetExtensionsForMineType() because it's too misleading)

[hidehiko, 2017-01-27 09:39:00]

lgtm

https://codereview.chromium.org/2656973003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h (right):

https://codereview.chromium.org/2656973003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h:70:
std::vector<base::FilePath::StringType> GetExtensionsForMimeTypeForArc(
On 2017/01/27 09:17:14, Shuhei Takahashi wrote:
> On 2017/01/27 07:39:44, hidehiko wrote:
> > Optional: two For may be annoying? How about
> > - GetExtensionsForArcMimeType
> > - GetExtensionsForMineType (defined in arc namespace, so ForArc may be
> > redundant).
> 
> OK, let's call it GetExtensionsForArcMimeType.
> 
> (I did not choose GetExtensionsForMineType() because it's too misleading)

sgtm

https://codereview.chromium.org/2656973003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right):

https://codereview.chromium.org/2656973003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:183: return
std::vector<base::FilePath::StringType>();
Optional:

return {};

may be simpler?
It looks allowed
https://google.github.io/styleguide/cppguide.html#Braced_Initializer_List
and actually several code in chromium uses.
(Though it's not much popular?). Totally up to you.

https://codereview.chromium.org/2656973003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:196: // This
is unlikely to happen, but there is no guarantee.
Nice comment!

https://codereview.chromium.org/2656973003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:209: return
base::SplitString(entry.extensions, ",", base::TRIM_WHITESPACE,
How about commenting;
// This module is only for ChromeOS where base::File::StringType is std::string.

[Shuhei Takahashi, 2017-01-27 09:44:32]

https://codereview.chromium.org/2656973003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right):

https://codereview.chromium.org/2656973003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:183: return
std::vector<base::FilePath::StringType>();
On 2017/01/27 09:39:00, hidehiko wrote:
> Optional:
> 
> return {};
> 
> may be simpler?
> It looks allowed
> https://google.github.io/styleguide/cppguide.html#Braced_Initializer_List
> and actually several code in chromium uses.
> (Though it's not much popular?). Totally up to you.

I also thought I can do that, but it gives a compilation error in some setups.
This is an output from target_os="chromeos", is_debug=true configuration:

../../chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:183:12:
error: chosen constructor is explicit in copy-initialization
    return {};
           ^~
../../build/linux/ubuntu_precise_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/debug/vector:77:7:
note: constructor declared here
      vector(const _Allocator& __a = _Allocator())
      ^

https://codereview.chromium.org/2656973003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:209: return
base::SplitString(entry.extensions, ",", base::TRIM_WHITESPACE,
On 2017/01/27 09:39:00, hidehiko wrote:
> How about commenting;
> // This module is only for ChromeOS where base::File::StringType is
std::string.
> 

Sure, added a short comment.

[Shuhei Takahashi, 2017-01-27 09:44:47]

The CQ bit was checked by nya@chromium.org

[Shuhei Takahashi, 2017-01-27 09:44:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org
Link to the patchset: https://codereview.chromium.org/2656973003/#ps40001
(title: "Added a short comment.")

[commit-bot: I haz the power, 2017-01-27 09:45:05]

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-01-27 10:22:03]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485510287917330,
"parent_rev": "4673f6675de445aa4efffd86f6f7d22a3280e9d8", "commit_rev":
"588a513a3772d7cc466568f9007780aee8251003"}

[commit-bot: I haz the power, 2017-01-27 10:22:38]

Description was changed from

==========
mediaview: Recognize more MIME types.

Android uses "audio/x-wav" MIME type to describe .wav files, while
Chromium knows "audio/wav" only. This patch adds hard-coded mapping
between MIME types and file name extensions taken from Android.

BUG=chromium:685330
TEST=unit_tests
TEST=.wav files recognized in media views
==========

to

==========
mediaview: Recognize more MIME types.

Android uses "audio/x-wav" MIME type to describe .wav files, while
Chromium knows "audio/wav" only. This patch adds hard-coded mapping
between MIME types and file name extensions taken from Android.

BUG=chromium:685330
TEST=unit_tests
TEST=.wav files recognized in media views

Review-Url: https://codereview.chromium.org/2656973003
Cr-Commit-Position: refs/heads/master@{#446633}
Committed:
https://chromium.googlesource.com/chromium/src/+/588a513a3772d7cc466568f90077...
==========

[commit-bot: I haz the power, 2017-01-27 10:22:39]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/588a513a3772d7cc466568f90077...