Do not show translate bar for MHTML files.

Do not show translate bar for MHTML files.

Add a check for MHTML files to TranslateManager::IsTranslatableURL(). This
causes the translate infobar to not be shown for MHTML files, because Chrome
fails when trying to translate MHTML files.
BUG=217945
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215878

[Dominik Grewe, 2013-07-22 17:36:04]

Please take a look at this CL.
Can you please also do a try (I haven't got permission to do so yet).

Thanks!

[Miguel Garcia, 2013-07-22 17:42:00]

Pass by reviews. You should be able to run try bots directly from the web
interface

https://codereview.chromium.org/19857005/diff/1/chrome/browser/translate/tran...
File chrome/browser/translate/translate_manager.cc (right):

https://codereview.chromium.org/19857005/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_manager.cc:107: // get file extension (empty
if URL does not reference a file)
I'd move this to a method and add a unittest for it (although it is interesting
that no such method exist already, can you double check it's not the case?)

https://codereview.chromium.org/19857005/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_manager.cc:126: !(file_extension == "mht" ||
file_extension == "mhtml");
create constants perhaps?

[Dominik Grewe, 2013-07-22 17:59:05]

On 2013/07/22 17:42:00, Miguel Garcia wrote:
> Pass by reviews. You should be able to run try bots directly from the web
> interface
Yes, the try bots seem to be running and I'm able to add more.

>
https://codereview.chromium.org/19857005/diff/1/chrome/browser/translate/tran...
> chrome/browser/translate/translate_manager.cc:107: // get file extension
(empty
> if URL does not reference a file)
> I'd move this to a method and add a unittest for it (although it is
interesting
> that no such method exist already, can you double check it's not the case?)
There's actually a method to extract the file name: GURL::ExtractFileName().
Should I add another method for the file extension
("GURL::ExtractFileExtension()") or simply do a rfind on the result?


>
https://codereview.chromium.org/19857005/diff/1/chrome/browser/translate/tran...
> chrome/browser/translate/translate_manager.cc:126: !(file_extension == "mht"
||
> file_extension == "mhtml");
> create constants perhaps?
I'll do that.

[Dominik Grewe, 2013-07-23 11:20:27]

Brett, can you please take a look at my implementation of
GURL::ExtractFileExtension().

[MAD, 2013-07-29 20:39:17]

One question...

BYE
MAD

https://chromiumcodereview.appspot.com/19857005/diff/18001/chrome/browser/tra...
File chrome/browser/translate/translate_manager.cc (right):

https://chromiumcodereview.appspot.com/19857005/diff/18001/chrome/browser/tra...
chrome/browser/translate/translate_manager.cc:122:
!(file_extension.compare(kFileExtMHTMLShort) == 0 ||
Shouldn't we do a case insensitive compare?

[Dominik Grewe, 2013-07-30 09:33:16]

On 2013/07/29 20:39:17, MAD wrote:
> chrome/browser/translate/translate_manager.cc:122:
> !(file_extension.compare(kFileExtMHTMLShort) == 0 ||
> Shouldn't we do a case insensitive compare?

Yeah, that's a good point, thanks. I've updated accordingly.

[MAD, 2013-07-30 13:29:19]

Thanks!

translate/ LGTM

BYE
MAD...

[brettw, 2013-08-01 22:50:49]

I'm not sure this is the right solution.

Generally on the web the extension is never used and we go by MIME types. It's
entirely possible and reasonable for me to serve HTML content or even GIFs from
my web server with a file name ending in .mhtml. Checking the stuff after the
dot in the file name doesn't tell you if the page is an MHTML file.

I think this is why we don't have an existing function to get the extension on
GURL, and I'm hesitant to add it, partially because I think it will encourage
people to do this incorrect type of sniffing.

[Dominik Grewe, 2013-08-02 09:46:22]

On 2013/08/01 22:50:49, brettw wrote:
> I'm not sure this is the right solution.
> 
> Generally on the web the extension is never used and we go by MIME types. It's
> entirely possible and reasonable for me to serve HTML content or even GIFs
from
> my web server with a file name ending in .mhtml. Checking the stuff after the
> dot in the file name doesn't tell you if the page is an MHTML file.

It seems like this is only true some of the time (at least for opening local
files). I tried opening a number of files in Chrome with "wrong" file
extensions. In some cases the file extension was ignored but in others it
wasn't. My best guess is that it is ignored for files stored in binary, e.g.
gif, but not for files stored in text, e.g. html, mhtml (try opening an HTML
file ending in ".gif", for example).
There's a function MimeUtil::GetMimeTypeFromFile() which returns the mime type
by looking at the file extension:
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_util...

Coming back to MHTML files, it seems they are only opened correctly if they're
stored locally (i.e. file://...) and they have the file extension "mht" or
"mhtml". When trying to open an .mht file from a web server by typing the URL
into the omnibox Chrome fails.

So I'd say that checking for the file extension (maybe in addition to checking
if the URL scheme is file) is a valid way of checking for MHTML files. What do
you think?

[Dominik Grewe, 2013-08-05 11:39:01]

I think I found a much cleaner solution following brettw's suggestion. What I
didn't realize before is that you can simply query the MIME type from the
WebContent object. This way there's no need to extract file extensions in order
to identify MHTML pages and I thus undid all changes to GURL.

MAD, can you please have another look at this patch?

[MAD, 2013-08-05 15:02:27]

I didn't know about it either, and I agree it's much cleaner like this...
Thanks!

LGTM with two comments nits...

BYE
MAD

https://codereview.chromium.org/19857005/diff/38001/chrome/browser/translate/...
File chrome/browser/translate/translate_manager.cc (right):

https://codereview.chromium.org/19857005/diff/38001/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:335: // MHTML pages currently
cannot be translated
Add a period (.) at the end of the sentence please.

https://codereview.chromium.org/19857005/diff/38001/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:336: // see bug: 217945
Same here. And use a Capital letter at beginning of sentence, or end previous
line with a , instead of a .

[brettw, 2013-08-05 23:10:55]

Sweet, thanks for looking for that.

[commit-bot: I haz the power, 2013-08-06 08:51:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominikg@chromium.org/19857005/44001

[commit-bot: I haz the power, 2013-08-06 12:42:51]

Change committed as 215878