Translate: display alpha language name with (alpha) suffix

Translate: display alpha language name with (alpha) suffix

To make alpha language support enabled by default, change display name for
alpha languages from Foo to Foo (Alpha). Actual format will be updated for
each locale, but it will be shown with an indication for "alpha".

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

[Takashi Toyoshima, 2013-05-29 09:09:21]

Hi mad,
This is an alpha language name support in infobar menus.

[MAD, 2013-05-29 14:09:32]

Some comments... I think we need to modify the approach to deal with the two
different requests for language lists... More details below...

BYE
MAD

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:135: scoped_ptr<const
net::URLFetcher> delete_ptr(url_fetcher_.release());
I don't understand why you need this. url_fetcher_ has already been released
above.

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:136:
FetchLanguageList(true);
I don't like this chaining either... I would prefer we use two distinct
URLFetcher objects, in that case, you wouldn't need the subclass I mentioned in
the header file comments. This way, you could simply call Fetch with false, and
then with true right after from within FetchLanguageList (that should be renamed
FetchLanguageLists). 

This would make things more explicit in my opinion.

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:217: if (!requesting_alpha_)
{
SetSupportedLanguages should receive an argument as opposed to rely on a data
member, it would make things clearer.

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.h (right):

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:71: // Flag if |ufl_fetcher_|
is requesting a language list including alpha
I usually try to avoid such flags, it's a bit risky. For example, you never
reset it to false after setting it to true, so if we would ever need to fetch
for languages again, we may have bugs.

I would prefer that you subclass the URLFetcher and add the boolean in there. It
will make things clearer.

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

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:729: NOTREACHED();
Thanks! :-)

[Takashi Toyoshima, 2013-05-30 06:25:31]

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:135: scoped_ptr<const
net::URLFetcher> delete_ptr(url_fetcher_.release());
Sorry, this looks mistakenly added on rebasing.

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:136:
FetchLanguageList(true);
agreed.

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:217: if (!requesting_alpha_)
{
On 2013/05/29 14:09:32, MAD wrote:
> SetSupportedLanguages should receive an argument as opposed to rely on a data
> member, it would make things clearer.

Done.

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.h (right):

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:71: // Flag if |ufl_fetcher_|
is requesting a language list including alpha
On 2013/05/29 14:09:32, MAD wrote:
> I usually try to avoid such flags, it's a bit risky. For example, you never
> reset it to false after setting it to true, so if we would ever need to fetch
> for languages again, we may have bugs.
> 
> I would prefer that you subclass the URLFetcher and add the boolean in there.
It
> will make things clearer.

Done.

[MAD, 2013-05-30 14:17:59]

Some more comments/questions...

Thanks for all the changes! :-)

BYE
MAD

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:238:
supported_languages_.insert(iter.key());
You used to add alpha languages to both lists, and now you only add it to the
alpha list. I don't get it...

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.cc (left):

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:166: if (url_fetcher_.get())
Why didn't you keep this?

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:111: std::set<std::string>*
set) {
DCHECK(set);

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:202:
delete_ptr.reset(language_list_fetcher_.get());
Shouldn't the .get() be a .release()?

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:205:
delete_ptr.reset(alpha_language_list_fetcher_.get());
Same here.

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:223:
supported_alpha_languages_.empty() ? supported_languages_ :
You used to always return the supported_laguages_ list here (as it used to also
contain the alpha languages), and now you only return the alpha languages as
soon as you have at least one, is this really what we want?

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:246:
supported_languages_.count(language) != 0;
I guess if we do this, we need to return the content of both lists in
GetSupportedLanguages.

[Takashi Toyoshima, 2013-05-30 14:41:38]

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15470004/diff/21002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:238:
supported_languages_.insert(iter.key());
Original logic is expected that stable list is fetched before alpha list. But
now both are fetched in parallel.
So, I decided to adopt different logic to handle them.

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.cc (left):

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:166: if (url_fetcher_.get())
Oops, you are right. I should keep it here.
Done.

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:111: std::set<std::string>*
set) {
On 2013/05/30 14:17:59, MAD wrote:
> DCHECK(set);

Done.

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:202:
delete_ptr.reset(language_list_fetcher_.get());
Thank you for catching this.

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:205:
delete_ptr.reset(alpha_language_list_fetcher_.get());
On 2013/05/30 14:17:59, MAD wrote:
> Same here.

Done.

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:223:
supported_alpha_languages_.empty() ? supported_languages_ :
It's no problem that providing only supported_languages_ when alpha language is
not fetched yet.
In that case, language menu contains only stable languages.

Also, this works before both languages are not fetched yet by initial language
list.

In the first logic, supported_languages_ contains all languages from stable and
alpha language lists.

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:246:
supported_languages_.count(language) != 0;
When supported_alpha_languages_ is empty,

GetSupportedLanguages() returns supported_languages_. It's a stable language
list.
Also, IsSupportedLanguage check is equivalent to
supported_languages_.count(language) != 0 because the first condition is always
false. So these two functions are consistent.

When it's not empty,
GetSupportedLanguages() returns supported_alpha_languages_. Basically it
contains stable and alpha languages.
Also, IsSupportedLanguage check is equivalent to
supported_alpha_languages_.count(language) because true set of the latter
condition is contained by true set of the former condition.

Thus, these two functions are consistent on two different conditions. Note that
supported_languages_ is always non-empty set because it contains initial
language set until it fetches server providing language list.

[MAD, 2013-05-30 14:58:01]

Thanks, but still unclear. More details below...

BYE
MAD

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15470004/diff/30001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:223:
supported_alpha_languages_.empty() ? supported_languages_ :
Sorry, I still don't understand.

What I was trying to say is if user asks IsSupportedLanguage for language A that
is in supported_languages_ and not in supported_alpha_languages_, we return
true. But when we ask for GetSupportedLanguages and supported_alpha_languages_
is not empty, language A is not part of the supported languages. I find this
confusing.

Unless we are sure that ALL languages in supported_languages_ are ALSO in
supported_alpha_languages_, then we should DCHECK that somewhere...

https://codereview.chromium.org/15470004/diff/46001/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15470004/diff/46001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:259: 
I was wondering if we should just isolate them individually, but I guess it's OK
to do both like this...

[Takashi Toyoshima, 2013-05-30 15:24:53]

Note that alpha language list contains stable and alpha languages. So if alpha
list is not empty, it contains stable language A.
Does it make sense?

From mobile.

[MAD, 2013-05-30 15:48:19]

On 2013/05/30 15:24:53, Takashi Toyoshima (chromium) wrote:
> Note that alpha language list contains stable and alpha languages. So if alpha
> list is not empty, it contains stable language A.
> Does it make sense?
> 
> From mobile.

OK, so we should add debug code that makes sure this is the case, at it will
also make it clearer that it is the case... And I would also use the same scheme
when asked for available language, i.e., if alpha is not empty, only check in
alpha list, otherwise look in other list. To make it symmetric...

[Takashi Toyoshima, 2013-05-30 16:28:07]

Hum... let me confirm that a precondition is shared correctly between you and
me.

Translate server provides two kinds of language list.
1) usual supported language list ... contains stable languages
2) alpha language list ... contains stable and alpha languages

supported_languages_ will contain 1)
supported_alpha_languages_ will contain 2)

GetSupportedLanguages() and IsSupportedLanguage() handles 1) when alpha language
support is disabled, or 2) when it is enabled.
Also, until language list fetches are finished, they handles initial list, or 1)
if it's available.

[MAD, 2013-05-30 17:02:25]

On 2013/05/30 16:28:07, Takashi Toyoshima (chromium) wrote:
> Hum... let me confirm that a precondition is shared correctly between you and
> me.
> 
> Translate server provides two kinds of language list.
> 1) usual supported language list ... contains stable languages
> 2) alpha language list ... contains stable and alpha languages
> 
> supported_languages_ will contain 1)
> supported_alpha_languages_ will contain 2)
> 
> GetSupportedLanguages() and IsSupportedLanguage() handles 1) when alpha
language
> support is disabled, or 2) when it is enabled.
> Also, until language list fetches are finished, they handles initial list, or
1)
> if it's available.

OK, let me try to rephrase what I was trying to say:

1) The code seems to assume that supported_languages_ is a strict subset of
supported_alpha_languages_ when supported_alpha_languages_ is not empty, but we
don't mention it anywhere, so it's not clear when we read the code. This is why
I asked to add debug code confirming that when we received both the alpha and
stable lists from the server, we should verify that this strict subset will
always be true, that all entries in supported_languages_ are also in
supported_alpha_languages_. This is to protect the code against eventual
changes/bugs on the server side.

2) Also, the code interacting with the lists is not symmetric right now,
IsSupportedLanguage() looks at both lists, whereas GetSupportedLanguages() uses
one list or the other. In the end, I agree that the result is the same
currently, and as long as 1) is validated. But conceptually, it makes the code
harder to understand, since it's not clear to the reader why we do it
differently.

Is this clearer?

[Takashi Toyoshima, 2013-05-31 04:54:50]

Thanks. Now I understand what you concern about.
I feel I should find simpler way to manage these lists.

[Takashi Toyoshima, 2013-05-31 06:05:56]

OK, now it's clearer.
Can you take a look patch set 9 again?
I'll decided to have another language list which contains both language lists
and make it up to date whenever one of them is updated.

[MAD, 2013-05-31 15:14:52]

Cool, thanks... Yes I agree this is cleaner...

LGTM! with a few final nits/requests...

BYE
MAD

https://codereview.chromium.org/15470004/diff/60001/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.h (right):

https://codereview.chromium.org/15470004/diff/60001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:41: // language support is
enabled, returns true if |language| is in alpha
This comment seems to say that when alpha is enabled we only return true for
alpha languages, I would rephrase by adding "also" as follows:

If alpha language support is enabled, also returns true if |language| is in
alpha...

https://codereview.chromium.org/15470004/diff/60001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:58: // Updates
|supported_all_languages_| contains union of |supported_languages_|
contains union -> to contain the union

https://codereview.chromium.org/15470004/diff/60001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:59: // and
|supported-alpha_languages_|.
Nit: supported-alpha_languages_ -> supported_alpha_languages_

https://codereview.chromium.org/15470004/diff/60001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:68: // The languages
supported by the translation server. It contains union of
I would say "All the languages supported..." and add "the" to "It contains the
union of"

https://codereview.chromium.org/15470004/diff/60001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:70: std::set<std::string>
supported_all_languages_;
I would rename it to all_supported_languages_.

https://codereview.chromium.org/15470004/diff/60001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:80: bool
alpha_language_support_is_enabled_;
You don't seem to be using that...

[Takashi Toyoshima, 2013-05-31 16:37:06]

https://codereview.chromium.org/15470004/diff/60001/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.h (right):

https://codereview.chromium.org/15470004/diff/60001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:41: // language support is
enabled, returns true if |language| is in alpha
thanks!

https://codereview.chromium.org/15470004/diff/60001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:58: // Updates
|supported_all_languages_| contains union of |supported_languages_|
On 2013/05/31 15:14:52, MAD wrote:
> contains union -> to contain the union

Done.

https://codereview.chromium.org/15470004/diff/60001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:59: // and
|supported-alpha_languages_|.
On 2013/05/31 15:14:52, MAD wrote:
> Nit: supported-alpha_languages_ -> supported_alpha_languages_

Done.

https://codereview.chromium.org/15470004/diff/60001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:68: // The languages
supported by the translation server. It contains union of
On 2013/05/31 15:14:52, MAD wrote:
> I would say "All the languages supported..." and add "the" to "It contains the
> union of"
> 

Done.

https://codereview.chromium.org/15470004/diff/60001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:70: std::set<std::string>
supported_all_languages_;
On 2013/05/31 15:14:52, MAD wrote:
> I would rename it to all_supported_languages_.

Done.

https://codereview.chromium.org/15470004/diff/60001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:80: bool
alpha_language_support_is_enabled_;
Thanks.
I stop using it at the last clean up.
Sorry for mistakes.

[commit-bot: I haz the power, 2013-05-31 16:46:54]

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

[commit-bot: I haz the power, 2013-05-31 20:05:52]

Change committed as 203462