Move TranslatePrefs to the Translate component

Move TranslatePrefs to the Translate component

TranslatePrefs now needs the path to preferences in its constructor.
To help with this, a helper method is added to TranslateTabHelper to build
TranslatePrefs instances.
As a result, TranslatePrefs are no longer instantiated on the stack, but rather
on the heap.

BUG=335079
TBR=battre, jochen, joi

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

[blundell, 2014-02-12 14:01:43]

LGTM

https://codereview.chromium.org/159883002/diff/170001/chrome/browser/translat...
File chrome/browser/translate/translate_tab_helper.h (right):

https://codereview.chromium.org/159883002/diff/170001/chrome/browser/translat...
chrome/browser/translate/translate_tab_helper.h:43: // accept language.
|language| will be converted if it has the synonym of
I don't understand the second sentence here.

https://codereview.chromium.org/159883002/diff/170001/components/translate/co...
File components/translate/core/browser/translate_prefs.cc (left):

https://codereview.chromium.org/159883002/diff/170001/components/translate/co...
components/translate/core/browser/translate_prefs.cc:296:
TranslateAcceptLanguages* accept_languages =
Bikeshedding: It seems to me that an alternative for this method would be to
have the class take in a TranslateAcceptLanguages instance in its constructor
(there should be a 1:1 mapping between them, right?). Then this method would
just be CanTranslateLanguage(language), which seems appealing. What do you
think?

https://codereview.chromium.org/159883002/diff/170001/components/translate/co...
File components/translate/core/browser/translate_prefs.h (right):

https://codereview.chromium.org/159883002/diff/170001/components/translate/co...
components/translate/core/browser/translate_prefs.h:89: // Sets the language
list of the language settings.
This comment reads oddly now: The method is called "Get" and the comment starts
with "Sets".

[droger, 2014-02-12 14:08:22]

https://codereview.chromium.org/159883002/diff/170001/components/translate/co...
File components/translate/core/browser/translate_prefs.cc (left):

https://codereview.chromium.org/159883002/diff/170001/components/translate/co...
components/translate/core/browser/translate_prefs.cc:296:
TranslateAcceptLanguages* accept_languages =
On 2014/02/12 14:01:43, blundell wrote:
> Bikeshedding: It seems to me that an alternative for this method would be to
> have the class take in a TranslateAcceptLanguages instance in its constructor
> (there should be a 1:1 mapping between them, right?). Then this method would
> just be CanTranslateLanguage(language), which seems appealing. What do you
> think?

I'm a bit scared because this assumes that the TranslateAcceptLanguages instance
won't be deleted before this.
Maybe we can pass the TranslateAcceptLanguages as a parameter to that function
instead?

[droger, 2014-02-12 14:38:46]

Thanks for the review.
I fixed/replied to all your comments.

https://codereview.chromium.org/159883002/diff/170001/components/translate/co...
File components/translate/core/browser/translate_prefs.cc (left):

https://codereview.chromium.org/159883002/diff/170001/components/translate/co...
components/translate/core/browser/translate_prefs.cc:296:
TranslateAcceptLanguages* accept_languages =
With your suggestion, this method can also no longer be static.

I went with my counter-suggestion: TranslateAcceptLanguages is now a parameter
to this method.
Let me know if you dislike it.

[droger, 2014-02-12 15:09:13]

+mad

[MAD, 2014-02-13 03:11:10]

I have one comment...

https://codereview.chromium.org/159883002/diff/300001/components/translate/co...
File components/translate/core/browser/translate_prefs.h (right):

https://codereview.chromium.org/159883002/diff/300001/components/translate/co...
components/translate/core/browser/translate_prefs.h:40: #if defined(OS_CHROMEOS)
I don't really like this idea of a different signature on different platforms. I
would prefer we pass NULL on non-CrOS platforms, or have a second constructor
for CrOS...

[droger, 2014-02-13 09:38:38]

Thanks for the review. Please take a look.

https://codereview.chromium.org/159883002/diff/300001/components/translate/co...
File components/translate/core/browser/translate_prefs.h (right):

https://codereview.chromium.org/159883002/diff/300001/components/translate/co...
components/translate/core/browser/translate_prefs.h:40: #if defined(OS_CHROMEOS)
On 2014/02/13 03:11:11, MAD wrote:
> I don't really like this idea of a different signature on different platforms.
I
> would prefer we pass NULL on non-CrOS platforms, or have a second constructor
> for CrOS...

Done.

[MAD, 2014-02-13 14:42:34]

Cool... Thanks!

LGTM

BYE
MAD

[droger, 2014-02-13 14:54:58]

TBR=battre for the new dependency of components/translate on
components/user_prefs

TBR=joi as OWNER of components_tests.gyp

TBR=jochen as OWNER of:
    chrome/browser/prefs/browser_prefs.cc
    chrome/browser/sync/test/integration/migration_test.cc
    chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc
    chrome/browser/tab_contents/render_view_context_menu.cc
    chrome/browser/ui/cocoa/infobars/translate_infobar_unittest.mm
    chrome/browser/ui/webui/options/language_options_handler_common.cc
   chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc

[droger, 2014-02-13 14:55:31]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-02-13 14:55:58]

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

[Jói, 2014-02-13 15:01:56]

components_tests.gyp LGTM

On Thu, Feb 13, 2014 at 2:55 PM,  <commit-bot@chromium.org> wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/droger@chromium.org/159883002/530001
>
>
> https://codereview.chromium.org/159883002/

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

[battre, 2014-02-13 15:07:42]

dependency of components/translate on components/user_prefs LGTM

[commit-bot: I haz the power, 2014-02-13 16:34:19]

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

[commit-bot: I haz the power, 2014-02-13 16:34:22]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, check_deps2git, chromeos_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, events_unittests, google_apis_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[droger, 2014-02-13 17:20:13]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-02-13 17:20:48]

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

[commit-bot: I haz the power, 2014-02-13 19:51:18]

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

[commit-bot: I haz the power, 2014-02-13 23:35:20]

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

[commit-bot: I haz the power, 2014-02-13 23:35:22]

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

[droger, 2014-02-14 03:20:23]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-02-14 03:22:21]

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

[commit-bot: I haz the power, 2014-02-14 06:08:17]

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

[commit-bot: I haz the power, 2014-02-14 06:08:18]

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

[droger, 2014-02-14 09:22:58]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-02-14 09:23:30]

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

[jochen (gone - plz use gerrit), 2014-02-14 09:35:12]

lgtm

[commit-bot: I haz the power, 2014-02-14 14:11:09]

Change committed as 251305

[tfarina, 2014-05-11 23:27:01]

https://codereview.chromium.org/159883002/diff/1360001/components/translate.gypi
File components/translate.gypi (right):

https://codereview.chromium.org/159883002/diff/1360001/components/translate.g...
components/translate.gypi:14: 'user_prefs',
By depending on this, now translate_core_browser does also depends on
content_browser. Do we want that in core target?

[tfarina, 2014-05-11 23:35:13]

On 2014/05/11 23:27:01, tfarina wrote:
>
https://codereview.chromium.org/159883002/diff/1360001/components/translate.gypi
> File components/translate.gypi (right):
> 
>
https://codereview.chromium.org/159883002/diff/1360001/components/translate.g...
> components/translate.gypi:14: 'user_prefs',
> By depending on this, now translate_core_browser does also depends on
> content_browser. Do we want that in core target?

My patch in  https://codereview.chromium.org/271793003/ helps with this, but I
need to update the deps there.

[droger, 2014-05-12 08:24:08]

https://codereview.chromium.org/159883002/diff/1360001/components/translate.gypi
File components/translate.gypi (right):

https://codereview.chromium.org/159883002/diff/1360001/components/translate.g...
components/translate.gypi:14: 'user_prefs',
On 2014/05/11 23:27:02, tfarina wrote:
> By depending on this, now translate_core_browser does also depends on
> content_browser. Do we want that in core target?

You are right, this target should not depend on content/, this is a bug.

[blundell, 2014-05-12 08:29:26]

On 2014/05/11 23:35:13, tfarina wrote:
> On 2014/05/11 23:27:01, tfarina wrote:
> >
>
https://codereview.chromium.org/159883002/diff/1360001/components/translate.gypi
> > File components/translate.gypi (right):
> > 
> >
>
https://codereview.chromium.org/159883002/diff/1360001/components/translate.g...
> > components/translate.gypi:14: 'user_prefs',
> > By depending on this, now translate_core_browser does also depends on
> > content_browser. Do we want that in core target?
> 
> My patch in  https://codereview.chromium.org/271793003/ helps with this, but I
> need to update the deps there.

Unless I'm missing something, your CL there fixes this problem.

Thanks,

Colin