TranslatePrefTest: Added test case for UpdateLanguageList.

TranslatePrefTest: Added test case for UpdateLanguageList.

I am looking at refactoring some code used by UpdateLanguageList and I
found that it had no test case.

Review-Url: https://codereview.chromium.org/2722413002
Cr-Commit-Position: refs/heads/master@{#462757}
Committed: https://chromium.googlesource.com/chromium/src/+/a16b847b4a744c81aff3d3ffc337da226aafa7c8

[Matt Giuca, 2017-03-02 03:33:05]

mgiuca@chromium.org changed reviewers:
+ groby@chromium.org

[Matt Giuca, 2017-03-02 04:01:13]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 04:03:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 06:04:08]

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

[commit-bot: I haz the power, 2017-03-02 06:04:09]

Dry run: This issue passed the CQ dry run.

[groby-ooo-7-16, 2017-03-08 00:49:15]

Thank you for adding tests!

https://codereview.chromium.org/2722413002/diff/1/components/translate/core/b...
File components/translate/core/browser/translate_prefs_unittest.cc (right):

https://codereview.chromium.org/2722413002/diff/1/components/translate/core/b...
components/translate/core/browser/translate_prefs_unittest.cc:26: #if
defined(OS_CHROMEOS)
If you're refactoring this anyways - could you factor this out into a function?
There are _numerous_ tests that do exactly this, and it's... annoying. (It's
also annoying that tests in components/ depend on things defined in chrome/, but
that's a larger story :)

https://codereview.chromium.org/2722413002/diff/1/components/translate/core/b...
components/translate/core/browser/translate_prefs_unittest.cc:51:
prefs_->registry()->RegisterStringPref(kPreferredLanguagesPref,
I'd really like to break the defined(OS_CHROMEOS) dependency - would you mind
changing this to 

if(kPreferredLanguagesPref)
  ...RegisterStringPref(kPreferred...

[Matt Giuca, 2017-03-15 02:40:13]

Patchset #2 (id:20001) has been deleted

[Matt Giuca, 2017-03-15 02:45:18]

https://codereview.chromium.org/2722413002/diff/1/components/translate/core/b...
File components/translate/core/browser/translate_prefs_unittest.cc (right):

https://codereview.chromium.org/2722413002/diff/1/components/translate/core/b...
components/translate/core/browser/translate_prefs_unittest.cc:26: #if
defined(OS_CHROMEOS)
On 2017/03/08 00:49:15, groby wrote:
> If you're refactoring this anyways - could you factor this out into a
function?
> There are _numerous_ tests that do exactly this, and it's... annoying. (It's
> also annoying that tests in components/ depend on things defined in chrome/,
but
> that's a larger story :)

Hmm, I had a bit of a think about it but it's not quite clear where this would
go (it doesn't really belong in translate_prefs.h, because it's only used in
tests). It isn't clear to me why this pref name is parameterised in the code
anyway (and only concrete in tests); why isn't it just hard-coded in the code
itself?

I'd rather not have to solve this issue: this CL is a side-yak on a
codebase-wide refactor (https://codereview.chromium.org/2695883003) and I can't
stop to clean up nearby problems in all of those places. I already went one step
further here and added a test. (This is kind of time-critical because the longer
it takes to land that CL, the more merge conflicts I have to deal with.)

https://codereview.chromium.org/2722413002/diff/1/components/translate/core/b...
components/translate/core/browser/translate_prefs_unittest.cc:51:
prefs_->registry()->RegisterStringPref(kPreferredLanguagesPref,
On 2017/03/08 00:49:15, groby wrote:
> I'd really like to break the defined(OS_CHROMEOS) dependency - would you mind
> changing this to 
> 
> if(kPreferredLanguagesPref)
>   ...RegisterStringPref(kPreferred...

Done.

[Matt Giuca, 2017-03-15 02:45:23]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 02:45:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 02:58:44]

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

[commit-bot: I haz the power, 2017-03-15 02:58:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Matt Giuca, 2017-03-15 07:45:14]

https://codereview.chromium.org/2722413002/diff/1/components/translate/core/b...
File components/translate/core/browser/translate_prefs_unittest.cc (right):

https://codereview.chromium.org/2722413002/diff/1/components/translate/core/b...
components/translate/core/browser/translate_prefs_unittest.cc:51:
prefs_->registry()->RegisterStringPref(kPreferredLanguagesPref,
On 2017/03/15 02:45:18, Matt Giuca wrote:
> On 2017/03/08 00:49:15, groby wrote:
> > I'd really like to break the defined(OS_CHROMEOS) dependency - would you
mind
> > changing this to 
> > 
> > if(kPreferredLanguagesPref)
> >   ...RegisterStringPref(kPreferred...
> 
> Done.

Undone... unfortunately I get this compile error on ChromeOS:

error: address of array 'kPreferredLanguagesPref' will always evaluate to 'true'
[-Werror,-Wpointer-bool-conversion]
    if (kPreferredLanguagesPref) {
    ~~  ^~~~~~~~~~~~~~~~~~~~~~~

So this has to remain a compile-time check.

[Matt Giuca, 2017-03-15 07:45:32]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 07:45:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 08:24:20]

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

[commit-bot: I haz the power, 2017-03-15 08:24:23]

Dry run: This issue passed the CQ dry run.

[Matt Giuca, 2017-03-27 02:43:56]

groby: Friendly ping?

[Matt Giuca, 2017-03-28 00:23:15]

Patchset #4 (id:80001) has been deleted

[groby-ooo-7-16, 2017-04-05 21:33:45]

Thank you for adding a test - LGTM

[Matt Giuca, 2017-04-06 00:16:27]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2017-04-06 00:17:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 00:21:54]

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

[commit-bot: I haz the power, 2017-04-06 00:21:55]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Matt Giuca, 2017-04-07 03:26:13]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2017-04-07 03:26:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 05:03:29]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491535573241230,
"parent_rev": "ffd62e54dade66c69d8db05c71cf5a0a2f923012", "commit_rev":
"a16b847b4a744c81aff3d3ffc337da226aafa7c8"}

[commit-bot: I haz the power, 2017-04-07 05:04:19]

Description was changed from

==========
TranslatePrefTest: Added test case for UpdateLanguageList.

I am looking at refactoring some code used by UpdateLanguageList and I
found that it had no test case.
==========

to

==========
TranslatePrefTest: Added test case for UpdateLanguageList.

I am looking at refactoring some code used by UpdateLanguageList and I
found that it had no test case.

Review-Url: https://codereview.chromium.org/2722413002
Cr-Commit-Position: refs/heads/master@{#462757}
Committed:
https://chromium.googlesource.com/chromium/src/+/a16b847b4a744c81aff3d3ffc337...
==========

[commit-bot: I haz the power, 2017-04-07 05:04:21]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a16b847b4a744c81aff3d3ffc337...