Redesign: Remove TsfEventRouter interface.

ui/base/ime/win/tsf_event_router.cc

Index: ui/base/ime/win/tsf_event_router.cc
diff --git a/ui/base/ime/win/tsf_event_router.cc b/ui/base/ime/win/tsf_event_router.cc
index b26b27d71bb5cf692de3fb88aa9a577ebc143a02..95157b5d3d60ad1fdcb94f0361775ee437c6a561 100644
--- a/ui/base/ime/win/tsf_event_router.cc
+++ b/ui/base/ime/win/tsf_event_router.cc
@@ -11,227 +11,68 @@
 #include "base/bind.h"
 #include "base/win/scoped_comptr.h"
 #include "base/win/metro.h"
+#include "ui/base/win/atl_module.h"
 
 namespace ui {
 
-class TsfEventRouterImpl : public TsfEventRouter,
-                           public ITfUIElementSink,
-                           public ITfTextEditSink {
+// TsfEventRouter::TsfEventRouterDelegate  ------------------------------------
+// The implementation class of ITfUIElementSink, whose member functions will be
+// called back by TSF when the UI element status is changed, for example when
+// the candidate window is opened or closed. This class also implements
+// ITfTextEditSink, whose member function is called back by TSF when the text
+// editting session is finished.
+class ATL_NO_VTABLE TsfEventRouter::TsfEventRouterDelegate
+    : public ATL::CComObjectRootEx<CComSingleThreadModel>,
+      public ITfUIElementSink,
+      public ITfTextEditSink {
  public:
-  TsfEventRouterImpl()
-      : observer_(NULL),
-        context_source_cookie_(TF_INVALID_COOKIE),
-        ui_source_cookie_(TF_INVALID_COOKIE),
-        ref_count_(0) {}
+  BEGIN_COM_MAP(TsfEventRouterDelegate)
+    COM_INTERFACE_ENTRY(ITfUIElementSink)
+    COM_INTERFACE_ENTRY(ITfTextEditSink)
+  END_COM_MAP()
 
-  virtual ~TsfEventRouterImpl() {}
+  DECLARE_NOT_AGGREGATABLE(TsfEventRouterDelegate);

[Peter Kasting, 2012/10/26 01:53:47]

OK, Carlos and I chatted some about how this class should work.  He
misunderstood a few things before.  The updated instructions are basically, nuke
this macro, the DECLARE_PROTECT_FINAL_CONSTRUCT macro, and the FinalConstruct()
and FinalRelease() overrides.  None of those are necessary or useful.

[Seigo Nonaka, 2012/10/26 02:23:10]

Sure, thank you! removing macros.
On 2012/10/26 01:53:47, Peter Kasting wrote:

 
-  // ITfTextEditSink override.
-  virtual ULONG STDMETHODCALLTYPE AddRef() OVERRIDE {
-    return InterlockedIncrement(&ref_count_);
-  }
+  DECLARE_PROTECT_FINAL_CONSTRUCT();
 
-  // ITfTextEditSink override.
-  virtual ULONG STDMETHODCALLTYPE Release() OVERRIDE {
-    const LONG count = InterlockedDecrement(&ref_count_);
-    if (!count) {
-      delete this;
-      return 0;
-    }
-    return static_cast<ULONG>(count);
-  }
+  TsfEventRouterDelegate();
+  ~TsfEventRouterDelegate();
 
-  // ITfTextEditSink override.
-  virtual STDMETHODIMP QueryInterface(REFIID iid, void** result) OVERRIDE {
-    if (!result)
-      return E_INVALIDARG;
-    if (iid == IID_IUnknown || iid == IID_ITfTextEditSink) {
-      *result = static_cast<ITfTextEditSink*>(this);
-    } else if (iid == IID_ITfUIElementSink) {
-      *result = static_cast<ITfUIElementSink*>(this);
-    } else {
-      *result = NULL;
-      return E_NOINTERFACE;
-    }
-    AddRef();
-    return S_OK;
-  }
+  HRESULT FinalConstruct();
+  void FinalRelease();
 
-  // ITfTextEditSink override.
-  virtual STDMETHODIMP OnEndEdit(ITfContext* context,
+  // ITfTextEditSink:
+  STDMETHOD_(HRESULT, OnEndEdit)(ITfContext* context,
                                  TfEditCookie read_only_cookie,
-                                 ITfEditRecord* edit_record) OVERRIDE {
-    if (!edit_record || !context)
-      return E_INVALIDARG;
-    if (!observer_)
-      return S_OK;
-
-    // |edit_record| can be used to obtain updated ranges in terms of text
-    // contents and/or text attributes. Here we are interested only in text
-    // update so we use TF_GTP_INCL_TEXT and check if there is any range which
-    // contains updated text.
-    base::win::ScopedComPtr<IEnumTfRanges> ranges;
-    if (FAILED(edit_record->GetTextAndPropertyUpdates(TF_GTP_INCL_TEXT,
-                                                      NULL,
-                                                      0,
-                                                      ranges.Receive()))) {
-       return S_OK;  // don't care about failures.
-    }
-
-    ULONG fetched_count = 0;
-    base::win::ScopedComPtr<ITfRange> range;
-    if (FAILED(ranges->Next(1, range.Receive(), &fetched_count)))
-      return S_OK;  // don't care about failures.
-
-    // |fetched_count| != 0 means there is at least one range that contains
-    // updated texts.
-    if (fetched_count != 0)
-      observer_->OnTextUpdated();
-    return S_OK;
-  }
-
-  // ITfUiElementSink override.
-  virtual STDMETHODIMP BeginUIElement(DWORD element_id, BOOL* is_show) {
-    if (is_show)
-      *is_show = TRUE;  // without this the UI element will not be shown.
-
-    if (!IsCandidateWindowInternal(element_id))
-      return S_OK;
-
-    std::pair<std::set<DWORD>::iterator, bool> insert_result =
-        open_candidate_window_ids_.insert(element_id);
-
-    if (!observer_)
-      return S_OK;
+                                 ITfEditRecord* edit_record) OVERRIDE;
 
-    // Don't call if |element_id| is already handled.
-    if (!insert_result.second)
-      return S_OK;
+  // ITfUiElementSink:
+  STDMETHOD_(HRESULT, BeginUIElement)(DWORD element_id, BOOL* is_show) OVERRIDE;
+  STDMETHOD_(HRESULT, UpdateUIElement)(DWORD element_id) OVERRIDE;
+  STDMETHOD_(HRESULT, EndUIElement)(DWORD element_id) OVERRIDE;
 
-    observer_->OnCandidateWindowCountChanged(open_candidate_window_ids_.size());
+  // Sets |manager| to be monitored. |manager| can be NULL.
+  void SetManager(ITfThreadMgr* manager);
 
-    return S_OK;
-  }
-
-  // ITfUiElementSink override.
-  virtual STDMETHODIMP UpdateUIElement(DWORD element_id) {
-    return S_OK;
-  }
+  // Returns true if the IME is composing text.
+  bool IsImeComposing();
 
-  // ITfUiElementSink override.
-  virtual STDMETHODIMP EndUIElement(DWORD element_id) {
-    if (open_candidate_window_ids_.erase(element_id) == 0)
-      return S_OK;
-
-    if (!observer_)
-      return S_OK;
-
-    observer_->OnCandidateWindowCountChanged(open_candidate_window_ids_.size());
-
-    return S_OK;
-  }
-
-  // TsfEventRouter override.
-  virtual void SetManager(ITfThreadMgr* manager, Observer* observer) OVERRIDE {
-    EnsureDeassociated();
-    if (manager && observer) {
-      Associate(manager, observer);
-    }
-  }
-
-  // TsfEventRouter override.
-  virtual bool IsImeComposing() OVERRIDE {
-    DCHECK(base::win::IsTsfAwareRequired())
-        << "Do not call without TSF environment.";
-    if (!context_)
-      return false;
-    return IsImeComposingInternal(context_);
-  }
+  // Sets |router| to be forward TSF related events.

[Peter Kasting, 2012/10/26 01:53:47]

Nit: forward TSF related -> forwarded TSF-related

[Seigo Nonaka, 2012/10/26 02:23:10]

Done.

+  void SetRouter(TsfEventRouter* router);
 
  private:
-  // Returns true if the given |context| is in composing.
-  static bool IsImeComposingInternal(ITfContext* context) {
-    DCHECK(base::win::IsTsfAwareRequired())
-        << "Do not call without TSF environment.";
-    DCHECK(context);
-    base::win::ScopedComPtr<ITfContextComposition> context_composition;
-    if (FAILED(context_composition.QueryFrom(context)))
-      return false;
-    base::win::ScopedComPtr<IEnumITfCompositionView> enum_composition_view;
-    if (FAILED(context_composition->EnumCompositions(
-            enum_composition_view.Receive())))
-      return false;
-    base::win::ScopedComPtr<ITfCompositionView> composition_view;
-    return enum_composition_view->Next(1, composition_view.Receive(),
-               NULL) == S_OK;
-  }
+  // Returns true if the given |context| is composing.
+  static bool IsImeComposingInternal(ITfContext* context);
 
-  // Returns true if the given |element_id| represents candidate window.
-  bool IsCandidateWindowInternal(DWORD element_id) {
-    DCHECK(ui_element_manager_.get());
-    base::win::ScopedComPtr<ITfUIElement> ui_element;
-    if (FAILED(ui_element_manager_->GetUIElement(element_id,
-                                                 ui_element.Receive()))) {
-      return false;
-    }
-    base::win::ScopedComPtr<ITfCandidateListUIElement>
-        candidate_list_ui_element;
-    return SUCCEEDED(candidate_list_ui_element.QueryFrom(ui_element));
-  }
+  // Returns true if the given |element_id| represents the candidate window.
+  bool IsCandidateWindowInternal(DWORD element_id);
 
-  // Associates this class with specified |manager|.
-  void Associate(ITfThreadMgr* thread_manager, Observer* observer) {
-    DCHECK(base::win::IsTsfAwareRequired())
-        << "Do not call without TSF environment.";
-    DCHECK(thread_manager);
-
-    base::win::ScopedComPtr<ITfDocumentMgr> document_manager;
-    if (FAILED(thread_manager->GetFocus(document_manager.Receive())))
-      return;
-
-    if (FAILED(document_manager->GetBase(context_.Receive())))
-      return;
-    if (FAILED(context_source_.QueryFrom(context_)))
-      return;
-    context_source_->AdviseSink(IID_ITfTextEditSink,
-                                static_cast<ITfTextEditSink*>(this),
-                                &context_source_cookie_);
-
-    if (FAILED(ui_element_manager_.QueryFrom(thread_manager)))
-      return;
-    if (FAILED(ui_source_.QueryFrom(ui_element_manager_)))
-      return;
-    ui_source_->AdviseSink(IID_ITfUIElementSink,
-                           static_cast<ITfUIElementSink*>(this),
-                           &ui_source_cookie_);
-    observer_ = observer;
-  }
+  // Associates this class with the specified |manager|.

[Peter Kasting, 2012/10/26 01:53:47]

Nit: manager -> thread_manager

[Seigo Nonaka, 2012/10/26 02:23:10]

Done.

+  void Associate(ITfThreadMgr* thread_manager);
 
-  // Resets the association, this function is safe to call if there is no
+  // Resets the association. This function is safe to call even if there is no
   // associated thread manager.
-  void EnsureDeassociated() {
-    DCHECK(base::win::IsTsfAwareRequired())
-        << "Do not call without TSF environment.";
-
-    context_.Release();
-
-    if (context_source_) {
-      context_source_->UnadviseSink(context_source_cookie_);
-      context_source_.Release();
-    }
-    context_source_cookie_ = TF_INVALID_COOKIE;
-
-    ui_element_manager_.Release();
-    if (ui_source_) {
-      ui_source_->UnadviseSink(ui_source_cookie_);
-      ui_source_.Release();
-    }
-    ui_source_cookie_ = TF_INVALID_COOKIE;
-
-    observer_ = NULL;
-  }
-
-  Observer* observer_;
+  void EnsureDeassociated();
 
   // A context associated with this class.
   base::win::ScopedComPtr<ITfContext> context_;
@@ -242,35 +83,212 @@ class TsfEventRouterImpl : public TsfEventRouter,
   // The cookie for |context_source_|.
   DWORD context_source_cookie_;
 
-  // A UiElementMgr associated with this class.
+  // A UIElementMgr associated with this class.
   base::win::ScopedComPtr<ITfUIElementMgr> ui_element_manager_;
 
   // The ITfSouce associated with |ui_element_manager_|.
   base::win::ScopedComPtr<ITfSource> ui_source_;
 
+  // The set of currently opened candidate window ids.
+  std::set<DWORD> open_candidate_window_ids_;
+
   // The cookie for |ui_source_|.
   DWORD ui_source_cookie_;
 
-  // The set of currently opend candidate window ids.
-  std::set<DWORD> open_candidate_window_ids_;
-
-  // The reference count of this instance.
-  volatile LONG ref_count_;
+  TsfEventRouter* router_;
 
-  DISALLOW_COPY_AND_ASSIGN(TsfEventRouterImpl);
+  DISALLOW_COPY_AND_ASSIGN(TsfEventRouterDelegate);
 };
 
-///////////////////////////////////////////////////////////////////////////////
-// TsfEventRouter
+TsfEventRouter::TsfEventRouterDelegate::TsfEventRouterDelegate()
+    : context_source_cookie_(TF_INVALID_COOKIE),
+      ui_source_cookie_(TF_INVALID_COOKIE),
+      router_(NULL) {
+}
+
+TsfEventRouter::TsfEventRouterDelegate::~TsfEventRouterDelegate() {}
+
+HRESULT TsfEventRouter::TsfEventRouterDelegate::FinalConstruct() {
+  return S_OK;
+}
+
+void TsfEventRouter::TsfEventRouterDelegate::FinalRelease() {}
+
+void TsfEventRouter::TsfEventRouterDelegate::SetRouter(TsfEventRouter* router) {
+  router_ = router;
+}
+
+STDMETHODIMP TsfEventRouter::TsfEventRouterDelegate::OnEndEdit(
+    ITfContext* context,
+    TfEditCookie read_only_cookie,
+    ITfEditRecord* edit_record) {
+  if (!edit_record || !context)
+    return E_INVALIDARG;
+  if (!router_)
+    return S_OK;
+
+  // |edit_record| can be used to obtain updated ranges in terms of text
+  // contents and/or text attributes. Here we are interested only in text update
+  // so we use TF_GTP_INCL_TEXT and check if there is any range which contains
+  // updated text.
+  base::win::ScopedComPtr<IEnumTfRanges> ranges;
+  if (FAILED(edit_record->GetTextAndPropertyUpdates(TF_GTP_INCL_TEXT, NULL, 0,
+                                                    ranges.Receive())))
+     return S_OK;  // Don't care about failures.
+
+  ULONG fetched_count = 0;
+  base::win::ScopedComPtr<ITfRange> range;
+  if (FAILED(ranges->Next(1, range.Receive(), &fetched_count)))
+    return S_OK;  // Don't care about failures.
+
+  // |fetched_count| != 0 means there is at least one range that contains
+  // updated text.
+  if (fetched_count != 0)
+    router_->OnTextUpdated();
+  return S_OK;
+}
+
+STDMETHODIMP TsfEventRouter::TsfEventRouterDelegate::BeginUIElement(
+    DWORD element_id,
+    BOOL* is_show) {
+  if (is_show)
+    *is_show = TRUE;  // Without this the UI element will not be shown.
+
+  if (!IsCandidateWindowInternal(element_id))
+    return S_OK;
+
+  std::pair<std::set<DWORD>::iterator, bool> insert_result =
+      open_candidate_window_ids_.insert(element_id);
+  // Don't call if |router_| is null or |element_id| is already handled.
+  if (router_ && insert_result.second)
+    router_->OnCandidateWindowCountChanged(open_candidate_window_ids_.size());
+
+  return S_OK;
+}
+
+STDMETHODIMP TsfEventRouter::TsfEventRouterDelegate::UpdateUIElement(
+    DWORD element_id) {
+  return S_OK;
+}
+
+STDMETHODIMP TsfEventRouter::TsfEventRouterDelegate::EndUIElement(
+    DWORD element_id) {
+  if ((open_candidate_window_ids_.erase(element_id) != 0) && router_)
+    router_->OnCandidateWindowCountChanged(open_candidate_window_ids_.size());
+  return S_OK;
+}
 
-TsfEventRouter::TsfEventRouter() {
+void TsfEventRouter::TsfEventRouterDelegate::SetManager(ITfThreadMgr* manager) {
+  EnsureDeassociated();
+  if (manager)
+    Associate(manager);
+}
+
+bool TsfEventRouter::TsfEventRouterDelegate::IsImeComposing() {
+  return context_ && IsImeComposingInternal(context_);
+}
+
+// static
+bool TsfEventRouter::TsfEventRouterDelegate::IsImeComposingInternal(
+    ITfContext* context) {
+  DCHECK(context);
+  base::win::ScopedComPtr<ITfContextComposition> context_composition;
+  if (FAILED(context_composition.QueryFrom(context)))
+    return false;
+  base::win::ScopedComPtr<IEnumITfCompositionView> enum_composition_view;
+  if (FAILED(context_composition->EnumCompositions(
+      enum_composition_view.Receive())))
+    return false;
+  base::win::ScopedComPtr<ITfCompositionView> composition_view;
+  return enum_composition_view->Next(1, composition_view.Receive(),
+                                     NULL) == S_OK;
+}
+
+bool TsfEventRouter::TsfEventRouterDelegate::IsCandidateWindowInternal(
+    DWORD element_id) {
+  DCHECK(ui_element_manager_.get());
+  base::win::ScopedComPtr<ITfUIElement> ui_element;
+  if (FAILED(ui_element_manager_->GetUIElement(element_id,
+                                               ui_element.Receive())))
+    return false;
+  base::win::ScopedComPtr<ITfCandidateListUIElement> candidate_list_ui_element;
+  return SUCCEEDED(candidate_list_ui_element.QueryFrom(ui_element));
+}
+
+void TsfEventRouter::TsfEventRouterDelegate::Associate(
+    ITfThreadMgr* thread_manager) {
+  DCHECK(thread_manager);
+  base::win::ScopedComPtr<ITfDocumentMgr> document_manager;
+  if (FAILED(thread_manager->GetFocus(document_manager.Receive())) ||
+      FAILED(document_manager->GetBase(context_.Receive())) ||
+      FAILED(context_source_.QueryFrom(context_)))
+    return;
+  context_source_->AdviseSink(IID_ITfTextEditSink,
+                              static_cast<ITfTextEditSink*>(this),
+                              &context_source_cookie_);
+
+  if (FAILED(ui_element_manager_.QueryFrom(thread_manager)) ||
+      FAILED(ui_source_.QueryFrom(ui_element_manager_)))
+    return;
+  ui_source_->AdviseSink(IID_ITfUIElementSink,
+                         static_cast<ITfUIElementSink*>(this),
+                         &ui_source_cookie_);
+}
+
+void TsfEventRouter::TsfEventRouterDelegate::EnsureDeassociated() {

[Peter Kasting, 2012/10/26 01:53:47]

Nit: Only called once, just inline this function into SetManager()

[Seigo Nonaka, 2012/10/26 02:23:10]

Sure, done and also done for Associate which is also called  only once, 
On 2012/10/26 01:53:47, Peter Kasting wrote:

+  context_.Release();
+
+  if (context_source_) {
+    context_source_->UnadviseSink(context_source_cookie_);
+    context_source_.Release();
+  }
+  context_source_cookie_ = TF_INVALID_COOKIE;
+
+  ui_element_manager_.Release();
+  if (ui_source_) {
+    ui_source_->UnadviseSink(ui_source_cookie_);
+    ui_source_.Release();
+  }
+  ui_source_cookie_ = TF_INVALID_COOKIE;
+}
+
+// TsfEventRouter  ------------------------------------------------------------

[Peter Kasting, 2012/10/26 01:53:47]

Nit: Add another blank line on both sides of this line

[Seigo Nonaka, 2012/10/26 02:23:10]

Done.

+TsfEventRouter::TsfEventRouter(TsfEventRouterObserver* observer)
+    : observer_(observer),
+      delegate_(NULL) {
+  DCHECK(base::win::IsTsfAwareRequired())
+      << "Do not use TsfEventRouter without TSF environment.";
+  ui::win::CreateATLModuleIfNeeded();
+  if (SUCCEEDED(CComObject<TsfEventRouterDelegate>::CreateInstance(
+      &delegate_))) {
+    delegate_->AddRef();
+    delegate_->SetRouter(this);
+  }
 }
 
 TsfEventRouter::~TsfEventRouter() {
+  if (delegate_) {
+    delegate_->SetManager(NULL);
+    delegate_->SetRouter(NULL);
+    delegate_->Release();
+    delegate_ = NULL;

[Peter Kasting, 2012/10/26 01:53:47]

Once you switch back to CComPtr, these last two lines should go away.

[Seigo Nonaka, 2012/10/26 02:23:10]

Done.

+  }
+}
+
+void TsfEventRouter::SetManager(ITfThreadMgr* manager) {
+  delegate_->SetManager(manager);
+}
+
+bool TsfEventRouter::IsImeComposing() {
+  return delegate_->IsImeComposing();
+}
+
+void TsfEventRouter::OnTextUpdated() {
+  observer_->OnTextUpdated();
 }
 
-TsfEventRouter* TsfEventRouter::Create() {
-  return new TsfEventRouterImpl();
+void TsfEventRouter::OnCandidateWindowCountChanged(size_t window_count) {
+  observer_->OnCandidateWindowCountChanged(window_count);
 }
 
 }  // namespace ui