Get the list of supported languages from the translate server.

Get the list of supported languages from the translate server.

BUG=35475
TEST=Make sure that we can translate to and from all languages supported by the translate server.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89892

[MAD, 2011-06-15 20:26:16]

Feel free to redirect if you think somebody else should do the review...

Thanks!

[Jay Civelli, 2011-06-16 07:11:13]

Would be good to add a couple of browser tests to
translate_manager_browsertest.cc to test this (normal case, case where we fail
to retrieve the language list, case where the script and language download run
concurently).
There are already tests that deal with the downloading of the script so
hopefully this should be easy.

http://codereview.chromium.org/7171002/diff/3004/chrome/browser/translate/tra...
File chrome/browser/translate/translate_manager.cc (right):

http://codereview.chromium.org/7171002/diff/3004/chrome/browser/translate/tra...
chrome/browser/translate/translate_manager.cc:132: const char* const
kLaguangeListCallbackName = "sl(";
Typo: kLaguange

http://codereview.chromium.org/7171002/diff/3004/chrome/browser/translate/tra...
chrome/browser/translate/translate_manager.cc:186: if (json_value == NULL ||
!json_value->IsType(Value::TYPE_DICTIONARY))
May be instead of repeating the condition in the DCHECK and the if you could
remove the DCHECK and have a NOTREACHED here.
Same below.

http://codereview.chromium.org/7171002/diff/3004/chrome/browser/translate/tra...
chrome/browser/translate/translate_manager.cc:192:
DCHECK(language_dict->HasKey(kTargetLanguagesKey));
Couldn't you replace these lines up 203 with a call to
language_dict->GetDictionary() (and simply assert if it returns NULL, which
would mean either the key is not there or it's not a dictionary).

[MAD, 2011-06-16 14:22:17]

Thanks...

Applied all comments, will add the browser test now and then ask for another
review...

BYE
MAD

http://codereview.chromium.org/7171002/diff/3004/chrome/browser/translate/tra...
File chrome/browser/translate/translate_manager.cc (right):

http://codereview.chromium.org/7171002/diff/3004/chrome/browser/translate/tra...
chrome/browser/translate/translate_manager.cc:132: const char* const
kLaguangeListCallbackName = "sl(";
On 2011/06/16 07:11:13, Jay Civelli wrote:
> Typo: kLaguange

Done.

http://codereview.chromium.org/7171002/diff/3004/chrome/browser/translate/tra...
chrome/browser/translate/translate_manager.cc:186: if (json_value == NULL ||
!json_value->IsType(Value::TYPE_DICTIONARY))
On 2011/06/16 07:11:13, Jay Civelli wrote:
> May be instead of repeating the condition in the DCHECK and the if you could
> remove the DCHECK and have a NOTREACHED here.
> Same below.

Done.

http://codereview.chromium.org/7171002/diff/3004/chrome/browser/translate/tra...
chrome/browser/translate/translate_manager.cc:192:
DCHECK(language_dict->HasKey(kTargetLanguagesKey));
On 2011/06/16 07:11:13, Jay Civelli wrote:
> Couldn't you replace these lines up 203 with a call to
> language_dict->GetDictionary() (and simply assert if it returns NULL, which
> would mean either the key is not there or it's not a dictionary).

Done.
Thanks, I forgot about this shortcut...

[MAD, 2011-06-17 14:05:54]

OK, the test is there...

I'll kick the try bots again, I seem to hit a lot of flakiness, including the
pyauto test that are also failing for others on Windows...

But the code should be ready for another look now...

Thanks!

[MAD, 2011-06-21 16:55:57]

Ping?

On Fri, Jun 17, 2011 at 10:05 AM, <mad@chromium.org> wrote:

> OK, the test is there...
>
> I'll kick the try bots again, I seem to hit a lot of flakiness, including
> the
> pyauto test that are also failing for others on Windows...
>
> But the code should be ready for another look now...
>
> Thanks!
>
>
>
http://codereview.chromium.**org/7171002/<http://codereview.chromium.org/7171...
>

[Jay Civelli, 2011-06-21 17:22:07]

LGTM
(sorry for the delay)

http://codereview.chromium.org/7171002/diff/13002/chrome/browser/translate/tr...
File chrome/browser/translate/translate_manager_browsertest.cc (right):

http://codereview.chromium.org/7171002/diff/13002/chrome/browser/translate/tr...
chrome/browser/translate/translate_manager_browsertest.cc:196:
std::vector<std::string> languages) {
const std::vector<std::string>&

[MAD, 2011-06-21 18:14:24]

Excellent, Thanks!

Will do the change (thanks for catching it, it was an oversight) and will
check the commit box once I uploaded this change...

On Tue, Jun 21, 2011 at 1:22 PM, <jcivelli@chromium.org> wrote:

> LGTM
> (sorry for the delay)
>
>
> http://codereview.chromium.**org/7171002/diff/13002/chrome/**
>
browser/translate/translate_**manager_browsertest.cc<http://codereview.chromium.org/7171002/diff/13002/chrome/browser/translate/translate_manager_browsertest.cc>
> File chrome/browser/translate/**translate_manager_browsertest.**cc
> (right):
>
> http://codereview.chromium.**org/7171002/diff/13002/chrome/**
>
browser/translate/translate_**manager_browsertest.cc#**newcode196<http://codereview.chromium.org/7171002/diff/13002/chrome/browser/translate/translate_manager_browsertest.cc#newcode196>
> chrome/browser/translate/**translate_manager_browsertest.**cc:196:
> std::vector<std::string> languages) {
> const std::vector<std::string>&
>
>
>
http://codereview.chromium.**org/7171002/<http://codereview.chromium.org/7171...
>

[commit-bot: I haz the power, 2011-06-21 21:07:39]

Change committed as 89892