replace memcmp with MagicCmp that supports '.' for single character of anythi...

replace memcmp with MagicCmp that supports '.' for single character of anything and add webp
BUG=67987
TEST=omnibox can show webp from filer or harddrive when mimetype or extension is wrong


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

[fbarchard, 2011-01-11 18:06:27]

Added MagicCmp() to compare two magic numbers

[Tom Finegan, 2011-01-11 18:51:11]

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

http://codereview.chromium.org/6124007/diff/1/net/base/mime_sniffer.cc#newcod...
net/base/mime_sniffer.cc:223: if (!*content && !magic_entry) {
I think you want !*magic_entry here; at least if your intent is to compare the
value instead of checking for a not null ptr.  If checking the ptr is necessary
I'd move that outside the loop.

http://codereview.chromium.org/6124007/diff/1/net/base/mime_sniffer.cc#newcod...
net/base/mime_sniffer.cc:226: if (!*content || !magic_entry)
!*magic_entry again, assuming I'm correct above anyway.

http://codereview.chromium.org/6124007/diff/1/net/base/mime_sniffer.cc#newcod...
net/base/mime_sniffer.cc:259: match = MagicCmp(magic_entry->magic, content,
len);
Not that I have any major issue with being more specific, and using MagicCmp to
explicitly say what we're doing here-- I just wonder what's wrong w/memcmp.

[fbarchard1, 2011-01-11 19:20:11]

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

http://codereview.chromium.org/6124007/diff/1/net/base/mime_sniffer.cc#newcod...
net/base/mime_sniffer.cc:223: if (!*content && !magic_entry) {
On 2011/01/11 18:51:11, tomf wrote:
> I think you want !*magic_entry here; at least if your intent is to compare the
> value instead of checking for a not null ptr.  If checking the ptr is
necessary
> I'd move that outside the loop.

Done.

http://codereview.chromium.org/6124007/diff/1/net/base/mime_sniffer.cc#newcod...
net/base/mime_sniffer.cc:226: if (!*content || !magic_entry)
On 2011/01/11 18:51:11, tomf wrote:
> !*magic_entry again, assuming I'm correct above anyway.

Done.

http://codereview.chromium.org/6124007/diff/1/net/base/mime_sniffer.cc#newcod...
net/base/mime_sniffer.cc:259: match = MagicCmp(magic_entry->magic, content,
len);
On 2011/01/11 18:51:11, tomf wrote:
> Not that I have any major issue with being more specific, and using MagicCmp
to
> explicitly say what we're doing here-- I just wonder what's wrong w/memcmp.

Done.  Added comments

[Tom Finegan, 2011-01-11 19:48:25]

LGTM

[abarth-chromium, 2011-01-11 19:50:46]

http://codereview.chromium.org/6124007/diff/7001/net/base/mime_sniffer.cc
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/6124007/diff/7001/net/base/mime_sniffer.cc#new...
net/base/mime_sniffer.cc:224: break;
We don't want to start at null bytes, right?  Also, how could magic_entry be
null?

http://codereview.chromium.org/6124007/diff/7001/net/base/mime_sniffer.cc#new...
net/base/mime_sniffer.cc:225: same = (*magic_entry == *content) || (*magic_entry
== '.' && *content);
So "." matches everything that's not zero?  Why wouldn't it match zero?

http://codereview.chromium.org/6124007/diff/7001/net/base/mime_sniffer.cc#new...
net/base/mime_sniffer.cc:232: return same;
We should just early return from the loop rather than have this extra bool.

[abarth-chromium, 2011-01-11 19:51:29]

http://codereview.chromium.org/6124007/diff/11001/net/base/mime_sniffer.cc
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/6124007/diff/11001/net/base/mime_sniffer.cc#ne...
net/base/mime_sniffer.cc:225: if (!*content && !*magic_entry)
I don't think this is correct.  We want to match null.

[fbarchard1, 2011-01-11 19:57:38]

http://codereview.chromium.org/6124007/diff/11001/net/base/mime_sniffer.cc
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/6124007/diff/11001/net/base/mime_sniffer.cc#ne...
net/base/mime_sniffer.cc:225: if (!*content && !*magic_entry)
On 2011/01/11 19:51:29, abarth wrote:
> I don't think this is correct.  We want to match null.

I think its correct.  If both strings have been the same up to this point, even
if len is >=1, and both terminate with null at the same time, then the compare
returns true... the strings are the same.

[Avi (use Gerrit), 2011-01-11 20:00:02]

http://codereview.chromium.org/6124007/diff/11001/net/base/mime_sniffer.cc
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/6124007/diff/11001/net/base/mime_sniffer.cc#ne...
net/base/mime_sniffer.cc:225: if (!*content && !*magic_entry)
Given this argument between you two, I think it's clear that MagicCmp needs a
unit test.

[fbarchard1, 2011-01-11 20:01:47]

redone as memcmp style instead of strcmp... ignore nulls
early out instead of bool variable.

http://codereview.chromium.org/6124007/diff/7001/net/base/mime_sniffer.cc
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/6124007/diff/7001/net/base/mime_sniffer.cc#new...
net/base/mime_sniffer.cc:224: break;
On 2011/01/11 19:50:46, abarth wrote:
> We don't want to start at null bytes, right?  Also, how could magic_entry be
> null?

This was a typo... fixed.

http://codereview.chromium.org/6124007/diff/7001/net/base/mime_sniffer.cc#new...
net/base/mime_sniffer.cc:225: same = (*magic_entry == *content) || (*magic_entry
== '.' && *content);
On 2011/01/11 19:50:46, abarth wrote:
> So "." matches everything that's not zero?  Why wouldn't it match zero?

Good point.  I was thinking strings, but this is a memcmp.

http://codereview.chromium.org/6124007/diff/7001/net/base/mime_sniffer.cc#new...
net/base/mime_sniffer.cc:232: return same;
On 2011/01/11 19:50:46, abarth wrote:
> We should just early return from the loop rather than have this extra bool.
ok, will refactor.

[abarth-chromium, 2011-01-11 20:50:03]

LGTM, thanks.