Implement mimetype sniffing for extensions.

Implement mimetype sniffing for extensions.

abarth: can you review the changes to mime_sniffer.cc?
paul: everything else?

BUG=13296
TEST=Added unit tests

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

[abarth-chromium, 2009-07-24 09:37:39]

http://codereview.chromium.org/159345/diff/1010/1017
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/159345/diff/1010/1017#newcode259
Line 259: if (counter) counter->Add(static_cast<int>(i));
This change will be unnecessary once you add the histogram (see below).

http://codereview.chromium.org/159345/diff/1010/1017#newcode456
Line 456: url.path().size() - kExtensionLength)
I don't think this is right.  Consider this URL:

http://www.example.com/foo.crx.bar.crx

You should use rfind instead.  Please add a test case for this.  It's too bad
all our cool file_utils don't work without FilePath any more.

http://codereview.chromium.org/159345/diff/1010/1017#newcode461
Line 461: NULL, result))
Please add a histogram and pass it in here.  We want to know how often this
heuristic fires.  (We can compare the absolute numbers with the other
histograms.)

http://codereview.chromium.org/159345/diff/1010/1018
File net/base/mime_sniffer_unittest.cc (right):

http://codereview.chromium.org/159345/diff/1010/1018#newcode115
Line 115: };
Please add a success test case without a query string.  Also, please add tests
for http://crx.example.com/foo http://www.example.crx and variations.  Also add
a failure test case for http://www.example.com/foocrx and a success test case
for http://www.example.com/foo..crx (also the test case for the bug mentioned
earlier).

These test cases are cheap.  Might as well test the snot out of it.

[Aaron Boodman, 2009-07-24 10:36:59]

http://codereview.chromium.org/159345/diff/1010/1017
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/159345/diff/1010/1017#newcode456
Line 456: url.path().size() - kExtensionLength)
On 2009/07/24 09:37:39, abarth wrote:
> I don't think this is right.  Consider this URL:
> 
> http://www.example.com/foo.crx.bar.crx
> 
> You should use rfind instead.  Please add a test case for this.  It's too bad
> all our cool file_utils don't work without FilePath any more.

Nuts. Fixed, thanks.

http://codereview.chromium.org/159345/diff/1010/1017#newcode461
Line 461: NULL, result))
On 2009/07/24 09:37:39, abarth wrote:
> Please add a histogram and pass it in here.  We want to know how often this
> heuristic fires.  (We can compare the absolute numbers with the other
> histograms.)

I tried creating one originally, but the Histogram class doesn't like having
only one bucket. So instead, I've created a histogram that contains the various
ways we can exit this function and a TODO to create a histogram for
CheckForMagicNumbers() if we ever have two of them.

http://codereview.chromium.org/159345/diff/1010/1018
File net/base/mime_sniffer_unittest.cc (right):

http://codereview.chromium.org/159345/diff/1010/1018#newcode115
Line 115: };
On 2009/07/24 09:37:39, abarth wrote:
> Please add a success test case without a query string.  Also, please add tests
> for http://crx.example.com/foo http://www.example.crx and variations.  Also
add
> a failure test case for http://www.example.com/foocrx and a success test case
> for http://www.example.com/foo..crx (also the test case for the bug mentioned
> earlier).
> 
> These test cases are cheap.  Might as well test the snot out of it.

Done.

[abarth-chromium, 2009-07-24 17:34:33]

The mime_sniffer part looks good (with two nits).  Thanks!

http://codereview.chromium.org/159345/diff/41/48
File net/base/mime_sniffer.cc (right):

http://codereview.chromium.org/159345/diff/41/48#newcode503
Line 503: };
trailing whitespace snuck in.

http://codereview.chromium.org/159345/diff/41/49
File net/base/mime_sniffer_unittest.cc (right):

http://codereview.chromium.org/159345/diff/41/49#newcode120
Line 120: "", "application/x-chrome-extension" },
You're missing http://www.example.com/foo.crx.bar.crx which is what caused the
bug before.

[Aaron Boodman, 2009-07-24 17:38:41]

On Fri, Jul 24, 2009 at 10:34 AM, <[email protected]> wrote:
> http://codereview.chromium.org/159345/diff/41/49#newcode120
> Line 120: "", "application/x-chrome-extension" },
> You're missing http://www.example.com/foo.crx.bar.crx which is what
> caused the bug before.

Yeah, I didn't quite get your point there. I did fix the bug, but it
seems like foo.crx.bar.crx would in fact be a valid extension. I guess
I'm not clear on the rules for file extension. In that case is the
extension "crx.bar.crx" or "crx"? If the former, we really need
different logic entirely, not rfind or find. If the latter, I have a
test case for that: "foo.crx.blech".

- a

[Paul Godavari, 2009-07-24 18:52:02]

LGTM (didn't look too hard at the mime sniffer changes since Adam covered that).