[Wait to be closed] [Translate] Migrate IPCs to Mojo interfaces.

[Translate] Migrate IPCs to Mojo interfaces.

BUG=619390

[leonhsl(Using Gerrit), 2016-06-14 03:37:51]

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

[leonhsl(Using Gerrit), 2016-06-14 03:37:52]

ps#1 is just for reference. Will update continuously. Thanks.

[leonhsl(Using Gerrit), 2016-07-04 06:09:06]

Will update this CL according new interface architecture:

interface Page {
  Translate(string translate_script, string source_lang, string target_lang)
     => (string original_lang, string translated_lang, TranslateErrorsType
error_type);
  RevertTranslation();
};

interface TranslateDriver {
  NewPage(Page page, LanguageDetectionDetails, bool page_needs_translation);
};

[leonhsl(Using Gerrit), 2016-07-05 07:53:04]

OK I think now ps#5 are ready for review, it's according from the new mojo
interfaces definition, would you PTAL? Thanks.

Manually test OK ;-) I'd like to touch related test codes after we think the
implementation itself OK.

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

Description was changed from

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

BUG=577685
==========

to

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

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

[leonhsl(Using Gerrit), 2016-07-08 05:12:00]

Soft ping, Thanks ;-)

[groby-ooo-7-16, 2016-07-11 22:30:28]

On 2016/07/08 05:12:00, leonhsl wrote:
> Soft ping, Thanks ;-)

A few initial thoughts. It looks mostly good from the translate side, but I
think you really want Anand's feedback for all things mojo.

I'm confused - why is translate_helper.* marked as "D", yet has changes to
existing code? And the fuzzer OWNERS is "A+", but has no changes? Maybe time to
re-upload the CL, in case Rietveld is confused?

Also, why the rename from ContentTranslateDriver to ContentTranslateDriverImpl ?
It was already an Impl (of TranslateDriver). Is that a mojo naming scheme, or
can we keep the old names?

[groby-ooo-7-16, 2016-07-11 22:34:43]

And now with additional questions :)

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
File components/translate/content/browser/content_translate_driver_impl.cc
(right):

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.cc:222: void
ContentTranslateDriverImpl::OnPageTranslated(
Is there a reason this moved up here? (Keeping it in the same position at the
end of the file would make the diff slightly clearer)

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.cc:237: void
ContentTranslateDriverImpl::OnPageAway(int page_seq_no) {
Do we actually still need the concept of page sequence numbers? IIUC, we used
them as pseudo-cross-process handles for translate requests. Now that we have
mojom::PagePtr, can't we just hand that to the TranslateManager?

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.cc:242: void
ContentTranslateDriverImpl::NewPage(
I take it all functions modifying/reading |mojo_pages_| are on the same thread?

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.cc:247:
mojo_pages_[next_page_seq_no_].set_connection_error_handler(
I'm new to this mojo thing, so possibly odd question: This means, in a nutshell,
that |mojo_pages_| holds persistent connections to  mojom::Page objects, and we
clear it out when the connection goes away?

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.cc:249:
base::Unretained(this), next_page_seq_no_));
Are we guaranteed that |this| will be alive until all error handlers have been
invoked/abandoned?

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
File components/translate/content/browser/content_translate_driver_impl.h
(right):

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.h:125: //
Records mojo connections with all current alive pages.
Previously, we only kept the last connection alive. If we keep all connections
alive until the page is navigated away or translated, we should probably close
mojo connections when the user declined to translate.

[leonhsl(Using Gerrit), 2016-07-12 02:04:52]

On 2016/07/11 22:30:28, groby wrote:
> On 2016/07/08 05:12:00, leonhsl wrote:
> > Soft ping, Thanks ;-)
> 
> A few initial thoughts. It looks mostly good from the translate side, but I
> think you really want Anand's feedback for all things mojo.
> 
> I'm confused - why is translate_helper.* marked as "D", yet has changes to
> existing code? And the fuzzer OWNERS is "A+", but has no changes? Maybe time
to
> re-upload the CL, in case Rietveld is confused?

Yeah, I think the mark is wrong because of confused Rietveld ;-) I'll take a try
to solve, if still can't get correct I'll create a new CL.

> Also, why the rename from ContentTranslateDriver to ContentTranslateDriverImpl
?
> It was already an Impl (of TranslateDriver). Is that a mojo naming scheme, or
> can we keep the old names?
I wanted to make it clear that ContentTranslateDriverImpl is a
mojom::ContentTranslateDriver impl. But seems no such strong rule for the Impl
suffix naming scheme, I'll change the name back to ContentTranslateDriver, this
way will also make the diff clearer.

[leonhsl(Using Gerrit), 2016-07-12 02:48:17]

Thanks groby a lot for review comments. Inline answers.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
File components/translate/content/browser/content_translate_driver_impl.cc
(right):

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.cc:222: void
ContentTranslateDriverImpl::OnPageTranslated(
On 2016/07/11 22:34:43, groby wrote:
> Is there a reason this moved up here? (Keeping it in the same position at the
> end of the file would make the diff slightly clearer)

Ah, I don't remember why it's moved here. I'll move it back.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.cc:237: void
ContentTranslateDriverImpl::OnPageAway(int page_seq_no) {
On 2016/07/11 22:34:43, groby wrote:
> Do we actually still need the concept of page sequence numbers? IIUC, we used
> them as pseudo-cross-process handles for translate requests. Now that we have
> mojom::PagePtr, can't we just hand that to the TranslateManager?

I think TranslateManager also serves IOSTranslateDriver, which has no mojo
concept and still has some complicated logic based on the page sequence numbers.
So I did not touch inside TranslateManager to eliminate the page sequence
numbers.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.cc:242: void
ContentTranslateDriverImpl::NewPage(
On 2016/07/11 22:34:43, groby wrote:
> I take it all functions modifying/reading |mojo_pages_| are on the same
thread?

Yeah, mojo is thread-hostile, all the mojo calls and connection error handlers
run on the same thread on which the mojo interface has been registered.
ContentTranslateDriver case is the UI/main thread.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.cc:247:
mojo_pages_[next_page_seq_no_].set_connection_error_handler(
On 2016/07/11 22:34:43, groby wrote:
> I'm new to this mojo thing, so possibly odd question: This means, in a
nutshell,
> that |mojo_pages_| holds persistent connections to  mojom::Page objects, and
we
> clear it out when the connection goes away?

Yeah it's right. TranslateHelper will close the connection once page navigated
away, then our connection error handler OnPageAway() here will clear it from
|mojo_pages_|.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.cc:249:
base::Unretained(this), next_page_seq_no_));
On 2016/07/11 22:34:43, groby wrote:
> Are we guaranteed that |this| will be alive until all error handlers have been
> invoked/abandoned?

Yeah this is guaranteed. Because destroying |this| will destroy |mojo_pages_|,
which could not invoke error handler after it's destroyed.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
File components/translate/content/browser/content_translate_driver_impl.h
(right):

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.h:125: //
Records mojo connections with all current alive pages.
On 2016/07/11 22:34:43, groby wrote:
> Previously, we only kept the last connection alive. If we keep all connections
> alive until the page is navigated away or translated, we should probably close
> mojo connections when the user declined to translate.

From my understanding, one WebContents : one ContentTranslateDriver, which
serves for all the main render frames of this WebContents. At one time point
there is only one main render frame, but when the main render frame switches
from old one to new one(maybe lives in different process), the NewPage() call
for new page and connection error handler OnPageAway() for old page will happen
asynchronously without ordering guarantee, so we need to use a map here.

As for "when the user declined to translate", I checked around but can't find
any clue in ContentTranslateDriver. If ContentTranslateDriver can get the
'declined' notification, and if we're sure that after 'declined' we'll never
translate this page, then we can clear it out from |mojo_pages_|.

[Anand Mistry (off Chromium), 2016-07-12 07:17:09]

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

https://codereview.chromium.org/2066483004/diff/80001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:146: // We try to bind to
the driver, but if driver is not ready for now
This comment is a bit hard to parse and the formatting looks a bit weird. Also,
move it down to where the translate client is retrieved. Getting WebContents*
from an RFH seems bog-standard and no comment is necessary for that.

https://codereview.chromium.org/2066483004/diff/80001/chrome/browser/translat...
File chrome/browser/translate/chrome_translate_client.h (right):

https://codereview.chromium.org/2066483004/diff/80001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.h:74: static void
BindContentTranslateDriver(
This deserves a comment. At the very least to be consistent with the class'
style.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
File components/translate/content/browser/content_translate_driver_impl.h
(right):

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.h:38: virtual
void OnIsPageTranslatedChanged(content::WebContents* source){};
I believe the chromium style is to always have a space after a closing brace.
Please revert this style to the original.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.h:57:
ContentTranslateDriverImpl(content::NavigationController* nav_controller);
explicit on single-arg constructors.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.h:125: //
Records mojo connections with all current alive pages.
On 2016/07/12 02:48:17, leonhsl wrote:
> On 2016/07/11 22:34:43, groby wrote:
> > Previously, we only kept the last connection alive. If we keep all
connections
> > alive until the page is navigated away or translated, we should probably
close
> > mojo connections when the user declined to translate.
> 
> From my understanding, one WebContents : one ContentTranslateDriver, which
> serves for all the main render frames of this WebContents. At one time point
> there is only one main render frame, but when the main render frame switches
> from old one to new one(maybe lives in different process), the NewPage() call
> for new page and connection error handler OnPageAway() for old page will
happen
> asynchronously without ordering guarantee, so we need to use a map here.

I don't really understand the translate parts, but it appears that the old code
only allow a single page to be "active" at a time. The receipt of a
ChromeFrameHostMsg_TranslateAssignedSequenceNumber message signalled the start
of a new page... from what I can tell.

You can get this behaviour with Mojo without having a map of pages by having a
single PagePtr being the current page. You're right that we don't have ordering
guarantees between two pipes. But since all requests are serialised to a single
thread, you can just reset() and rebind the page when you get a new one. That
will cancel the error handler so you're guaranteed not to get an error handler
for an old page if you replace it with a new one.

> 
> As for "when the user declined to translate", I checked around but can't find
> any clue in ContentTranslateDriver. If ContentTranslateDriver can get the
> 'declined' notification, and if we're sure that after 'declined' we'll never
> translate this page, then we can clear it out from |mojo_pages_|.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
File components/translate/content/renderer/translate_helper.cc (right):

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/renderer/translate_helper.cc:127:
CancelPendingTranslation();
This seems unnecessary. The translate callback comes from |binding_|, so you
need to reset it. But if you run it, the response won't be sent since you've
closed the message pipe.

You probably do need to reset the callback though.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
File components/translate/content/renderer/translate_helper.h (right):

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/renderer/translate_helper.h:134:
std::unique_ptr<TranslateCallback> translate_callback_pending_;
This doesn't need to be a unique_ptr<>. Callbacks can be copied. There's also
Callback::is_null() and Callback::Reset().

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

Thanks Anand a lot for comments. Inline discussion firstly, and later I'll
modify codes to address the 'Acknowledged' issues.

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

https://codereview.chromium.org/2066483004/diff/80001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:146: // We try to bind to
the driver, but if driver is not ready for now
On 2016/07/12 07:17:08, Anand Mistry wrote:
> This comment is a bit hard to parse and the formatting looks a bit weird.
Also,
> move it down to where the translate client is retrieved. Getting WebContents*
> from an RFH seems bog-standard and no comment is necessary for that.

Acknowledged.

https://codereview.chromium.org/2066483004/diff/80001/chrome/browser/translat...
File chrome/browser/translate/chrome_translate_client.h (right):

https://codereview.chromium.org/2066483004/diff/80001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.h:74: static void
BindContentTranslateDriver(
On 2016/07/12 07:17:09, Anand Mistry wrote:
> This deserves a comment. At the very least to be consistent with the class'
> style.

Acknowledged.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
File components/translate/content/browser/content_translate_driver_impl.h
(right):

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.h:38: virtual
void OnIsPageTranslatedChanged(content::WebContents* source){};
On 2016/07/12 07:17:09, Anand Mistry wrote:
> I believe the chromium style is to always have a space after a closing brace.
> Please revert this style to the original.

Acknowledged.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.h:57:
ContentTranslateDriverImpl(content::NavigationController* nav_controller);
On 2016/07/12 07:17:09, Anand Mistry wrote:
> explicit on single-arg constructors.

Acknowledged.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.h:125: //
Records mojo connections with all current alive pages.
On 2016/07/12 07:17:09, Anand Mistry wrote:
> On 2016/07/12 02:48:17, leonhsl wrote:
> > On 2016/07/11 22:34:43, groby wrote:
> > > Previously, we only kept the last connection alive. If we keep all
> connections
> > > alive until the page is navigated away or translated, we should probably
> close
> > > mojo connections when the user declined to translate.
> > 
> > From my understanding, one WebContents : one ContentTranslateDriver, which
> > serves for all the main render frames of this WebContents. At one time point
> > there is only one main render frame, but when the main render frame switches
> > from old one to new one(maybe lives in different process), the NewPage()
call
> > for new page and connection error handler OnPageAway() for old page will
> happen
> > asynchronously without ordering guarantee, so we need to use a map here.
> 
> I don't really understand the translate parts, but it appears that the old
code
> only allow a single page to be "active" at a time. The receipt of a
> ChromeFrameHostMsg_TranslateAssignedSequenceNumber message signalled the start
> of a new page... from what I can tell.
> 
> You can get this behaviour with Mojo without having a map of pages by having a
> single PagePtr being the current page. You're right that we don't have
ordering
> guarantees between two pipes. But since all requests are serialised to a
single
> thread, you can just reset() and rebind the page when you get a new one. That
> will cancel the error handler so you're guaranteed not to get an error handler
> for an old page if you replace it with a new one.
> 

Oh,, yes we can keep only one PagePtr for the latest page. But if without the
mapping, when TranslateDriver::{TranslatePage,RevertTranslation} callbacks got
called with a |page_seq_no| of old page from TranslateManager, seems we can't
find out whether this |page_seq_no| had become old. Eh, this issue relates with
the issue about the necessity of page sequence numbers.

> > 
> > As for "when the user declined to translate", I checked around but can't
find
> > any clue in ContentTranslateDriver. If ContentTranslateDriver can get the
> > 'declined' notification, and if we're sure that after 'declined' we'll never
> > translate this page, then we can clear it out from |mojo_pages_|.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
File components/translate/content/renderer/translate_helper.cc (right):

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/renderer/translate_helper.cc:127:
CancelPendingTranslation();
On 2016/07/12 07:17:09, Anand Mistry wrote:
> This seems unnecessary. The translate callback comes from |binding_|, so you
> need to reset it. But if you run it, the response won't be sent since you've
> closed the message pipe.
> 
> You probably do need to reset the callback though.

Yeah, you're right, here I only need to reset the callback, no effect to run it.
Thanks!

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
File components/translate/content/renderer/translate_helper.h (right):

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/renderer/translate_helper.h:134:
std::unique_ptr<TranslateCallback> translate_callback_pending_;
On 2016/07/12 07:17:09, Anand Mistry wrote:
> This doesn't need to be a unique_ptr<>. Callbacks can be copied. There's also
> Callback::is_null() and Callback::Reset().

Got it, thanks for pointing out this!

[leonhsl(Using Gerrit), 2016-07-13 09:56:18]

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

https://codereview.chromium.org/2066483004/diff/80001/chrome/browser/translat...
chrome/browser/translate/chrome_translate_client.cc:146: // We try to bind to
the driver, but if driver is not ready for now
On 2016/07/12 10:15:09, leonhsl wrote:
> On 2016/07/12 07:17:08, Anand Mistry wrote:
> > This comment is a bit hard to parse and the formatting looks a bit weird.
> Also,
> > move it down to where the translate client is retrieved. Getting
WebContents*
> > from an RFH seems bog-standard and no comment is necessary for that.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
File components/translate/content/browser/content_translate_driver_impl.cc
(right):

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.cc:222: void
ContentTranslateDriverImpl::OnPageTranslated(
On 2016/07/12 02:48:17, leonhsl wrote:
> On 2016/07/11 22:34:43, groby wrote:
> > Is there a reason this moved up here? (Keeping it in the same position at
the
> > end of the file would make the diff slightly clearer)
> 
> Ah, I don't remember why it's moved here. I'll move it back.

Done.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
File components/translate/content/browser/content_translate_driver_impl.h
(right):

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.h:38: virtual
void OnIsPageTranslatedChanged(content::WebContents* source){};
On 2016/07/12 10:15:10, leonhsl wrote:
> On 2016/07/12 07:17:09, Anand Mistry wrote:
> > I believe the chromium style is to always have a space after a closing
brace.
> > Please revert this style to the original.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/browser/content_translate_driver_impl.h:57:
ContentTranslateDriverImpl(content::NavigationController* nav_controller);
On 2016/07/12 10:15:09, leonhsl wrote:
> On 2016/07/12 07:17:09, Anand Mistry wrote:
> > explicit on single-arg constructors.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
File components/translate/content/renderer/translate_helper.cc (right):

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/renderer/translate_helper.cc:127:
CancelPendingTranslation();
On 2016/07/12 10:15:10, leonhsl wrote:
> On 2016/07/12 07:17:09, Anand Mistry wrote:
> > This seems unnecessary. The translate callback comes from |binding_|, so you
> > need to reset it. But if you run it, the response won't be sent since you've
> > closed the message pipe.
> > 
> > You probably do need to reset the callback though.
> 
> Yeah, you're right, here I only need to reset the callback, no effect to run
it.
> Thanks!

Seems we still need to call CancelPendingTranslation(), because we also need
'weak_method_factory_.InvalidateWeakPtrs();' inside it to prevent firing any
pending callback(CheckTranslateStatus or TranslatePageImpl).

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
File components/translate/content/renderer/translate_helper.h (right):

https://codereview.chromium.org/2066483004/diff/80001/components/translate/co...
components/translate/content/renderer/translate_helper.h:134:
std::unique_ptr<TranslateCallback> translate_callback_pending_;
On 2016/07/12 10:15:10, leonhsl wrote:
> On 2016/07/12 07:17:09, Anand Mistry wrote:
> > This doesn't need to be a unique_ptr<>. Callbacks can be copied. There's
also
> > Callback::is_null() and Callback::Reset().
> 
> Got it, thanks for pointing out this!

Done.

[leonhsl(Using Gerrit), 2016-07-13 10:05:28]

Uploaded ps#6 to address all the comments until now, and because the error marks
"D" still exist, I also created another new CL, whose ps#1 is identical with
this CL's ps#6. 
Would you PTAL at that CL? Thanks. I'll close this CL if you all agree.

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

Close this CL as the new one https://codereview.chromium.org/2143383002/ has
landed.