Copy the implementations of TranslateUIDelegate to TransalteInfobarDelegate

Copy the implementations of TranslateUIDelegate to TransalteInfobarDelegate

TranslateInfobarDelegate won't own a WebContents by https://codereview.chromium.org/22694006/, while TranslateUIDelegate's implementation relies on the existence of a WebContents. Thus, I copied a part of implementations of TranslateUIDelegate to TranslteInfobarDelegate.

BUG=62154
TEST=browser_test --gtest_filter=TranslateManagerBrowserTest.*

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

[hajimehoshi, 2013-11-08 09:26:16]

Can you take a look?  Thanks.

[Peter Kasting, 2013-11-08 23:47:47]

LGTM

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
File chrome/browser/translate/translate_infobar_delegate.h (right):

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
chrome/browser/translate/translate_infobar_delegate.h:74: // updated.
Let's not put these comments into every function in both files.  A big "NOTE:"
sort of comment in the file comments for each file seems sufficient.

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
chrome/browser/translate/translate_infobar_delegate.h:217: // The list supported
languages for translation.
Nit: All three of these comments have grammar issues.  Are these the original
comments for these variables, back before the translate delegate?  If not,
restore the original comments.

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
File chrome/browser/translate/translate_ui_delegate.h (right):

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.h:30: // A pair whose key is a
language code and value is a language displyayable
Nit: a language displyayable -> the language's displayable

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.h:40: // in alphabetical order
taking |locale| into account.
I have no idea what language names this is returning or what "sorted taking
|locale| into account" means.

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.h:43:
std::vector<LanguageNamePair>* language_names);
Don't return via outparam, return by value.  RVO will make this fast.

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.h:45: // Gets the host of the
page of |web_content|, or an empty string if no URL is
Nit: Call this |web_contents|

[hajimehoshi, 2013-11-11 04:01:01]

Thanks!

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
File chrome/browser/translate/translate_infobar_delegate.h (right):

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
chrome/browser/translate/translate_infobar_delegate.h:74: // updated.
On 2013/11/08 23:47:47, Peter Kasting wrote:
> Let's not put these comments into every function in both files.  A big "NOTE:"
> sort of comment in the file comments for each file seems sufficient.

Done.

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
chrome/browser/translate/translate_infobar_delegate.h:217: // The list supported
languages for translation.
Done. They were original.: https://codereview.chromium.org/23708029

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
File chrome/browser/translate/translate_ui_delegate.h (right):

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.h:30: // A pair whose key is a
language code and value is a language displyayable
On 2013/11/08 23:47:47, Peter Kasting wrote:
> Nit: a language displyayable -> the language's displayable

Done.

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.h:40: // in alphabetical order
taking |locale| into account.
On 2013/11/08 23:47:47, Peter Kasting wrote:
> I have no idea what language names this is returning or what "sorted taking
> |locale| into account" means.

Done.

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.h:43:
std::vector<LanguageNamePair>* language_names);
On 2013/11/08 23:47:47, Peter Kasting wrote:
> Don't return via outparam, return by value.  RVO will make this fast.

Done.

https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.h:45: // Gets the host of the
page of |web_content|, or an empty string if no URL is
On 2013/11/08 23:47:47, Peter Kasting wrote:
> Nit: Call this |web_contents|

Done.

[hajimehoshi, 2013-11-11 04:02:01]

I'll wait for Toyoshima-san's response just in case.

[Peter Kasting, 2013-11-12 00:03:26]

(Still LGTM)

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
chrome/browser/translate/translate_infobar_delegate.cc:44: // updated.
Nit: Don't bother with this in the .cc files... or if you do, put it above all
the TranslateInfoBarDelegate pieces here, not after a few of them

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
File chrome/browser/translate/translate_ui_delegate.cc (right):

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.cc:53:
TranslateUIDelegate::GetSortedLanguageNames(const std::string& locale) {
Nit: Indent 4

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.cc:54:
std::vector<LanguageNamePair> language_names;
Nit: Declare this where first used

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
File chrome/browser/translate/translate_ui_delegate.h (right):

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.h:34: // displyayable name.
Nit: This still has a typo

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.h:43: // order, according to
|locale|.
So does this mean that the displayable names are chosen based on the locale?  If
so how about:

Retrieves the displayable names of the supported languages, based on |locale|. 
The returned names are sorted in alphabetical order.

[hajimehoshi, 2013-11-12 03:07:25]

Thank you!

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
chrome/browser/translate/translate_infobar_delegate.cc:44: // updated.
On 2013/11/12 00:03:27, Peter Kasting wrote:
> Nit: Don't bother with this in the .cc files... or if you do, put it above all
> the TranslateInfoBarDelegate pieces here, not after a few of them

Done (Removed)

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
File chrome/browser/translate/translate_ui_delegate.cc (right):

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.cc:53:
TranslateUIDelegate::GetSortedLanguageNames(const std::string& locale) {
On 2013/11/12 00:03:27, Peter Kasting wrote:
> Nit: Indent 4

Done.

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.cc:54:
std::vector<LanguageNamePair> language_names;
On 2013/11/12 00:03:27, Peter Kasting wrote:
> Nit: Declare this where first used

Done.

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
File chrome/browser/translate/translate_ui_delegate.h (right):

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.h:34: // displyayable name.
On 2013/11/12 00:03:27, Peter Kasting wrote:
> Nit: This still has a typo

Done.

https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate...
chrome/browser/translate/translate_ui_delegate.h:43: // order, according to
|locale|.
On 2013/11/12 00:03:27, Peter Kasting wrote:
> So does this mean that the displayable names are chosen based on the locale? 
If
> so how about:
> 
> Retrieves the displayable names of the supported languages, based on |locale|.

> The returned names are sorted in alphabetical order.

Done.

[Takashi Toyoshima, 2013-11-12 22:16:37]

LGTM with optional nits.

[Takashi Toyoshima, 2013-11-12 22:16:55]

https://codereview.chromium.org/64823005/diff/360001/chrome/browser/translate...
File chrome/browser/translate/translate_browser_metrics.cc (right):

https://codereview.chromium.org/64823005/diff/360001/chrome/browser/translate...
chrome/browser/translate/translate_browser_metrics.cc:29: const char
kTranslateRevertTranslation[] = "Translate.RevertTranslation";
Ah, I see. The UMA strings was originally unsymmetrical.
We should not change the strings, but how about changing variable names?

https://codereview.chromium.org/64823005/diff/360001/chrome/browser/translate...
File chrome/browser/translate/translate_browser_metrics.h (right):

https://codereview.chromium.org/64823005/diff/360001/chrome/browser/translate...
chrome/browser/translate/translate_browser_metrics.h:21: UMA_REVERT_TRANSLATION,
DECLINE and PERFORM use _TRANSLATE, but REVER uses _TRANSLATION.
Why do you name them unsymmetrically?

https://codereview.chromium.org/64823005/diff/360001/chrome/browser/translate...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/64823005/diff/360001/chrome/browser/translate...
chrome/browser/translate/translate_infobar_delegate.cc:34: const char
kShowErrorInfobar[] = "Translate.ShowErrorInfobar";
Can we move them to TranslateBrowserMetrics?

[hajimehoshi, 2013-11-13 01:15:20]

Thank you!

https://codereview.chromium.org/64823005/diff/360001/chrome/browser/translate...
File chrome/browser/translate/translate_browser_metrics.cc (right):

https://codereview.chromium.org/64823005/diff/360001/chrome/browser/translate...
chrome/browser/translate/translate_browser_metrics.cc:29: const char
kTranslateRevertTranslation[] = "Translate.RevertTranslation";
On 2013/11/12 22:16:56, Takashi Toyoshima (chromium) wrote:
> Ah, I see. The UMA strings was originally unsymmetrical.
> We should not change the strings, but how about changing variable names?

Thanks, but I prefer each variable name matches each UMA name.

https://codereview.chromium.org/64823005/diff/360001/chrome/browser/translate...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/64823005/diff/360001/chrome/browser/translate...
chrome/browser/translate/translate_infobar_delegate.cc:34: const char
kShowErrorInfobar[] = "Translate.ShowErrorInfobar";
On 2013/11/12 22:16:56, Takashi Toyoshima (chromium) wrote:
> Can we move them to TranslateBrowserMetrics?

Hmm, these are used only at the infobar. I'll leave them as they are.

[Takashi Toyoshima, 2013-11-13 01:17:07]

ok, that's optional. please go ahead.

[commit-bot: I haz the power, 2013-11-13 01:21:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/64823005/430001

[commit-bot: I haz the power, 2013-11-13 01:48:28]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-11-13 02:15:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/64823005/430001

[commit-bot: I haz the power, 2013-11-13 05:07:14]

Change committed as 234759