Translate: enable against unknown languages with server side detection

Translate: enable against unknown languages with server side detection

Enable context menu for Chrome Translate to perform translation even if
a page is written in unknown language to CLD.
This change will need platform specific UI change for Win, Mac, and Linux.

BUG=172593
TBR=estade@chromium.org, mad@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201715

[Takashi Toyoshima, 2013-05-17 13:59:23]

Hi mad,
Sorry for many CLs, but can you take a look this?

Also add OWNERs, brettw for tab_contents, estade for resources.

Thanks in advance.

[MAD, 2013-05-17 17:33:11]

Two requests before I approve this...

Thanks!

BYE
MAD

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tab_...
File chrome/browser/tab_contents/render_view_context_menu.cc (left):

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tab_...
chrome/browser/tab_contents/render_view_context_menu.cc:1219:
TranslateManager::IsTranslatableURL(params_.page_url) &&
Why did you remove this? I don't think we want to translate these pages, and if
we do, we should handle this change separately from this change, which is about
a fallback, not adding new support for pages that we didn't want to translate
before.

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tran...
File chrome/browser/translate/translate_manager.cc (right):

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tran...
chrome/browser/translate/translate_manager.cc:777: // TODO(jcivelli):
http://crbug.com/48918 We should change the "after
Do we still need this TODO? The bug you refer to is actually marked as fixed...
We should at least change the owner of the TODO since jcivelli doesn't work on
translate anymore.

[Takashi Toyoshima, 2013-05-17 17:57:07]

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tab_...
File chrome/browser/tab_contents/render_view_context_menu.cc (left):

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tab_...
chrome/browser/tab_contents/render_view_context_menu.cc:1219:
TranslateManager::IsTranslatableURL(params_.page_url) &&
Sorry, I just mistakenly mix a part of another CL into this CL.

In another CL, I stop to add menu item itself when IsTranslatableURL() is false.
So, this check is redundant and is always true when this code path runs.

Anyway, I'll restore this line in this CL.

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tran...
File chrome/browser/translate/translate_manager.cc (right):

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tran...
chrome/browser/translate/translate_manager.cc:777: // TODO(jcivelli):
http://crbug.com/48918 We should change the "after
It looks fine to remove this TODO now.
Or we should check if l10n_util::GetDisplayNameForLocale() supports the source
language, I think.

Also, following UMA can be removed in the future, because I'll add another UMA
to report on showing all kinds of error infobars.

[MAD, 2013-05-17 18:42:24]

Once the requested changes have been done, LGTM!

Thanks...

BYE
MAD

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tab_...
File chrome/browser/tab_contents/render_view_context_menu.cc (left):

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tab_...
chrome/browser/tab_contents/render_view_context_menu.cc:1219:
TranslateManager::IsTranslatableURL(params_.page_url) &&
On 2013/05/17 17:57:07, Takashi Toyoshima (chromium) wrote:
> Sorry, I just mistakenly mix a part of another CL into this CL.
> 
> In another CL, I stop to add menu item itself when IsTranslatableURL() is
false.
> So, this check is redundant and is always true when this code path runs.
> 
> Anyway, I'll restore this line in this CL. 

Thanks!

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tran...
File chrome/browser/translate/translate_manager.cc (right):

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tran...
chrome/browser/translate/translate_manager.cc:777: // TODO(jcivelli):
http://crbug.com/48918 We should change the "after
OK, so for now, can you replace jcivelli with your name and come back to it
later?

[Evan Stade, 2013-05-18 23:53:52]

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/reso...
File chrome/browser/resources/translate.js (right):

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/reso...
chrome/browser/resources/translate.js:132: * the translate function was 'auto'.
Is empty otherwise.
this comment needs updating.

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/reso...
chrome/browser/resources/translate.js:139: return 'und';
why 'und' and not undefined or 'undefined' or whatever 'und' is supposed to
stand for?

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/reso...
chrome/browser/resources/translate.js:140: return lib.getDetectedLanguage();
you can write this as lib.getDetectedLanguage() || 'und';

[Takashi Toyoshima, 2013-05-20 06:56:08]

Thanks, mad and evan.

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/reso...
File chrome/browser/resources/translate.js (right):

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/reso...
chrome/browser/resources/translate.js:132: * the translate function was 'auto'.
Is empty otherwise.
On 2013/05/18 23:53:52, Evan Stade wrote:
> this comment needs updating.

Done.

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/reso...
chrome/browser/resources/translate.js:139: return 'und';
'und' is a magic word defined as chrome::kUnknownLanguageCode and widely used in
Chrome Translate.
I'll add comment here.

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/reso...
chrome/browser/resources/translate.js:140: return lib.getDetectedLanguage();
It doesn't work.
Undefined one is not returned value, but lib.getDetectedLanguage itself.
So, when call lib.getDetectedLanguage(), JavaScript stops running due to calling
undefined attribute.

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tab_...
File chrome/browser/tab_contents/render_view_context_menu.cc (left):

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tab_...
chrome/browser/tab_contents/render_view_context_menu.cc:1219:
TranslateManager::IsTranslatableURL(params_.page_url) &&
On 2013/05/17 18:42:25, MAD wrote:
> On 2013/05/17 17:57:07, Takashi Toyoshima (chromium) wrote:
> > Sorry, I just mistakenly mix a part of another CL into this CL.
> > 
> > In another CL, I stop to add menu item itself when IsTranslatableURL() is
> false.
> > So, this check is redundant and is always true when this code path runs.
> > 
> > Anyway, I'll restore this line in this CL. 
> 
> Thanks!

Done.

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tran...
File chrome/browser/translate/translate_manager.cc (right):

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/tran...
chrome/browser/translate/translate_manager.cc:777: // TODO(jcivelli):
http://crbug.com/48918 We should change the "after
On 2013/05/17 18:42:25, MAD wrote:
> OK, so for now, can you replace jcivelli with your name and come back to it
> later?

Done.

[Takashi Toyoshima, 2013-05-23 00:00:03]

ping estade

[Evan Stade, 2013-05-23 00:53:28]

lgtm

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/reso...
File chrome/browser/resources/translate.js (right):

https://chromiumcodereview.appspot.com/15295004/diff/2001/chrome/browser/reso...
chrome/browser/resources/translate.js:140: return lib.getDetectedLanguage();
On 2013/05/20 06:56:08, Takashi Toyoshima (chromium) wrote:
> It doesn't work.
> Undefined one is not returned value, but lib.getDetectedLanguage itself.
> So, when call lib.getDetectedLanguage(), JavaScript stops running due to
calling
> undefined attribute.

ah, I misread it

[commit-bot: I haz the power, 2013-05-23 02:44:52]

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

[commit-bot: I haz the power, 2013-05-23 02:59:25]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Takashi Toyoshima, 2013-05-23 03:10:22]

aha, presubmit check doesn't work on change toughing translate.js because of
unavoidable style check error... I'll use dcommit after git try.

[Takashi Toyoshima, 2013-05-23 05:41:47]

Committed patchset #10 manually as r201715 (presubmit successful).