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

components/translate/content/browser/content_translate_driver_impl.h

Index: components/translate/content/browser/content_translate_driver_impl.h
diff --git a/components/translate/content/browser/content_translate_driver.h b/components/translate/content/browser/content_translate_driver_impl.h
similarity index 68%
rename from components/translate/content/browser/content_translate_driver.h
rename to components/translate/content/browser/content_translate_driver_impl.h
index 685f8e61daadc6b5762f65103605a59ac2960c2d..398075860d4cea59dc457720150dc5588fc5afcc 100644
--- a/components/translate/content/browser/content_translate_driver.h
+++ b/components/translate/content/browser/content_translate_driver_impl.h
@@ -2,15 +2,19 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef COMPONENTS_TRANSLATE_CONTENT_BROWSER_CONTENT_TRANSLATE_DRIVER_H_
-#define COMPONENTS_TRANSLATE_CONTENT_BROWSER_CONTENT_TRANSLATE_DRIVER_H_
+#ifndef COMPONENTS_TRANSLATE_CONTENT_BROWSER_CONTENT_TRANSLATE_DRIVER_IMPL_H_
+#define COMPONENTS_TRANSLATE_CONTENT_BROWSER_CONTENT_TRANSLATE_DRIVER_IMPL_H_
+
+#include <map>
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "components/translate/content/public/interfaces/translate.mojom.h"
 #include "components/translate/core/browser/translate_driver.h"
 #include "components/translate/core/common/translate_errors.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "mojo/public/cpp/bindings/binding_set.h"
 
 namespace content {
 class NavigationController;
@@ -22,37 +26,38 @@ namespace translate {
 struct LanguageDetectionDetails;
 class TranslateManager;
 
-
 // Content implementation of TranslateDriver.
-class ContentTranslateDriver : public TranslateDriver,
-                               public content::WebContentsObserver {
+class ContentTranslateDriverImpl : public TranslateDriver,
+                                   public content::WebContentsObserver,
+                                   public mojom::ContentTranslateDriver {
  public:
-
-  // The observer for the ContentTranslateDriver.
+  // The observer for the ContentTranslateDriverImpl.
   class Observer {
    public:
     // Handles when the value of IsPageTranslated is changed.
-    virtual void OnIsPageTranslatedChanged(content::WebContents* source) {};
+    virtual void OnIsPageTranslatedChanged(content::WebContents* source){};

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

I believe the chromium style is to always have a space after a closing brace.
Please revert this style to the original.

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

Acknowledged.

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

Done.

 
     // Handles when the value of translate_enabled is changed.
-    virtual void OnTranslateEnabledChanged(content::WebContents* source) {};
+    virtual void OnTranslateEnabledChanged(content::WebContents* source){};
 
     // Called when the page language has been determined.
     virtual void OnLanguageDetermined(
-        const translate::LanguageDetectionDetails& details) {};
+        const translate::LanguageDetectionDetails& details){};
 
     // Called when the page has been translated.
     virtual void OnPageTranslated(
         const std::string& original_lang,
         const std::string& translated_lang,
-        translate::TranslateErrors::Type error_type) {};
+        translate::TranslateErrors::Type error_type){};
 
    protected:
     virtual ~Observer() {}
   };
 
-  ContentTranslateDriver(content::NavigationController* nav_controller);
-  ~ContentTranslateDriver() override;
+  ContentTranslateDriverImpl(content::NavigationController* nav_controller);

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

explicit on single-arg constructors.

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

Acknowledged.

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

Done.

+  ~ContentTranslateDriverImpl() override;
+
+  void BindRequest(mojom::ContentTranslateDriverRequest request);
 
   // Adds or Removes observers.
   void AddObserver(Observer* observer);
@@ -93,18 +98,20 @@ class ContentTranslateDriver : public TranslateDriver,
   void DidNavigateAnyFrame(content::RenderFrameHost* render_frame_host,
                            const content::LoadCommittedDetails& details,
                            const content::FrameNavigateParams& params) override;
-  bool OnMessageReceived(const IPC::Message& message,
-                         content::RenderFrameHost* render_frame_host) override;
-
-  // IPC handlers.
-  void OnTranslateAssignedSequenceNumber(int page_seq_no);
-  void OnLanguageDetermined(const LanguageDetectionDetails& details,
-                            bool page_needs_translation);
-  void OnPageTranslated(const std::string& original_lang,
-                        const std::string& translated_lang,
+
+  void OnPageTranslated(bool cancelled,
+                        mojo::String original_lang,
+                        mojo::String translated_lang,
                         TranslateErrors::Type error_type);
 
+  // mojom::ContentTranslateDriver implementation.
+  void NewPage(mojom::PagePtr page,
+               const LanguageDetectionDetails& details,
+               bool page_needs_translation) override;
+
  private:
+  void OnPageAway(int page_seq_no);
+
   // The navigation controller of the tab we are associated with.
   content::NavigationController* navigation_controller_;
 
@@ -115,11 +122,19 @@ class ContentTranslateDriver : public TranslateDriver,
   // Max number of attempts before checking if a page has been reloaded.
   int max_reload_check_attempts_;
 
-  base::WeakPtrFactory<ContentTranslateDriver> weak_pointer_factory_;
+  // Records mojo connections with all current alive pages.

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

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:48:17]

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]

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.

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

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.

+  int next_page_seq_no_;
+  std::map<int, mojom::PagePtr> mojo_pages_;
+
+  // ContentTranslateDriverImpl is singleton per web contents ,
+  // serve for multiple render frames.
+  mojo::BindingSet<mojom::ContentTranslateDriver> bindings_;
+
+  base::WeakPtrFactory<ContentTranslateDriverImpl> weak_pointer_factory_;
 
-  DISALLOW_COPY_AND_ASSIGN(ContentTranslateDriver);
+  DISALLOW_COPY_AND_ASSIGN(ContentTranslateDriverImpl);
 };
 
 }  // namespace translate
 
-#endif  // COMPONENTS_TRANSLATE_CONTENT_BROWSER_CONTENT_TRANSLATE_DRIVER_H_
+#endif  // COMPONENTS_TRANSLATE_CONTENT_BROWSER_CONTENT_TRANSLATE_DRIVER_IMPL_H_