Disallow apps with the wrong content type header.

Disallow apps with the wrong content type header.

BUG=41290
TEST=manual

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

[Aaron Boodman, 2010-06-22 20:32:44]

Cool. Just some nits.

http://codereview.chromium.org/2830017/diff/8001/3010
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/2830017/diff/8001/3010#newcode270
chrome/browser/download/download_manager.h:270: std::string content_type_;
How about original_mime_type_. This makes its relationship to mime_type_ clear.
People use "mime type" and "content type" interchangably.

http://codereview.chromium.org/2830017/diff/8001/3012
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/2830017/diff/8001/3012#newcode139
chrome/browser/extensions/crx_installer.cc:139: if (extension->is_app() &&
We should also skip this check if the source URL is file:/// to make development
easier.

http://codereview.chromium.org/2830017/diff/8001/3013
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/2830017/diff/8001/3013#newcode93
chrome/browser/extensions/crx_installer.h:93: void WasDownloaded(const
std::string& content_type) {
Naming nits:

* use original_mime_type_ here for consistency.
* instead of was_downloaded_, how about making the purpose of this flag more
explicit: require_extension_mime_type.
* setters in Chromium should use unix_hacker_style()
* I would prefer separating the two setters (require_extension_mime_type() and
original_mime_type()). I think that feels a little simpler.

http://codereview.chromium.org/2830017/diff/8001/3014
File chrome/browser/history/download_types.h (right):

http://codereview.chromium.org/2830017/diff/8001/3014#newcode84
chrome/browser/history/download_types.h:84: std::string content_type;
Comment nit: The mime_type cannot be determined by examining the file. The mime
type sniffing that we do is a hack because sometimes servers don't correctly set
the content type header.

[Sam Kerner (Chrome), 2010-06-22 22:24:39]

Ready for another look.

http://codereview.chromium.org/2830017/diff/8001/3010
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/2830017/diff/8001/3010#newcode270
chrome/browser/download/download_manager.h:270: std::string content_type_;
On 2010/06/22 20:32:45, Aaron Boodman wrote:
> How about original_mime_type_. This makes its relationship to mime_type_
clear.
> People use "mime type" and "content type" interchangably.

Done.

http://codereview.chromium.org/2830017/diff/8001/3012
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/2830017/diff/8001/3012#newcode139
chrome/browser/extensions/crx_installer.cc:139: if (extension->is_app() &&
On 2010/06/22 20:32:45, Aaron Boodman wrote:
> We should also skip this check if the source URL is file:/// to make
development
> easier.

Done.

http://codereview.chromium.org/2830017/diff/8001/3013
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/2830017/diff/8001/3013#newcode93
chrome/browser/extensions/crx_installer.h:93: void WasDownloaded(const
std::string& content_type) {
On 2010/06/22 20:32:45, Aaron Boodman wrote:
> Naming nits:
> 
> * use original_mime_type_ here for consistency.
> * instead of was_downloaded_, how about making the purpose of this flag more
> explicit: require_extension_mime_type.
> * setters in Chromium should use unix_hacker_style()

I went with CamelCase because the function as written is not a normal setter. 
It alters more than one member variable.

> * I would prefer separating the two setters (require_extension_mime_type() and
> original_mime_type()). I think that feels a little simpler.

Done.

http://codereview.chromium.org/2830017/diff/8001/3014
File chrome/browser/history/download_types.h (right):

http://codereview.chromium.org/2830017/diff/8001/3014#newcode84
chrome/browser/history/download_types.h:84: std::string content_type;
On 2010/06/22 20:32:45, Aaron Boodman wrote:
> Comment nit: The mime_type cannot be determined by examining the file. The
mime
> type sniffing that we do is a hack because sometimes servers don't correctly
set
> the content type header.

Done.

[Aaron Boodman, 2010-06-24 18:07:19]

lgtm