Translate: split supporting language list handling to TranslateLanguageList

Translate: split supporting language list handling to TranslateLanguageList

Before adding alpha language display name conversion, split language list
related codes to TranslateLanguageList to make maintenance easy.

This patch contains no functional changes.

BUG=242178
TEST=build all, and run all existing tests

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

[Takashi Toyoshima, 2013-05-20 15:02:01]

Hi mad,

Sorry, this is a little big refactoring to split language list handling from
TranslateManager to TranslateLanguageList. This will be useful to handle alpha
language list in the future. To display alpha languages as "Javanese (Alpha)" in
infobars is a criterion to enable alpha language support by default.

Thanks in advance.

[Takashi Toyoshima, 2013-05-21 04:00:43]

FYI, here is the following change to display alpha languages;
https://chromiumcodereview.appspot.com/15470004/.

I'll ask another review once this change is landed.
Sorry for many review request. Once hajime becomes full committer, we can review
by ourselves.

[MAD, 2013-05-22 14:00:04]

Sorry, I don't agree with this approach...
More details below...

BYE
MAD

https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
File chrome/browser/translate/translate_language_list.h (right):

https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
chrome/browser/translate/translate_language_list.h:8: #include <set>
I don't think this is needed here. Right?

https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
chrome/browser/translate/translate_language_list.h:26: 
No empty line here.

https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
chrome/browser/translate/translate_language_list.h:58: friend class
TranslateManager;
I'm a bit unsure about this... We usually do this, only temporary, or tests,
base helper or impl classes, but very rarely should have two separate classes
friend of eachother like this...

One alternative could be to make this class a private static member of the
translate manager, and still have the static method exposed by the translate
manager, and have this whole class a lazy initialized singleton, making sure the
translate manager's is the only instance, and then make these methods that the
translate manager needs... public...

So I'll stop the CR here and wait for a resolution on this issue before
continuing...

[Takashi Toyoshima, 2013-05-22 14:29:17]

https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
File chrome/browser/translate/translate_language_list.h (right):

https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
chrome/browser/translate/translate_language_list.h:58: friend class
TranslateManager;
How about just exposing  RequestLanguageList(), HasPendingRequest(), and
ResetPendingRequest() as public functions, and remove this friend line?

Currently, translate_manager.cc is a little complicated. I want to split
language list handling code to another file.

Alternative idea is make this class non-singleton class, and just make
TranslateManager own an instance for composition.

[MAD, 2013-05-22 15:18:17]

OK, let's just make them public, this will be simpler indeed...

I'll continue the code review assuming that this friendship will go away.
Thanks!

BYE
MAD

[MAD, 2013-05-22 15:28:55]

Actually, I'm not sure anymore, I will need to think this through some more... I
just realized that we have a URLFetcher in there, and it's usually bad to do
this in a singleton class. See this thread:

https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-de...

[Takashi Toyoshima, 2013-05-22 16:00:45]

Thank you for useful information. I didn't know that, but it seems to be handled
correctly here. Please read the following inline comment.

https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
File chrome/browser/translate/translate_manager.cc (right):

https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
chrome/browser/translate/translate_manager.cc:666: void
TranslateManager::CleanupPendingUlrFetcher() {
This function is called from browser main thread before killing I/O thread and
so on to stop and delete all URLFetcher instance correctly.

[MAD, 2013-05-22 16:07:44]

yes, i noticed that... Still... It's dangerous/fragile... ;-(

BYE
MAD...
sent from Mobile Answering Device!
Le 22 mai 2013 12:00, <toyoshim@chromium.org> a écrit :

> Thank you for useful information. I didn't know that, but it seems to be
> handled
> correctly here. Please read the following inline comment.
>
>
> https://chromiumcodereview.**appspot.com/15317007/diff/**
>
9004/chrome/browser/translate/**translate_manager.cc<https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/translate/translate_manager.cc>
> File chrome/browser/translate/**translate_manager.cc (right):
>
> https://chromiumcodereview.**appspot.com/15317007/diff/**
>
9004/chrome/browser/translate/**translate_manager.cc#**newcode666<https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/translate/translate_manager.cc#newcode666>
> chrome/browser/translate/**translate_manager.cc:666: void
> TranslateManager::**CleanupPendingUlrFetcher() {
> This function is called from browser main thread before killing I/O
> thread and so on to stop and delete all URLFetcher instance correctly.
>
>
https://chromiumcodereview.**appspot.com/15317007/<https://chromiumcodereview...
>

[Takashi Toyoshima, 2013-05-23 08:21:18]

IMO, to use singleton is safer.
Here is a reason.

1. When we use non-singleton object to own URLFetcher, the object should be
deleted explicitly before IO thread dies. This is no more easy than to call
cleanup function for singleton.

2. Paradoxically, singleton's life cycle is very obvious. On the other hands,
non-singleton object's life cycle can be difficult to manage. The object can be
owned by another parent object, and the parent object also may be... thus life
cycle management may be forgotten even though it is very critical if it owns
URLFetcher.

What do you think about it?

https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
File chrome/browser/translate/translate_language_list.h (right):

https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
chrome/browser/translate/translate_language_list.h:8: #include <set>
On 2013/05/22 14:00:04, MAD wrote:
> I don't think this is needed here. Right?

Done.

https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
chrome/browser/translate/translate_language_list.h:26: 
On 2013/05/22 14:00:04, MAD wrote:
> No empty line here.

Done.

https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
chrome/browser/translate/translate_language_list.h:58: friend class
TranslateManager;
Done (move these three functions to public)

[MAD, 2013-05-23 15:57:00]

On 2013/05/23 08:21:18, Takashi Toyoshima (chromium) wrote:
> IMO, to use singleton is safer.
> Here is a reason.
> 
> 1. When we use non-singleton object to own URLFetcher, the object should be
> deleted explicitly before IO thread dies. This is no more easy than to call
> cleanup function for singleton.
> 
> 2. Paradoxically, singleton's life cycle is very obvious. On the other hands,
> non-singleton object's life cycle can be difficult to manage. The object can
be
> owned by another parent object, and the parent object also may be... thus life
> cycle management may be forgotten even though it is very critical if it owns
> URLFetcher.
> 
> What do you think about it?
> 
>
https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
> File chrome/browser/translate/translate_language_list.h (right):
> 
>
https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
> chrome/browser/translate/translate_language_list.h:8: #include <set>
> On 2013/05/22 14:00:04, MAD wrote:
> > I don't think this is needed here. Right?
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
> chrome/browser/translate/translate_language_list.h:26: 
> On 2013/05/22 14:00:04, MAD wrote:
> > No empty line here.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
> chrome/browser/translate/translate_language_list.h:58: friend class
> TranslateManager;
> Done (move these three functions to public)

I think your statement #1 answers your statement #2, because it is as simple to
explicitly cleanup your URLFetcher as it is to call
the cleanup function on the singleton. And although the life cycle of the
singleton is obvious, it is dangerously long, and more prone to error than if we
have an explicit life cycle of the object (e.g., what if RequestLanguageList
gets called after ResetPendingRequest?).

You could revive that chromium-dev thread I shared with you and get other
people's opinion, but I trust the opinions of those who started that thread, and
I would rather not go against their judgement. :-)

[MAD, 2013-05-23 15:58:13]

And a small nit...

https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
File chrome/browser/translate/translate_language_list.h (right):

https://chromiumcodereview.appspot.com/15317007/diff/9004/chrome/browser/tran...
chrome/browser/translate/translate_language_list.h:71: // Cleanups pending
URLFetcher objects to make sure they get released in the
Cleanups -> Cleans up

[Takashi Toyoshima, 2013-05-23 17:32:27]

Let's discuss a plan before deeper review and updating CL.

Basically, I want to tackle this singleton and multithreading problem in another
independent change.
The problem is not trivial, so I don't want to mix a serious problem into this
CL.

But, if a following idea makes sense, it is simple enough to adopt it in this
CL.

- Make TranslateLanguageList non-singleton class
- TranslateManager owns TranslateLanguageList as scoped_ptr
- TranslateManager::TranslateManager constructs TranslateLanguageList and set it
to scoped_ptr.
- TranslateManager::CleanupPendingUrlFetcher reset scoped_ptr to delete it
explicitly

FYI, I noticed some problems in TranslateManager. This CL will be basis to fix
them.
1) It's singleton, but owns multiple URLFetchers (script fetcher is more
dangerous because it runs periodically)
2) Language list is never updated in Chrome OS.
3) Ditto if the first fetch fails
4) It should manage two kinds of language list, stable and alpha, to support
alpha language by default.

At least, 2) and 4) should be fixed by M29 branch.

[MAD, 2013-05-23 17:43:57]

Yes, this idea makes me feel better, having the language list object owned
by the translate manager.

Also, I think we should keep the existing static methods on the translate
manager, I don't think we gain much by exposing them from the list object
as opposed to the translate manager, which can then delegate to the
appropriate list object, right?



On Thu, May 23, 2013 at 1:32 PM, <toyoshim@chromium.org> wrote:

> Let's discuss a plan before deeper review and updating CL.
>
> Basically, I want to tackle this singleton and multithreading problem in
> another
> independent change.
> The problem is not trivial, so I don't want to mix a serious problem into
> this
> CL.
>
> But, if a following idea makes sense, it is simple enough to adopt it in
> this
> CL.
>
> - Make TranslateLanguageList non-singleton class
> - TranslateManager owns TranslateLanguageList as scoped_ptr
> - TranslateManager::**TranslateManager constructs TranslateLanguageList
> and set it
> to scoped_ptr.
> - TranslateManager::**CleanupPendingUrlFetcher reset scoped_ptr to delete
> it
> explicitly
>
> FYI, I noticed some problems in TranslateManager. This CL will be basis to
> fix
> them.
> 1) It's singleton, but owns multiple URLFetchers (script fetcher is more
> dangerous because it runs periodically)
> 2) Language list is never updated in Chrome OS.
> 3) Ditto if the first fetch fails
> 4) It should manage two kinds of language list, stable and alpha, to
> support
> alpha language by default.
>
> At least, 2) and 4) should be fixed by M29 branch.
>
>
https://chromiumcodereview.**appspot.com/15317007/<https://chromiumcodereview...
>

[Takashi Toyoshima, 2013-05-27 14:04:42]

Thank you mad,
I upload patch set 10.
It will meet our agreements at #13

[Takashi Toyoshima, 2013-05-27 14:05:30]

Oops, not patch set 10, but 9 (plan b).

[MAD, 2013-05-27 21:34:35]

LGTM with a few nits...

Thanks!

BYE
MAD

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

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:108: 
One empty line is enough.

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:117: //
|kDefaultSupportedLanguages|. This list will be override by a server
override -> overriden

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:118: // providing support
langauges list.
support -> supported

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:124: url_fetcher_.reset();
This is not needed since the destructor of the scoped_ptr does it.

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:129: 
Nit: Maybe remove this empty line.

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

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:60: // before killing IO
thread because this class is singleton, and it may be
Comment about singleton should be updated.

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

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:720: if (language_list_.get())
We usually don't have both a DCHECK and an if...

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
File chrome/browser/translate/translate_manager.h (right):

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_manager.h:228: // An instance of
TranslateLanguageList which manages supporting language
supporting -> supported

[Takashi Toyoshima, 2013-05-28 07:35:27]

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

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:108: 
On 2013/05/27 21:34:35, MAD wrote:
> One empty line is enough.

Done.

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:117: //
|kDefaultSupportedLanguages|. This list will be override by a server
On 2013/05/27 21:34:35, MAD wrote:
> override -> overriden

Done.

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:118: // providing support
langauges list.
On 2013/05/27 21:34:35, MAD wrote:
> support -> supported

Done.

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:124: url_fetcher_.reset();
On 2013/05/27 21:34:35, MAD wrote:
> This is not needed since the destructor of the scoped_ptr does it.

Done.

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:129: 
On 2013/05/27 21:34:35, MAD wrote:
> Nit: Maybe remove this empty line.

Done.

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

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.h:60: // before killing IO
thread because this class is singleton, and it may be
On 2013/05/27 21:34:35, MAD wrote:
> Comment about singleton should be updated.

Done.

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

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:720: if (language_list_.get())
This is intended.
In the browser process, use-after-free bug is a critical security issue.
So, I often used both DCHECK and if in net/ because DCHECK works only on debug
build, and sometime tests don't cover the problematic timing. if check works
just in case on release build.

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
File chrome/browser/translate/translate_manager.h (right):

https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/...
chrome/browser/translate/translate_manager.h:228: // An instance of
TranslateLanguageList which manages supporting language
On 2013/05/27 21:34:35, MAD wrote:
> supporting -> supported

Done.

[commit-bot: I haz the power, 2013-05-28 07:35:55]

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

[commit-bot: I haz the power, 2013-05-28 10:55:32]

Change committed as 202527

[MAD, 2013-05-28 13:33:11]

The usual pattern in that case is to use a NOTREACH() in the else clause of
the if that checks for null... And note that a null pointer is not the same
as a use after free... The dcheck would not catch a use after free since it
checks for null and not free...

BYE
MAD...
sent from Mobile Answering Device!
Le 28 mai 2013 03:35, <toyoshim@chromium.org> a écrit :

>
> https://codereview.chromium.**org/15317007/diff/51009/**
>
chrome/browser/translate/**translate_language_list.cc<https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/translate_language_list.cc>
> File chrome/browser/translate/**translate_language_list.cc (right):
>
> https://codereview.chromium.**org/15317007/diff/51009/**
>
chrome/browser/translate/**translate_language_list.cc#**newcode108<https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/translate_language_list.cc#newcode108>
> chrome/browser/translate/**translate_language_list.cc:**108:
> On 2013/05/27 21:34:35, MAD wrote:
>
>> One empty line is enough.
>>
>
> Done.
>
> https://codereview.chromium.**org/15317007/diff/51009/**
>
chrome/browser/translate/**translate_language_list.cc#**newcode117<https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/translate_language_list.cc#newcode117>
> chrome/browser/translate/**translate_language_list.cc:**117: //
> |kDefaultSupportedLanguages|. This list will be override by a server
> On 2013/05/27 21:34:35, MAD wrote:
>
>> override -> overriden
>>
>
> Done.
>
> https://codereview.chromium.**org/15317007/diff/51009/**
>
chrome/browser/translate/**translate_language_list.cc#**newcode118<https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/translate_language_list.cc#newcode118>
> chrome/browser/translate/**translate_language_list.cc:**118: // providing
> support langauges list.
> On 2013/05/27 21:34:35, MAD wrote:
>
>> support -> supported
>>
>
> Done.
>
> https://codereview.chromium.**org/15317007/diff/51009/**
>
chrome/browser/translate/**translate_language_list.cc#**newcode124<https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/translate_language_list.cc#newcode124>
> chrome/browser/translate/**translate_language_list.cc:**124:
> url_fetcher_.reset();
> On 2013/05/27 21:34:35, MAD wrote:
>
>> This is not needed since the destructor of the scoped_ptr does it.
>>
>
> Done.
>
> https://codereview.chromium.**org/15317007/diff/51009/**
>
chrome/browser/translate/**translate_language_list.cc#**newcode129<https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/translate_language_list.cc#newcode129>
> chrome/browser/translate/**translate_language_list.cc:**129:
> On 2013/05/27 21:34:35, MAD wrote:
>
>> Nit: Maybe remove this empty line.
>>
>
> Done.
>
> https://codereview.chromium.**org/15317007/diff/51009/**
>
chrome/browser/translate/**translate_language_list.h<https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/translate_language_list.h>
> File chrome/browser/translate/**translate_language_list.h (right):
>
> https://codereview.chromium.**org/15317007/diff/51009/**
>
chrome/browser/translate/**translate_language_list.h#**newcode60<https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/translate_language_list.h#newcode60>
> chrome/browser/translate/**translate_language_list.h:60: // before killing
> IO thread because this class is singleton, and it may be
> On 2013/05/27 21:34:35, MAD wrote:
>
>> Comment about singleton should be updated.
>>
>
> Done.
>
> https://codereview.chromium.**org/15317007/diff/51009/**
>
chrome/browser/translate/**translate_manager.cc<https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/translate_manager.cc>
> File chrome/browser/translate/**translate_manager.cc (right):
>
> https://codereview.chromium.**org/15317007/diff/51009/**
>
chrome/browser/translate/**translate_manager.cc#**newcode720<https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/translate_manager.cc#newcode720>
> chrome/browser/translate/**translate_manager.cc:720: if
> (language_list_.get())
> This is intended.
> In the browser process, use-after-free bug is a critical security issue.
> So, I often used both DCHECK and if in net/ because DCHECK works only on
> debug build, and sometime tests don't cover the problematic timing. if
> check works just in case on release build.
>
> https://codereview.chromium.**org/15317007/diff/51009/**
>
chrome/browser/translate/**translate_manager.h<https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/translate_manager.h>
> File chrome/browser/translate/**translate_manager.h (right):
>
> https://codereview.chromium.**org/15317007/diff/51009/**
>
chrome/browser/translate/**translate_manager.h#newcode228<https://codereview.chromium.org/15317007/diff/51009/chrome/browser/translate/translate_manager.h#newcode228>
> chrome/browser/translate/**translate_manager.h:228: // An instance of
> TranslateLanguageList which manages supporting language
> On 2013/05/27 21:34:35, MAD wrote:
>
>> supporting -> supported
>>
>
> Done.
>
>
https://codereview.chromium.**org/15317007/<https://codereview.chromium.org/1...
>