Replace OnPageTranslate Obsever by a responder of delegate.

components/translate/core/browser/translate_infobar_delegate.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_TRANSLATE_CORE_BROWSER_TRANSLATE_INFOBAR_DELEGATE_H_
 #define COMPONENTS_TRANSLATE_CORE_BROWSER_TRANSLATE_INFOBAR_DELEGATE_H_
 
 #include <stddef.h>
 
 #include <memory>
@@ skipping 19 matching lines @@
 namespace translate {
 
 // Feature flag for "Translate Compact Infobar UI" project.
 extern const base::Feature kTranslateCompactUI;
 
 class TranslateDriver;
 class TranslateManager;
 
 class TranslateInfoBarDelegate : public infobars::InfoBarDelegate {
  public:
+  // A responder to handle different translate steps' UI changes.

[groby-ooo-7-16, 2017/05/20 21:30:39]

If I remember correctly, you don't need this class at all.

ChromeTranslateClient::CreateInfobar is called every single time the
TranslateStep changes. You can query that value from the delegate. (See
chrome/browser/ui/cocoa/infobars/translate_infobar_base.mm for an example)

If I read the Android code correctly, that means you're rebulding the Infobar
every time TranslateStep changes anyways, with a new delegate. 

Can you use that fact instead of adding an additional object? (Or is this subtly
different on Android, and I am misunderstanding?)

[Leo, 2017/05/21 04:15:20]

I think the fact you described is  the original logic for handling each step of
the translation.

In our new design, translate infobar will be created only once then it will be
reused for responding all the possible steps later. 

Eg. after showing the initialized UI, when user asks for translating. Old one
will create a "Translating" UI to replace initialized UI but in the new design, 
we will modify part of the current UI to indicate that it's being translating.

Since the rule of creating its own UI for each step is defined in very common
level and followed by its hierarchical logics, which means breaking the rule
will take a lots of cost(eg. in bubble UI, translateDelegate is abandoned and
they have to create quite a few modules to make it work...)

In our case, in order to reuse the current code as much as possible, adding a
responder(or observer) inside translateDelegate is the smallest change we can
do.

+  class Responder {

[groby-ooo-7-16, 2017/05/20 21:30:39]

We usually call those "Observer"

[Leo, 2017/05/22 09:08:10]

Thanks for questioning the class name, I have struggled quite a while to name
it. "Observer" was one of my options and "Responder" was closer.

The role of this class here is kind of A commitment. If Any delegate was
assigned with this class, it means this delegate will let it (the class
assigned) to handle all the possible steps without recreating another delegate
or infobar.

Now I am persuaded, so I Changed the name to "Observer".

+   public:
+    // Handles UI changes on the translate step given.
+    virtual void ActionOnStep(translate::TranslateStep step,

[groby-ooo-7-16, 2017/05/20 21:30:39]

Suggested name: OnTranslateStepChanged()

That's closer to the usual Chrome naming scheme.

[Leo, 2017/05/22 09:08:10]

Thanks, Done.

+                              TranslateErrors::Type error_type){};
+    // Return whether user declined translate service.

[groby-ooo-7-16, 2017/05/20 21:30:39]

Does "declined" include dismissing the infobar, or is it an active decline only?

[Leo, 2017/05/22 09:08:10]

"declined" here is just a flag used to evaluate user's behaviors. PM and UX of
the new design, want to understand users better, in our user flow, "declined"
means things below
1, user closed the infobar without using it at all.
2, user didn't touch the infobar at all until the page got navigated
3, user kept the infobar for a few seconds (eg. 20s) without touching
anything...

All these behaviors are recorded in the front-end and before user closing
infobar, we need a clear decision about whether this infobar server is declined
by user or not. If yes, we will commit a "Declined" logging.

So the method itself is just a few checking, and doesn't introduce any operation
of the infobar.

I will rename it to IsDeclinedByUser()

[groby-ooo-7-16, 2017/05/22 14:34:11]

Thank you - the reason I asked was that we use "Declined" for many different
things in the translate system. It's very helpful to have a name that explains!

+    virtual bool Declined();
+
+   protected:
+    virtual ~Responder() {}
+  };
+
   static const size_t kNoIndex;
 
   ~TranslateInfoBarDelegate() override;
 
   // Factory method to create a translate infobar.  |error_type| must be
   // specified iff |step| == TRANSLATION_ERROR.  For other translate steps,
   // |original_language| and |target_language| must be ASCII language codes
   // (e.g. "en", "fr", etc.) for languages the TranslateManager supports
   // translating.  The lone exception is when the user initiates translation
   // from the context menu, in which case it's legal to call this with
@@ skipping 125 matching lines @@
   // been translate to <lang2> from <lang1>."). It is ignored if
   // |autodetermined_source_language| is true.
   static void GetAfterTranslateStrings(std::vector<base::string16>* strings,
                                        bool* swap_languages,
                                        bool autodetermined_source_language);
 
   // Gets the TranslateDriver associated with this object.
   // May return NULL if the driver has been destroyed.
   TranslateDriver* GetTranslateDriver();
 
+  // Set a responder.
+  void SetResponder(Responder* responder);
+
  protected:
   TranslateInfoBarDelegate(
       const base::WeakPtr<TranslateManager>& translate_manager,
       bool is_off_the_record,
       translate::TranslateStep step,
       const std::string& original_language,
       const std::string& target_language,
       TranslateErrors::Type error_type,
       bool triggered_from_menu);
 
@@ skipping 17 matching lines @@
   // The error that occurred when trying to translate (NONE if no error).
   TranslateErrors::Type error_type_;
 
   // The translation related preferences.
   std::unique_ptr<TranslatePrefs> prefs_;
 
   // Whether the translation was triggered via a menu click vs automatically
   // (due to language detection, preferences...)
   bool triggered_from_menu_;
 
+  // A responder to handle front-end changes on different steps.
+  // It's only used when we try to reuse the existing UI.
+  Responder* responder_;
+
   DISALLOW_COPY_AND_ASSIGN(TranslateInfoBarDelegate);
 };
 
 }  // namespace translate
 
 #endif  // COMPONENTS_TRANSLATE_CORE_BROWSER_TRANSLATE_INFOBAR_DELEGATE_H_