Rename misleading |is_ambiguous| parameter

media/base/mime_util_internal.h

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef MEDIA_BASE_MIME_UTIL_INTERNAL_H_
 #define MEDIA_BASE_MIME_UTIL_INTERNAL_H_
 
 #include <map>
 #include <string>
 #include <vector>
@@ skipping 65 matching lines @@
       bool is_encrypted,
       const PlatformInfo& platform_info);
 
  private:
   typedef base::hash_set<int> CodecSet;
   typedef std::map<std::string, CodecSet> MediaFormatMappings;
   struct CodecEntry {
     CodecEntry() : codec(INVALID_CODEC), is_ambiguous(true) {}
     CodecEntry(Codec c, bool ambiguous) : codec(c), is_ambiguous(ambiguous) {}
     Codec codec;
     bool is_ambiguous;

[xhwang, 2016/04/19 23:59:43]

Do you want rename this as well?

[ddorwin, 2016/04/21 00:50:30]

Hmm. It is set for ambiguous strings (kAmbiguousCodecStringMap). It's just a
matter of where we do the conversion from ambiguous to not known supported.
Currently, that is in StringToCodec().

To the extent that this is an object representation of the static tables, I
think it probably makes sense to leave it this way, but I don't feel strongly.

   };
   typedef std::map<std::string, CodecEntry> StringToCodecMappings;
 
   // Initializes the supported media types into hash sets for faster lookup.
   void InitializeMimeTypeMaps();
 
   // Initializes the supported media formats (|media_format_map_|).
   void AddSupportedMediaFormats();
 
   // Adds |mime_type| with the specified codecs to |media_format_map_|.
   void AddContainerWithCodecs(const std::string& mime_type,
                               const CodecSet& codecs_list,
                               bool is_proprietary_mime_type);
 
-  // Returns IsSupported if all codec IDs in |codecs| are unambiguous and are
-  // supported in |mime_type_lower_case|. MayBeSupported is returned if at least
-  // one codec ID in |codecs| is ambiguous but all the codecs are supported.
+  // Returns IsSupported if all codec IDs in |codecs| are unambiguous, known to

[ddorwin, 2016/04/18 23:43:37]

"known to be supported" here, which is not "confidently known to be supported"
below, is the most problematic naming issue.

[DaleCurtis, 2016/04/20 19:18:54]

Yeah, this is just redundant. It doesn't make sense to have both "known to be
supported" and "are supported". Replace one or the other with your preferred
wording.

[ddorwin, 2016/04/21 00:50:30]

It turns out this was a bit of a bad example. I've fixed the comments. We now
have:
1. "confidently known to be supported" - a codec string we fully recognize and
"know" we support
2. "supported in |mime_type_lower_case|" - a valid codec for the container.

We still have the larger question of how to describe #1.

+  // be supported, and are supported in |mime_type_lower_case|. MayBeSupported

[xhwang, 2016/04/19 23:59:42]

nit and not for this CL: Since "maybe" is one word, this should be
MaybeSupported?

[ddorwin, 2016/04/21 00:50:30]

I believe this is based on "the codec may be supported" just like "the codec is
[not] supported."

The single word would be something like "Maybe the codec is supported."

[xhwang, 2016/04/21 05:59:44]

Acknowledged.

+  // is returned if all the codecs are supported but at least one codec ID in
+  // |codecs| is ambiguous or not confidently known to be supported.

[DaleCurtis, 2016/04/20 19:18:54]

maybe something like "support can not be determined at this time" ?

[ddorwin, 2016/04/21 00:50:30]

Reworded. WDYT?

   // IsNotSupported is returned if |mime_type_lower_case| is not supported or at
-  // least one is not supported in |mime_type_lower_case|. |is_encrypted| means
-  // the codec will be used with encrypted blocks.
+  // least one of the |codecs| is not supported in |mime_type_lower_case|.
+  // |is_encrypted| means the codec will be used with encrypted blocks.
   SupportsType AreSupportedCodecs(const CodecSet& supported_codecs,

[xhwang, 2016/04/19 23:59:42]

What is this parameter for? It's not documented and is not obvious.

[ddorwin, 2016/04/21 00:50:30]

Done.

                                   const std::vector<std::string>& codecs,
                                   const std::string& mime_type_lower_case,
                                   bool is_encrypted) const;
 
   // Converts a codec ID into an Codec enum value and indicates

[xhwang, 2016/04/19 23:59:42]

nit: s/an/a

also needs reflow

[ddorwin, 2016/04/21 00:50:30]

Done.
Done.

-  // whether the conversion was ambiguous.
+  // whether the specified codec, including profile, etc., is is known supported

[xhwang, 2016/04/19 23:59:42]

s/is is/is/

[ddorwin, 2016/04/21 00:50:30]

Done.

+  // ("probably") or not ("maybe"), such as a potentially unsupported profile.
   // Returns true if this method was able to map |codec_id| to a specific
-  // Codec enum value. |codec| and |is_ambiguous| are only valid if true
+  // Codec enum value. |codec| and |is_known_supported| are only valid if true
   // is returned. Otherwise their value is undefined after the call.
-  // |is_ambiguous| is true if |codec_id| did not have enough information to
-  // unambiguously determine the proper Codec enum value. If |is_ambiguous|
-  // is true |codec| contains the best guess for the intended Codec enum value.
   // |is_encrypted| means the codec will be used with encrypted blocks.
   bool StringToCodec(const std::string& codec_id,
                      Codec* codec,
-                     bool* is_ambiguous,
+                     bool* is_known_supported,

[xhwang, 2016/04/19 23:59:42]

Thanks for the background context on naming. So is |is_known_supported| strictly
the opposite of |is_ambiguous|? If so, can this just be |is_supported|, which is
"unambiguously" supported? It may seems odd in MimeUtil::AreSupportedCodecs()
when we check Is*Supported twice. But I don't really feel |is_known_supported|
is clearer.

Or, can we have a comment here about what |is_known_supported| exactly means?

[ddorwin, 2016/04/21 00:50:30]

No, it is not strictly the opposite. Ambiguous (now) means the string does not
give us enough information to determine whether we support it. As I said above,
this is now just the three strings in kAmbiguousCodecStringMap.

Ambiguous is a subset of things that are not |is_known_supported|. There are
others, including values we don't support (i.e. AVC extended profile). Most
relevant to this discussion, there are things like Hi10p, which may be supported
but we don't actually know (except for FFmpeg in this specific case).

I agree that we should have a clearer name. Maybe |is_probably_supported|, which
is consistent with "probably". It's also similar to "likely", which was one of
my suggestions.

I've updated the comment. I'll hold off on renaming until we pick a name.

[xhwang, 2016/04/21 05:59:44]

|is_probably_supported| is probably better, but not known better :) Joke aside,
I do feel |is_known_supported| is a bit too strong for what you described.
|is_probably_supported| does sound better to me.

[ddorwin, 2016/04/21 17:30:48]

Done.

                      bool is_encrypted) const;
 
   // Returns true if |codec| is supported when contained in
   // |mime_type_lower_case|. Note: This method will always return false for
   // proprietary codecs if |allow_proprietary_codecs_| is set to false.
   // |is_encrypted| means the codec will be used with encrypted blocks.
   bool IsCodecSupported(Codec codec,
                         const std::string& mime_type_lower_case,
                         bool is_encrypted) const;
 
@@ skipping 28 matching lines @@
   // Lookup table for string compare based string -> Codec mappings.
   StringToCodecMappings string_to_codec_map_;
 
   DISALLOW_COPY_AND_ASSIGN(MimeUtil);
 };
 
 }  // namespace internal
 }  // namespace media
 
 #endif  // MEDIA_BASE_MIME_UTIL_INTERNAL_H_