This CL makes the TranslationService class send the text to be translated to ...

This CL makes the TranslationService class send the text to be translated to the translation
server.  It groups requests as to limit the number of requests sent to the server.
Also this CL adds a flag to automatically turn on translation on pages that are not in the
language Chrome is configured in.

BUG=None
TEST=Run the unit-tests. Add the --auto-translate flag then navigate to pages in a language
     which is not the language Chrome is configured. They should get translated.


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

[jcampan, 2010-01-28 19:17:24]

Jungshik,
This is a new CL based on the previous CL you reviewed:
http://codereview.chromium.org/464054/show
Sorry I had to create a new CL as I was sick OOO and did not have access to my
work computer.
It adds some major changes, most notably it now groups requests when it sends
them to the translate server (to avoid spamming the server with short requests).
I addressed the comments you made in the previous CL, you can refer to that
review for my comments.

[jcampan, 2010-01-28 19:19:59]

Adding Scott as well to the list of reviewers.

[Paweł Hajdan Jr., 2010-01-28 19:43:07]

Drive-by with minor test-related comments.

http://codereview.chromium.org/552216/diff/2001/2007
File chrome/browser/renderer_host/translation_service_unittest.cc (right):

http://codereview.chromium.org/552216/diff/2001/2007#newcode42
chrome/browser/renderer_host/translation_service_unittest.cc:42:
STLDeleteContainerPointers(messages_.begin(), messages_.end());
nit: May be simpler to just use STLDeleteElements.

http://codereview.chromium.org/552216/diff/2001/2007#newcode60
chrome/browser/renderer_host/translation_service_unittest.cc:60: int
send_request_delay_;
nit: Seems unused.

http://codereview.chromium.org/552216/diff/2001/2007#newcode152
chrome/browser/renderer_host/translation_service_unittest.cc:152: amp_pos =
upload_data.find("&", q_pos);
It may result in a more straightforward code if you use StringTokenizer here.

http://codereview.chromium.org/552216/diff/2001/2007#newcode393
chrome/browser/renderer_host/translation_service_unittest.cc:393: for (size_t i
= 0; i < 4; i++) {
Please add SCOPED_TRACE here (a gtest macro) so that the output in case of
failures is more helpful.

[sky, 2010-01-28 20:51:09]

http://codereview.chromium.org/552216/diff/2001/2008
File chrome/browser/renderer_host/translation_service.cc (right):

http://codereview.chromium.org/552216/diff/2001/2008#newcode31
chrome/browser/renderer_host/translation_service.cc:31: const char*
locale_language;
Can you document what they two are? Which one corresponds to that sent to the
server?

http://codereview.chromium.org/552216/diff/2001/2008#newcode43
chrome/browser/renderer_host/translation_service.cc:43: const size_t
kTextRequestMaxSize = 1024 * 30;
Make this static, or move into namespace.

http://codereview.chromium.org/552216/diff/2001/2008#newcode46
chrome/browser/renderer_host/translation_service.cc:46: const int
kSendRequestDelay = 100;
Same here.

http://codereview.chromium.org/552216/diff/2001/2008#newcode53
chrome/browser/renderer_host/translation_service.cc:53: class
SendTranslationRequestTask : public CancelableTask {
Move this into the anonymous namespace.

http://codereview.chromium.org/552216/diff/2001/2008#newcode70
chrome/browser/renderer_host/translation_service.cc:70: class
TranslateURLFetcherDelegate : public URLFetcher::Delegate {
Description?
Also, how come you didn't make the TranslationService extends
URLFetcher::Delegate.

http://codereview.chromium.org/552216/diff/2001/2008#newcode135
chrome/browser/renderer_host/translation_service.cc:135:
scoped_ptr<RendererRequestInfoList> renderer_request_info;
Can you document why this is a scoped_ptr. Specifically that ownership may get
transferred.

http://codereview.chromium.org/552216/diff/2001/2008#newcode263
chrome/browser/renderer_host/translation_service.cc:263: const string16
kClosingTag = ASCIIToUTF16("</a>");
nit: It would be nice if these two were symetrical, eg kOpenTag and kCloseTag.

http://codereview.chromium.org/552216/diff/2001/2008#newcode280
chrome/browser/renderer_host/translation_service.cc:280: stop_index =
translated_text.find(kMagicTag, stop_index);
If stop_index is set to npos on line 277, does find do the right thing?

http://codereview.chromium.org/552216/diff/2001/2008#newcode284
chrome/browser/renderer_host/translation_service.cc:284: start_index =
translated_text.find(kMagicTag, stop_index);
because of line 280, isn't the result of translate_text.find(kMagicTag,
stop_index); stop_index? In other words, can this be:

start_index = stop_index;
?

http://codereview.chromium.org/552216/diff/2001/2008#newcode308
chrome/browser/renderer_host/translation_service.cc:308: size_t new_tag_index =
(start_index < input.size() - 1) ?
Won't this always be true because kStartTag is 3 chars long?

http://codereview.chromium.org/552216/diff/2001/2008#newcode309
chrome/browser/renderer_host/translation_service.cc:309:
input.find(kLessThan,start_index + 1) :
nit: space before start_index

http://codereview.chromium.org/552216/diff/2001/2008#newcode321
chrome/browser/renderer_host/translation_service.cc:321: input = r;
input.swap(r);

This loop would be more efficient if you didn't reset input time each through,
but rather built up a resulting string and only iterated through text.

http://codereview.chromium.org/552216/diff/2001/2008#newcode326
chrome/browser/renderer_host/translation_service.cc:326: start_index =
input.find(kEndTag);
I believe string_util's ReplaceSubstrings can do this for you.

http://codereview.chromium.org/552216/diff/2001/2008#newcode397
chrome/browser/renderer_host/translation_service.cc:397: string16 utf16_text =
TranslationService::MergeTextChunks(text_chunks);
nit: you shouldn't need TranslationService:: here.

http://codereview.chromium.org/552216/diff/2001/2008#newcode426
chrome/browser/renderer_host/translation_service.cc:426: translation_request =
NULL;
Could you also reset iter here, just to be safe.

http://codereview.chromium.org/552216/diff/2001/2008#newcode451
chrome/browser/renderer_host/translation_service.cc:451:
request_map.erase(iter);
Do you need to delete translation_request here?

http://codereview.chromium.org/552216/diff/2001/2008#newcode512
chrome/browser/renderer_host/translation_service.cc:512: URLFetcher* url_fetcher
= const_cast<URLFetcher*>(const_url_fetcher);
Ick, can your map be: <const URLFetcher*> so that you don't need to const_cast
here?

http://codereview.chromium.org/552216/diff/2001/2008#newcode531
chrome/browser/renderer_host/translation_service.cc:531: delete url_fetcher;
I would put both the url_fetcher and request_info_list in a scoped-ptr.

http://codereview.chromium.org/552216/diff/2001/2011
File chrome/browser/renderer_host/translation_service.h (right):

http://codereview.chromium.org/552216/diff/2001/2011#newcode84
chrome/browser/renderer_host/translation_service.h:84: typedef std::map<int,
TranslationRequest*> TranslationRequestMap;
Document what the int is here.

http://codereview.chromium.org/552216/diff/2001/2011#newcode116
chrome/browser/renderer_host/translation_service.h:116: static void
AddTextToRequestString(std::string* request,
out params should be the params.

http://codereview.chromium.org/552216/diff/2001/2011#newcode136
chrome/browser/renderer_host/translation_service.h:136:
FRIEND_TEST(TranslationServiceTest, MergeTestChunks);
I would put these next to the other friends.

http://codereview.chromium.org/552216/diff/2001/2005
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/552216/diff/2001/2005#newcode1817
chrome/browser/tab_contents/tab_contents.cc:1817: if
(CommandLine::ForCurrentProcess()->HasSwitch(
I would put this before the notification is sent.

http://codereview.chromium.org/552216/diff/2001/2005#newcode1822
chrome/browser/tab_contents/tab_contents.cc:1822: NavigationEntry* entry =
controller_.GetActiveEntry();
this is already defined on 1805.

http://codereview.chromium.org/552216/diff/2001/2020
File chrome/renderer/render_view.cc (left):

http://codereview.chromium.org/552216/diff/2001/2020#oldcode1525
chrome/renderer/render_view.cc:1525:
MessageLoop::current()->PostDelayedTask(FROM_HERE,
Is this intentional?

http://codereview.chromium.org/552216/diff/2001/2017
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/552216/diff/2001/2017#newcode697
chrome/renderer/render_view.h:697: void OnTranslatePage(int page_id, std::string
from_lang, std::string to_lang);
const std::string&

[jcampan, 2010-01-28 23:08:54]

New snapshot pushed.

Thanks!

http://codereview.chromium.org/552216/diff/2001/2008
File chrome/browser/renderer_host/translation_service.cc (right):

http://codereview.chromium.org/552216/diff/2001/2008#newcode31
chrome/browser/renderer_host/translation_service.cc:31: const char*
locale_language;
On 2010/01/28 20:51:10, sky wrote:
> Can you document what they two are? Which one corresponds to that sent to the
> server?
Actually this table has changed and the comment had not been updated. Changed
the comment. Let me know if it is still not clear.

http://codereview.chromium.org/552216/diff/2001/2008#newcode43
chrome/browser/renderer_host/translation_service.cc:43: const size_t
kTextRequestMaxSize = 1024 * 30;
On 2010/01/28 20:51:10, sky wrote:
> Make this static, or move into namespace.

Done.

http://codereview.chromium.org/552216/diff/2001/2008#newcode46
chrome/browser/renderer_host/translation_service.cc:46: const int
kSendRequestDelay = 100;
On 2010/01/28 20:51:10, sky wrote:
> Same here.

Done.

http://codereview.chromium.org/552216/diff/2001/2008#newcode53
chrome/browser/renderer_host/translation_service.cc:53: class
SendTranslationRequestTask : public CancelableTask {
On 2010/01/28 20:51:10, sky wrote:
> Move this into the anonymous namespace.

Done.

http://codereview.chromium.org/552216/diff/2001/2008#newcode70
chrome/browser/renderer_host/translation_service.cc:70: class
TranslateURLFetcherDelegate : public URLFetcher::Delegate {
On 2010/01/28 20:51:10, sky wrote:
> Description?
> Also, how come you didn't make the TranslationService extends
> URLFetcher::Delegate.
Good point. Got rid of this class, TranlsationService is now implementing
URLFetcher::Delegate.

http://codereview.chromium.org/552216/diff/2001/2008#newcode135
chrome/browser/renderer_host/translation_service.cc:135:
scoped_ptr<RendererRequestInfoList> renderer_request_info;
On 2010/01/28 20:51:10, sky wrote:

Done.

http://codereview.chromium.org/552216/diff/2001/2008#newcode263
chrome/browser/renderer_host/translation_service.cc:263: const string16
kClosingTag = ASCIIToUTF16("</a>");
On 2010/01/28 20:51:10, sky wrote:

Done.

http://codereview.chromium.org/552216/diff/2001/2008#newcode280
chrome/browser/renderer_host/translation_service.cc:280: stop_index =
translated_text.find(kMagicTag, stop_index);
On 2010/01/28 20:51:10, sky wrote:
Added a new case for that.

http://codereview.chromium.org/552216/diff/2001/2008#newcode284
chrome/browser/renderer_host/translation_service.cc:284: start_index =
translated_text.find(kMagicTag, stop_index);
On 2010/01/28 20:51:10, sky wrote:
Yes, replaced.

http://codereview.chromium.org/552216/diff/2001/2008#newcode308
chrome/browser/renderer_host/translation_service.cc:308: size_t new_tag_index =
(start_index < input.size() - 1) ?
On 2010/01/28 20:51:10, sky wrote:
Yeah, removed.

http://codereview.chromium.org/552216/diff/2001/2008#newcode309
chrome/browser/renderer_host/translation_service.cc:309:
input.find(kLessThan,start_index + 1) :
On 2010/01/28 20:51:10, sky wrote:

Done.

http://codereview.chromium.org/552216/diff/2001/2008#newcode321
chrome/browser/renderer_host/translation_service.cc:321: input = r;
On 2010/01/28 20:51:10, sky wrote:

Done.

http://codereview.chromium.org/552216/diff/2001/2008#newcode326
chrome/browser/renderer_host/translation_service.cc:326: start_index =
input.find(kEndTag);
On 2010/01/28 20:51:10, sky wrote:

Done.

http://codereview.chromium.org/552216/diff/2001/2008#newcode397
chrome/browser/renderer_host/translation_service.cc:397: string16 utf16_text =
TranslationService::MergeTextChunks(text_chunks);
On 2010/01/28 20:51:10, sky wrote:

Done.

http://codereview.chromium.org/552216/diff/2001/2008#newcode426
chrome/browser/renderer_host/translation_service.cc:426: translation_request =
NULL;
On 2010/01/28 20:51:10, sky wrote:

Done.

http://codereview.chromium.org/552216/diff/2001/2008#newcode451
chrome/browser/renderer_host/translation_service.cc:451:
request_map.erase(iter);
On 2010/01/28 20:51:10, sky wrote:
Ah! Good catch!

http://codereview.chromium.org/552216/diff/2001/2008#newcode512
chrome/browser/renderer_host/translation_service.cc:512: URLFetcher* url_fetcher
= const_cast<URLFetcher*>(const_url_fetcher);
On 2010/01/28 20:51:10, sky wrote:
Yes I could but I'd need the const cast to be able to delete the URLFetcher
anyway.

http://codereview.chromium.org/552216/diff/2001/2008#newcode531
chrome/browser/renderer_host/translation_service.cc:531: delete url_fetcher;
On 2010/01/28 20:51:10, sky wrote:

Done.

http://codereview.chromium.org/552216/diff/2001/2011
File chrome/browser/renderer_host/translation_service.h (right):

http://codereview.chromium.org/552216/diff/2001/2011#newcode84
chrome/browser/renderer_host/translation_service.h:84: typedef std::map<int,
TranslationRequest*> TranslationRequestMap;
On 2010/01/28 20:51:10, sky wrote:
> Document what the int is here.

Done.

http://codereview.chromium.org/552216/diff/2001/2011#newcode116
chrome/browser/renderer_host/translation_service.h:116: static void
AddTextToRequestString(std::string* request,
On 2010/01/28 20:51:10, sky wrote:
> out params should be the params.
Not sure I understand what you mean. Updated the comment to mention that request
is an in/out param.

http://codereview.chromium.org/552216/diff/2001/2011#newcode136
chrome/browser/renderer_host/translation_service.h:136:
FRIEND_TEST(TranslationServiceTest, MergeTestChunks);
On 2010/01/28 20:51:10, sky wrote:
> I would put these next to the other friends.

Done.

http://codereview.chromium.org/552216/diff/2001/2007
File chrome/browser/renderer_host/translation_service_unittest.cc (right):

http://codereview.chromium.org/552216/diff/2001/2007#newcode42
chrome/browser/renderer_host/translation_service_unittest.cc:42:
STLDeleteContainerPointers(messages_.begin(), messages_.end());
On 2010/01/28 19:43:07, Paweł Hajdan Jr. wrote:
> nit: May be simpler to just use STLDeleteElements.

Done.

http://codereview.chromium.org/552216/diff/2001/2007#newcode60
chrome/browser/renderer_host/translation_service_unittest.cc:60: int
send_request_delay_;
On 2010/01/28 19:43:07, Paweł Hajdan Jr. wrote:
> nit: Seems unused.

Removed

http://codereview.chromium.org/552216/diff/2001/2007#newcode152
chrome/browser/renderer_host/translation_service_unittest.cc:152: amp_pos =
upload_data.find("&", q_pos);
On 2010/01/28 19:43:07, Paweł Hajdan Jr. wrote:
> It may result in a more straightforward code if you use StringTokenizer here.

Done.

http://codereview.chromium.org/552216/diff/2001/2007#newcode393
chrome/browser/renderer_host/translation_service_unittest.cc:393: for (size_t i
= 0; i < 4; i++) {
On 2010/01/28 19:43:07, Paweł Hajdan Jr. wrote:
> Please add SCOPED_TRACE here (a gtest macro) so that the output in case of
> failures is more helpful.

Done.

http://codereview.chromium.org/552216/diff/2001/2005
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/552216/diff/2001/2005#newcode1817
chrome/browser/tab_contents/tab_contents.cc:1817: if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2010/01/28 20:51:10, sky wrote:

Done.

http://codereview.chromium.org/552216/diff/2001/2005#newcode1822
chrome/browser/tab_contents/tab_contents.cc:1822: NavigationEntry* entry =
controller_.GetActiveEntry();
On 2010/01/28 20:51:10, sky wrote:

Removed

http://codereview.chromium.org/552216/diff/2001/2020
File chrome/renderer/render_view.cc (left):

http://codereview.chromium.org/552216/diff/2001/2020#oldcode1525
chrome/renderer/render_view.cc:1525:
MessageLoop::current()->PostDelayedTask(FROM_HERE,
On 2010/01/28 20:51:10, sky wrote:

Yes, it is. But it should be part of a separate CL.
Reverted.

http://codereview.chromium.org/552216/diff/2001/2017
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/552216/diff/2001/2017#newcode697
chrome/renderer/render_view.h:697: void OnTranslatePage(int page_id, std::string
from_lang, std::string to_lang);
On 2010/01/28 20:51:10, sky wrote:

Done.

[sky, 2010-01-28 23:53:45]

http://codereview.chromium.org/552216/diff/4003/4010
File chrome/browser/renderer_host/translation_service.cc (right):

http://codereview.chromium.org/552216/diff/4003/4010#newcode141
chrome/browser/renderer_host/translation_service.cc:141: // static
nit: definition order should match that of declarations.

http://codereview.chromium.org/552216/diff/4003/4010#newcode223
chrome/browser/renderer_host/translation_service.cc:223: (next_tag_index !=
std::string::npos && stop_index > next_tag_index)) {
nit > 80

http://codereview.chromium.org/552216/diff/4003/4010#newcode368
chrome/browser/renderer_host/translation_service.cc:368:
DCHECK(!translation_request->send_query_task);
While unlikely it seems like there is a possibility for there to already be
send_query_task. For example, if for whatever reason you end up with two calls
to this method before the query runs, would you hit this DCHECK?

http://codereview.chromium.org/552216/diff/4003/4010#newcode488
chrome/browser/renderer_host/translation_service.cc:488: scoped_ptr<URLFetcher>
url_fetcher(
If you make the scoped_ptr a scoped_ptr<const URLFetcher> do you still need the
const cast?

http://codereview.chromium.org/552216/diff/4003/4013
File chrome/browser/renderer_host/translation_service.h (right):

http://codereview.chromium.org/552216/diff/4003/4013#newcode26
chrome/browser/renderer_host/translation_service.h:26: class TranslationService
: public URLFetcher::Delegate {
Could you add a description that documents the flow like you told me in IM. It's
very helpful for folks not familiar with the class trying to get a feel for how
it all works.

http://codereview.chromium.org/552216/diff/4003/4013#newcode146
chrome/browser/renderer_host/translation_service.h:146: typedef
std::map<URLFetcher*, RendererRequestInfoList*>
nit: typedefs should be grouped together. Style guide says you should have the
following order: Typedefs and Enums
Constants
Constructors
Destructor
Methods, including static

[jcampan, 2010-01-29 00:33:22]

New snapshot uploaded.

http://codereview.chromium.org/552216/diff/4003/4010
File chrome/browser/renderer_host/translation_service.cc (right):

http://codereview.chromium.org/552216/diff/4003/4010#newcode141
chrome/browser/renderer_host/translation_service.cc:141: // static
On 2010/01/28 23:53:45, sky wrote:

OK, shuffled around all methods so they match the .h order.

http://codereview.chromium.org/552216/diff/4003/4010#newcode223
chrome/browser/renderer_host/translation_service.cc:223: (next_tag_index !=
std::string::npos && stop_index > next_tag_index)) {
On 2010/01/28 23:53:45, sky wrote:

Done.

http://codereview.chromium.org/552216/diff/4003/4010#newcode368
chrome/browser/renderer_host/translation_service.cc:368:
DCHECK(!translation_request->send_query_task);
On 2010/01/28 23:53:45, sky wrote:

I am not sure I am following you.
If this gets called and there is already a pending query then the task would
have been canceled and NULLed either on l312 or 322/323.

http://codereview.chromium.org/552216/diff/4003/4010#newcode488
chrome/browser/renderer_host/translation_service.cc:488: scoped_ptr<URLFetcher>
url_fetcher(
On 2010/01/28 23:53:45, sky wrote:
I thought I had tried a scoped_ref<const URLFetcher> and that it had failed, but
I just tried now and it works!
So removed the const_cast and changed the map to use a const URLFecher* as it
key.

http://codereview.chromium.org/552216/diff/4003/4013
File chrome/browser/renderer_host/translation_service.h (right):

http://codereview.chromium.org/552216/diff/4003/4013#newcode26
chrome/browser/renderer_host/translation_service.h:26: class TranslationService
: public URLFetcher::Delegate {
On 2010/01/28 23:53:45, sky wrote:

Done.

http://codereview.chromium.org/552216/diff/4003/4013#newcode146
chrome/browser/renderer_host/translation_service.h:146: typedef
std::map<URLFetcher*, RendererRequestInfoList*>
On 2010/01/28 23:53:45, sky wrote:

Done.

[sky, 2010-01-29 00:42:24]

LGTM

[jungshik at Google, 2010-01-30 01:27:17]

On finishing the review, I realized that it's submitted because my review was
delayed for which I'm sorry.

Anyway, it's ok because it looks good except for one issue (missing mapping for
pt-BR/pt-PT to pt) and some very minor nits. I guess, the missing mapping
entries and a test involving non-ASCII text can be added in a follow-up.

http://codereview.chromium.org/552216/diff/5023/2029
File chrome/browser/renderer_host/translation_service.cc (right):

http://codereview.chromium.org/552216/diff/5023/2029#newcode45
chrome/browser/renderer_host/translation_service.cc:45: { "es-419", "es" },
Chrome's UI languages for Brazillian Portuguese and Iberian Portuguese are
'pt-BR' and 'pt-PT'. However, CLD just returns 'pt'. So, both 'pt-PT' and
'pt-BR' have to be mapped to 'pt'.

http://codereview.chromium.org/552216/diff/5023/2029#newcode275
chrome/browser/renderer_host/translation_service.cc:275: std::string str;
nit: wrapped_data?

http://codereview.chromium.org/552216/diff/5023/2029#newcode293
chrome/browser/renderer_host/translation_service.cc:293: string16 str16;
nit: translated_text?

http://codereview.chromium.org/552216/diff/5023/2029#newcode299
chrome/browser/renderer_host/translation_service.cc:299: TextChunks text_chunks;
nit: translated_chunks might be a better name.

http://codereview.chromium.org/552216/diff/5023/2029#newcode309
chrome/browser/renderer_host/translation_service.cc:309: ListValue* list =
static_cast<ListValue*>(value.get());
nit: translated_text_list?

http://codereview.chromium.org/552216/diff/5023/2029#newcode319
chrome/browser/renderer_host/translation_service.cc:319:
TranslationService::TextChunks text_chunks;
nit: translated_chunks might be a better name.

http://codereview.chromium.org/552216/diff/5023/2029#newcode334
chrome/browser/renderer_host/translation_service.cc:334: // API uses, but for
the ones longer than 2 chars.
Hmm, the input parameters for this function are the language code returned by
CLD and the Chrome's UI locale code. However, kLocaleToCLDLanguage array
(despite its name) is mapping the Chrome's UI locale code to the language code
accepted by Google Translate. There are 3 different codes. 

The logic in this function is correct, but either we have to add more entries to
the array (because CLD returns 'no', 'iw', 'tl' instead of 'nb', 'he', 'fil') or
we have to change languages.h (in cld/...i18n/languages/....) to return  the
correct language codes : 'nb', 'he', 'fil'. I think the latter is better because
it's yet another step toward open-sourcing CLD (there's no reason to carry along
a google3 legacy when CLD is open-sourced).

The CLD also returns 'zh' instead of 'zh-CN'. It has to be taken care of as
well. (we can call LanguageCodeWithDialect instead of LanguageCode for this). 

Ok. I just updated my existing CLD clean-up CL to do two things I'm suggesting
here. If you're ok with the other aspect of that CL (which I asked you to
review), you can leave alone this part and the array except that pt-BR and pt-PT
have to be mapped to 'pt'.

http://codereview.chromium.org/552216/diff/5023/2029#newcode432
chrome/browser/renderer_host/translation_service.cc:432: void
TranslationService::SplitTextChunks(const string16& translated_text,
nit: SplitIntoTextChunks seems better.

http://codereview.chromium.org/552216/diff/5023/2032
File chrome/browser/renderer_host/translation_service.h (right):

http://codereview.chromium.org/552216/diff/5023/2032#newcode24
chrome/browser/renderer_host/translation_service.h:24: // There is one
TranslationService is per renderer process.
nit: is per ==> per

http://codereview.chromium.org/552216/diff/5023/2032#newcode65
chrome/browser/renderer_host/translation_service.h:65: // given the values 'en'
and 'en-US'.
A TODO comment about a future enhancement (either overloading the existing
Accept-Language UI or a separate UI for translation languages) here (or in the
implementation) may be nice to have.

http://codereview.chromium.org/552216/diff/5023/2032#newcode110
chrome/browser/renderer_host/translation_service.h:110: // Warning the request
is deleted when this call returns.
nit: A colon is necessary after 'Warning', isn't it?

http://codereview.chromium.org/552216/diff/5023/2032#newcode131
chrome/browser/renderer_host/translation_service.h:131: static void
SplitTextChunks(const string16& translated_text,
nit: SplitIntoTextChunks seems better.

http://codereview.chromium.org/552216/diff/5023/2028
File chrome/browser/renderer_host/translation_service_unittest.cc (right):

http://codereview.chromium.org/552216/diff/5023/2028#newcode231
chrome/browser/renderer_host/translation_service_unittest.cc:231:
TEST_F(TranslationServiceTest, SimpleSuccessfulTranslation) {
as a sanity check, adding one more case (e.g. en => ja) involving non-ASCII text
would be nice.

http://codereview.chromium.org/552216/diff/5023/2028#newcode405
chrome/browser/renderer_host/translation_service_unittest.cc:405: // Send 2
small requests, than a big one.
nit: than ==> then

http://codereview.chromium.org/552216/diff/5023/2026
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/552216/diff/5023/2026#newcode1814
chrome/browser/tab_contents/tab_contents.cc:1814: std::string locale =
g_browser_process->GetApplicationLocale();
nit: perhaps, s/locale/ui_language/ and s/language/page_language/ would be
clearer.

[jcampan, 2010-01-30 01:31:21]

> On finishing the review, I realized that it's submitted because my review
> was
> delayed for which I'm sorry.
No pb, thanks for the review!
I'll create a new CL addressing these.

> Anyway, it's ok because it looks good except for one issue (missing mapping
> for
> pt-BR/pt-PT to pt) and some very minor nits. I guess, the missing mapping
> entries and a test involving non-ASCII text can be added in a follow-up.
>
>
> http://codereview.chromium.org/552216/diff/5023/2029
> File chrome/browser/renderer_host/translation_service.cc (right):
>
> http://codereview.chromium.org/552216/diff/5023/2029#newcode45
> chrome/browser/renderer_host/translation_service.cc:45: { "es-419", "es"
> },
> Chrome's UI languages for Brazillian Portuguese and Iberian Portuguese
> are 'pt-BR' and 'pt-PT'. However, CLD just returns 'pt'. So, both
> 'pt-PT' and 'pt-BR' have to be mapped to 'pt'.
>
> http://codereview.chromium.org/552216/diff/5023/2029#newcode275
> chrome/browser/renderer_host/translation_service.cc:275: std::string
> str;
> nit: wrapped_data?
>
> http://codereview.chromium.org/552216/diff/5023/2029#newcode293
> chrome/browser/renderer_host/translation_service.cc:293: string16 str16;
> nit: translated_text?
>
> http://codereview.chromium.org/552216/diff/5023/2029#newcode299
> chrome/browser/renderer_host/translation_service.cc:299: TextChunks
> text_chunks;
> nit: translated_chunks might be a better name.
>
> http://codereview.chromium.org/552216/diff/5023/2029#newcode309
> chrome/browser/renderer_host/translation_service.cc:309: ListValue* list
> = static_cast<ListValue*>(value.get());
> nit: translated_text_list?
>
> http://codereview.chromium.org/552216/diff/5023/2029#newcode319
> chrome/browser/renderer_host/translation_service.cc:319:
> TranslationService::TextChunks text_chunks;
> nit: translated_chunks might be a better name.
>
> http://codereview.chromium.org/552216/diff/5023/2029#newcode334
> chrome/browser/renderer_host/translation_service.cc:334: // API uses,
> but for the ones longer than 2 chars.
> Hmm, the input parameters for this function are the language code
> returned by CLD and the Chrome's UI locale code. However,
> kLocaleToCLDLanguage array (despite its name) is mapping the Chrome's UI
> locale code to the language code accepted by Google Translate. There are
> 3 different codes.
>
> The logic in this function is correct, but either we have to add more
> entries to the array (because CLD returns 'no', 'iw', 'tl' instead of
> 'nb', 'he', 'fil') or we have to change languages.h (in
> cld/...i18n/languages/....) to return  the correct language codes :
> 'nb', 'he', 'fil'. I think the latter is better because it's yet another
> step toward open-sourcing CLD (there's no reason to carry along a
> google3 legacy when CLD is open-sourced).
>
> The CLD also returns 'zh' instead of 'zh-CN'. It has to be taken care of
> as well. (we can call LanguageCodeWithDialect instead of LanguageCode
> for this).
>
> Ok. I just updated my existing CLD clean-up CL to do two things I'm
> suggesting here. If you're ok with the other aspect of that CL (which I
> asked you to review), you can leave alone this part and the array except
> that pt-BR and pt-PT have to be mapped to 'pt'.
>
> http://codereview.chromium.org/552216/diff/5023/2029#newcode432
> chrome/browser/renderer_host/translation_service.cc:432: void
> TranslationService::SplitTextChunks(const string16& translated_text,
> nit: SplitIntoTextChunks seems better.
>
> http://codereview.chromium.org/552216/diff/5023/2032
> File chrome/browser/renderer_host/translation_service.h (right):
>
> http://codereview.chromium.org/552216/diff/5023/2032#newcode24
> chrome/browser/renderer_host/translation_service.h:24: // There is one
> TranslationService is per renderer process.
> nit: is per ==> per
>
> http://codereview.chromium.org/552216/diff/5023/2032#newcode65
> chrome/browser/renderer_host/translation_service.h:65: // given the
> values 'en' and 'en-US'.
> A TODO comment about a future enhancement (either overloading the
> existing Accept-Language UI or a separate UI for translation languages)
> here (or in the implementation) may be nice to have.
>
> http://codereview.chromium.org/552216/diff/5023/2032#newcode110
> chrome/browser/renderer_host/translation_service.h:110: // Warning the
> request is deleted when this call returns.
> nit: A colon is necessary after 'Warning', isn't it?
>
> http://codereview.chromium.org/552216/diff/5023/2032#newcode131
> chrome/browser/renderer_host/translation_service.h:131: static void
> SplitTextChunks(const string16& translated_text,
> nit: SplitIntoTextChunks seems better.
>
> http://codereview.chromium.org/552216/diff/5023/2028
> File chrome/browser/renderer_host/translation_service_unittest.cc
> (right):
>
> http://codereview.chromium.org/552216/diff/5023/2028#newcode231
> chrome/browser/renderer_host/translation_service_unittest.cc:231:
> TEST_F(TranslationServiceTest, SimpleSuccessfulTranslation) {
> as a sanity check, adding one more case (e.g. en => ja) involving
> non-ASCII text would be nice.
>
> http://codereview.chromium.org/552216/diff/5023/2028#newcode405
> chrome/browser/renderer_host/translation_service_unittest.cc:405: //
> Send 2 small requests, than a big one.
> nit: than ==> then
>
> http://codereview.chromium.org/552216/diff/5023/2026
> File chrome/browser/tab_contents/tab_contents.cc (right):
>
> http://codereview.chromium.org/552216/diff/5023/2026#newcode1814
> chrome/browser/tab_contents/tab_contents.cc:1814: std::string locale =
> g_browser_process->GetApplicationLocale();
> nit: perhaps, s/locale/ui_language/ and s/language/page_language/ would
> be clearer.
>
> http://codereview.chromium.org/552216
>