[Translate] Migrate IPCs to Mojo interfaces.

[Translate] Migrate IPCs to Mojo interfaces.

BUG=619390
TEST=browser_tests, unit_tests

Committed: https://crrev.com/3134fc62afcbbc25e0deeb674e5252be9f458d7b
Cr-Commit-Position: refs/heads/master@{#410992}

[leonhsl(Using Gerrit), 2016-07-13 10:06:16]

leon.han@intel.com changed reviewers:
+ amistry@chromium.org, groby@chromium.org

[leonhsl(Using Gerrit), 2016-07-13 10:06:16]

This is the new CL. Thanks.

[leonhsl(Using Gerrit), 2016-07-15 12:21:43]

Description was changed from

==========
[Translate] Migrate IPCs to Mojo interfaces.

BUG=619390
==========

to

==========
[Translate] Migrate IPCs to Mojo interfaces.

BUG=619390
TEST=browser_tests, unit_tests
==========

[leonhsl(Using Gerrit), 2016-07-15 12:24:30]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 12:24:48]

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

[leonhsl(Using Gerrit), 2016-07-15 12:43:22]

ps#2 added changes for browser tests and locally confirmed PASS. But I have a
question about the unit tests..

https://codereview.chromium.org/2143383002/diff/20001/chrome/browser/translat...
File chrome/browser/translate/translate_manager_render_view_host_unittest.cc
(right):

https://codereview.chromium.org/2143383002/diff/20001/chrome/browser/translat...
chrome/browser/translate/translate_manager_render_view_host_unittest.cc:662: #if
!defined(USE_AURA)
Hi, groby, would you please share with me how to get these test cases built and
run? Seems can't simply set gn args 'use_aura = false'... Thanks~

[commit-bot: I haz the power, 2016-07-15 13:21:30]

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

[commit-bot: I haz the power, 2016-07-15 13:21:30]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-07-20 02:08:45]

Ping~

[Anand Mistry (off Chromium), 2016-07-20 13:06:33]

I'm leaving the chromium project. You should get someone else to review for
Mojo.

https://codereview.chromium.org/2143383002/diff/20001/chrome/browser/translat...
File chrome/browser/translate/chrome_translate_client.cc (right):

https://codereview.chromium.org/2143383002/diff/20001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:153: // this would cause
closing the message pipe which would raise connection
nit: Make "this would cause..." a new sentence.

Also, I think this would be better worded as:
This would cause the message pipe to be closed, which will raise a connection
error on the peer side.

[leonhsl(Using Gerrit), 2016-07-21 03:27:40]

On 2016/07/20 13:06:33, Anand Mistry wrote:
> I'm leaving the chromium project. You should get someone else to review for
> Mojo.
> 

Ah.. I see.. Thanks a lot for great support on Mojo until now! I learnt a lot
from you.

>
https://codereview.chromium.org/2143383002/diff/20001/chrome/browser/translat...
> File chrome/browser/translate/chrome_translate_client.cc (right):
> 
>
https://codereview.chromium.org/2143383002/diff/20001/chrome/browser/translat...
> chrome/browser/translate/chrome_translate_client.cc:153: // this would cause
> closing the message pipe which would raise connection
> nit: Make "this would cause..." a new sentence.
> 
> Also, I think this would be better worded as:
> This would cause the message pipe to be closed, which will raise a connection
> error on the peer side.

[leonhsl(Using Gerrit), 2016-07-21 05:43:20]

leon.han@intel.com changed reviewers:
+ rockot@chromium.org, sammc@chromium.org

[leonhsl(Using Gerrit), 2016-07-21 05:43:21]

+Ken +Sam 
Would you PTAL at this as Mojo experts?

Ping groby for the question about USE_AURA unit tests.

Thanks.

[groby-ooo-7-16, 2016-07-21 14:41:29]

https://codereview.chromium.org/2143383002/diff/20001/chrome/browser/translat...
File chrome/browser/translate/translate_manager_render_view_host_unittest.cc
(right):

https://codereview.chromium.org/2143383002/diff/20001/chrome/browser/translat...
chrome/browser/translate/translate_manager_render_view_host_unittest.cc:662: #if
!defined(USE_AURA)
On 2016/07/15 12:43:22, leonhsl wrote:
> Hi, groby, would you please share with me how to get these test cases built
and
> run? Seems can't simply set gn args 'use_aura = false'... Thanks~

aura is automatically enabled for Windows/Linux builds (see build/config/ui.gni)

Which means these test cases require to be built on OSX. (If you don't have an
OSX machine handy, just run it on the trybots. )

[leonhsl(Using Gerrit), 2016-07-22 07:59:16]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 07:59:31]

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, 2016-07-22 08:18:53]

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

[commit-bot: I haz the power, 2016-07-22 08:18:54]

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

[leonhsl(Using Gerrit), 2016-07-22 15:09:07]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 15:09:18]

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, 2016-07-22 15:41:42]

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

[commit-bot: I haz the power, 2016-07-22 15:41:43]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Ken Rockot(use gerrit already), 2016-07-28 20:16:07]

Very sorry for the long delay, this review fell off my radar. Mostly just nits
here, looks pretty good!

https://codereview.chromium.org/2143383002/diff/210001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2143383002/diff/210001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2863: // Register mojo
ContentTranslateDriver service only for main frame.
nit: "service" -> "interface"

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
File components/translate/content/public/interfaces/translate.mojom (right):

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
components/translate/content/public/interfaces/translate.mojom:10: // enum
translate::TranslateErrors::Type
I don't think we should reference typemapped types in the mojom file. We
intentionally keep typemaps separate from mojom because an arbitrary typemap may
be applied to this type in any given build config.

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
components/translate/content/public/interfaces/translate.mojom:11: enum
TranslateErrorsType {
nit: TranslateError seems like a better name?

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
components/translate/content/public/interfaces/translate.mojom:40: // Tells the
renderer to translate the page contents from one language to
I wonder if we could make this documentation more clear. It also doesn't need to
refer to details like "renderer" since this could be implemented by anyone.
Something like:

// Requests that the page be translated from |source_lang| to
// |target_lang|.
//
// If a Translate request is already in progress with a matching
// |target_lang|, this request will respond with |cancelled| set
// to |true|.
//
// If a Translate request is already in progress with a different
// |target_lang|, that request will respond with |cancelled| set
// to |true| and this request will proceed normally.
//
// If |cancelled| is |true| all other response values should be
// ignored.

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
components/translate/content/public/interfaces/translate.mojom:57:
RevertTranslation();
suggestion:

// Requests that the page be reverted to its original language with
// no translation applied.

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
components/translate/content/public/interfaces/translate.mojom:63: NewPage(Page
page,
nit: RegisterPage? Method names should generally be imperative verb phrases.

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
File components/translate/content/renderer/translate_helper.h (right):

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
components/translate/content/renderer/translate_helper.h:107: const
mojom::ContentTranslateDriverPtr& GetMojoTranslateDriver();
nit: Just GetTranslateDriver() or translate_driver() please.

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
components/translate/content/renderer/translate_helper.h:151:
mojom::ContentTranslateDriverPtr mojo_translate_driver_;
nit: just translate_driver_ please.

[leonhsl(Using Gerrit), 2016-07-29 07:14:00]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 07:14:19]

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, 2016-07-29 07:52:10]

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

[commit-bot: I haz the power, 2016-07-29 07:52:11]

Dry run: Try jobs failed on following builders:
  mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gyp_...)

[leonhsl(Using Gerrit), 2016-07-29 12:52:24]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 12:52:31]

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

[leonhsl(Using Gerrit), 2016-07-29 12:54:32]

Hi, Ken, Thank you very much for comments and suggestions! 
Uploaded ps#15 to address comments and add gyp changes, PTAnL, Thanks.

https://codereview.chromium.org/2143383002/diff/20001/chrome/browser/translat...
File chrome/browser/translate/chrome_translate_client.cc (right):

https://codereview.chromium.org/2143383002/diff/20001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:153: // this would cause
closing the message pipe which would raise connection
On 2016/07/20 13:06:33, Anand Mistry (off Chromium) wrote:
> nit: Make "this would cause..." a new sentence.
> 
> Also, I think this would be better worded as:
> This would cause the message pipe to be closed, which will raise a connection
> error on the peer side.

Done.

https://codereview.chromium.org/2143383002/diff/210001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2143383002/diff/210001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2863: // Register mojo
ContentTranslateDriver service only for main frame.
On 2016/07/28 20:16:06, Ken Rockot wrote:
> nit: "service" -> "interface"

Done.

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
File components/translate/content/public/interfaces/translate.mojom (right):

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
components/translate/content/public/interfaces/translate.mojom:10: // enum
translate::TranslateErrors::Type
On 2016/07/28 20:16:06, Ken Rockot wrote:
> I don't think we should reference typemapped types in the mojom file. We
> intentionally keep typemaps separate from mojom because an arbitrary typemap
may
> be applied to this type in any given build config.

Yeah, it is true. Done. Also removed other similar comments.

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
components/translate/content/public/interfaces/translate.mojom:11: enum
TranslateErrorsType {
On 2016/07/28 20:16:06, Ken Rockot wrote:
> nit: TranslateError seems like a better name?

Done.

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
components/translate/content/public/interfaces/translate.mojom:40: // Tells the
renderer to translate the page contents from one language to
On 2016/07/28 20:16:06, Ken Rockot wrote:
> I wonder if we could make this documentation more clear. It also doesn't need
to
> refer to details like "renderer" since this could be implemented by anyone.
> Something like:
> 
> // Requests that the page be translated from |source_lang| to
> // |target_lang|.
> //
> // If a Translate request is already in progress with a matching
> // |target_lang|, this request will respond with |cancelled| set
> // to |true|.
> //
> // If a Translate request is already in progress with a different
> // |target_lang|, that request will respond with |cancelled| set
> // to |true| and this request will proceed normally.
> //
> // If |cancelled| is |true| all other response values should be
> // ignored.

Aha,, Thank you!! Now it's very clear and easy to understand!

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
components/translate/content/public/interfaces/translate.mojom:57:
RevertTranslation();
On 2016/07/28 20:16:06, Ken Rockot wrote:
> suggestion:
> 
> // Requests that the page be reverted to its original language with
> // no translation applied.

Done.

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
components/translate/content/public/interfaces/translate.mojom:63: NewPage(Page
page,
On 2016/07/28 20:16:06, Ken Rockot wrote:
> nit: RegisterPage? Method names should generally be imperative verb phrases.

Got it and done.

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
File components/translate/content/renderer/translate_helper.h (right):

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
components/translate/content/renderer/translate_helper.h:107: const
mojom::ContentTranslateDriverPtr& GetMojoTranslateDriver();
On 2016/07/28 20:16:06, Ken Rockot wrote:
> nit: Just GetTranslateDriver() or translate_driver() please.

Done.

https://codereview.chromium.org/2143383002/diff/210001/components/translate/c...
components/translate/content/renderer/translate_helper.h:151:
mojom::ContentTranslateDriverPtr mojo_translate_driver_;
On 2016/07/28 20:16:06, Ken Rockot wrote:
> nit: just translate_driver_ please.

Done.

[commit-bot: I haz the power, 2016-07-29 13:55:24]

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

[commit-bot: I haz the power, 2016-07-29 13:55:24]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-07-29 17:21:17]

as a total non-owner, all mojo bits lgtm % some nits

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
File components/translate/content/browser/content_translate_driver.h (right):

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
components/translate/content/browser/content_translate_driver.h:128:
std::map<int, mojom::PagePtr> mojo_pages_;
nit: Please, no |mojo| or |Mojo| in symbol names. It's just an implementation
detail at this point. Just |pages_| is a fine name.

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
File components/translate/content/public/cpp/translate.typemap (right):

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
components/translate/content/public/cpp/translate.typemap:10: traits_headers =
nit: Weird formatting. Should be

traits_headers = [
  "//....",
]

like sources below.

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
File components/translate/content/public/cpp/translate_struct_traits.h (right):

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
components/translate/content/public/cpp/translate_struct_traits.h:18: struct
EnumTraits<translate::mojom::TranslateError,
It would be nice if we could just move the enum definition into mojom, but that
looks like it will touch a lot of code. I'm fine with doing that later.

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
File components/translate/content/public/interfaces/translate.mojom (right):

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
components/translate/content/public/interfaces/translate.mojom:52: => (bool
cancelled,
nit: it's fine (preferrable imho) to fit as many arguments on one line as
possible, so please feel free to write

    => (bool cancelled, string original_lang, string translated_lang,
        TranslateError error);

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
components/translate/content/public/interfaces/translate.mojom:55:
TranslateError error_type);
nit nit: Maybe just error instead of error_type?

[leonhsl(Using Gerrit), 2016-07-30 04:18:10]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-30 04:18:20]

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, 2016-07-30 05:20:13]

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

[commit-bot: I haz the power, 2016-07-30 05:20:15]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-07-30 08:45:05]

leon.han@intel.com changed reviewers:
+ tsepez@chromium.org

[leonhsl(Using Gerrit), 2016-07-30 08:45:07]

Uploaded ps#16 to address nits. Thanks Ken!

+Tom
Would you PTAL for security review, Thanks.

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
File components/translate/content/browser/content_translate_driver.h (right):

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
components/translate/content/browser/content_translate_driver.h:128:
std::map<int, mojom::PagePtr> mojo_pages_;
On 2016/07/29 17:21:17, Ken Rockot wrote:
> nit: Please, no |mojo| or |Mojo| in symbol names. It's just an implementation
> detail at this point. Just |pages_| is a fine name.

Done.

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
File components/translate/content/public/cpp/translate.typemap (right):

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
components/translate/content/public/cpp/translate.typemap:10: traits_headers =
On 2016/07/29 17:21:17, Ken Rockot wrote:
> nit: Weird formatting. Should be
> 
> traits_headers = [
>   "//....",
> ]
> 
> like sources below.

Done.

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
File components/translate/content/public/cpp/translate_struct_traits.h (right):

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
components/translate/content/public/cpp/translate_struct_traits.h:18: struct
EnumTraits<translate::mojom::TranslateError,
On 2016/07/29 17:21:17, Ken Rockot wrote:
> It would be nice if we could just move the enum definition into mojom, but
that
> looks like it will touch a lot of code. I'm fine with doing that later.

Yeah agree, lots of code need to be changed, I'd like to do this work later.

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
File components/translate/content/public/interfaces/translate.mojom (right):

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
components/translate/content/public/interfaces/translate.mojom:52: => (bool
cancelled,
On 2016/07/29 17:21:17, Ken Rockot wrote:
> nit: it's fine (preferrable imho) to fit as many arguments on one line as
> possible, so please feel free to write
> 
>     => (bool cancelled, string original_lang, string translated_lang,
>         TranslateError error);

Done.

https://codereview.chromium.org/2143383002/diff/270001/components/translate/c...
components/translate/content/public/interfaces/translate.mojom:55:
TranslateError error_type);
On 2016/07/29 17:21:17, Ken Rockot wrote:
> nit nit: Maybe just error instead of error_type?

Done.

[Tom Sepez, 2016-08-01 17:08:17]

LGTM with comments.

https://codereview.chromium.org/2143383002/diff/290001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2143383002/diff/290001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2828: // Register mojo
ContentTranslateDriver interface only for main frame.
nit: does the order matter?  Can/Should we combine with line 2818 so that we
have a single block for all the main frame only cases?

https://codereview.chromium.org/2143383002/diff/290001/chrome/browser/transla...
File chrome/browser/translate/translate_manager_render_view_host_unittest.cc
(right):

https://codereview.chromium.org/2143383002/diff/290001/chrome/browser/transla...
chrome/browser/translate/translate_manager_render_view_host_unittest.cc:137: //
Ensure pending callback get called.
nit: gets

https://codereview.chromium.org/2143383002/diff/290001/components/translate/c...
File components/translate/content/renderer/translate_helper.cc (right):

https://codereview.chromium.org/2143383002/diff/290001/components/translate/c...
components/translate/content/renderer/translate_helper.cc:220: std::string
script = "cr.googleTranslate.translate('" + source_lang_ + "','" +
Note: pre-existing.  Although this is invoked in response to a browser message,
and the message flows in a less secure direction, we might want to protect
ourselves if source_lang contains unexpected punctuation, in case souce_lang is
somehow taken by the browser from some user or page input.

I generally get nervous when JS strings are cobbled together without first
filtering the parameter.

Need not address this now.

[jam, 2016-08-01 17:14:42]

jam@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2016-08-01 17:14:43]

drive-by: why create a components/translate/content/public/cpp/ directory? That
seems unnecessary, they can just go into components/translate/content/common

[leonhsl(Using Gerrit), 2016-08-02 02:11:39]

On 2016/08/01 17:14:43, jam wrote:
> drive-by: why create a components/translate/content/public/cpp/ directory?
That
> seems unnecessary, they can just go into components/translate/content/common

Oh,, I suppose we have the convention that we put type converters and typemap
impl codes into a separate directory xxx/public/cpp, which corresponds with
xxx/public/interfaces holding all the mojom files. Maybe this is my
misunderstanding?.. Thanks.

[leonhsl(Using Gerrit), 2016-08-02 02:26:46]

Patchset #17 (id:310001) has been deleted

[leonhsl(Using Gerrit), 2016-08-02 02:44:21]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 02:44:35]

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

[leonhsl(Using Gerrit), 2016-08-02 02:52:24]

Thanks Tom a lot for the review, uploaded ps#17 to address comments, PTAnL,
Thanks.

https://codereview.chromium.org/2143383002/diff/290001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2143383002/diff/290001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2828: // Register mojo
ContentTranslateDriver interface only for main frame.
On 2016/08/01 17:08:17, Tom Sepez wrote:
> nit: does the order matter?  Can/Should we combine with line 2818 so that we
> have a single block for all the main frame only cases?

Yeah good point! Sorry for my carelessness. Done and thanks.

https://codereview.chromium.org/2143383002/diff/290001/chrome/browser/transla...
File chrome/browser/translate/translate_manager_render_view_host_unittest.cc
(right):

https://codereview.chromium.org/2143383002/diff/290001/chrome/browser/transla...
chrome/browser/translate/translate_manager_render_view_host_unittest.cc:137: //
Ensure pending callback get called.
On 2016/08/01 17:08:17, Tom Sepez wrote:
> nit: gets

Done. And also addressed other similar places.

https://codereview.chromium.org/2143383002/diff/290001/components/translate/c...
File components/translate/content/renderer/translate_helper.cc (right):

https://codereview.chromium.org/2143383002/diff/290001/components/translate/c...
components/translate/content/renderer/translate_helper.cc:220: std::string
script = "cr.googleTranslate.translate('" + source_lang_ + "','" +
On 2016/08/01 17:08:17, Tom Sepez wrote:
> Note: pre-existing.  Although this is invoked in response to a browser
message,
> and the message flows in a less secure direction, we might want to protect
> ourselves if source_lang contains unexpected punctuation, in case souce_lang
is
> somehow taken by the browser from some user or page input.
> 
> I generally get nervous when JS strings are cobbled together without first
> filtering the parameter.
> 
> Need not address this now.

Understood. I filed an issue crbug.com/633460 to discuss/track this problem.
Thanks.

[commit-bot: I haz the power, 2016-08-02 04:20:24]

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

[commit-bot: I haz the power, 2016-08-02 04:20:26]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-08-02 07:33:18]

Soft ping groby@ for translate OWNER review, Thanks.

[Tom Sepez, 2016-08-02 18:26:17]

++LGTM

[groby-ooo-7-16, 2016-08-04 02:35:58]

https://codereview.chromium.org/2143383002/diff/330001/components/translate/c...
File components/translate/content/browser/content_translate_driver.cc (right):

https://codereview.chromium.org/2143383002/diff/330001/components/translate/c...
components/translate/content/browser/content_translate_driver.cc:219: void
ContentTranslateDriver::OnPageAway(int page_seq_no) {
Not quite clear what that function name means - I assume this is
OnPageNavigatedAway?

https://codereview.chromium.org/2143383002/diff/330001/components/translate/c...
File components/translate/content/renderer/translate_helper.cc (right):

https://codereview.chromium.org/2143383002/diff/330001/components/translate/c...
components/translate/content/renderer/translate_helper.cc:203: "typeof
cr.googleTranslate.translate == 'function'",
Please don't reformat unrelated code. (git cl format should only format lines
you changed)

https://codereview.chromium.org/2143383002/diff/330001/components/translate/c...
components/translate/content/renderer/translate_helper.cc:319: if
(translate_callback_pending_ && target_lang_ == target_lang) {
Isn't this page-specific? This didn't matter when we only allowed a single valid
page_seq_no, but we probably should check this. (Or, if there still can only be
a single active callback, we should get rid of the map of seq_no -> page)

Otherwise, it's kind of unclear to a reader if we do or do not support multiple
pending translates.

https://codereview.chromium.org/2143383002/diff/330001/components/translate/c...
components/translate/content/renderer/translate_helper.cc:374: //
Kill that last line/replace w/ empty line.

(I'm not a huge fan of separator comments in general, but I'm not going to hold
up the CL for it :)

[leonhsl(Using Gerrit), 2016-08-04 07:10:49]

Patchset #18 (id:350001) has been deleted

[leonhsl(Using Gerrit), 2016-08-04 07:13:59]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 07:14:13]

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, 2016-08-04 08:15:55]

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

[commit-bot: I haz the power, 2016-08-04 08:15:56]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-08-04 08:41:09]

Thanks groby a lot for review. Uploaded ps#18 to address comments, PTAnL,
Thanks.

https://codereview.chromium.org/2143383002/diff/330001/components/translate/c...
File components/translate/content/browser/content_translate_driver.cc (right):

https://codereview.chromium.org/2143383002/diff/330001/components/translate/c...
components/translate/content/browser/content_translate_driver.cc:219: void
ContentTranslateDriver::OnPageAway(int page_seq_no) {
On 2016/08/04 02:35:58, groby wrote:
> Not quite clear what that function name means - I assume this is
> OnPageNavigatedAway?

Each time RegisterPage() gets called, 'Page' means a new translate session
arises, it does not map to a web page 1:1 strictly. 
For example, it's possible that render side triggers text capture several times
for 1 web page, this would cause multiple 'Page'(translate session) created, and
because render side(TranslateHelper) always keeps only 1 'Page' alive at any
time, message pipe underlying old 'Page' would be closed and OnPageAway() would
be triggered here. So, we can't assert that there must have a web page
navigation when OnPageAway() got called.

https://codereview.chromium.org/2143383002/diff/330001/components/translate/c...
File components/translate/content/renderer/translate_helper.cc (right):

https://codereview.chromium.org/2143383002/diff/330001/components/translate/c...
components/translate/content/renderer/translate_helper.cc:203: "typeof
cr.googleTranslate.translate == 'function'",
On 2016/08/04 02:35:58, groby wrote:
> Please don't reformat unrelated code. (git cl format should only format lines
> you changed)

Done and also handled other similar places in this file and
content_translate_driver.cc.

https://codereview.chromium.org/2143383002/diff/330001/components/translate/c...
components/translate/content/renderer/translate_helper.cc:319: if
(translate_callback_pending_ && target_lang_ == target_lang) {
On 2016/08/04 02:35:58, groby wrote:
> Isn't this page-specific? This didn't matter when we only allowed a single
valid
> page_seq_no, but we probably should check this. (Or, if there still can only
be
> a single active callback, we should get rid of the map of seq_no -> page)
> 
> Otherwise, it's kind of unclear to a reader if we do or do not support
multiple
> pending translates.

Yeah this is page-specific. The 'page' here means 1 single translate session,
within which we only allow 1 active translate process.

The same with before(Legacy IPC era), we do not support multiple pending
translates. Legacy IPC does not send any response back for the ignored request,
in correspondence we run the response callback with the 'cancelled' parameter
set as true, because Mojo requests that a response callback MUST get executed
while the binding is still alive.

https://codereview.chromium.org/2143383002/diff/330001/components/translate/c...
components/translate/content/renderer/translate_helper.cc:374: //
On 2016/08/04 02:35:58, groby wrote:
> Kill that last line/replace w/ empty line.
> 
> (I'm not a huge fan of separator comments in general, but I'm not going to
hold
> up the CL for it :)

Got it and done ;-)

[groby-ooo-7-16, 2016-08-04 13:47:47]

Thank you, and LGTM!

[leonhsl(Using Gerrit), 2016-08-04 16:33:08]

Hi, John, would you PTAL for OWNER review of bellowing? Thanks.
  chrome/ except tranlsate bits
  components/typemaps.gni

[jam, 2016-08-09 00:29:11]

On 2016/08/02 02:11:39, leonhsl wrote:
> On 2016/08/01 17:14:43, jam wrote:
> > drive-by: why create a components/translate/content/public/cpp/ directory?
> That
> > seems unnecessary, they can just go into components/translate/content/common
> 
> Oh,, I suppose we have the convention that we put type converters and typemap
> impl codes into a separate directory xxx/public/cpp, which corresponds with
> xxx/public/interfaces holding all the mojom files. Maybe this is my
> misunderstanding?.. Thanks.

The public directory is when there's a service that's used by other directories.
This is all internal to the translate components. As such, you can move them all
to components/translate/content/common/

(sorry for delay, I missed this)

[leonhsl(Using Gerrit), 2016-08-09 07:18:47]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 07:19:21]

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, 2016-08-09 09:08:35]

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

[commit-bot: I haz the power, 2016-08-09 09:08:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[leonhsl(Using Gerrit), 2016-08-09 09:10:47]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 09:11:09]

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, 2016-08-09 10:14:10]

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

[commit-bot: I haz the power, 2016-08-09 10:14:11]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-08-09 12:40:57]

On 2016/08/09 00:29:11, jam wrote:
> On 2016/08/02 02:11:39, leonhsl wrote:
> > On 2016/08/01 17:14:43, jam wrote:
> > > drive-by: why create a components/translate/content/public/cpp/ directory?
> > That
> > > seems unnecessary, they can just go into
components/translate/content/common
> > 
> > Oh,, I suppose we have the convention that we put type converters and
typemap
> > impl codes into a separate directory xxx/public/cpp, which corresponds with
> > xxx/public/interfaces holding all the mojom files. Maybe this is my
> > misunderstanding?.. Thanks.
> 
> The public directory is when there's a service that's used by other
directories.
> This is all internal to the translate components. As such, you can move them
all
> to components/translate/content/common/
> 
> (sorry for delay, I missed this)

Got it ;-) Uploaded ps#19 to address the comment, would you PTAnL for the OWNER
review of bellowing files? Thanks.
  chrome/ except tranlsate bits
  components/typemaps.gni

[jam, 2016-08-09 22:22:18]

lgtm

https://codereview.chromium.org/2143383002/diff/390001/components/translate/c...
File components/translate/content/common/translate_struct_traits.cc (right):

https://codereview.chromium.org/2143383002/diff/390001/components/translate/c...
components/translate/content/common/translate_struct_traits.cc:7: #include
"ipc/ipc_message_utils.h"
nit: do you need this?

[leonhsl(Using Gerrit), 2016-08-10 02:13:50]

On 2016/08/09 22:22:18, jam wrote:
> lgtm
> 
>
https://codereview.chromium.org/2143383002/diff/390001/components/translate/c...
> File components/translate/content/common/translate_struct_traits.cc (right):
> 
>
https://codereview.chromium.org/2143383002/diff/390001/components/translate/c...
> components/translate/content/common/translate_struct_traits.cc:7: #include
> "ipc/ipc_message_utils.h"
> nit: do you need this?

Thanks a lot for the review. And yes we need this line.
At line 98 in translate_struct_traits.cc,
'translate::mojom::LanguageDetectionDetailsDataView::ReadTime<base::Time>'
template specification needs 'IPC::ParamTraits<base::Time>::Read()' declaration
in "ipc/ipc_message_utils.h". Actually the xxx_struct_traits.cc file must
include all traits headers of the struct's member variables types.

[leonhsl(Using Gerrit), 2016-08-10 02:14:33]

The CQ bit was checked by leon.han@intel.com

[leonhsl(Using Gerrit), 2016-08-10 02:14:33]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
tsepez@chromium.org, groby@chromium.org
Link to the patchset: https://codereview.chromium.org/2143383002/#ps390001
(title: "Put public/* into common/")

[commit-bot: I haz the power, 2016-08-10 02:15: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, 2016-08-10 02:22:29]

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

[commit-bot: I haz the power, 2016-08-10 02:22:31]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[leonhsl(Using Gerrit), 2016-08-10 02:35:51]

On 2016/08/10 02:22:31, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

The merge conflict is trivial and has been solved by ps#20, will land ps#20 now.

[leonhsl(Using Gerrit), 2016-08-10 02:36:10]

The CQ bit was checked by leon.han@intel.com

[leonhsl(Using Gerrit), 2016-08-10 02:36:11]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
groby@chromium.org, jam@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2143383002/#ps410001
(title: "Rebase only")

[commit-bot: I haz the power, 2016-08-10 02:36:59]

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, 2016-08-10 03:36:53]

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

[commit-bot: I haz the power, 2016-08-10 03:36:56]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[leonhsl(Using Gerrit), 2016-08-10 05:13:58]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 05:14:31]

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

[leonhsl(Using Gerrit), 2016-08-10 05:53:12]

The CQ bit was checked by leon.han@intel.com

[leonhsl(Using Gerrit), 2016-08-10 05:53:13]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
groby@chromium.org, jam@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2143383002/#ps430001
(title: "Fix trybots")

[commit-bot: I haz the power, 2016-08-10 05:53:37]

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, 2016-08-10 07:26:15]

Committed patchset #21 (id:430001)

[commit-bot: I haz the power, 2016-08-10 07:28:34]

Description was changed from

==========
[Translate] Migrate IPCs to Mojo interfaces.

BUG=619390
TEST=browser_tests, unit_tests
==========

to

==========
[Translate] Migrate IPCs to Mojo interfaces.

BUG=619390
TEST=browser_tests, unit_tests

Committed: https://crrev.com/3134fc62afcbbc25e0deeb674e5252be9f458d7b
Cr-Commit-Position: refs/heads/master@{#410992}
==========

[commit-bot: I haz the power, 2016-08-10 07:28:36]

Patchset 21 (id:??) landed as
https://crrev.com/3134fc62afcbbc25e0deeb674e5252be9f458d7b
Cr-Commit-Position: refs/heads/master@{#410992}

[xidachen, 2016-08-11 13:36:25]

A revert of this CL (patchset #21 id:430001) has been created in
https://codereview.chromium.org/2232233002/ by xidachen@chromium.org.

The reason for reverting is: suspect this CL causes compilation failure:
https://build.chromium.org/p/chromium/builders/Win/builds/46119
Error message:
src\components\translate\content\browser\content_translate_driver.h(13): fatal
error C1083: Cannot open include file:
'components/translate/content/common/translate.mojom.h': No such file or
directory.

[xidachen, 2016-08-11 13:42:58]

On 2016/08/11 13:36:25, xidachen wrote:
> A revert of this CL (patchset #21 id:430001) has been created in
> https://codereview.chromium.org/2232233002/ by mailto:xidachen@chromium.org.
> 
> The reason for reverting is: suspect this CL causes compilation failure:
> https://build.chromium.org/p/chromium/builders/Win/builds/46119
> Error message:
> src\components\translate\content\browser\content_translate_driver.h(13): fatal
> error C1083: Cannot open include file:
> 'components/translate/content/common/translate.mojom.h': No such file or
> directory.

sorry, my bad. It looks like the compile error is due to a flaky bot. This CL is
not reverted.