Redesign: Remove TsfEventRouter interface.

ui/base/ime/win/tsf_event_router.h

 // Copyright (c) 2012 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 UI_BASE_IME_WIN_TSF_EVENT_ROUTER_H_
 #define UI_BASE_IME_WIN_TSF_EVENT_ROUTER_H_
 
 #include <msctf.h>
+#include <atlbase.h>
+#include <atlcom.h>

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

Nit: alphabetical order

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

Done.

+
+#include <set>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
+#include "base/win/scoped_comptr.h"
 #include "ui/base/ime/text_input_type.h"
 #include "ui/base/ui_export.h"
 
 struct ITfDocumentMgr;
 
 namespace ui {
 
-// This is an abstract interface that monitors events associated with TSF and
-// forwards them to the observer. In order to manage the life cycle of this
-// object by scoped_refptr and the implementation class of this interface is COM
-// class anyway, this interface is derived from IUnknown.
-class TsfEventRouter : public IUnknown {
+// 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.

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

This comment isn't correct, because TsfEventRouter doesn't implement either of
these interfaces.

Talk about how consumers use this; comments about the COM implementation can
happen on the declaration of the inner class.

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

Done.

+class UI_EXPORT TsfEventRouter {
  public:
   class Observer {

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

Nit: Use a separate TSFEventRouterObserver class instead of a member class
(since this is the pattern we're moving all Chrome code towards)

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

Done.

    public:
     virtual ~Observer() {}

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

Nit: Probably should be protected since I doubt we ever intend to delete via an
Observer*

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

Done.

 
     // Called when the text contents are updated.
     virtual void OnTextUpdated() = 0;
 
     // Called when the number of currently opened candidate windows changes.
     virtual void OnCandidateWindowCountChanged(size_t window_count) = 0;
   };
 
+  class ATL_NO_VTABLE TsfEventRouterDelegate : public ATL::CComObjectRoot,

[cpu_(ooo_6.6-7.5), 2012/10/24 17:40:14]

can you use CComObjectRootEx so it is clear what threading model you are using?

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

Can this class simply be forward-declared here and then get the actual
declaration in the .cc file?

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

Yes, we can, done.
On 2012/10/24 20:48:11, Peter Kasting wrote:

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

Done.

+                                               public ITfUIElementSink,
+                                               public ITfTextEditSink {
+   public:
+    BEGIN_COM_MAP(TsfEventRouterDelegate)
+      COM_INTERFACE_ENTRY(ITfUIElementSink)
+      COM_INTERFACE_ENTRY(ITfTextEditSink)
+    END_COM_MAP()
+
+    TsfEventRouterDelegate();
+    ~TsfEventRouterDelegate();
+
+    // ITfTextEditSink override.

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

Tiny nit: I've been trying to use this style:

  // ITfTextEditSink:
  ...

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

Done.

+    virtual STDMETHODIMP OnEndEdit(ITfContext* context,
+                                   TfEditCookie read_only_cookie,
+                                   ITfEditRecord* edit_record) OVERRIDE;
+
+    // ITfUiElementSink overrides.
+    virtual STDMETHODIMP BeginUIElement(DWORD element_id, BOOL* is_show);

[cpu_(ooo_6.6-7.5), 2012/10/24 17:48:14]

if we are going to use this style over STDMETHOD(xxxx)
then please convert the accessibility files using the other scheme
src/ui/base/win/accessibility_misc_utils.h

That can be done in a different CL.

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

Based on the chromium-dev thread about this, I think we should instead change
the code here to use the STDMETHOD(...) style as the accessibility code already
does.

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

Done.

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

Chose STDMETHOD_(HRESULT, hoge) as Peter suggested.
On 2012/10/24 17:48:14, cpu wrote:

+    virtual STDMETHODIMP UpdateUIElement(DWORD element_id);
+    virtual STDMETHODIMP EndUIElement(DWORD element_id);
+
+    // Sets |manager| to be monitored and |observer| to be notified. |manager|
+    // and |observer| can be NULL.
+    void SetManager(ITfThreadMgr* manager, Observer* observer);

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

Nit: Seems like this should be SetManagerAndObserver(), or else should be split
into two functions that each set one member.

I wonder whether this is really the right design anyway, though.  Maybe this
inner class should simply talk to the containing TsfEventRouter, leaving that
class responsible for relaying those messages to its consumers.

Maybe also there should be one TsfEventRouter for the whole browser process, and
it should support multiple observers.  We have an ObserverList class that can be
used to easily keep track of and notify interested parties.  Right now you
create at least one TsfEventRouter per window if not more (if there are
consumers outside the omnibox or will be in the future).  I don't know if that's
correct or not.

WDYT?

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

I removed |observer|.
Sure, changed the design.
TsfEventRouter should be one per ITfContext which is owned by RichEdit.
And currently there is (and will be in future) no consumer who use omnibox TSF
event, so I think it's not time to introduce ObserverList.

+
+    // Returns true if the IME is composing texts.

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

Nit: texts -> text

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

Done.

+    bool IsImeComposing();
+

[cpu_(ooo_6.6-7.5), 2012/10/24 17:40:14]

I don't see any overrides of CComObjectRoot, like FinalConstruct or
FinalRelease. But maybe that is what you want.

See the documentation on FinalConstruct in particular think if 
CComObject<TsfEventRouterDelegate>::CreateInstance is better done there.

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

If I understand correctly, this would require that the inner class be handed a
CComPtr* or something in its constructor so that FinalConstruct() could set the
right object, right?

What are the advantages of using the FinalConstruct() route?  It's not clear to
me from the MSDN docs what the best route is, when to use
DECLARE_PROTECT_FINAL_CONSTRUCT, etc.

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

Regarding FinalConstruct/FinalRelease, I introduced them.
Regarding CComPtr, I'm replying to .cc review comment.
On 2012/10/24 20:48:11, Peter Kasting wrote:

+   private:
+    // Returns true if the given |context| is in composing.

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

Nit: remove "in"

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

Done.

+    static bool IsImeComposingInternal(ITfContext* context);
+
+    // Returns true if the given |element_id| represents candidate window.

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

Nit: represents -> represents the

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

Done.

+    bool IsCandidateWindowInternal(DWORD element_id);
+
+    // Associates this class with specified |manager|.

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

Nit: with -> with the.  What about |observer|?

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

Done.

+    void Associate(ITfThreadMgr* thread_manager, Observer* observer);
+
+    // Resets the association, this function is safe to call if there is no

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

Nit: "association, this" -> "association.  This", "call if" -> "call even if"

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

Done.

+    // associated thread manager.
+    void EnsureDeassociated();
+
+    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.

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

Nit: Ui -> UI

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

Done.

+    base::win::ScopedComPtr<ITfUIElementMgr> ui_element_manager_;
+
+    // The ITfSouce associated with |ui_element_manager_|.
+    base::win::ScopedComPtr<ITfSource> ui_source_;
+
+    // The set of currently opend candidate window ids.

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

Nit: opend -> opened

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

Done.

+    std::set<DWORD> open_candidate_window_ids_;
+
+    // The cookie for |ui_source_|.
+    DWORD ui_source_cookie_;
+
+    DISALLOW_COPY_AND_ASSIGN(TsfEventRouterDelegate);
+  };
+
+  TsfEventRouter();
   virtual ~TsfEventRouter();
 
   // Sets |manager| to be monitored and |observer| to be notified. |manager| and
   // |observer| can be NULL.
-  virtual void SetManager(ITfThreadMgr* manager,
-                          Observer* observer) = 0;
+  void SetManager(ITfThreadMgr* manager, Observer* observer);
 
   // Returns true if the IME is composing texts.
-  virtual bool IsImeComposing() = 0;
+  bool IsImeComposing();
 
-  // Factory function, creates a new instance which the caller owns.
-  static UI_EXPORT TsfEventRouter* Create();
-
- protected:
-  // Create should be used instead.
-  TsfEventRouter();
+ private:
+  CComPtr<TsfEventRouterDelegate> delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(TsfEventRouter);
 };
 
 }  // namespace ui
 
 #endif  // UI_BASE_IME_WIN_TSF_EVENT_ROUTER_H_