Move TranslateLanguageList to the Translate component

Move TranslateLanguageList to the Translate component

A lot of static methods from TranslateManager are moved to
TranslateDownloadManager, as they are related to the management of the
TranslateLanguageList.

The observer interface for translate events has been moved from
TranslateManager to TranslateLanguageList. It also has been changed into a
callback list rather than an observer list, since observers with a single method
are generally discouraged.

The SetSupportedLanguages() method in TranslateLanguageList has been moved
from the anonymous namespace as it needs to invoke the callbacks for
translate events.

Finally, this CL fixes a bug where the TranslateList was not listening to
ResourceRequestNotifications.

BUG=335077, 335085, 339508
R=blundell@chromium.org, jochen@chromium.org, joi@chromium.org, mad@chromium.org
TBR=jochen, joi, sky

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

[droger, 2014-01-24 17:50:19]

FYI, do not review yet. This is the CL where start moving files under the
TranslateDownloadManager.

[droger, 2014-01-30 17:28:27]

+mad

I added a fix for http://crbug.com/339508.

https://codereview.chromium.org/143003020/diff/210002/components/translate/co...
File components/translate/core/browser/translate_language_list.cc (left):

https://codereview.chromium.org/143003020/diff/210002/components/translate/co...
components/translate/core/browser/translate_language_list.cc:123: void
SetSupportedLanguages(const std::string& language_list,
This function has been moved in the class (below) without changes.
This was needed because it calls NotifyEvent, which is now in the class too.

https://codereview.chromium.org/143003020/diff/210002/components/translate/co...
File components/translate/core/browser/translate_language_list.cc (right):

https://codereview.chromium.org/143003020/diff/210002/components/translate/co...
components/translate/core/browser/translate_language_list.cc:202:
RequestLanguageList();
This is the fix for http://crbug.com/339508
When resources requests become allowed, and if we failed a request previously
because it was disallowed, retry now.

[blundell, 2014-01-31 10:49:45]

LGTM

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

https://codereview.chromium.org/143003020/diff/210002/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:29: // Create the TranslateManager
singleton.
nit: blank line before this line.
TranslateManager will go away as a singleton and there will only be this
singleton, right?

https://codereview.chromium.org/143003020/diff/210002/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:44:
TranslateManager::GetInstance()->CleanupPendingUlrFetcher();
aargh!

https://codereview.chromium.org/143003020/diff/210002/chrome/browser/ui/webui...
File chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc
(right):

https://codereview.chromium.org/143003020/diff/210002/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:40:
// |event_subscription_| is deleted automatically and un-registers the
nit: blank line before.

https://codereview.chromium.org/143003020/diff/210002/components/translate/co...
File components/translate/core/browser/translate_download_manager.cc (right):

https://codereview.chromium.org/143003020/diff/210002/components/translate/co...
components/translate/core/browser/translate_download_manager.cc:55: if
(!language_list) {
Is it ever OK for TranslateDownloadManager::language_list() to return NULL? If
not, should this NOTREACHED() everywhere just be a DCHECK in the method?

https://codereview.chromium.org/143003020/diff/210002/components/translate/co...
File components/translate/core/browser/translate_language_list.h (right):

https://codereview.chromium.org/143003020/diff/210002/components/translate/co...
components/translate/core/browser/translate_language_list.h:21: // This class is
defined to be owned only by TranslateDownloadManager.
This is an odd comment to have in the header.

https://codereview.chromium.org/143003020/diff/210002/components/translate/co...
components/translate/core/browser/translate_language_list.h:79: // Notify the
callback list of a translate event.
s/Notify/Notifies

https://codereview.chromium.org/143003020/diff/210002/components/translate/co...
components/translate/core/browser/translate_language_list.h:86: // Callbacks
called on translate events.
What are "translate events"? Maybe take this chance to add some more expansive
documentation to the callback defined above?

[droger, 2014-01-31 13:40:49]

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

https://codereview.chromium.org/143003020/diff/210002/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:29: // Create the TranslateManager
singleton.
On 2014/01/31 10:49:45, blundell wrote:
> nit: blank line before this line.
> TranslateManager will go away as a singleton and there will only be this
> singleton, right?

Right.

https://codereview.chromium.org/143003020/diff/210002/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:44:
TranslateManager::GetInstance()->CleanupPendingUlrFetcher();
On 2014/01/31 10:49:45, blundell wrote:
> aargh!

This function will go away in my next CL :)

https://codereview.chromium.org/143003020/diff/210002/components/translate/co...
File components/translate/core/browser/translate_download_manager.cc (right):

https://codereview.chromium.org/143003020/diff/210002/components/translate/co...
components/translate/core/browser/translate_download_manager.cc:55: if
(!language_list) {
On 2014/01/31 10:49:45, blundell wrote:
> Is it ever OK for TranslateDownloadManager::language_list() to return NULL? If
> not, should this NOTREACHED() everywhere just be a DCHECK in the method?

I agree, but the existing code was like this: every time the language list was
NULL, there was a NOTREACHED but it was handled gracefully.
I am ok to change this logic, but maybe not as part of this CL.

https://codereview.chromium.org/143003020/diff/210002/components/translate/co...
File components/translate/core/browser/translate_language_list.h (right):

https://codereview.chromium.org/143003020/diff/210002/components/translate/co...
components/translate/core/browser/translate_language_list.h:86: // Callbacks
called on translate events.
On 2014/01/31 10:49:45, blundell wrote:
> What are "translate events"? Maybe take this chance to add some more expansive
> documentation to the callback defined above?

I updated the documentation of the RegisterEventCallback() function above.
Maybe we could rename TranslateEventDetails to LanguageListEventDetails, as they
are only used in this file.

[droger, 2014-02-03 09:20:46]

mad: ping

[MAD, 2014-02-03 14:02:43]

Sorry about the delay... I have a few comments...

Thanks by the way... very clean changes...

BYE
MAD

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

https://codereview.chromium.org/143003020/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:28:
g_translate_service->OnResourceRequestsAllowed();
I would add a comment here stating why we make this call... It wasn't clear to
me initially.

https://codereview.chromium.org/143003020/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:47:
download_manager->set_request_context(NULL);
Seems weird to call this after calling Shutdown above... No?

Couldn't Shutdown do it, and this other line would be in an else clause?

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

https://codereview.chromium.org/143003020/diff/320001/components/translate/co...
components/translate/core/browser/translate_download_manager.h:61: // Returns
the last-updated time when Chrome receives a language list from a
receives -> received?

https://codereview.chromium.org/143003020/diff/320001/components/translate/co...
components/translate/core/browser/translate_download_manager.h:73: // alpha
language.
a alpha -> an alpha?

https://codereview.chromium.org/143003020/diff/320001/components/translate/co...
File components/translate/core/browser/translate_language_list.cc (right):

https://codereview.chromium.org/143003020/diff/320001/components/translate/co...
components/translate/core/browser/translate_language_list.cc:171: if
(!resource_requests_allowed_)
Shouldn't we set request_pending_ to true before returning? I don't see it set
to true anywhere...

[droger, 2014-02-03 15:07:20]

Thanks for the review, especially for catching the |request_pending_| bug.
I applied all your comments.

https://codereview.chromium.org/143003020/diff/320001/components/translate/co...
File components/translate/core/browser/translate_language_list.cc (right):

https://codereview.chromium.org/143003020/diff/320001/components/translate/co...
components/translate/core/browser/translate_language_list.cc:171: if
(!resource_requests_allowed_)
On 2014/02/03 14:02:43, MAD wrote:
> Shouldn't we set request_pending_ to true before returning? I don't see it set
> to true anywhere...

Oops, thanks for catching this.

[MAD, 2014-02-03 18:58:51]

Thanks!

LGTM...

BYE
MAD

[droger, 2014-02-03 19:40:19]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-02-03 19:42:02]

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

[commit-bot: I haz the power, 2014-02-03 20:04:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-03 20:04:27]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2014-02-03 20:04:28]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 20:04:38]

CQ bit was unchecked on CL. Ignoring.

[droger, 2014-02-04 11:08:31]

TBR jochen (only renaming methods and includes) for:
chrome/browser/chrome_browser_main.cc
chrome/browser/extensions/
chrome/browser/policy/
chrome/browser/sync/
chrome/browser/tab/
chrome/browser/ui/

TBR joi as the translate component now depends on google_apis/.

TBR sky as the translate component now depends on ui/.

[droger, 2014-02-04 11:09:28]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-02-04 11:10:47]

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

[commit-bot: I haz the power, 2014-02-04 11:30:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 11:30:23]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[droger, 2014-02-04 11:57:47]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-02-04 11:57:59]

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

[jochen (gone - plz use gerrit), 2014-02-04 12:17:44]

lgtm

[Jói, 2014-02-04 13:55:47]

translate dep on google_apis LGTM

On Tue, Feb 4, 2014 at 12:17 PM,  <jochen@chromium.org> wrote:
> lgtm
>
>
>
> https://codereview.chromium.org/143003020/

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

[commit-bot: I haz the power, 2014-02-04 14:02:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 14:02:51]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[skia-commit-bot, 2014-02-04 14:03:00]

CQ bit was unchecked on CL. Ignoring.

[droger, 2014-02-04 14:26:35]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-02-04 14:27:18]

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

[commit-bot: I haz the power, 2014-02-04 15:56:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 15:56:05]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[skia-commit-bot, 2014-02-04 15:56:05]

CQ bit was unchecked on CL. Ignoring.

[droger, 2014-02-04 21:19:08]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-02-04 22:27:56]

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

[commit-bot: I haz the power, 2014-02-05 01:03:10]

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

[droger, 2014-02-05 10:05:41]

The CQ bit was unchecked by droger@chromium.org

[droger, 2014-02-05 10:05:44]

The CQ bit was checked by droger@chromium.org

[droger, 2014-02-05 10:50:40]

The CQ bit was unchecked by droger@chromium.org

[droger, 2014-02-05 10:51:00]

The CQ bit was checked by droger@chromium.org

[droger, 2014-02-05 13:14:32]

Committed patchset #7 manually as r248975 (presubmit successful).