Fixing mime type so that MHTML files can be opened directly from Google Drive.

Fixing mime type so that MHTML files can be opened directly from Google Drive.

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

[Greg Spencer (Chromium), 2012-06-20 16:44:56]

Just some style comments.

http://codereview.chromium.org/10559086/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_protocol_handler.cc (right):

http://codereview.chromium.org/10559086/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_protocol_handler.cc:326: const struct
MimeTypeReplacement {
Can you move this to the top of the file, under
kInitialDownloadBufferSizeInBytes?

http://codereview.chromium.org/10559086/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_protocol_handler.cc:328: const char*
new_type;
Can you please split this into a struct definition and a separate declaration of
the const array?  I know it's semantically the same, but it's easier to read.

http://codereview.chromium.org/10559086/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_protocol_handler.cc:333: std::string
FixupMimeType(const std::string& type) {
Since this isn't a member of the class here, can you move it to the top of the
anonymous namespace, probably after CancelGDataDownloadOnUIThread (the last
non-member anonymous namespace function).

Typical ordering in a file is:
1) #includes
2) static class member variable declarations
3) anonymous namespace start
4) file-local constants and types
5) file-local functions
6) file-local classes
7) end anonymous namespace
8) class member function definitions

(#2 could also go before #8, I think).

[tudalex(Chromium), 2012-06-20 18:20:30]

http://codereview.chromium.org/10559086/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_protocol_handler.cc (right):

http://codereview.chromium.org/10559086/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_protocol_handler.cc:326: const struct
MimeTypeReplacement {
On 2012/06/20 16:44:56, Greg Spencer (Chromium) wrote:
> Can you move this to the top of the file, under
> kInitialDownloadBufferSizeInBytes?

Done.

http://codereview.chromium.org/10559086/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_protocol_handler.cc:328: const char*
new_type;
On 2012/06/20 16:44:56, Greg Spencer (Chromium) wrote:
> Can you please split this into a struct definition and a separate declaration
of
> the const array?  I know it's semantically the same, but it's easier to read.

Done.

http://codereview.chromium.org/10559086/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_protocol_handler.cc:333: std::string
FixupMimeType(const std::string& type) {
On 2012/06/20 16:44:56, Greg Spencer (Chromium) wrote:
> Since this isn't a member of the class here, can you move it to the top of the
> anonymous namespace, probably after CancelGDataDownloadOnUIThread (the last
> non-member anonymous namespace function).
> 
> Typical ordering in a file is:
> 1) #includes
> 2) static class member variable declarations
> 3) anonymous namespace start
> 4) file-local constants and types
> 5) file-local functions
> 6) file-local classes
> 7) end anonymous namespace
> 8) class member function definitions
> 
> (#2 could also go before #8, I think).

Done.

[Greg Spencer (Chromium), 2012-06-20 18:29:24]

LGTM from my point of view.

Tony, Alex needs an OWNERS review for this.

[tbarzic, 2012-06-20 18:32:30]

On 2012/06/20 18:29:24, Greg Spencer (Chromium) wrote:
> LGTM from my point of view.
> 
> Tony, Alex needs an OWNERS review for this.

lgtm

[commit-bot: I haz the power, 2012-06-20 18:46:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tudalex@chromium.org/10559086/6001

[commit-bot: I haz the power, 2012-06-20 20:44:23]

Change committed as 143253