Bug fix: Append a language to the list after blocking it for Translate on Chrome OS

Bug fix: Append a language to the list after blocking it for Translate on Chrome OS

Chrome OS and the other platforms are different in terms of the preference for the language list of chrome://settings/languages, so I factored the functions to manipulate the list and have it called when blocking the language for Translate.

NOTE: Removed DCHECK(...) in translate_infobar_delegate.cc to check if the language is listed in the blocked language list here because the language might already be listed. For example:
1. After the user add the language, he/she removes the language chrome://settings/languages. In this case, Translate infobar will appear.
2. While the Translate infobar is shown, the user can change if Translate should be offered in the language. In this case, the user can push Never Translate (language)" button while the language is already blocked.

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

[hajimehoshi, 2013-09-09 10:38:07]

estade, can you take a look at language_list.js and
language_option_handler_common.*?

toyoshim, can you take a look at the other files?

Thank you in advance.

[Takashi Toyoshima, 2013-09-09 12:26:11]

https://chromiumcodereview.appspot.com/23923007/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options/language_list.js (left):

https://chromiumcodereview.appspot.com/23923007/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_list.js:411: if (cr.isChromeOS)
Originally, preferredLanguagesPref is used by other platforms, too.
http://src.chromium.org/viewvc/chrome?revision=69521&view=revision
Did you investigate why this is changed to be used only by Chrome OS?

https://chromiumcodereview.appspot.com/23923007/diff/1/chrome/browser/transla...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/23923007/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_infobar_delegate.cc:215: // removed from the
language list of chrome://settings/languages.
This comment is difficult to understand...

IIUC, the chrome://settings/languages allows a user to change settings
asynchronously. So, UI can not keep disable/enable status consistently with the
setting value. As a result, this delegate function may be called even though the
language is disabled by the setting, right?

I'm not sure your comment is aimed to say the same thing.

https://chromiumcodereview.appspot.com/23923007/diff/1/chrome/browser/transla...
File chrome/browser/translate/translate_prefs.cc (right):

https://chromiumcodereview.appspot.com/23923007/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_prefs.cc:74: DCHECK(expanded_languages);
DCHECK(expanded_langauges->empty()) ?

https://chromiumcodereview.appspot.com/23923007/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_prefs.cc:101: // TranslatePrefs: public:
-----------------------------------------------------
BTW, this separator is out of chromium coding style.
Can you remove these separators from this file?
I'm often suggested by other reviewers to remove them, too.

https://chromiumcodereview.appspot.com/23923007/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_prefs.cc:262: DCHECK(languages);
DCHECK(languages->empty()) ?

https://chromiumcodereview.appspot.com/23923007/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_prefs.cc:285: ExpandLanguageCodes(languages,
&accept_languages);
I don't think this works as you expect.

In the case of Chrome OS, it works since it uses kLanguagePreferredLanguages to
get the language list for the preference. If you want to apply the same logic
here, you should update GetLanguageList to return kLanguagePreferredLanguages.
Of course, you need to copy kAcceptLanguages to kLanguagePreferredLanguages if
it doesn't exist.

Otherwise, unexpected languages will appear in the preference. E.g., what happen
if the languages contain "zh-CN", but "zh".

[hajimehoshi, 2013-09-10 03:21:44]

Thanks!

https://codereview.chromium.org/23923007/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/language_list.js (left):

https://codereview.chromium.org/23923007/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/language_list.js:411: if (cr.isChromeOS)
On 2013/09/09 12:26:11, Takashi Toyoshima (chromium) wrote:
> Originally, preferredLanguagesPref is used by other platforms, too.
> http://src.chromium.org/viewvc/chrome?revision=69521&view=revision
> Did you investigate why this is changed to be used only by Chrome OS?

I guess that this option page was just only for Chrome OS, but now it is used
for all platforms. Originally Windows, Mac OS X, Linux didn't have the option
pages in Chrome, and instead, 'Accept-Languages' was managed in each way.

See: https://codereview.chromium.org/6296017/ and
https://code.google.com/p/chromium/issues/detail?id=43716

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

https://codereview.chromium.org/23923007/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_infobar_delegate.cc:215: // removed from the
language list of chrome://settings/languages.
On 2013/09/09 12:26:11, Takashi Toyoshima (chromium) wrote:
> This comment is difficult to understand...
> 
> IIUC, the chrome://settings/languages allows a user to change settings
> asynchronously. So, UI can not keep disable/enable status consistently with
the
> setting value. As a result, this delegate function may be called even though
the
> language is disabled by the setting, right?
> 
> I'm not sure your comment is aimed to say the same thing.

I aimed to say the other thing.

1. Open a Japanese page
2. Click 'Never Translate Japanese', and Japanese is added to the
blocked-languages list and Accept languages list.
3. Remove Japanese from Accept languages list
4. Open a Japanese page again. In this case, Translation infobar will appear
even though Japanese is still in the blocked-languages list

This is an expected behavior. Before using the blocked-languages list, this
never happens because the old blacklist (you know, the legacy list) was used
regardless of the Accept languages.

Anyway, I'll modify the comment later.

And I didn't come up with your case. Maybe your case causes the Asset error. Let
me check this later.

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

https://codereview.chromium.org/23923007/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_prefs.cc:74: DCHECK(expanded_languages);
On 2013/09/09 12:26:11, Takashi Toyoshima (chromium) wrote:
> DCHECK(expanded_langauges->empty()) ?

Done.

https://codereview.chromium.org/23923007/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_prefs.cc:101: // TranslatePrefs: public:
-----------------------------------------------------
On 2013/09/09 12:26:11, Takashi Toyoshima (chromium) wrote:
> BTW, this separator is out of chromium coding style.
> Can you remove these separators from this file?
> I'm often suggested by other reviewers to remove them, too.

Done.

https://codereview.chromium.org/23923007/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_prefs.cc:262: DCHECK(languages);
On 2013/09/09 12:26:11, Takashi Toyoshima (chromium) wrote:
> DCHECK(languages->empty()) ?

Done.

https://codereview.chromium.org/23923007/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_prefs.cc:285: ExpandLanguageCodes(languages,
&accept_languages);
On 2013/09/09 12:26:11, Takashi Toyoshima (chromium) wrote:
> I don't think this works as you expect.
> 
> In the case of Chrome OS, it works since it uses kLanguagePreferredLanguages
to
> get the language list for the preference. If you want to apply the same logic
> here, you should update GetLanguageList to return kLanguagePreferredLanguages.
> Of course, you need to copy kAcceptLanguages to kLanguagePreferredLanguages if
> it doesn't exist.
> 
> Otherwise, unexpected languages will appear in the preference. E.g., what
happen
> if the languages contain "zh-CN", but "zh".

Sorry but I couldn't understand. Please discuss later.

[hajimehoshi, 2013-09-10 05:23:04]

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

https://codereview.chromium.org/23923007/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_infobar_delegate.cc:215: // removed from the
language list of chrome://settings/languages.
On 2013/09/10 03:21:45, hajimehoshi wrote:
> On 2013/09/09 12:26:11, Takashi Toyoshima (chromium) wrote:
> > This comment is difficult to understand...
> > 
> > IIUC, the chrome://settings/languages allows a user to change settings
> > asynchronously. So, UI can not keep disable/enable status consistently with
> the
> > setting value. As a result, this delegate function may be called even though
> the
> > language is disabled by the setting, right?
> > 
> > I'm not sure your comment is aimed to say the same thing.
> 
> I aimed to say the other thing.
> 
> 1. Open a Japanese page
> 2. Click 'Never Translate Japanese', and Japanese is added to the
> blocked-languages list and Accept languages list.
> 3. Remove Japanese from Accept languages list
> 4. Open a Japanese page again. In this case, Translation infobar will appear
> even though Japanese is still in the blocked-languages list
> 
> This is an expected behavior. Before using the blocked-languages list, this
> never happens because the old blacklist (you know, the legacy list) was used
> regardless of the Accept languages.
> 
> Anyway, I'll modify the comment later.
> 
> And I didn't come up with your case. Maybe your case causes the Asset error.
Let
> me check this later.
> 

Done.

[Takashi Toyoshima, 2013-09-10 05:51:53]

LGTM with nits

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

https://codereview.chromium.org/23923007/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_infobar_delegate.cc:215: // The language
might already be in the blocked list. For example:
This comment shows the reason why you want to remove the DCHECK. Can you change
the first sentence to "Don't use DCHECK(...) because the language ...", or
something?

https://codereview.chromium.org/23923007/diff/10001/chrome/browser/translate/...
File chrome/browser/translate/translate_prefs.cc (right):

https://codereview.chromium.org/23923007/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_prefs.cc:74:
DCHECK(expanded_languages->empty());
Oops, sorry for confusing you.
I think we should keep the original DCHECK(expanded_languages), too.

https://codereview.chromium.org/23923007/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_prefs.cc:260: DCHECK(languages->empty());
ditto

[hajimehoshi, 2013-09-10 06:11:33]

Thanks!

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

https://codereview.chromium.org/23923007/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_infobar_delegate.cc:215: // The language
might already be in the blocked list. For example:
On 2013/09/10 05:51:54, Takashi Toyoshima (chromium) wrote:
> This comment shows the reason why you want to remove the DCHECK. Can you
change
> the first sentence to "Don't use DCHECK(...) because the language ...", or
> something?

Done.

https://codereview.chromium.org/23923007/diff/10001/chrome/browser/translate/...
File chrome/browser/translate/translate_prefs.cc (right):

https://codereview.chromium.org/23923007/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_prefs.cc:74:
DCHECK(expanded_languages->empty());
On 2013/09/10 05:51:54, Takashi Toyoshima (chromium) wrote:
> Oops, sorry for confusing you.
> I think we should keep the original DCHECK(expanded_languages), too.

Done.

https://codereview.chromium.org/23923007/diff/10001/chrome/browser/translate/...
chrome/browser/translate/translate_prefs.cc:260: DCHECK(languages->empty());
On 2013/09/10 05:51:54, Takashi Toyoshima (chromium) wrote:
> ditto

Done.

[Evan Stade, 2013-09-12 22:53:41]

lgtm

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

https://codereview.chromium.org/23923007/diff/16001/chrome/browser/translate/...
chrome/browser/translate/translate_infobar_delegate.cc:215: // Don't use
DCHECK(...) to check if the language is listed in the blocked
this comment seems pretty long to explain why a thing is NOT here. I would
remove it.

https://codereview.chromium.org/23923007/diff/16001/chrome/browser/translate/...
File chrome/browser/translate/translate_prefs.h (right):

https://codereview.chromium.org/23923007/diff/16001/chrome/browser/translate/...
chrome/browser/translate/translate_prefs.h:82: // Returns the language list of
chrome://settings/languages.
nit: s/returns/sets/

[hajimehoshi, 2013-09-13 01:39:19]

Thanks!

https://chromiumcodereview.appspot.com/23923007/diff/16001/chrome/browser/tra...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/23923007/diff/16001/chrome/browser/tra...
chrome/browser/translate/translate_infobar_delegate.cc:215: // Don't use
DCHECK(...) to check if the language is listed in the blocked
On 2013/09/12 22:53:41, Evan Stade wrote:
> this comment seems pretty long to explain why a thing is NOT here. I would
> remove it.

Ok, I'll write the explanation at the description of this CL instead of here.

https://chromiumcodereview.appspot.com/23923007/diff/16001/chrome/browser/tra...
File chrome/browser/translate/translate_prefs.h (right):

https://chromiumcodereview.appspot.com/23923007/diff/16001/chrome/browser/tra...
chrome/browser/translate/translate_prefs.h:82: // Returns the language list of
chrome://settings/languages.
On 2013/09/12 22:53:41, Evan Stade wrote:
> nit: s/returns/sets/

Done.

[commit-bot: I haz the power, 2013-09-13 01:42:45]

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

[commit-bot: I haz the power, 2013-09-13 03:22:47]

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=win7_aura&...

[commit-bot: I haz the power, 2013-09-13 03:36:07]

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

[commit-bot: I haz the power, 2013-09-13 04:33:13]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-13 06:30:46]

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

[commit-bot: I haz the power, 2013-09-13 10:08:43]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-17 01:13:08]

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

[commit-bot: I haz the power, 2013-09-17 05:15:30]

Change committed as 223542