Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11235023:
Redesign: Remove TsfEventRouter interface. (Closed)
|
Created:
8 years, 2 months ago by Seigo Nonaka Modified:
8 years, 1 month ago CC:
chromium-reviews, James Su, penghuang+watch_chromium.org, yusukes+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@ominifix Visibility:
Public. |
DescriptionRedesign: Remove TsfEventRouter interface.
I will rename all acronym from Tsf to TSF in the next change.
BUG=156867
TEST=Manually done on Win8
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164481
Patch Set 1 : #Patch Set 2 : Use ATL for COM object management #
Total comments: 68
Patch Set 3 : WIP #Patch Set 4 : WIP #Patch Set 5 : Patch for review #
Total comments: 4
Patch Set 6 : Address comments #
Total comments: 26
Patch Set 7 : Address comments #
Total comments: 12
Patch Set 8 : Address comments #Patch Set 9 : fix nit #Patch Set 10 : fix nit #
Messages
Total messages: 25 (0 generated)
lgtm
Yukawa: Thank you for your review! Adding pkasting as the owner of Omnibox.
Yukawa: Thank you for your review! Adding pkasting as the owner of Omnibox.
I'm still concerned that this is a ref-counted object (since you haven't changed any external consumers away from using scoped_refptr to refer to it). Not only that, the refcounting functions are implemented in a threadsafe fashion. We shouldn't be using this object across threads, should we? Do we really need to refcount this? It seems like "no" or "yes, but we could have a core class that's refcounted and stored as a member in the main class everyone external uses" would both be better answers. The latter is what I meant by "composition instead of inheritance". Note that "no" is the better answer of these two, if possible. You should get advice about these issues from someone who understands COM interfaces and how to implement them better than I do. Try cpu@ for starters.
OK, based on chromium-dev email thread, I suggest an outer class that is non-refcounted and provides the few APIs consumers need, which holds a CComObject* or a ScopedComPtr to an inner class that implements the COM APIs. I would use one class if consumers needed more sophisticated access to the inner workings, but since they don't, insulating them from the complexity and avoiding the need for ref-counting or specialized pointers seems like a win. Rather than defining your own AddRef/Release/etc., your inner class should inherit from CComObjectRootEx. Do a codesearch to see how that's used elsewhere in the codebase.
pkasting@: sure, thank you for your suggestions. This is the new version which uses ATL to implement COM class. Adding cpu@ as the COM expert. If you have any concerns about our usage of ATL, please let me know. Thank you.
https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... File ui/base/ime/win/tsf_event_router.h (right): https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:43: class ATL_NO_VTABLE TsfEventRouterDelegate : public ATL::CComObjectRoot, can you use CComObjectRootEx so it is clear what threading model you are using? https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:71: 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.
https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... File ui/base/ime/win/tsf_event_router.h (right): https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:61: virtual STDMETHODIMP BeginUIElement(DWORD element_id, BOOL* is_show); 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.
https://codereview.chromium.org/11235023/diff/15001/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11235023/diff/15001/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:485: ? new ui::TsfEventRouter() : NULL) { Nit: '?' goes on previous line https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... File ui/base/ime/win/tsf_event_router.cc (right): https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:22: &delegate))) { 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... https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:34: ui_source_cookie_(TF_INVALID_COOKIE), Nit: Indent even https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:39: STDMETHODIMP TsfEventRouter::TsfEventRouterDelegate::OnEndEdit( 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 ------------------------------------------------------------- ... https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:54: ranges.Receive()))) { Nit: {} unnecessary when body is one line (3 places) https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:55: return S_OK; // don't care about failures. Nit: Capitalize first word (3 places) https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:64: // updated texts. Nit: texts -> text https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:82: if (!observer_) Nit: Simpler: if (observer_ && insert_result.second) observer_->OnCandidateWindowCountChanged(open_candidate_window_ids_.size()); return S_OK; (Also remove blank line above this) https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:101: if (open_candidate_window_ids_.erase(element_id) == 0) Nit: Simpler: if ((open_candidate_window_ids_.erase(element_id) != 0) && observer_) observer_->OnCandidateWindowCountChanged(open_candidate_window_ids_.size()); return S_OK; https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:129: DCHECK(base::win::IsTsfAwareRequired()) Seems like this DCHECK belongs in the constructor instead of the individual API functions, no? (several places) https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:131: if (!context_) Nit: Simpler: return context_ && IsImeComposingInternal(context_); https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:141: DCHECK(context); 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. https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:147: enum_composition_view.Receive()))) Nit: indent 4, not 8 https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:150: return Nit: I suggest breaking as: return enum_composition_view->Next(1, composition_view.Receive(), NULL) == S_OK; https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:174: if (FAILED(thread_manager->GetFocus(document_manager.Receive()))) Nit: Can combine multiple FAILED() calls together in one statement with || https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... File ui/base/ime/win/tsf_event_router.h (right): https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:10: #include <atlcom.h> Nit: alphabetical order https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:29: // editting session is finished. 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. https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:32: class Observer { Nit: Use a separate TSFEventRouterObserver class instead of a member class (since this is the pattern we're moving all Chrome code towards) https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:34: virtual ~Observer() {} Nit: Probably should be protected since I doubt we ever intend to delete via an Observer* https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:43: class ATL_NO_VTABLE TsfEventRouterDelegate : public ATL::CComObjectRoot, Can this class simply be forward-declared here and then get the actual declaration in the .cc file? https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:55: // ITfTextEditSink override. Tiny nit: I've been trying to use this style: // ITfTextEditSink: ... https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:61: virtual STDMETHODIMP BeginUIElement(DWORD element_id, BOOL* is_show); On 2012/10/24 17:48:14, cpu wrote: > 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 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. https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:67: void SetManager(ITfThreadMgr* manager, Observer* observer); 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? https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:69: // Returns true if the IME is composing texts. Nit: texts -> text https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:71: On 2012/10/24 17:40:14, cpu wrote: > 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. 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. https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:73: // Returns true if the given |context| is in composing. Nit: remove "in" https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:76: // Returns true if the given |element_id| represents candidate window. Nit: represents -> represents the https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:79: // Associates this class with specified |manager|. Nit: with -> with the. What about |observer|? https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:82: // Resets the association, this function is safe to call if there is no Nit: "association, this" -> "association. This", "call if" -> "call even if" https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:97: // A UiElementMgr associated with this class. Nit: Ui -> UI https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:103: // The set of currently opend candidate window ids. Nit: opend -> opened
Regarding STDMETHOD() I am fine either way, as long things are at least consistent in a given directory.
https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... File ui/base/ime/win/tsf_event_router.cc (right): https://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:22: &delegate))) { On 2012/10/24 20:48:11, Peter Kasting wrote: > 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... 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.
"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?" not sure I get the issue, is just a virtual function that is called by atl after the object is fully constructed in the COM sense but before the reference is given to the caller of CoCreateIntance. In other words, after windows finds your CoClass (aka factory) via magic this is what more or less happens inside CoCreateInstance. CComObject<Foo>* foo = new CComObject<Foo>(); foo->AddRef(); hresult hr = foo->FinalConstruct(); (actually I don't know if AddRef happens before or after finalconstruct). "What are the advantages of using the FinalConstruct() route? It's not" The main one is that it allows you to fail CoCreateInstance and return a meaningful hresult. It is useful when your class in turns CoCreates other objects, aggregated or contained or whatnot.
On 2012/10/25 00:43:29, cpu wrote: > 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. FWIW, MSDN basically says that whenever you use a CComPtr, you should create the original object using CComObject::CreateInstance() and then explicitly AddRef(). This is apparently more efficient than CoCreateInstance(). If we went to CoCreateInstance() then I'd say to go further and switch to ScopedComPtr::CreateInstance(). But sticking with CComPtr and just explicitly AddRef()ing seems OK to me.
http://codereview.chromium.org/11235023/diff/15001/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): http://codereview.chromium.org/11235023/diff/15001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:485: ? new ui::TsfEventRouter() : NULL) { On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: '?' goes on previous line Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... File ui/base/ime/win/tsf_event_router.cc (right): http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:22: &delegate))) { 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: > On 2012/10/24 20:48:11, Peter Kasting wrote: > > 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... > > 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. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:34: ui_source_cookie_(TF_INVALID_COOKIE), On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: Indent even Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:39: STDMETHODIMP TsfEventRouter::TsfEventRouterDelegate::OnEndEdit( On 2012/10/24 20:48:11, Peter Kasting wrote: > 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 ------------------------------------------------------------- > > ... Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:54: ranges.Receive()))) { On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: {} unnecessary when body is one line (3 places) Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:55: return S_OK; // don't care about failures. On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: Capitalize first word (3 places) Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:64: // updated texts. On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: texts -> text Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:82: if (!observer_) On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: Simpler: > > if (observer_ && insert_result.second) > observer_->OnCandidateWindowCountChanged(open_candidate_window_ids_.size()); > return S_OK; > > (Also remove blank line above this) Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:101: if (open_candidate_window_ids_.erase(element_id) == 0) On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: Simpler: > > if ((open_candidate_window_ids_.erase(element_id) != 0) && observer_) > observer_->OnCandidateWindowCountChanged(open_candidate_window_ids_.size()); > return S_OK; Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:129: DCHECK(base::win::IsTsfAwareRequired()) Yes, removed. On 2012/10/24 20:48:11, Peter Kasting wrote: > Seems like this DCHECK belongs in the constructor instead of the individual API > functions, no? (several places) http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:131: if (!context_) On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: Simpler: > > return context_ && IsImeComposingInternal(context_); Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:141: DCHECK(context); Sure, removing all blank line after DCHECK. On 2012/10/24 20:48:11, Peter Kasting wrote: > 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. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:147: enum_composition_view.Receive()))) On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: indent 4, not 8 Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:150: return Sure, done. On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: I suggest breaking as: > > return enum_composition_view->Next(1, composition_view.Receive(), > NULL) == S_OK; http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:174: if (FAILED(thread_manager->GetFocus(document_manager.Receive()))) On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: Can combine multiple FAILED() calls together in one statement with || Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... File ui/base/ime/win/tsf_event_router.h (right): http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:10: #include <atlcom.h> On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: alphabetical order Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:29: // editting session is finished. On 2012/10/24 20:48:11, Peter Kasting wrote: > 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. Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:32: class Observer { On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: Use a separate TSFEventRouterObserver class instead of a member class > (since this is the pattern we're moving all Chrome code towards) Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:34: virtual ~Observer() {} On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: Probably should be protected since I doubt we ever intend to delete via an > Observer* Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:43: class ATL_NO_VTABLE TsfEventRouterDelegate : public ATL::CComObjectRoot, Yes, we can, done. On 2012/10/24 20:48:11, Peter Kasting wrote: > Can this class simply be forward-declared here and then get the actual > declaration in the .cc file? http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:43: class ATL_NO_VTABLE TsfEventRouterDelegate : public ATL::CComObjectRoot, On 2012/10/24 17:40:14, cpu wrote: > can you use CComObjectRootEx so it is clear what threading model you are using? Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:55: // ITfTextEditSink override. On 2012/10/24 20:48:11, Peter Kasting wrote: > Tiny nit: I've been trying to use this style: > > // ITfTextEditSink: > ... Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:61: virtual STDMETHODIMP BeginUIElement(DWORD element_id, BOOL* is_show); On 2012/10/24 20:48:11, Peter Kasting wrote: > On 2012/10/24 17:48:14, cpu wrote: > > 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 > > 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. Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:61: virtual STDMETHODIMP BeginUIElement(DWORD element_id, BOOL* is_show); Chose STDMETHOD_(HRESULT, hoge) as Peter suggested. On 2012/10/24 17:48:14, cpu wrote: > 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. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:67: void SetManager(ITfThreadMgr* manager, Observer* observer); On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: Seems like this should be SetManagerAndObserver(), or else should be split > into two functions that each set one member. > I removed |observer|. > 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. Sure, changed the design. > 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? 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. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:69: // Returns true if the IME is composing texts. On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: texts -> text Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:71: 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: > On 2012/10/24 17:40:14, cpu wrote: > > 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. > > 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. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:73: // Returns true if the given |context| is in composing. On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: remove "in" Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:76: // Returns true if the given |element_id| represents candidate window. On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: represents -> represents the Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:79: // Associates this class with specified |manager|. On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: with -> with the. What about |observer|? Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:82: // Resets the association, this function is safe to call if there is no On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: "association, this" -> "association. This", "call if" -> "call even if" Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:97: // A UiElementMgr associated with this class. On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: Ui -> UI Done. http://codereview.chromium.org/11235023/diff/15001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:103: // The set of currently opend candidate window ids. On 2012/10/24 20:48:11, Peter Kasting wrote: > Nit: opend -> opened Done.
https://codereview.chromium.org/11235023/diff/24001/ui/base/ime/win/tsf_event... File ui/base/ime/win/tsf_event_router.cc (right): https://codereview.chromium.org/11235023/diff/24001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:114: return S_OK; if (!base::win::IsTsfAwareRequired()) return some_error; if (!delegate_) return some_error; But my preference would be to do lines 261-264 here. https://codereview.chromium.org/11235023/diff/24001/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:259: : observer_(observer) { , delegate_(NULL)
http://codereview.chromium.org/11235023/diff/24001/ui/base/ime/win/tsf_event_... File ui/base/ime/win/tsf_event_router.cc (right): http://codereview.chromium.org/11235023/diff/24001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:114: return S_OK; Sure, move DCHECK TsfEventRoter::TsfEventRouter() On 2012/10/25 19:30:42, cpu wrote: > if (!base::win::IsTsfAwareRequired()) > return some_error; > if (!delegate_) > return some_error; > > > But my preference would be to do lines 261-264 here. http://codereview.chromium.org/11235023/diff/24001/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:259: : observer_(observer) { On 2012/10/25 19:30:42, cpu wrote: > , delegate_(NULL) Done.
OK, pretty close now! https://codereview.chromium.org/11235023/diff/12002/chrome/browser/ui/views/o... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): https://codereview.chromium.org/11235023/diff/12002/chrome/browser/ui/views/o... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:559: tsf_event_router_->SetManager(NULL); Is this necessary? Can't we just let the TsfEventRouter destructor do this automatically? https://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event... File ui/base/ime/win/tsf_event_router.cc (right): https://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:34: DECLARE_NOT_AGGREGATABLE(TsfEventRouterDelegate); 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. https://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:60: // Sets |router| to be forward TSF related events. Nit: forward TSF related -> forwarded TSF-related https://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:70: // Associates this class with the specified |manager|. Nit: manager -> thread_manager https://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:238: void TsfEventRouter::TsfEventRouterDelegate::EnsureDeassociated() { Nit: Only called once, just inline this function into SetManager() https://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:255: // TsfEventRouter ------------------------------------------------------------ Nit: Add another blank line on both sides of this line https://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.cc:274: delegate_ = NULL; Once you switch back to CComPtr, these last two lines should go away. https://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event... File ui/base/ime/win/tsf_event_router.h (right): https://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:45: explicit TsfEventRouter(TsfEventRouterObserver* observer); Nit: Say whether |observer| can be NULL. https://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:48: // Sets |manager| to be monitored. |manager| and can be NULL. Nit: Remove "and" https://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:51: // Returns true if the IME is composing texts. Nit: texts -> text https://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:54: // Called when the text contents are updated. Nit: Called by whom? (2 places) https://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:57: // Called when the number of currently opend candidate window changes. Nit: opend -> opened https://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event... ui/base/ime/win/tsf_event_router.h:63: CComObject<TsfEventRouterDelegate>* delegate_; This should use CComPtr like you used to, not CComObject*. That way you'll still have to manually AddRef(), but you won't need to Release().
http://codereview.chromium.org/11235023/diff/12002/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): http://codereview.chromium.org/11235023/diff/12002/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:559: tsf_event_router_->SetManager(NULL); Yes! done. On 2012/10/26 01:53:47, Peter Kasting wrote: > Is this necessary? Can't we just let the TsfEventRouter destructor do this > automatically? http://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event_... File ui/base/ime/win/tsf_event_router.cc (right): http://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:34: DECLARE_NOT_AGGREGATABLE(TsfEventRouterDelegate); Sure, thank you! removing macros. On 2012/10/26 01:53:47, Peter Kasting wrote: > 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. http://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:60: // Sets |router| to be forward TSF related events. On 2012/10/26 01:53:47, Peter Kasting wrote: > Nit: forward TSF related -> forwarded TSF-related Done. http://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:70: // Associates this class with the specified |manager|. On 2012/10/26 01:53:47, Peter Kasting wrote: > Nit: manager -> thread_manager Done. http://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:238: void TsfEventRouter::TsfEventRouterDelegate::EnsureDeassociated() { Sure, done and also done for Associate which is also called only once, On 2012/10/26 01:53:47, Peter Kasting wrote: > Nit: Only called once, just inline this function into SetManager() http://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:255: // TsfEventRouter ------------------------------------------------------------ On 2012/10/26 01:53:47, Peter Kasting wrote: > Nit: Add another blank line on both sides of this line Done. http://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:274: delegate_ = NULL; On 2012/10/26 01:53:47, Peter Kasting wrote: > Once you switch back to CComPtr, these last two lines should go away. Done. http://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event_... File ui/base/ime/win/tsf_event_router.h (right): http://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:45: explicit TsfEventRouter(TsfEventRouterObserver* observer); On 2012/10/26 01:53:47, Peter Kasting wrote: > Nit: Say whether |observer| can be NULL. Done. http://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:48: // Sets |manager| to be monitored. |manager| and can be NULL. On 2012/10/26 01:53:47, Peter Kasting wrote: > Nit: Remove "and" Done. http://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:51: // Returns true if the IME is composing texts. On 2012/10/26 01:53:47, Peter Kasting wrote: > Nit: texts -> text Done. http://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:54: // Called when the text contents are updated. On 2012/10/26 01:53:47, Peter Kasting wrote: > Nit: Called by whom? (2 places) Done. http://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:57: // Called when the number of currently opend candidate window changes. On 2012/10/26 01:53:47, Peter Kasting wrote: > Nit: opend -> opened Done. http://codereview.chromium.org/11235023/diff/12002/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:63: CComObject<TsfEventRouterDelegate>* delegate_; Sure, done. On 2012/10/26 01:53:47, Peter Kasting wrote: > This should use CComPtr like you used to, not CComObject*. That way you'll > still have to manually AddRef(), but you won't need to Release().
LGTM http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... File ui/base/ime/win/tsf_event_router.cc (right): http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:17: Tiny nit: I suggest one more blank line above each of the two divider comments, just to set each section off more from the stuff above it. http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:19: // The implementation class of ITfUIElementSink, whose member functions will be Nit: Add blank line here http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:48: void SetManager(ITfThreadMgr* thread_manager); Nit: I suggest using the same parameter name in this function and TsfEventRouter::SetManager(). (I don't care which one you use.) http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... File ui/base/ime/win/tsf_event_router.h (right): http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:45: Nit: Remove this blank line http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:46: // |observer| can be NULL. Nit: Say why this is important, e.g. does test code need to pass NULL here? If it's never used, then ban passing NULL. http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:57: // |delegate_|. Nit: Slightly less awkward: // Called by the TsfEventRouterDelegate when the text contents are updated. Or, do this: // Callbacks from the TSfEventRouterDelegate: void OnTextUpdated(); void OnCandidateWindowCountChanged(); This might be even better as the function names are already pretty clear as to when they're reached.
pkasting@: Thank you for your kindly review! cpu@: PTAL? http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... File ui/base/ime/win/tsf_event_router.cc (right): http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:17: On 2012/10/26 02:31:57, Peter Kasting wrote: > Tiny nit: I suggest one more blank line above each of the two divider comments, > just to set each section off more from the stuff above it. Done. http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:19: // The implementation class of ITfUIElementSink, whose member functions will be On 2012/10/26 02:31:57, Peter Kasting wrote: > Nit: Add blank line here Done. http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.cc:48: void SetManager(ITfThreadMgr* thread_manager); On 2012/10/26 02:31:57, Peter Kasting wrote: > Nit: I suggest using the same parameter name in this function and > TsfEventRouter::SetManager(). (I don't care which one you use.) Done. http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... File ui/base/ime/win/tsf_event_router.h (right): http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:45: On 2012/10/26 02:31:57, Peter Kasting wrote: > Nit: Remove this blank line Done. http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:46: // |observer| can be NULL. On 2012/10/26 02:31:57, Peter Kasting wrote: > Nit: Say why this is important, e.g. does test code need to pass NULL here? If > it's never used, then ban passing NULL. Done. http://codereview.chromium.org/11235023/diff/20015/ui/base/ime/win/tsf_event_... ui/base/ime/win/tsf_event_router.h:57: // |delegate_|. Changed to the latter, thanks. On 2012/10/26 02:31:57, Peter Kasting wrote: > Nit: Slightly less awkward: > > // Called by the TsfEventRouterDelegate when the text contents are updated. > > Or, do this: > > // Callbacks from the TSfEventRouterDelegate: > void OnTextUpdated(); > void OnCandidateWindowCountChanged(); > > This might be even better as the function names are already pretty clear as to > when they're reached.
lgtm
Thanks! committing.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nona@chromium.org/11235023/23017
Change committed as 164481 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
