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..aa89a0652e4cb7e1510b94bd3712aad264aa9dba 100644
--- a/ui/base/ime/win/tsf_event_router.cc
+++ b/ui/base/ime/win/tsf_event_router.cc
@@ -11,266 +11,206 @@
 #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 {
- public:
-  TsfEventRouterImpl()
-      : observer_(NULL),
-        context_source_cookie_(TF_INVALID_COOKIE),
-        ui_source_cookie_(TF_INVALID_COOKIE),
-        ref_count_(0) {}
-
-  virtual ~TsfEventRouterImpl() {}
-
-  // ITfTextEditSink override.
-  virtual ULONG STDMETHODCALLTYPE AddRef() OVERRIDE {
-    return InterlockedIncrement(&ref_count_);
+TsfEventRouter::TsfEventRouter() {
+  ui::win::CreateATLModuleIfNeeded();
+  CComObject<TsfEventRouterDelegate>* delegate = NULL;
+  if (SUCCEEDED(CComObject<TsfEventRouterDelegate>::CreateInstance(
+          &delegate))) {

[Peter Kasting, 2012/10/24 20:48:11]

I believe you have to explicitly AddRef() here, since
CComObject::CreateInstance() does not do so for you (unlike CoCreateInstance).

Carlos, please correct me if I'm wrong...

[cpu_(ooo_6.6-7.5), 2012/10/25 00:43:29]

Good catch. This is a bug. I can even guess the reason. CreateInstance is there
to use a COM object as a regular c++ object, handy when it is contained and you
are not doling out references to it. In that case you don't even need AddRef,
however you are storing it as a CComPtr and that is the problem because that
will call Release() and now you have a problem. I am guessing that before
delegate_ was plain pointer.

I think is best to create it via CoCreateInstance or I think you can use 
CComCoClass<T>::CreateInstance() if you go this way check with a debugger ref
count is 1. It has been too long ago.

[Seigo Nonaka, 2012/10/25 14:37:16]

I'm getting confused about what type is should be placed as <T> of CComPtr<T> in
this case.
Currently I'm using CComObject<TsfEventRouterDelegate>* as usual C++ object
pointer.
I also confirmed the new design works well including ref count.

However, explicitly calling AddRef/Release might look a little ugly. WDYT?
 
On 2012/10/25 00:43:29, cpu wrote:

+    delegate_ = delegate;
   }
+}
 
-  // ITfTextEditSink override.
-  virtual ULONG STDMETHODCALLTYPE Release() OVERRIDE {
-    const LONG count = InterlockedDecrement(&ref_count_);
-    if (!count) {
-      delete this;
-      return 0;
-    }
-    return static_cast<ULONG>(count);
-  }
+TsfEventRouter::~TsfEventRouter() {
+  if (delegate_)
+    delegate_->SetManager(NULL, NULL);
+}
 
-  // 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;
-  }
+TsfEventRouter::TsfEventRouterDelegate::TsfEventRouterDelegate()
+    : context_source_cookie_(TF_INVALID_COOKIE),
+    ui_source_cookie_(TF_INVALID_COOKIE),

[Peter Kasting, 2012/10/24 20:48:11]

Nit: Indent even

[Seigo Nonaka, 2012/10/25 14:37:16]

Done.

+    observer_(NULL) {}
+
+TsfEventRouter::TsfEventRouterDelegate::~TsfEventRouterDelegate() {}
 
-  // ITfTextEditSink override.
-  virtual STDMETHODIMP 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();
+STDMETHODIMP TsfEventRouter::TsfEventRouterDelegate::OnEndEdit(

[Peter Kasting, 2012/10/24 20:48:11]

Please separate all TsfEventRouterDelegate functions from the TsfEventRouter
functions (and order as in the .h file).

I suggest divider comments between blocks, a la:

...


// TsfEventRouter::TsfEventRouterDelegate ------------------------------------

...


// TsfEventRouter -------------------------------------------------------------

...

[Seigo Nonaka, 2012/10/25 14:37:16]

Done.

+    ITfContext* context,
+    TfEditCookie read_only_cookie,
+    ITfEditRecord* edit_record) {
+  if (!edit_record || !context)
+    return E_INVALIDARG;
+  if (!observer_)
     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.
+  // |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()))) {

[Peter Kasting, 2012/10/24 20:48:11]

Nit: {} unnecessary when body is one line (3 places)

[Seigo Nonaka, 2012/10/25 14:37:16]

Done.

+     return S_OK;  // don't care about failures.

[Peter Kasting, 2012/10/24 20:48:11]

Nit: Capitalize first word (3 places)

[Seigo Nonaka, 2012/10/25 14:37:16]

Done.

+  }
 
-    if (!IsCandidateWindowInternal(element_id))
-      return S_OK;
+  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.
 
-    std::pair<std::set<DWORD>::iterator, bool> insert_result =
-        open_candidate_window_ids_.insert(element_id);
+  // |fetched_count| != 0 means there is at least one range that contains
+  // updated texts.

[Peter Kasting, 2012/10/24 20:48:11]

Nit: texts -> text

[Seigo Nonaka, 2012/10/25 14:37:16]

Done.

+  if (fetched_count != 0)
+    observer_->OnTextUpdated();
+  return S_OK;
+}
 
-    if (!observer_)
-      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.
 
-    // Don't call if |element_id| is already handled.
-    if (!insert_result.second)
-      return S_OK;
+  if (!IsCandidateWindowInternal(element_id))
+    return S_OK;
 
-    observer_->OnCandidateWindowCountChanged(open_candidate_window_ids_.size());
+  std::pair<std::set<DWORD>::iterator, bool> insert_result =
+      open_candidate_window_ids_.insert(element_id);
 
+  if (!observer_)

[Peter Kasting, 2012/10/24 20:48:11]

Nit: Simpler:

  if (observer_ && insert_result.second)
    observer_->OnCandidateWindowCountChanged(open_candidate_window_ids_.size());
  return S_OK;

(Also remove blank line above this)

[Seigo Nonaka, 2012/10/25 14:37:16]

Done.

     return S_OK;
-  }
 
-  // ITfUiElementSink override.
-  virtual STDMETHODIMP UpdateUIElement(DWORD element_id) {
+  // Don't call if |element_id| is already handled.
+  if (!insert_result.second)
     return S_OK;
-  }
 
-  // ITfUiElementSink override.
-  virtual STDMETHODIMP EndUIElement(DWORD element_id) {
-    if (open_candidate_window_ids_.erase(element_id) == 0)
-      return S_OK;
+  observer_->OnCandidateWindowCountChanged(open_candidate_window_ids_.size());
 
-    if (!observer_)
-      return S_OK;
+  return S_OK;
+}
 
-    observer_->OnCandidateWindowCountChanged(open_candidate_window_ids_.size());
+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)

[Peter Kasting, 2012/10/24 20:48:11]

Nit: Simpler:

  if ((open_candidate_window_ids_.erase(element_id) != 0) && observer_)
    observer_->OnCandidateWindowCountChanged(open_candidate_window_ids_.size());
  return S_OK;

[Seigo Nonaka, 2012/10/25 14:37:16]

Done.

     return S_OK;
-  }
 
-  // TsfEventRouter override.
-  virtual void SetManager(ITfThreadMgr* manager, Observer* observer) OVERRIDE {
-    EnsureDeassociated();
-    if (manager && observer) {
-      Associate(manager, observer);
-    }
-  }
+  if (!observer_)
+    return S_OK;
 
-  // TsfEventRouter override.
-  virtual bool IsImeComposing() OVERRIDE {
-    DCHECK(base::win::IsTsfAwareRequired())
-        << "Do not call without TSF environment.";
-    if (!context_)
-      return false;
-    return IsImeComposingInternal(context_);
-  }
+  observer_->OnCandidateWindowCountChanged(open_candidate_window_ids_.size());
 
- 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 |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));
-  }
+  return S_OK;
+}
 
-  // 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;
+void TsfEventRouter::TsfEventRouterDelegate::SetManager(ITfThreadMgr* manager,
+                                                        Observer* observer) {
+  EnsureDeassociated();
+  if (manager && observer) {
+    Associate(manager, observer);
   }
+}
 
-  // Resets the association, this function is safe to call if there is no
-  // associated thread manager.
-  void EnsureDeassociated() {
-    DCHECK(base::win::IsTsfAwareRequired())
-        << "Do not call without TSF environment.";
+void TsfEventRouter::SetManager(ITfThreadMgr* manager, Observer* observer) {
+  delegate_->SetManager(manager, observer);
+}
 
-    context_.Release();
+bool TsfEventRouter::IsImeComposing() {
+  return delegate_->IsImeComposing();
+}
 
-    if (context_source_) {
-      context_source_->UnadviseSink(context_source_cookie_);
-      context_source_.Release();
-    }
-    context_source_cookie_ = TF_INVALID_COOKIE;
+bool TsfEventRouter::TsfEventRouterDelegate::IsImeComposing() {
+  DCHECK(base::win::IsTsfAwareRequired())

[Peter Kasting, 2012/10/24 20:48:11]

Seems like this DCHECK belongs in the constructor instead of the individual API
functions, no?  (several places)

[Seigo Nonaka, 2012/10/25 14:37:16]

Yes, removed.
On 2012/10/24 20:48:11, Peter Kasting wrote:

+      << "Do not call without TSF environment.";
+  if (!context_)

[Peter Kasting, 2012/10/24 20:48:11]

Nit: Simpler:

  return context_ && IsImeComposingInternal(context_);

[Seigo Nonaka, 2012/10/25 14:37:16]

Done.

+    return false;
+  return IsImeComposingInternal(context_);
+}
 
-    ui_element_manager_.Release();
-    if (ui_source_) {
-      ui_source_->UnadviseSink(ui_source_cookie_);
-      ui_source_.Release();
-    }
-    ui_source_cookie_ = TF_INVALID_COOKIE;
+// static
+bool TsfEventRouter::TsfEventRouterDelegate::IsImeComposingInternal(
+    ITfContext* context) {
+  DCHECK(base::win::IsTsfAwareRequired())
+      << "Do not call without TSF environment.";
+  DCHECK(context);

[Peter Kasting, 2012/10/24 20:48:11]

Nit: Be consistent about whether precondition DCHECKs atop functions get a blank
line after them or not.  Some functions do one and some do the other.

[Seigo Nonaka, 2012/10/25 14:37:16]

Sure, removing all blank line after DCHECK.
On 2012/10/24 20:48:11, Peter Kasting wrote:

+  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())))

[Peter Kasting, 2012/10/24 20:48:11]

Nit: indent 4, not 8

[Seigo Nonaka, 2012/10/25 14:37:16]

Done.

+    return false;
+  base::win::ScopedComPtr<ITfCompositionView> composition_view;
+  return

[Peter Kasting, 2012/10/24 20:48:11]

Nit: I suggest breaking as:

  return enum_composition_view->Next(1, composition_view.Receive(),
                                     NULL) == S_OK;

[Seigo Nonaka, 2012/10/25 14:37:16]

Sure, done.
On 2012/10/24 20:48:11, Peter Kasting wrote:

+      enum_composition_view->Next(1, composition_view.Receive(), NULL) == S_OK;
+}
 
-    observer_ = NULL;
+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));
+}
 
-  Observer* observer_;
-
-  // A context associated with this class.
-  base::win::ScopedComPtr<ITfContext> context_;
-
-  // The ITfSource associated with |context_|.
-  base::win::ScopedComPtr<ITfSource> context_source_;
-
-  // The cookie for |context_source_|.
-  DWORD context_source_cookie_;
-
-  // 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 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_;
+void TsfEventRouter::TsfEventRouterDelegate::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())))

[Peter Kasting, 2012/10/24 20:48:11]

Nit: Can combine multiple FAILED() calls together in one statement with ||

[Seigo Nonaka, 2012/10/25 14:37:16]

Done.

+    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;
+}
 
-  DISALLOW_COPY_AND_ASSIGN(TsfEventRouterImpl);
-};
+void TsfEventRouter::TsfEventRouterDelegate::EnsureDeassociated() {
+  DCHECK(base::win::IsTsfAwareRequired())
+      << "Do not call without TSF environment.";
 
-///////////////////////////////////////////////////////////////////////////////
-// TsfEventRouter
+  context_.Release();
 
-TsfEventRouter::TsfEventRouter() {
-}
+  if (context_source_) {
+    context_source_->UnadviseSink(context_source_cookie_);
+    context_source_.Release();
+  }
+  context_source_cookie_ = TF_INVALID_COOKIE;
 
-TsfEventRouter::~TsfEventRouter() {
-}
+  ui_element_manager_.Release();
+  if (ui_source_) {
+    ui_source_->UnadviseSink(ui_source_cookie_);
+    ui_source_.Release();
+  }
+  ui_source_cookie_ = TF_INVALID_COOKIE;
 
-TsfEventRouter* TsfEventRouter::Create() {
-  return new TsfEventRouterImpl();
+  observer_ = NULL;
 }
-
 }  // namespace ui