Introduce TranslateService and TranslateDownloadManager

Introduce TranslateService and TranslateDownloadManager

This CL removes the TranslateDelegate interface and introduces
TranslateService and TranslateDownloadManager.

This will enable (in a future CL) to move TranslateScript and
TranslateLanguageList in the Translate component, and
TranslateDownloadManager will own them.

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

[blundell, 2014-01-24 12:36:58]

Question before I do a full review. Also, to make sure I understand,
TranslateService will be the long-term //chrome-side singleton "helper object"
that e.g. sets the resource requests allowed status, right?

https://codereview.chromium.org/145023015/diff/80001/components/translate/cor...
File components/translate/core/browser/translate_url_fetcher.cc (right):

https://codereview.chromium.org/145023015/diff/80001/components/translate/cor...
components/translate/core/browser/translate_url_fetcher.cc:54:
TranslateDownloadManager::GetInstance()->request_context());
Could you explain this a little more to me? It's not obvious why these two
request contexts should necessarily be the same one at a conceptual level.

[droger, 2014-01-24 13:40:34]

On 2014/01/24 12:36:58, blundell wrote:
> Question before I do a full review. Also, to make sure I understand,
> TranslateService will be the long-term //chrome-side singleton "helper object"
> that e.g. sets the resource requests allowed status, right?
> 
Yes

[droger, 2014-01-24 13:44:31]

https://codereview.chromium.org/145023015/diff/80001/components/translate/cor...
File components/translate/core/browser/translate_url_fetcher.cc (right):

https://codereview.chromium.org/145023015/diff/80001/components/translate/cor...
components/translate/core/browser/translate_url_fetcher.cc:54:
TranslateDownloadManager::GetInstance()->request_context());
On 2014/01/24 12:36:59, blundell wrote:
> Could you explain this a little more to me? It's not obvious why these two
> request contexts should necessarily be the same one at a conceptual level.

I'm not sure what your concern is here.
Previously it was using TranslateDelegate, which was calling into
g_browser_process.
Now we're using the request context that was initialized at startup (by
TranslateService) with the same request context from g_browser_process.

[blundell, 2014-01-24 15:01:45]

https://codereview.chromium.org/145023015/diff/80001/components/translate/cor...
File components/translate/core/browser/translate_url_fetcher.cc (right):

https://codereview.chromium.org/145023015/diff/80001/components/translate/cor...
components/translate/core/browser/translate_url_fetcher.cc:54:
TranslateDownloadManager::GetInstance()->request_context());
I agree that the change is safe to make at the current time, i.e., it doesn't
change behavior. What I'm wondering is whether it's conceptually correct, i.e.,
is there some inherent reason that TranslateURLFetcher should always be using
TranslateDownloadManager's request context (e.g., vs. one passed in from the
embedder).

On 2014/01/24 13:44:31, droger wrote:
> On 2014/01/24 12:36:59, blundell wrote:
> > Could you explain this a little more to me? It's not obvious why these two
> > request contexts should necessarily be the same one at a conceptual level.
> 
> I'm not sure what your concern is here.
> Previously it was using TranslateDelegate, which was calling into
> g_browser_process.
> Now we're using the request context that was initialized at startup (by
> TranslateService) with the same request context from g_browser_process.

[droger, 2014-01-24 15:14:54]

https://codereview.chromium.org/145023015/diff/80001/components/translate/cor...
File components/translate/core/browser/translate_url_fetcher.cc (right):

https://codereview.chromium.org/145023015/diff/80001/components/translate/cor...
components/translate/core/browser/translate_url_fetcher.cc:54:
TranslateDownloadManager::GetInstance()->request_context());
Do you mean that somehow g_browser_process->system_request_context() could
change?

Or do you mean that some hypothetical embedder other than chrome might want to
change it?

If it's the latter, then there is the set_request_context() method on
TranslateDownloadManager that can be used, and I don't think we need more
flexibility: this fetcher is typically only used twice (once for the translate
script and once for the language list), so it doesn't need a complex logic.

[droger, 2014-01-24 17:47:59]

+mad

[blundell, 2014-01-24 17:52:43]

I like the direction of this CL a lot. My question about TranslateURLFetcher was
basically whether it was download-specific; you informed me in an offline
discussion that it was, so I'm OK with tying it together with
TranslateDownloadManager.

https://codereview.chromium.org/145023015/diff/240001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/145023015/diff/240001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1550:
TranslateManager::GetInstance()->FetchLanguageListFromTranslateServer(
So this will become a call on TranslateDownloadManager in followup work, right?

https://codereview.chromium.org/145023015/diff/240001/chrome/browser/translat...
File chrome/browser/translate/translate_service.cc (right):

https://codereview.chromium.org/145023015/diff/240001/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:14:
TranslateDownloadManager::GetInstance()->set_request_context(
Is this class going to become more meaty in the future? As it is now, it doesn't
seem to have much value over inlining the calls to TranlateDownloadManager in
//chrome code.

https://codereview.chromium.org/145023015/diff/240001/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:21:
TranslateManager::GetInstance()->CleanupPendingUlrFetcher();
Could you file a bug to rename this CleanupPendingUrlFetcher?

https://codereview.chromium.org/145023015/diff/240001/chrome/browser/translat...
File chrome/browser/translate/translate_service.h (right):

https://codereview.chromium.org/145023015/diff/240001/chrome/browser/translat...
chrome/browser/translate/translate_service.h:8: // Manages the resoureces
required for Translate.
resources

Mention here that it's a singleton?

https://codereview.chromium.org/145023015/diff/240001/chrome/browser/translat...
chrome/browser/translate/translate_service.h:15: // Must be called to shutdown
the Translate feature.
shut down

https://codereview.chromium.org/145023015/diff/240001/components/translate/co...
File components/translate/core/browser/translate_download_manager.h (right):

https://codereview.chromium.org/145023015/diff/240001/components/translate/co...
components/translate/core/browser/translate_download_manager.h:19: //
TranslateScript.
bug?

https://codereview.chromium.org/145023015/diff/240001/components/translate/co...
components/translate/core/browser/translate_download_manager.h:27: void
set_request_context(net::URLRequestContextGetter* context) {
Does this have to be set before anything can be done with this class? Worth
noting if so.

https://codereview.chromium.org/145023015/diff/240001/components/translate/co...
components/translate/core/browser/translate_download_manager.h:33: void
set_application_locale(const std::string& locale) {
Same question as above.

[MAD, 2014-01-24 18:05:14]

I'm Sheriffing today and Monday, so may not get to this before Tuesday...
But I will try...

Maybe you could ask the other translate OWNERs if you can't wait?



On Fri, Jan 24, 2014 at 12:48 PM, <droger@chromium.org> wrote:

> +mad
>
> https://codereview.chromium.org/145023015/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[droger, 2014-01-24 18:12:47]

On 2014/01/24 18:05:14, MAD wrote:
> I'm Sheriffing today and Monday, so may not get to this before Tuesday...
> But I will try...
> 
> Maybe you could ask the other translate OWNERs if you can't wait?
> 

Thanks for the head up!
I'll wait, no problem.

[droger, 2014-01-27 14:13:43]

https://codereview.chromium.org/145023015/diff/240001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/145023015/diff/240001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1550:
TranslateManager::GetInstance()->FetchLanguageListFromTranslateServer(
On 2014/01/24 17:52:43, blundell wrote:
> So this will become a call on TranslateDownloadManager in followup work,
right?

Yes.

https://codereview.chromium.org/145023015/diff/240001/chrome/browser/translat...
File chrome/browser/translate/translate_service.cc (right):

https://codereview.chromium.org/145023015/diff/240001/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:14:
TranslateDownloadManager::GetInstance()->set_request_context(
On 2014/01/24 17:52:43, blundell wrote:
> Is this class going to become more meaty in the future? As it is now, it
doesn't
> seem to have much value over inlining the calls to TranlateDownloadManager in
> //chrome code.

Yes, it is. See the followup CL:
https://codereview.chromium.org/143003020/diff/80001/chrome/browser/translate...

https://codereview.chromium.org/145023015/diff/240001/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:21:
TranslateManager::GetInstance()->CleanupPendingUlrFetcher();
On 2014/01/24 17:52:43, blundell wrote:
> Could you file a bug to rename this CleanupPendingUrlFetcher?

Why do you want to rename this?

https://codereview.chromium.org/145023015/diff/240001/chrome/browser/translat...
File chrome/browser/translate/translate_service.h (right):

https://codereview.chromium.org/145023015/diff/240001/chrome/browser/translat...
chrome/browser/translate/translate_service.h:8: // Manages the resoureces
required for Translate.
On 2014/01/24 17:52:43, blundell wrote:
> resources
> 
> Mention here that it's a singleton?

This is not a singleton so far (it's just a couple of static functions).
It will become a singleton in my next CL though, so I can update the comment
now.
Done.

[blundell, 2014-01-27 16:02:32]

Looks good. The only thing that's a little unclear to me is the relationship
between TranslateService and TranslateDownloadManager. TranslateService is
basically the embedder-level glue for the component-level global state?

https://codereview.chromium.org/145023015/diff/400001/chrome/browser/translat...
File chrome/browser/translate/translate_manager_browsertest.cc (right):

https://codereview.chromium.org/145023015/diff/400001/chrome/browser/translat...
chrome/browser/translate/translate_manager_browsertest.cc:239:
TranslateDownloadManager::GetInstance()->set_application_locale(
Should this not be part of TranslateService::Initialize() ?

https://codereview.chromium.org/145023015/diff/400001/components/translate/co...
File components/translate/core/browser/translate_download_manager.h (right):

https://codereview.chromium.org/145023015/diff/400001/components/translate/co...
components/translate/core/browser/translate_download_manager.h:19: //
TranslateScript.
bug?

https://codereview.chromium.org/145023015/diff/400001/components/translate/co...
components/translate/core/browser/translate_download_manager.h:26: // Should be
set before this class can be used.
Can we DCHECK this "should" and the one below?

https://codereview.chromium.org/145023015/diff/400001/components/translate/co...
File components/translate/core/browser/translate_url_fetcher.h (left):

https://codereview.chromium.org/145023015/diff/400001/components/translate/co...
components/translate/core/browser/translate_url_fetcher.h:15: class
TranslateURLFetcher : public net::URLFetcherDelegate {
Maybe a class-level comment on what this is used for?

[droger, 2014-01-28 14:16:48]

https://codereview.chromium.org/145023015/diff/400001/chrome/browser/translat...
File chrome/browser/translate/translate_manager_browsertest.cc (right):

https://codereview.chromium.org/145023015/diff/400001/chrome/browser/translat...
chrome/browser/translate/translate_manager_browsertest.cc:239:
TranslateDownloadManager::GetInstance()->set_application_locale(
On 2014/01/27 16:02:33, blundell wrote:
> Should this not be part of TranslateService::Initialize() ?

Good suggestion.
Done.

https://codereview.chromium.org/145023015/diff/400001/components/translate/co...
File components/translate/core/browser/translate_download_manager.h (right):

https://codereview.chromium.org/145023015/diff/400001/components/translate/co...
components/translate/core/browser/translate_download_manager.h:26: // Should be
set before this class can be used.
On 2014/01/27 16:02:33, blundell wrote:
> Can we DCHECK this "should" and the one below?

Done for the application locale.
Tests use a NULL request context, thus I can't DCHECK(request_context_);

https://codereview.chromium.org/145023015/diff/400001/components/translate/co...
File components/translate/core/browser/translate_url_fetcher.h (left):

https://codereview.chromium.org/145023015/diff/400001/components/translate/co...
components/translate/core/browser/translate_url_fetcher.h:15: class
TranslateURLFetcher : public net::URLFetcherDelegate {
On 2014/01/27 16:02:33, blundell wrote:
> Maybe a class-level comment on what this is used for?

Done.

https://codereview.chromium.org/145023015/diff/670001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/145023015/diff/670001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:640:
TranslateDownloadManager::GetInstance()->set_application_locale(locale);
This line can be removed if we assume that the locale never changes, as the
locale is already set in TranslateService::Initialize.

[MAD, 2014-01-28 15:08:25]

lgtm

[blundell, 2014-01-29 12:25:13]

LGTM

https://codereview.chromium.org/145023015/diff/670001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/145023015/diff/670001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:640:
TranslateDownloadManager::GetInstance()->set_application_locale(locale);
I think that this line should stay -- it does no harm, and it seems conceptually
correct to me to have this line be in this function.

On 2014/01/28 14:16:48, droger wrote:
> This line can be removed if we assume that the locale never changes, as the
> locale is already set in TranslateService::Initialize.

[droger, 2014-01-29 12:56:02]

+jochen as OWNER of:
chrome/browser/browser_process_impl.cc
chrome/browser/chrome_browser_main.cc
chrome/browser/chrome_browser_main.h

[jochen (gone - plz use gerrit), 2014-01-29 14:39:46]

lgtm

[commit-bot: I haz the power, 2014-01-29 14:56:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/145023015/690001

[commit-bot: I haz the power, 2014-01-29 18:22:21]

Change committed as 247724