Various MimeUtil cleanups.

Various MimeUtil cleanups.

Cleanly separate roles of codec parsing from checking codec support.
Merge various codec support checks into single function.
Separate notions of ambiguous codec strings from ambiguous support.
Make ambiguous codec strings less extensible (avoid adding more).
Prepare to eliminate notions of ambiguous support. 
Naming improvements.

More details in the bug. Kudos to ddorwin.

BUG=672327
Review-Url: https://codereview.chromium.org/2700893003
Cr-Commit-Position: refs/heads/master@{#452243}
Committed: https://chromium.googlesource.com/chromium/src/+/38e97f421bcf90b224b895a405f3ba0de3efe5ac

[chcunningham, 2017-02-17 23:04:36]

Description was changed from

==========
Various MimeUtil cleanups.

Cleanly separate roles of codec parsing from checking codec support.
Merge various codec support checks into single function.
Separate notions of ambiguous codec strings from ambiguous support.

More details in the bug. Kudos to ddorwin.

BUG=672327
==========

to

==========
Various MimeUtil cleanups.

Cleanly separate roles of codec parsing from checking codec support.
Merge various codec support checks into single function.
Separate notions of ambiguous codec strings from ambiguous support.
Prepare to eliminate notions of ambiguous support. 

More details in the bug. Kudos to ddorwin.

BUG=672327
==========

[chcunningham, 2017-02-17 23:13:57]

Description was changed from

==========
Various MimeUtil cleanups.

Cleanly separate roles of codec parsing from checking codec support.
Merge various codec support checks into single function.
Separate notions of ambiguous codec strings from ambiguous support.
Prepare to eliminate notions of ambiguous support. 

More details in the bug. Kudos to ddorwin.

BUG=672327
==========

to

==========
Various MimeUtil cleanups.

Cleanly separate roles of codec parsing from checking codec support.
Merge various codec support checks into single function.
Separate notions of ambiguous codec strings from ambiguous support.
Make ambiguous codec strings less extensible (avoid adding more).
Prepare to eliminate notions of ambiguous support. 
Naming improvements.

More details in the bug. Kudos to ddorwin.

BUG=672327
==========

[chcunningham, 2017-02-17 23:18:00]

chcunningham@chromium.org changed reviewers:
+ jrummell@chromium.org, junov@chromium.org

[chcunningham, 2017-02-17 23:18:01]

Adding reviewers.

Junov for WebKit.
Jrummell for everything else.

[chcunningham, 2017-02-17 23:18:29]

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

[commit-bot: I haz the power, 2017-02-17 23:20:16]

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

[DaleCurtis, 2017-02-17 23:39:03]

https://codereview.chromium.org/2700893003/diff/40001/media/base/mime_util_in...
File media/base/mime_util_internal.cc (right):

https://codereview.chromium.org/2700893003/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.cc:28: static const std::map<std::string,
MimeUtil::Codec> kStringToCodecMap = {
Can't do this since it's a static initializer.

[commit-bot: I haz the power, 2017-02-18 00:36:30]

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

[commit-bot: I haz the power, 2017-02-18 00:36:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chcunningham, 2017-02-20 22:25:05]

https://codereview.chromium.org/2700893003/diff/40001/media/base/mime_util_in...
File media/base/mime_util_internal.cc (right):

https://codereview.chromium.org/2700893003/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.cc:28: static const std::map<std::string,
MimeUtil::Codec> kStringToCodecMap = {
On 2017/02/17 23:39:03, DaleCurtis wrote:
> Can't do this since it's a static initializer.

Fixed! thanks

[chcunningham, 2017-02-20 22:25:12]

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

[commit-bot: I haz the power, 2017-02-20 22:25:19]

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-02-20 23:07:35]

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

[commit-bot: I haz the power, 2017-02-20 23:07:36]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Justin Novosad, 2017-02-21 17:01:28]

lgtm for third_party/WebKit

[chcunningham, 2017-02-21 19:06:34]

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

[commit-bot: I haz the power, 2017-02-21 19:07:48]

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-02-21 20:18:18]

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

[commit-bot: I haz the power, 2017-02-21 20:18:20]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[chcunningham, 2017-02-21 22:28:05]

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

[commit-bot: I haz the power, 2017-02-21 22:28: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-02-22 00:07:52]

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

[commit-bot: I haz the power, 2017-02-22 00:07:53]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[chcunningham, 2017-02-22 00:12:02]

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

[commit-bot: I haz the power, 2017-02-22 00:13:41]

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-02-22 02:16:10]

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

[commit-bot: I haz the power, 2017-02-22 02:16:11]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[jrummell, 2017-02-22 19:23:19]

lgtm w/nits.

https://codereview.chromium.org/2700893003/diff/120001/media/base/mime_util_i...
File media/base/mime_util_internal.cc (right):

https://codereview.chromium.org/2700893003/diff/120001/media/base/mime_util_i...
media/base/mime_util_internal.cc:201: combined_result = MayBeSupported;
Not sure why you added |combined_result|. Can you simplify this to:

// Downgrade to MayBeSupported if ...
if (result == IsSupported && ambiguous_codec_string)
  return MayBeSupported;

return result;

https://codereview.chromium.org/2700893003/diff/120001/media/base/mime_util_i...
media/base/mime_util_internal.cc:418: bool MimeUtil::IsCodecSupportedOnAndroid(
Should this be #if defined(ANDROID)? Maybe not for testing.

CS doesn't show any external references to IsCodecSupportedOnPlatform() outside
of the test. Should this be an internal method and use friend for the test?

https://codereview.chromium.org/2700893003/diff/120001/media/base/mime_util_i...
media/base/mime_util_internal.cc:570: // NOTE: avc1/avc3.XXXXXX may be ambiguous
handled after ParseAVCCodecId().
Like the comment!

https://codereview.chromium.org/2700893003/diff/120001/media/base/mime_util_i...
media/base/mime_util_internal.cc:604: LOG(WARNING) << __func__ << ":
Unrecognized codec id " << codec_id;
Do we want WARNINGs for this?

https://codereview.chromium.org/2700893003/diff/120001/media/base/mime_util_i...
media/base/mime_util_internal.cc:741: SupportsType
MimeUtil::IsDefaultCodecSupportedLowerCase(
I see you're not changing this, but it seems weird to have "Lowercase" in both
the title and the parameter name.

[chcunningham, 2017-02-22 20:44:12]

Thanks for review

https://codereview.chromium.org/2700893003/diff/120001/media/base/mime_util_i...
File media/base/mime_util_internal.cc (right):

https://codereview.chromium.org/2700893003/diff/120001/media/base/mime_util_i...
media/base/mime_util_internal.cc:201: combined_result = MayBeSupported;
On 2017/02/22 19:23:19, jrummell wrote:
> Not sure why you added |combined_result|. Can you simplify this to:
> 
> // Downgrade to MayBeSupported if ...
> if (result == IsSupported && ambiguous_codec_string)
>   return MayBeSupported;
> 
> return result;

Unfortunately not. Its possible that this codec is "MayBeSupported", but the
next codec in the for-loop is "NotSupported", so its not safe to return early.

https://codereview.chromium.org/2700893003/diff/120001/media/base/mime_util_i...
media/base/mime_util_internal.cc:418: bool MimeUtil::IsCodecSupportedOnAndroid(
On 2017/02/22 19:23:19, jrummell wrote:
> Should this be #if defined(ANDROID)? Maybe not for testing.

Leaving as is for testing.
 
> CS doesn't show any external references to IsCodecSupportedOnPlatform()
outside
> of the test. Should this be an internal method and use friend for the test?

Just tried to go the friend route, but its tricky since these tests are just
TEST(...) not TEST_F(...), so they don't supply their own fixture for me to
friend. I agree, I don't love the public static - but I'm also about to move all
of that into a new AndroidMediaClient, so will be short lived.

https://codereview.chromium.org/2700893003/diff/120001/media/base/mime_util_i...
media/base/mime_util_internal.cc:570: // NOTE: avc1/avc3.XXXXXX may be ambiguous
handled after ParseAVCCodecId().
On 2017/02/22 19:23:19, jrummell wrote:
> Like the comment!

Thanks :)

https://codereview.chromium.org/2700893003/diff/120001/media/base/mime_util_i...
media/base/mime_util_internal.cc:604: LOG(WARNING) << __func__ << ":
Unrecognized codec id " << codec_id;
On 2017/02/22 19:23:19, jrummell wrote:
> Do we want WARNINGs for this?

Changing to DVLOG(2) (4 seems a bit high)

https://codereview.chromium.org/2700893003/diff/120001/media/base/mime_util_i...
media/base/mime_util_internal.cc:741: SupportsType
MimeUtil::IsDefaultCodecSupportedLowerCase(
On 2017/02/22 19:23:19, jrummell wrote:
> I see you're not changing this, but it seems weird to have "Lowercase" in both
> the title and the parameter name.

Removed it from the method name to match the other methods in this file (which
have lower_case in just the argument name).

I also sprinkled some DCHECKS to check that its really lower case in some of the
main paths.

[chcunningham, 2017-02-22 20:44:22]

The CQ bit was checked by chcunningham@chromium.org

[chcunningham, 2017-02-22 20:44:22]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org,
jrummell@chromium.org
Link to the patchset: https://codereview.chromium.org/2700893003/#ps140001
(title: "Feedback")

[commit-bot: I haz the power, 2017-02-22 20:45:13]

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-22 22:43:24]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487796262367710,
"parent_rev": "59ff1923a9f2ca78b740bc98f1bf492435f33715", "commit_rev":
"38e97f421bcf90b224b895a405f3ba0de3efe5ac"}

[commit-bot: I haz the power, 2017-02-22 22:44:01]

Description was changed from

==========
Various MimeUtil cleanups.

Cleanly separate roles of codec parsing from checking codec support.
Merge various codec support checks into single function.
Separate notions of ambiguous codec strings from ambiguous support.
Make ambiguous codec strings less extensible (avoid adding more).
Prepare to eliminate notions of ambiguous support. 
Naming improvements.

More details in the bug. Kudos to ddorwin.

BUG=672327
==========

to

==========
Various MimeUtil cleanups.

Cleanly separate roles of codec parsing from checking codec support.
Merge various codec support checks into single function.
Separate notions of ambiguous codec strings from ambiguous support.
Make ambiguous codec strings less extensible (avoid adding more).
Prepare to eliminate notions of ambiguous support. 
Naming improvements.

More details in the bug. Kudos to ddorwin.

BUG=672327

Review-Url: https://codereview.chromium.org/2700893003
Cr-Commit-Position: refs/heads/master@{#452243}
Committed:
https://chromium.googlesource.com/chromium/src/+/38e97f421bcf90b224b895a405f3...
==========

[commit-bot: I haz the power, 2017-02-22 22:44:02]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/38e97f421bcf90b224b895a405f3...