Add translate client mock.

Add translate client mock.

Remove duplicates from translate_manager_unittest and translate_ui_delegate_unittest.

BUG=727436
Review-Url: https://codereview.chromium.org/2906233004
Cr-Commit-Position: refs/heads/master@{#476191}
Committed: https://chromium.googlesource.com/chromium/src/+/8f6a87ae5a06c8a93d64ddef451563414ddad9de

[Leo, 2017-05-30 01:41:40]

Description was changed from

==========
Add translate client mock.

BUG=
==========

to

==========
Add translate client mock.

Remove duplicates from translate_manager_unittest and
translate_ui_delegate_unittest.

BUG=727436
==========

[Leo, 2017-05-30 03:26:28]

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

[commit-bot: I haz the power, 2017-05-30 03:26:51]

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-05-30 04:57:05]

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

[commit-bot: I haz the power, 2017-05-30 04:57:06]

Dry run: This issue passed the CQ dry run.

[Leo, 2017-05-30 05:53:58]

googleo@chromium.org changed reviewers:
+ groby@chromium.org, napper@chromium.org

[Leo, 2017-05-30 05:53:59]

Create the client mock first, So I can use it in delegate tests

[napper, 2017-05-30 06:24:36]

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
File components/translate/core/browser/mock_translate_client.cc (right):

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
components/translate/core/browser/mock_translate_client.cc:15: const char*
preferred_languages_prefs = NULL;
nullptr

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
components/translate/core/browser/mock_translate_client.cc:26: TranslateDriver*
MockTranslateClient::GetTranslateDriver() {
Can these be inline?

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
components/translate/core/browser/mock_translate_client.cc:39: #if
!defined(USE_AURA)
remove

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
File components/translate/core/browser/mock_translate_client.h (right):

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
components/translate/core/browser/mock_translate_client.h:45: return
base::WrapUnique(CreateInfoBarMock(std::move(delegate).get()));
This doesn't make sense to me:

std::move(delegate).get()

the move makes an xvalue, but then get() turns it into a raw pointer, which is
passed to CreateInfoBarMock. What is the purpose of the move()?

[groby-ooo-7-16, 2017-05-31 00:41:15]

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
File components/translate/core/browser/mock_translate_client.h (right):

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
components/translate/core/browser/mock_translate_client.h:20: extern const char*
preferred_languages_prefs;
We really shouldn't expose global constants from a mock object in a non-testing
namespace. (Ideally, we don't expose them at all.)

Note: I'm not sure how we can achieve this easily, so if Jon is OK, feel free to
skip that for this CL - but overall, these constants are showing up all over the
place. (And they're technically Chrome constants, not components constants)

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
components/translate/core/browser/mock_translate_client.h:45: return
base::WrapUnique(CreateInfoBarMock(std::move(delegate).get()));
On 2017/05/30 06:24:36, napper wrote:
> This doesn't make sense to me:
> 
> std::move(delegate).get()
> 
> the move makes an xvalue, but then get() turns it into a raw pointer, which is
> passed to CreateInfoBarMock. What is the purpose of the move()?

I've done digging through history, and it seems that way from almost beginning
of time... Maybe weird behavioral differences between unique_ptr/scoped_ptr,
maybe an oversight in the original review. But yes, a delegate.get() should be
enough.

[napper, 2017-05-31 01:16:37]

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
File components/translate/core/browser/mock_translate_client.h (right):

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
components/translate/core/browser/mock_translate_client.h:20: extern const char*
preferred_languages_prefs;
On 2017/05/31 at 00:41:15, groby wrote:
> We really shouldn't expose global constants from a mock object in a
non-testing namespace. (Ideally, we don't expose them at all.)
> 
> Note: I'm not sure how we can achieve this easily, so if Jon is OK, feel free
to skip that for this CL - but overall, these constants are showing up all over
the place. (And they're technically Chrome constants, not components constants)

Yes this isn't very nice. Can you put the constants in the testing namespace?

[Leo, 2017-05-31 01:38:26]

Thanks a lot, Rachel and Jon.

I have fixed all the comments and tested locally.

PTAL

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
File components/translate/core/browser/mock_translate_client.cc (right):

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
components/translate/core/browser/mock_translate_client.cc:15: const char*
preferred_languages_prefs = NULL;
On 2017/05/30 06:24:35, napper wrote:
> nullptr

Acknowledged.

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
components/translate/core/browser/mock_translate_client.cc:26: TranslateDriver*
MockTranslateClient::GetTranslateDriver() {
On 2017/05/30 06:24:35, napper wrote:
> Can these be inline?

I did write them inline. But "git cl format" prefers the current format.

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
components/translate/core/browser/mock_translate_client.cc:39: #if
!defined(USE_AURA)
On 2017/05/30 06:24:35, napper wrote:
> remove

Thanks for the catch.

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
File components/translate/core/browser/mock_translate_client.h (right):

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
components/translate/core/browser/mock_translate_client.h:20: extern const char*
preferred_languages_prefs;
On 2017/05/31 01:16:37, napper wrote:
> On 2017/05/31 at 00:41:15, groby wrote:
> > We really shouldn't expose global constants from a mock object in a
> non-testing namespace. (Ideally, we don't expose them at all.)
> > 
> > Note: I'm not sure how we can achieve this easily, so if Jon is OK, feel
free
> to skip that for this CL - but overall, these constants are showing up all
over
> the place. (And they're technically Chrome constants, not components
constants)
> 
> Yes this isn't very nice. Can you put the constants in the testing namespace?

Thanks for the concern and tips. I moved these to testing namespace. So they can
be used by all translate testing modules.

https://codereview.chromium.org/2906233004/diff/20001/components/translate/co...
components/translate/core/browser/mock_translate_client.h:45: return
base::WrapUnique(CreateInfoBarMock(std::move(delegate).get()));
On 2017/05/31 00:41:15, groby wrote:
> On 2017/05/30 06:24:36, napper wrote:
> > This doesn't make sense to me:
> > 
> > std::move(delegate).get()
> > 
> > the move makes an xvalue, but then get() turns it into a raw pointer, which
is
> > passed to CreateInfoBarMock. What is the purpose of the move()?
> 
> I've done digging through history, and it seems that way from almost beginning
> of time... Maybe weird behavioral differences between unique_ptr/scoped_ptr,
> maybe an oversight in the original review. But yes, a delegate.get() should be
> enough. 

Great to know the details.
Removed std::move()

[napper, 2017-05-31 04:57:14]

lgtm

[Leo, 2017-05-31 04:57:56]

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

[commit-bot: I haz the power, 2017-05-31 04:58:42]

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-05-31 06:01:20]

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

[commit-bot: I haz the power, 2017-05-31 06:01:21]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[groby-ooo-7-16, 2017-05-31 20:02:07]

On 2017/05/31 04:57:14, napper wrote:
> lgtm

lgtm

[Leo, 2017-06-01 04:29:13]

The CQ bit was checked by googleo@chromium.org

[commit-bot: I haz the power, 2017-06-01 04:29:26]

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-06-01 04:34:01]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496291353641640,
"parent_rev": "c8947f9db1b1bd58ea60a6638e8e889f07c5a362", "commit_rev":
"8f6a87ae5a06c8a93d64ddef451563414ddad9de"}

[commit-bot: I haz the power, 2017-06-01 04:34:08]

Description was changed from

==========
Add translate client mock.

Remove duplicates from translate_manager_unittest and
translate_ui_delegate_unittest.

BUG=727436
==========

to

==========
Add translate client mock.

Remove duplicates from translate_manager_unittest and
translate_ui_delegate_unittest.

BUG=727436

Review-Url: https://codereview.chromium.org/2906233004
Cr-Commit-Position: refs/heads/master@{#476191}
Committed:
https://chromium.googlesource.com/chromium/src/+/8f6a87ae5a06c8a93d64ddef4515...
==========

[commit-bot: I haz the power, 2017-06-01 04:34:10]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8f6a87ae5a06c8a93d64ddef4515...