Move function for classifying a string as a mime type into MimeUtil

Move function for classifying a string as a mime type into MimeUtil

R=rvargas@chromium.org
BUG=None
TEST=MimeUtilTest.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140657

[cbentzel, 2012-05-31 23:58:48]

Asanka may also be interested.

[rvargas (doing something else), 2012-06-01 01:51:14]

http://codereview.chromium.org/10448109/diff/1/chrome/browser/intents/web_int...
File chrome/browser/intents/web_intents_registry.cc (right):

http://codereview.chromium.org/10448109/diff/1/chrome/browser/intents/web_int...
chrome/browser/intents/web_intents_registry.cc:44: return
net::MatchesMimeType(UTF16ToUTF8(type_str));
IsStringMimeType?

http://codereview.chromium.org/10448109/diff/1/net/base/mime_util.cc
File net/base/mime_util.cc (right):

http://codereview.chromium.org/10448109/diff/1/net/base/mime_util.cc#newcode491
net/base/mime_util.cc:491: bool MimeUtil::IsStringMimeType(const std::string&
type_string) const {
Shouldn't this build upon IsSupportedMimeType instead?

http://codereview.chromium.org/10448109/diff/1/net/base/mime_util.cc#newcode513
net/base/mime_util.cc:513: // If there's a "/" separator character, and the
token before it is
If we have no code to cover this case maybe we can add something specifically
for it. Enumerating mime types again doesn't make much sense.

[Greg Billock, 2012-06-01 04:58:22]

http://codereview.chromium.org/10448109/diff/1/net/base/mime_util.cc
File net/base/mime_util.cc (right):

http://codereview.chromium.org/10448109/diff/1/net/base/mime_util.cc#newcode491
net/base/mime_util.cc:491: bool MimeUtil::IsStringMimeType(const std::string&
type_string) const {
On 2012/06/01 01:51:14, rvargas wrote:
> Shouldn't this build upon IsSupportedMimeType instead?

How so? I think this function would basically have to do the same checks anyway,
since IsSupportedMimeTypes is about a whitelist, whereas this is checking
syntactically whether a string is a valid mime type specifier.

[rvargas (doing something else), 2012-06-01 19:11:25]

http://codereview.chromium.org/10448109/diff/1/net/base/mime_util.cc
File net/base/mime_util.cc (right):

http://codereview.chromium.org/10448109/diff/1/net/base/mime_util.cc#newcode491
net/base/mime_util.cc:491: bool MimeUtil::IsStringMimeType(const std::string&
type_string) const {
On 2012/06/01 04:58:22, Greg Billock wrote:
> On 2012/06/01 01:51:14, rvargas wrote:
> > Shouldn't this build upon IsSupportedMimeType instead?
> 
> How so? I think this function would basically have to do the same checks
anyway,
> since IsSupportedMimeTypes is about a whitelist, whereas this is checking
> syntactically whether a string is a valid mime type specifier.

You're right, the "supported" part makes all the difference :)

http://codereview.chromium.org/10448109/diff/1/net/base/mime_util.cc#newcode497
net/base/mime_util.cc:497: StringToLowerASCII(&raw_type);
either this is not needed, or the comparisons should be case sensitive.

http://codereview.chromium.org/10448109/diff/1/net/base/mime_util.cc#newcode500
net/base/mime_util.cc:500: if (StartsWithASCII(raw_type, "application/", false)
||
Then let's follow the pattern of this class and make this into a static array.

[Greg Billock, 2012-06-05 01:08:55]

http://codereview.chromium.org/10448109/diff/1/net/base/mime_util.cc
File net/base/mime_util.cc (right):

http://codereview.chromium.org/10448109/diff/1/net/base/mime_util.cc#newcode497
net/base/mime_util.cc:497: StringToLowerASCII(&raw_type);
On 2012/06/01 19:11:25, rvargas wrote:
> either this is not needed, or the comparisons should be case sensitive.

Done.

http://codereview.chromium.org/10448109/diff/1/net/base/mime_util.cc#newcode500
net/base/mime_util.cc:500: if (StartsWithASCII(raw_type, "application/", false)
||
On 2012/06/01 19:11:25, rvargas wrote:
> Then let's follow the pattern of this class and make this into a static array.

Done.

[rvargas (doing something else), 2012-06-05 17:54:07]

Mostly nits.

https://chromiumcodereview.appspot.com/10448109/diff/8001/chrome/browser/inte...
File chrome/browser/intents/web_intents_registry.cc (right):

https://chromiumcodereview.appspot.com/10448109/diff/8001/chrome/browser/inte...
chrome/browser/intents/web_intents_registry.cc:44: return
net::MatchesMimeType(UTF16ToUTF8(type_str));
Still using a different fn name.

https://chromiumcodereview.appspot.com/10448109/diff/8001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://chromiumcodereview.appspot.com/10448109/diff/8001/net/base/mime_util....
net/base/mime_util.cc:517: top_level_legal = true;
Why not return true instead?

https://chromiumcodereview.appspot.com/10448109/diff/8001/net/base/mime_util....
net/base/mime_util.cc:529: return false;
nit: use {} here.

https://chromiumcodereview.appspot.com/10448109/diff/8001/net/base/mime_util.h
File net/base/mime_util.h (right):

https://chromiumcodereview.appspot.com/10448109/diff/8001/net/base/mime_util....
net/base/mime_util.h:64: NET_EXPORT bool IsStringMimeType(const std::string&
type_string);
nit: maybe IsMimeType is a better name (and blends better with the rest)

https://chromiumcodereview.appspot.com/10448109/diff/8001/net/base/mime_util_...
File net/base/mime_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/10448109/diff/8001/net/base/mime_util_...
net/base/mime_util_unittest.cc:153: EXPECT_FALSE(IsStringMimeType("x-/mime"));
Maybe add one for "x-video/"

[Greg Billock, 2012-06-05 18:09:53]

https://chromiumcodereview.appspot.com/10448109/diff/8001/chrome/browser/inte...
File chrome/browser/intents/web_intents_registry.cc (right):

https://chromiumcodereview.appspot.com/10448109/diff/8001/chrome/browser/inte...
chrome/browser/intents/web_intents_registry.cc:44: return
net::MatchesMimeType(UTF16ToUTF8(type_str));
On 2012/06/05 17:54:07, rvargas wrote:
> Still using a different fn name.

Done.

https://chromiumcodereview.appspot.com/10448109/diff/8001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://chromiumcodereview.appspot.com/10448109/diff/8001/net/base/mime_util....
net/base/mime_util.cc:517: top_level_legal = true;
On 2012/06/05 17:54:07, rvargas wrote:
> Why not return true instead?

Thanks, I failed the refactor there.

https://chromiumcodereview.appspot.com/10448109/diff/8001/net/base/mime_util....
net/base/mime_util.cc:529: return false;
On 2012/06/05 17:54:07, rvargas wrote:
> nit: use {} here.

Done.

https://chromiumcodereview.appspot.com/10448109/diff/8001/net/base/mime_util.h
File net/base/mime_util.h (right):

https://chromiumcodereview.appspot.com/10448109/diff/8001/net/base/mime_util....
net/base/mime_util.h:64: NET_EXPORT bool IsStringMimeType(const std::string&
type_string);
On 2012/06/05 17:54:07, rvargas wrote:
> nit: maybe IsMimeType is a better name (and blends better with the rest)

Good point. Changed.

https://chromiumcodereview.appspot.com/10448109/diff/8001/net/base/mime_util_...
File net/base/mime_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/10448109/diff/8001/net/base/mime_util_...
net/base/mime_util_unittest.cc:153: EXPECT_FALSE(IsStringMimeType("x-/mime"));
On 2012/06/05 17:54:07, rvargas wrote:
> Maybe add one for "x-video/"

Done.

[rvargas (doing something else), 2012-06-05 18:31:19]

LGTM

[commit-bot: I haz the power, 2012-06-05 18:36:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10448109/17001

[commit-bot: I haz the power, 2012-06-05 19:29:17]

Try job failure for 10448109-17001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-06-05 22:25:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10448109/23003

[commit-bot: I haz the power, 2012-06-06 00:06:15]

Change committed as 140657