Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10826223:
Replace PeekMessage for TSF awareness (Closed)
|
Created:
8 years, 4 months ago by yoichio Modified:
8 years, 3 months ago Reviewers:
sky, cpu_(ooo_6.6-7.5), yukawa, rvargas (doing something else), darin (slow to review), jar (doing other things) CC:
chromium-reviews, erikwright (departed), sadrul, brettw-cc_chromium.org, Seigo Nonaka, horo, Hiro Komatsu, ananta, robert Base URL:
http://git.chromium.org/chromium/src.git@yukawa Visibility:
Public. |
DescriptionReplace PeekMessage for TSF awareness
Replace PeekMessage with TSF interface's one in MessagePumpForIO
Add MessagePumpTSFInternal class for above replacing.
BUG=137627
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155589
Patch Set 1 : refactor #Patch Set 2 : merge from metro.cc #Patch Set 3 : modify member name #
Total comments: 6
Patch Set 4 : update names #
Total comments: 2
Patch Set 5 : modify indentation #
Total comments: 51
Patch Set 6 : move TextServiceBridge to new file #Patch Set 7 : add MessageFilter interface #
Total comments: 22
Patch Set 8 : add comments #
Total comments: 8
Patch Set 9 : rename and refactor #Patch Set 10 : delete unused file #Patch Set 11 : rename class name and rebirth ScopedComInitialiser #Patch Set 12 : change BOOL to bool, rename variables. #
Total comments: 8
Patch Set 13 : remove ScopedComInitializer #Patch Set 14 : injects TextServicesMessageFilter from ChromeBrowserMainPartsWin #Patch Set 15 : use scoped_ptr #Patch Set 16 : use just PeekMessage for WM_PAINT/TIMER messages #
Total comments: 15
Patch Set 17 : use PeekMessage for PumpOutPendingPaintMessages/WaitForWork #
Total comments: 5
Patch Set 18 : check MessageFilter::Init() out of DCHECK #
Total comments: 1
Patch Set 19 : update comments for MessageFilter::ProcessMessage #
Total comments: 9
Patch Set 20 : remove Init() from MessageFilter and some nitpicks #Patch Set 21 : nitpicks #Patch Set 22 : sync #
Total comments: 2
Patch Set 23 : nitpick #
Messages
Total messages: 53 (0 generated)
I refactored TSF stuff class.
update
http://codereview.chromium.org/10826223/diff/7004/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/7004/base/message_pump_win.h#new... base/message_pump_win.h:128: class MessagePumpTSFInternal; Any ideas for this class name? http://codereview.chromium.org/10826223/diff/7004/base/message_pump_win.h#new... base/message_pump_win.h:158: bool MessagePumpForUI::PeekMessageInternal( How about ProcessPeekMessage? http://codereview.chromium.org/10826223/diff/7004/base/message_pump_win.h#new... base/message_pump_win.h:159: MSG* msg, HWND hwnd, UINT wmin, UINT wmax, UINT wmsg); nit: indent.
http://codereview.chromium.org/10826223/diff/7004/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/7004/base/message_pump_win.h#new... base/message_pump_win.h:128: class MessagePumpTSFInternal; On 2012/08/15 09:10:49, Yohei Yukawa wrote: > Any ideas for this class name? Done. http://codereview.chromium.org/10826223/diff/7004/base/message_pump_win.h#new... base/message_pump_win.h:158: bool MessagePumpForUI::PeekMessageInternal( On 2012/08/15 09:10:49, Yohei Yukawa wrote: > How about ProcessPeekMessage? Done. http://codereview.chromium.org/10826223/diff/7004/base/message_pump_win.h#new... base/message_pump_win.h:159: MSG* msg, HWND hwnd, UINT wmin, UINT wmax, UINT wmsg); On 2012/08/15 09:10:49, Yohei Yukawa wrote: > nit: indent. Done.
http://codereview.chromium.org/10826223/diff/3004/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/3004/base/message_pump_win.cc#ne... base/message_pump_win.cc:504: ProcessPeekMessage(&msg, NULL, WM_PAINT, WM_PAINT, PM_REMOVE) || nit: indent.
http://codereview.chromium.org/10826223/diff/3004/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/3004/base/message_pump_win.cc#ne... base/message_pump_win.cc:504: ProcessPeekMessage(&msg, NULL, WM_PAINT, WM_PAINT, PM_REMOVE) || On 2012/08/16 02:54:46, Yohei Yukawa wrote: > nit: indent. Done.
lgtm
Could you please take a look?
Add cpu@chromium.org as a metro owner.
http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:34: base::win::ScopedComPtr<ITfKeystrokeMgr> keystroke_mgr; nit: members come after methods. Members should have a postfix "_" since they are in a class. Members should all be private. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:36: : client_id(TF_CLIENTID_NULL), is_initialized(true) { nit: one initializer per line (once you had to wrap). http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:79: UINT wMsgFilterMax, UINT wRemoveMsg) { nit: one arg per line. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:87: wRemoveMsg, &result))) nit: use curly braces since the "if" line took two lines. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:91: }; nit: please add DISALLOW_COPY_AND_ASSIGN(...); Also, it appears that the associated header file in this CL is missing this from all the classes. Please add it, or add a comment explaining why it can't be done (usually, because of use in an STL container... but I don't think that happens to these classes). http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:172: text_services_bridge_.reset(new TextServicesBridge); Once you set the text_service_bridge_ to have a pointer, I think you should stop calling IsTsfAwareRequired() and start just testing text_service_bridge_.get() for non-NULLness. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:177: text_services_bridge_.reset(NULL); Why don't you count on scoped ptr handling this delete? Is the order critical?? ...and if so, adding a comment would be helpful. Also, even if the order is critical (delete before lines 178), why not just do the reset() without the test? I'm assuming TSF awareness doesn't change during the lifetime of the message loop. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:260: if (!ProcessPeekMessage(&msg, NULL, 0, 0, PM_REMOVE | PM_QS_PAINT)) Why are you asking for help from TSF in this case? This is *ONLY* supposed to handle PAINT messages, and not handle generic other messages. Is there another queue that must be service here, and is hidden of fis TSF land? http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:374: // intended for the child window. Occurs if the child window has capture) nit: try to make a sentence out of the text in the parens. It looks like two fragments. Suggest something like: Specifically, mouse messages intended for the child window may appear if the child window has capture. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:375: // The subsequent PeekMessages call fails to return any messages thus nit: "call fails" --> "call may fail" http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:378: // sometime to process its input messages. nit: sometime --> some time http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:467: if (base::win::IsTsfAwareRequired()) { Please change to: if (text_services_bridge_.get()) http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:530: MSG* pMsg, HWND hwnd, UINT wMsgFilterMin, UINT wMsgFilterMax, nit: one arg per line, wrap at paren is preferred. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:533: if (base::win::IsTsfAwareRequired()) Please change to: if (text_services_bridge_.get()) http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.h#ne... base/message_pump_win.h:158: bool ProcessPeekMessage(MSG* pMsg, HWND hwnd, UINT wMsgFilterMin, nit: one arg per line (unless args are "intimately" related). Line 149 should be fixed as well (in both cases, wrapping to the paren is preferred).
Sorry the TSF code here seems weird, let me add another code reviewer, Ricardo.
I feel like having explicit TSF references here is not good. How about defining a more general purpose MessageFilter interface that the pump always invoke (with a default one that just forwards to the regular win32 API)? http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:28: class TextServicesBridge { In any case, this doesn't belong to this file. It should be on a dedicated file. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:36: : client_id(TF_CLIENTID_NULL), is_initialized(true) { There is no point in having is_initialized in the initialization list, especially setting it to true. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:37: is_initialized &= SUCCEEDED(thread_mgr.CreateInstance(CLSID_TF_ThreadMgr)); There is no guarantee so far that a MessagePump can use com. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:38: if (!thread_mgr) This is a case of checking two different signals for fn success. You should use is_initialized (the actual return value from the call) at this point. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:47: if (thread_mgr && client_id != TF_CLIENTID_NULL) if (is_initialized_) http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:51: bool ForwardKeyMessageForTSF(const MSG& msg) { This method requires a description. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:80: if (!is_initialized) braces http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:82: return !!::PeekMessage(msg, hwnd, wMsgFilterMin, wMsgFilterMax, If this method is replacing a windows API it should have the same signature (including the return type) http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:469: text_services_bridge_->ForwardKeyMessageForTSF(msg); PreProcessMessage(msg) ? http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:533: if (base::win::IsTsfAwareRequired()) braces http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.h#ne... base/message_pump_win.h:128: class TextServicesBridge; You are inserting this between a class and its documentation.
TextServicesBridge was moved to base/win/text_services_framework.h. To create interface that peeks message is beautiful way but we need process message in ITfStrokeMgr after peeking message. And if add above interface, how to insert implemented instance? http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:28: class TextServicesBridge { On 2012/08/18 02:42:07, rvargas wrote: > In any case, this doesn't belong to this file. It should be on a dedicated file. Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:34: base::win::ScopedComPtr<ITfKeystrokeMgr> keystroke_mgr; On 2012/08/16 18:21:40, jar wrote: > nit: members come after methods. > Members should have a postfix "_" since they are in a class. Members should all > be private. Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:36: : client_id(TF_CLIENTID_NULL), is_initialized(true) { On 2012/08/18 02:42:07, rvargas wrote: > There is no point in having is_initialized in the initialization list, > especially setting it to true. Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:37: is_initialized &= SUCCEEDED(thread_mgr.CreateInstance(CLSID_TF_ThreadMgr)); On 2012/08/18 02:42:07, rvargas wrote: > There is no guarantee so far that a MessagePump can use com. So I added ScopedComInitialiser. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:38: if (!thread_mgr) On 2012/08/18 02:42:07, rvargas wrote: > This is a case of checking two different signals for fn success. You should use > is_initialized (the actual return value from the call) at this point. Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:38: if (!thread_mgr) On 2012/08/18 02:42:07, rvargas wrote: > This is a case of checking two different signals for fn success. You should use > is_initialized (the actual return value from the call) at this point. Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:47: if (thread_mgr && client_id != TF_CLIENTID_NULL) On 2012/08/18 02:42:07, rvargas wrote: > if (is_initialized_) Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:51: bool ForwardKeyMessageForTSF(const MSG& msg) { On 2012/08/18 02:42:07, rvargas wrote: > This method requires a description. Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:79: UINT wMsgFilterMax, UINT wRemoveMsg) { On 2012/08/16 18:21:40, jar wrote: > nit: one arg per line. Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:80: if (!is_initialized) On 2012/08/18 02:42:07, rvargas wrote: > braces Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:82: return !!::PeekMessage(msg, hwnd, wMsgFilterMin, wMsgFilterMax, On 2012/08/18 02:42:07, rvargas wrote: > If this method is replacing a windows API it should have the same signature > (including the return type) Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:87: wRemoveMsg, &result))) On 2012/08/16 18:21:40, jar wrote: > nit: use curly braces since the "if" line took two lines. Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:91: }; On 2012/08/16 18:21:40, jar wrote: > nit: please add DISALLOW_COPY_AND_ASSIGN(...); > > Also, it appears that the associated header file in this CL is missing this from > all the classes. Please add it, or add a comment explaining why it can't be > done (usually, because of use in an STL container... but I don't think that > happens to these classes). Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:172: text_services_bridge_.reset(new TextServicesBridge); On 2012/08/16 18:21:40, jar wrote: > Once you set the text_service_bridge_ to have a pointer, I think you should stop > calling IsTsfAwareRequired() and start just testing text_service_bridge_.get() > for non-NULLness. Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:177: text_services_bridge_.reset(NULL); On 2012/08/16 18:21:40, jar wrote: > Why don't you count on scoped ptr handling this delete? Is the order critical?? > ...and if so, adding a comment would be helpful. > > Also, even if the order is critical (delete before lines 178), why not just do > the reset() without the test? > > I'm assuming TSF awareness doesn't change during the lifetime of the message > loop. Oops! I overlooked scoped_ptr's basic function. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:260: if (!ProcessPeekMessage(&msg, NULL, 0, 0, PM_REMOVE | PM_QS_PAINT)) On 2012/08/16 18:21:40, jar wrote: > Why are you asking for help from TSF in this case? > > This is *ONLY* supposed to handle PAINT messages, and not handle generic other > messages. Is there another queue that must be service here, and is hidden of > fis TSF land? I don't know what kind of message is peeked from ITfMessagePump::PeekMessage and not so I just replaced all. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:374: // intended for the child window. Occurs if the child window has capture) On 2012/08/16 18:21:40, jar wrote: > nit: try to make a sentence out of the text in the parens. It looks like two > fragments. Suggest something like: > > Specifically, mouse messages intended for the child window may appear if the > child window has capture. Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:375: // The subsequent PeekMessages call fails to return any messages thus On 2012/08/16 18:21:40, jar wrote: > nit: "call fails" --> "call may fail" Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:378: // sometime to process its input messages. On 2012/08/16 18:21:40, jar wrote: > nit: sometime --> some time Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:467: if (base::win::IsTsfAwareRequired()) { On 2012/08/16 18:21:40, jar wrote: > Please change to: > if (text_services_bridge_.get()) Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:530: MSG* pMsg, HWND hwnd, UINT wMsgFilterMin, UINT wMsgFilterMax, On 2012/08/16 18:21:40, jar wrote: > nit: one arg per line, wrap at paren is preferred. Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:533: if (base::win::IsTsfAwareRequired()) On 2012/08/18 02:42:07, rvargas wrote: > braces Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.cc#n... base/message_pump_win.cc:533: if (base::win::IsTsfAwareRequired()) On 2012/08/16 18:21:40, jar wrote: > Please change to: > if (text_services_bridge_.get()) Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.h#ne... base/message_pump_win.h:128: class TextServicesBridge; On 2012/08/18 02:42:07, rvargas wrote: > You are inserting this between a class and its documentation. Done. http://codereview.chromium.org/10826223/diff/10002/base/message_pump_win.h#ne... base/message_pump_win.h:158: bool ProcessPeekMessage(MSG* pMsg, HWND hwnd, UINT wMsgFilterMin, On 2012/08/16 18:21:40, jar wrote: > nit: one arg per line (unless args are "intimately" related). > > Line 149 should be fixed as well (in both cases, wrapping to the paren is > preferred). Done.
How about:
// blah
class MessageFilter {
public:
virtual ~MessageFilter() {}
virtual bool Init() = 0;
// blah... Returns true if the message was completely handled by the filter.
virtual bool ProcessMessage(const MSG& message) = 0;
// Implements the functionality exposed by the OS through PeekMessage.
virtual BOOL DoPeekMessage(...) = 0;
};
-----
class SimpleMessageFilter : public MessagePumpForUI::MessageFilter {
public:
virtual ~SimpleMessageFilter() {}
virtual bool Init() OVERRIDE {
return true;
}
virtual bool ProcessMessage(const MSG& message) OVERRIDE {
return false;
}
virtual BOOL DoPeekMessage(...) OVERRIDE {
return PeekMesssage(...);
}
};
And the message pump always calls a filter.
BTW, I would expect unit tests to be able top run without TSF on any platform.
And that reminds me, unit tests?
http://codereview.chromium.org/10826223/diff/1007/base/base.gypi
File base/base.gypi (right):
http://codereview.chromium.org/10826223/diff/1007/base/base.gypi#newcode513
base/base.gypi:513: 'win/text_services_framework.cc',
alpha ordered
http://codereview.chromium.org/10826223/diff/1007/base/message_pump_win.cc
File base/message_pump_win.cc (right):
http://codereview.chromium.org/10826223/diff/1007/base/message_pump_win.cc#ne...
base/message_pump_win.cc:16: namespace base {
nit: keep the namespace where it was (the anonymous namespace outside of base).
http://codereview.chromium.org/10826223/diff/1007/base/message_pump_win.cc#ne...
base/message_pump_win.cc:466: msg, hwnd, msg_filter_min, msg_filter_max,
remove_msg);
nit: this style is not actually supported by the style guide. the first three
args go into the previous line, and the last three on the second line, under the
first arg.
http://codereview.chromium.org/10826223/diff/1007/base/win/text_services_fram...
File base/win/text_services_framework.cc (right):
http://codereview.chromium.org/10826223/diff/1007/base/win/text_services_fram...
base/win/text_services_framework.cc:10: TextServicesBridge::TextServicesBridge()
:
":" goes on the next line.
http://codereview.chromium.org/10826223/diff/1007/base/win/text_services_fram...
File base/win/text_services_framework.h (right):
http://codereview.chromium.org/10826223/diff/1007/base/win/text_services_fram...
base/win/text_services_framework.h:19: class TextServicesBridge {
The name of the file should match the name of the class.
http://codereview.chromium.org/10826223/diff/1007/base/win/text_services_fram...
base/win/text_services_framework.h:24: // This method send msg to Text Service
Manager.
nit: remove "this method". send -> sends
http://codereview.chromium.org/10826223/diff/1007/base/win/text_services_fram...
base/win/text_services_framework.h:26: // If it was consumed, return true.
nit: if "it" is the message, this sentence cannot be on a separate paragraph
from the previous sentence. return -> returns.
Returns true if |msg| was consumed by TSF. (or "by the test service manager")
http://codereview.chromium.org/10826223/diff/1007/base/win/text_services_fram...
base/win/text_services_framework.h:29: BOOL PeekMessageForTSF(MSG* msg,
Requires documentation.
http://codereview.chromium.org/10826223/diff/1007/base/win/text_services_fram...
base/win/text_services_framework.h:38:
scoped_ptr<base::win::ScopedCOMInitializer> scoped_com_initialiser_;
Why the scoped_ptr ?
Add MessageFilter interface which wraps PeekMessage. Rename base/win/text_services_framework.* to base/win/text_services_bridge.*. About message pump unit tests, there are already in message_loop_unittest.cc. About ones for win8 metro style environment in where base::win::IsTsfAwareRequired() returns true, message_loop_unittest does it when it runs with such a option. http://codereview.chromium.org/10826223/diff/1007/base/base.gypi File base/base.gypi (right): http://codereview.chromium.org/10826223/diff/1007/base/base.gypi#newcode513 base/base.gypi:513: 'win/text_services_framework.cc', On 2012/08/20 22:24:12, rvargas wrote: > alpha ordered Done. http://codereview.chromium.org/10826223/diff/1007/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/1007/base/message_pump_win.cc#ne... base/message_pump_win.cc:16: namespace base { On 2012/08/20 22:24:12, rvargas wrote: > nit: keep the namespace where it was (the anonymous namespace outside of base). Done.
What I meant by unit tests is that current tests are verifying the existing functionality... this is new functionality that probably needs to be unit tested by something else than running the current test on a system that happens to go through TSF and do nothing. Maybe that's good enough (I don't have suggestions about what/how to test), but I'd like to hear what other reviewers think. http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.cc#ne... base/message_pump_win.cc:32: HWND hwnd, Indent under the first arg http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.cc#ne... base/message_pump_win.cc:124: message_filter_.reset(new SimpleMessageFilter); + DCHECK Init() http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.cc#ne... base/message_pump_win.cc:419: if (state_->dispatcher) { Are you sure this is the correct behavior when the pump has a dispatcher?. This ignores the dispatcher and that probably breaks nested loops. http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h#new... base/message_pump_win.h:24: class TextServicesBridge; remove this. http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h#new... base/message_pump_win.h:133: // Interface gets message and may process it. How about: A MessageFilter implements the common Peek/Translate/Dispatch code to deal with windows messages. http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h#new... base/message_pump_win.h:139: virtual BOOL DoPeekMessage(MSG* msg, nit: message http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h#new... base/message_pump_win.h:144: // Returns true if |message| was completely consumed by the filter and It is not clear what is expected from an implementation of this interface. http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h#new... base/message_pump_win.h:146: virtual bool ProcessMessage(const MSG& msg) = 0; nit: message http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h#new... base/message_pump_win.h:146: virtual bool ProcessMessage(const MSG& msg) = 0; On a more serious note, the interaction between a MessageFilter and a MessagePumpDispatcher should be documented somewhere... who has priority, given that both implement similar functionality (which is a bad thing by itself, but they serve two quite different purposes). http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h#new... base/message_pump_win.h:184: // A Proxy which gets message. nit: remove comment http://codereview.chromium.org/10826223/diff/2007/base/win/text_services_brid... File base/win/text_services_bridge.h (right): http://codereview.chromium.org/10826223/diff/2007/base/win/text_services_brid... base/win/text_services_bridge.h:26: // Obtains messages from application message queue. These comments belong to the cc file. This is just the implementation of an interface, and the interface is documented somewhere else.
Add comments about ProcessMessage. http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.cc#ne... base/message_pump_win.cc:32: HWND hwnd, On 2012/08/21 19:45:37, rvargas wrote: > Indent under the first arg Done. http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.cc#ne... base/message_pump_win.cc:124: message_filter_.reset(new SimpleMessageFilter); On 2012/08/21 19:45:37, rvargas wrote: > + DCHECK Init() Done. http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.cc#ne... base/message_pump_win.cc:419: if (state_->dispatcher) { On 2012/08/21 19:45:37, rvargas wrote: > Are you sure this is the correct behavior when the pump has a dispatcher?. This > ignores the dispatcher and that probably breaks nested loops. Yes it is prospected about ITfKeystrokeMgr::KeyDown. http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h#new... base/message_pump_win.h:24: class TextServicesBridge; On 2012/08/21 19:45:37, rvargas wrote: > remove this. Done. http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h#new... base/message_pump_win.h:133: // Interface gets message and may process it. On 2012/08/21 19:45:37, rvargas wrote: > How about: > A MessageFilter implements the common Peek/Translate/Dispatch code to deal with > windows messages. Done. http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h#new... base/message_pump_win.h:139: virtual BOOL DoPeekMessage(MSG* msg, On 2012/08/21 19:45:37, rvargas wrote: > nit: message Done. http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h#new... base/message_pump_win.h:144: // Returns true if |message| was completely consumed by the filter and On 2012/08/21 19:45:37, rvargas wrote: > It is not clear what is expected from an implementation of this interface. How about this? http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h#new... base/message_pump_win.h:146: virtual bool ProcessMessage(const MSG& msg) = 0; On 2012/08/21 19:45:37, rvargas wrote: > nit: message Done. http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h#new... base/message_pump_win.h:146: virtual bool ProcessMessage(const MSG& msg) = 0; On 2012/08/21 19:45:37, rvargas wrote: > On a more serious note, the interaction between a MessageFilter and a > MessagePumpDispatcher should be documented somewhere... who has priority, given > that both implement similar functionality (which is a bad thing by itself, but > they serve two quite different purposes). Add comments. http://codereview.chromium.org/10826223/diff/2007/base/message_pump_win.h#new... base/message_pump_win.h:184: // A Proxy which gets message. On 2012/08/21 19:45:37, rvargas wrote: > nit: remove comment Done. http://codereview.chromium.org/10826223/diff/2007/base/win/text_services_brid... File base/win/text_services_bridge.h (right): http://codereview.chromium.org/10826223/diff/2007/base/win/text_services_brid... base/win/text_services_bridge.h:26: // Obtains messages from application message queue. On 2012/08/21 19:45:37, rvargas wrote: > These comments belong to the cc file. This is just the implementation of an > interface, and the interface is documented somewhere else. Done.
http://codereview.chromium.org/10826223/diff/17001/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/17001/base/message_pump_win.h#ne... base/message_pump_win.h:137: virtual BOOL DoPeekMessage(MSG* messge, should this be a bool instead of a BOOL ? http://codereview.chromium.org/10826223/diff/17001/base/win/text_services_bri... File base/win/text_services_bridge.cc (right): http://codereview.chromium.org/10826223/diff/17001/base/win/text_services_bri... base/win/text_services_bridge.cc:11: is_initialized_ = scoped_com_initialiser_.succeeded(); COM is initialized on this thread, don't do this. http://codereview.chromium.org/10826223/diff/17001/base/win/text_services_bri... base/win/text_services_bridge.cc:32: return is_initialized_; maybe we should do lines 16 to 23 here instead. http://codereview.chromium.org/10826223/diff/17001/base/win/text_services_bri... File base/win/text_services_bridge.h (right): http://codereview.chromium.org/10826223/diff/17001/base/win/text_services_bri... base/win/text_services_bridge.h:20: class TextServicesBridge : public base::MessagePumpForUI::MessageFilter { name it TextServicesMessageFilter or something like that http://codereview.chromium.org/10826223/diff/17001/base/win/text_services_bri... base/win/text_services_bridge.h:35: base::win::ScopedCOMInitializer scoped_com_initialiser_; see my comment about COM in the cc file
http://codereview.chromium.org/10826223/diff/17001/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/17001/base/message_pump_win.h#ne... base/message_pump_win.h:137: virtual BOOL DoPeekMessage(MSG* messge, On 2012/08/22 21:37:18, cpu wrote: > should this be a bool instead of a BOOL ? This follows the signature of the function that is being replaced. http://codereview.chromium.org/10826223/diff/17001/base/win/text_services_bri... File base/win/text_services_bridge.cc (right): http://codereview.chromium.org/10826223/diff/17001/base/win/text_services_bri... base/win/text_services_bridge.cc:11: is_initialized_ = scoped_com_initialiser_.succeeded(); On 2012/08/22 21:37:18, cpu wrote: > COM is initialized on this thread, don't do this. However, this CL is introducing requirements that were not there before, and should not rely on the callers fixing the situation for it.
please upgrade hoge.cc to hoge2.cc
On 2012/08/23 01:59:17, cpu wrote: > please upgrade hoge.cc to hoge2.cc discard this comment :)
Four safety, I add ScopedComInitializer http://codereview.chromium.org/10826223/diff/17001/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/17001/base/message_pump_win.h#ne... base/message_pump_win.h:137: virtual BOOL DoPeekMessage(MSG* messge, On 2012/08/22 21:37:18, cpu wrote: > should this be a bool instead of a BOOL ? This is thin wrapper for PeekMessage so signature is same as it.
According to coding style, I changed BOOL to bool. PeekMessage returns binary BOOL unlike GetMessage.
http://codereview.chromium.org/10826223/diff/26007/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/26007/base/message_pump_win.cc#n... base/message_pump_win.cc:123: I discussed it a bit with others and the COM issue is a side effect of this code here. Let me explain the COM issue first. Objects should not try to initialize COM, COM initialization should be a property of the owner of the thread, in the case of the main UI thread is browser_main, it should be initialized (or not) once, with a well defined threading_model (like sta, mta, none, etc) and no other code should attempt to do this. For example the chrome main UI thread in desktop mode I believe needs to be STA, but the main thread in chrome metro mode is not sta or mta (!!) is an ASTA: http://stackoverflow.com/questions/10951750/about-windows-metro-threading-model The ideal scenario would be that the message_filter_ be specified during construction or initialization of the message pump, that way we can for the browser process (and only for that one) pass win::TextServicesMessageFilter at the chrome/browser layer With that change then the win::TextServicesMessageFilter does not need to be here. That option has a fair amount of work but otherwise is very clean. I think it will address Ricardo's concerns as well. Now there is another way, which is *mostly* clean, one that windows itself provides. For that to work the only thing we need to add here is a call to CallMsgFilter If this message pump calls this, then from outside this code we can cleanly filter messages using the technique explained here http://blogs.msdn.com/b/oldnewthing/archive/2005/04/28/412574.aspx So you have two options. I don't know if Ricardo will like the second one. I like them both.
http://codereview.chromium.org/10826223/diff/26007/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/26007/base/message_pump_win.cc#n... base/message_pump_win.cc:123: The first option is actually quite simple. The filter doesn't even have to be supplied at construction time: we can instantiate a default filter at construction and just have a method to set another filter after that (knowingly replacing whatever was there before, no chaining here, maybe a debug only check that the method is not called twice). The second option is fine because we already have the code in place for that. The problem would be PeekMessage, which brings me to my next point. It is unfortunate that we need two closely related hooks for the same thing (do we really need both?) : - PeekMessage (for TSF pump) - MessageFilter (for key state) given that conceptually they do the same thing (first chance to look at messages). Unless they don't, in which case we have something else to worry about... At first it seemed natural to forward all Peek calls through the new code... but now I'm not so sure. There are multiple calls that we care about when TSF should not really be involved (only paint and timers, peeking without removing) and at this point I'm not sure anymore that it's a good idea to forward everything. Carlos? http://codereview.chromium.org/10826223/diff/26007/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/26007/base/message_pump_win.h#ne... base/message_pump_win.h:142: // Implements some special procedure for got message. I'm sorry, but this sentence provides no information. http://codereview.chromium.org/10826223/diff/26007/base/message_pump_win.h#ne... base/message_pump_win.h:144: // DispatchMessage nor MessagePumpDispatcher. How about: Returns true if |message| was consumed by the filter and no extra processing is required. If this method returns false, the normal message processing will take place. The priority to consume messages is the following: - Native Windows' messsage filter (CallMsgFilter). - MessageFilter::ProcessMessage. - MessagePumpDispatcher. - TranslateMessage / DispatchMessage. http://codereview.chromium.org/10826223/diff/26007/base/win/text_services_mes... File base/win/text_services_message_filter.cc (right): http://codereview.chromium.org/10826223/diff/26007/base/win/text_services_mes... base/win/text_services_message_filter.cc:54: return !!result; nit: Between this and having a BOOL i'd go with the BOOL as a return value. The return value is part of the signature of a function, and if we are not simply following the signature of PeekMessage, then using UINT for argument types would not be appropriate either. http://codereview.chromium.org/10826223/diff/26007/base/win/text_services_mes... base/win/text_services_message_filter.cc:57: // Sends msg to Text Service Manager. s/msg/message http://codereview.chromium.org/10826223/diff/26007/base/win/text_services_mes... base/win/text_services_message_filter.cc:58: // The msg will be used to input composition text. s/msg/message http://codereview.chromium.org/10826223/diff/26007/base/win/text_services_mes... base/win/text_services_message_filter.cc:59: // Returns true if|message| was consumed by text service manager. nit: space before |message|
Just remove ScopedComInitializer from TextServicesMessageFilter.
Replying to Ricardo's question: My gut reaction is that is not appropriate to forward all peek messages but I don't have the whole picture. I just reviewed a bunch of TSF code and still I am not clear, so far it seems that it requires keypresses only, but again, I don't know what other code is needed. In particular this code that starts a line 457: message_filter_->DoPeekMessage(&message, NULL, WM_PAINT,WM_PAINT, PM_REMOVE) || message_filter_->DoPeekMessage(&message, NULL, WM_TIMER, WM_TIMER, PM_REMOVE); I don't imagine ITfMessagePump is expecting to be called twice (or 3 times) for the same message. -- Also upon further looking at TSF in msdn I don't think my second option with callmsgfilter works for your case.
Mainly I did - move TSF awareness from MessagePumpUI to ChromeBrowserMainPartsWin - use win32 PeekMessage for WM_PAINT,WM_TIMER I tried to append new function, which peeks only WM_PAINT/WM_TIMER message, to MessageFilter interface but it looked something strange so I just use original PeekMessage.
http://codereview.chromium.org/10826223/diff/38001/base/message_loop.h File base/message_loop.h (right): http://codereview.chromium.org/10826223/diff/38001/base/message_loop.h#newcod... base/message_loop.h:576: void SetMessageFilter(base::MessagePumpForUI::MessageFilter* message_filter) { nit: Please add a typedef for MessageFilter so that the MessageLoop API is not MessagePump dependent. (see IOHandler & Co below) http://codereview.chromium.org/10826223/diff/38001/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/38001/base/message_pump_win.cc#n... base/message_pump_win.cc:208: if (!message_filter_->DoPeekMessage(&message, NULL, 0, 0, This is also a candidate for not using the filter. http://codereview.chromium.org/10826223/diff/38001/base/message_pump_win.cc#n... base/message_pump_win.cc:332: !message_filter_->DoPeekMessage(&msg, NULL, WM_MOUSEFIRST, WM_MOUSELAST, I assume that TSF can generate mouse events... and that's why this is here http://codereview.chromium.org/10826223/diff/38001/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/38001/base/message_pump_win.h#ne... base/message_pump_win.h:158: // Sets a new MessageFilter. MessagePumpForUI has ownership of nit: has -> takes http://codereview.chromium.org/10826223/diff/38001/base/message_pump_win.h#ne... base/message_pump_win.h:160: void SetMessageFilter(MessageFilter* message_filter); There has to be documentation somewhere that states what happens when SetMessageFilter is called multiple times. This looks like the right place to do that... but then you should instruct readers to refer to this file instead of relying on the description from message_loop.h. See how RegisterIOHandler declaration forwards the documentation. http://codereview.chromium.org/10826223/diff/38001/base/message_pump_win.h#ne... base/message_pump_win.h:171: private: nit: don't remove the space http://codereview.chromium.org/10826223/diff/38001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_main_win.cc (right): http://codereview.chromium.org/10826223/diff/38001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main_win.cc:196: // Sets TSF message filter to MessagePumpForUI. MessagePumpForUI has nit: this is not a fn declaration, so sets -> set. Maybe "Create a TSF message filter for the message loop. MessageLoop takes ownership of the filter." http://codereview.chromium.org/10826223/diff/38001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main_win.cc:198: scoped_ptr<base::MessagePumpForUI::MessageFilter> tsf_message_filter( nit: there should be no need to mention the pump here.
http://codereview.chromium.org/10826223/diff/38001/base/message_loop.h File base/message_loop.h (right): http://codereview.chromium.org/10826223/diff/38001/base/message_loop.h#newcod... base/message_loop.h:576: void SetMessageFilter(base::MessagePumpForUI::MessageFilter* message_filter) { On 2012/08/31 21:39:49, rvargas wrote: > nit: Please add a typedef for MessageFilter so that the MessageLoop API is not > MessagePump dependent. (see IOHandler & Co below) Done. http://codereview.chromium.org/10826223/diff/38001/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/38001/base/message_pump_win.cc#n... base/message_pump_win.cc:208: if (!message_filter_->DoPeekMessage(&message, NULL, 0, 0, On 2012/08/31 21:39:49, rvargas wrote: > This is also a candidate for not using the filter. Done. http://codereview.chromium.org/10826223/diff/38001/base/message_pump_win.cc#n... base/message_pump_win.cc:332: !message_filter_->DoPeekMessage(&msg, NULL, WM_MOUSEFIRST, WM_MOUSELAST, On 2012/08/31 21:39:49, rvargas wrote: > I assume that TSF can generate mouse events... and that's why this is here Done. http://codereview.chromium.org/10826223/diff/38001/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/38001/base/message_pump_win.h#ne... base/message_pump_win.h:158: // Sets a new MessageFilter. MessagePumpForUI has ownership of On 2012/08/31 21:39:49, rvargas wrote: > nit: has -> takes Done. http://codereview.chromium.org/10826223/diff/38001/base/message_pump_win.h#ne... base/message_pump_win.h:160: void SetMessageFilter(MessageFilter* message_filter); On 2012/08/31 21:39:49, rvargas wrote: > There has to be documentation somewhere that states what happens when > SetMessageFilter is called multiple times. > > This looks like the right place to do that... but then you should instruct > readers to refer to this file instead of relying on the description from > message_loop.h. See how RegisterIOHandler declaration forwards the > documentation. Done. http://codereview.chromium.org/10826223/diff/38001/base/message_pump_win.h#ne... base/message_pump_win.h:171: private: On 2012/08/31 21:39:49, rvargas wrote: > nit: don't remove the space Done. http://codereview.chromium.org/10826223/diff/38001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_main_win.cc (right): http://codereview.chromium.org/10826223/diff/38001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main_win.cc:196: // Sets TSF message filter to MessagePumpForUI. MessagePumpForUI has On 2012/08/31 21:39:49, rvargas wrote: > nit: this is not a fn declaration, so sets -> set. > Maybe > "Create a TSF message filter for the message loop. MessageLoop takes ownership > of the filter." Done.
Ping.
Version 17 LGTM after fixing the following issues. jar, cpu? http://codereview.chromium.org/10826223/diff/48001/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/48001/base/message_pump_win.cc#n... base/message_pump_win.cc:120: DCHECK(message_filter_->Init()); Sorry, I missed this. This skips the execution of Init for release builds and that's not what we want (even if that's a nop). http://codereview.chromium.org/10826223/diff/48001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_main_win.cc (right): http://codereview.chromium.org/10826223/diff/48001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main_win.cc:198: scoped_ptr<base::MessagePumpForUI::MessageFilter> tsf_message_filter( Should not use MessagePumpForUI directly.
lgtm http://codereview.chromium.org/10826223/diff/48001/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/48001/base/message_pump_win.cc#n... base/message_pump_win.cc:121: +1
Thank you for a suite of much helpful reviewing! http://codereview.chromium.org/10826223/diff/48001/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/48001/base/message_pump_win.cc#n... base/message_pump_win.cc:120: DCHECK(message_filter_->Init()); On 2012/09/05 21:25:03, rvargas wrote: > Sorry, I missed this. This skips the execution of Init for release builds and > that's not what we want (even if that's a nop). Done. http://codereview.chromium.org/10826223/diff/48001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_main_win.cc (right): http://codereview.chromium.org/10826223/diff/48001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main_win.cc:198: scoped_ptr<base::MessagePumpForUI::MessageFilter> tsf_message_filter( On 2012/09/05 21:25:03, rvargas wrote: > Should not use MessagePumpForUI directly. Done.
Add sky@ for an owner of chrome/ tree. Could you check this?
darin@ might be a better reviewer for this than sky@ considering that he did a lot of work on the message pump long ago.
LGTM for base (this looks much more reasonable now... and I'm rubber stamping the windows details approved by Ricardo and Carlos). http://codereview.chromium.org/10826223/diff/46002/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/46002/base/message_pump_win.h#ne... base/message_pump_win.h:144: // processing will take place. nit: passive language "will take place" Suggest: If this method returns false, it is the responsibility of the caller to ensure that normal processing takes place.
Hmm...interesting. It seems like the condition for enabling this code is if we are running under Metro mode, but the comment suggests that we might end up enabling this for Win7 too? It seems like you guys have already gone back-n-forth on implementation strategy, so I don't really mean to randomize things further. I'm curious why you didn't just add a mode toggle on MessagePumpWin to optionally engage this feature. Will the MessageFilter abstraction be used for other things? On one hand, MessageFilter makes it so that MessagePumpWin can be studied without having to know anything about TSF. On the other hand, it is now difficult to reason about what happens when DoPeekMessage and ProcessMessage are called. Just reading MessagePumpWin code isn't enough anymore. Hmm... http://codereview.chromium.org/10826223/diff/39012/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/39012/base/message_pump_win.cc#n... base/message_pump_win.cc:120: bool init = message_filter_->Init(); here, you know that Init() is a no-op. http://codereview.chromium.org/10826223/diff/39012/base/message_pump_win.cc#n... base/message_pump_win.cc:208: MSG message; nit: gratuitous variable re-naming. elsewhere in this file it is common to use "msg" instead of "message" for variables of type MSG. consider cleanup changes in a separate CL? I'm not sure this is a good change considering that the structure has a member named message. It is a bit jarring to see "message.message" in the code. Also, if we are going to make this change, then we should do it uniformly. There are still some "MSG msg" left in this .cc file. http://codereview.chromium.org/10826223/diff/39012/base/message_pump_win.cc#n... base/message_pump_win.cc:415: if (!message_filter_->ProcessMessage(message)) { It seems like there is some overlap here between MessageFilter and MessagePumpDispatcher. Hmm... http://codereview.chromium.org/10826223/diff/39012/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/39012/base/message_pump_win.h#ne... base/message_pump_win.h:135: virtual bool Init() = 0; nit: This "Init" method doesn't seem to be necessary here. It can just live on the subclass. http://codereview.chromium.org/10826223/diff/39012/base/message_pump_win.h#ne... base/message_pump_win.h:162: void SetMessageFilter(MessageFilter* message_filter); nit: argument type should be "scoped_ptr<MessageFilter>" since this function takes ownership of the given object. caller should use .Pass() to pass ownership to SetMessageFilter.
> Hmm...interesting. It seems like the condition for enabling this code is if we > are running under Metro mode, but the comment suggests that we might end up > enabling this for Win7 too? No, TextServicesMessagFilter runs on only Win8 Metro mode. > > It seems like you guys have already gone back-n-forth on implementation > strategy, so I don't really mean to randomize things further. > > I'm curious why you didn't just add a mode toggle on MessagePumpWin to > optionally engage this feature. Will the MessageFilter abstraction be used for > other things? On one hand, MessageFilter makes it so that MessagePumpWin can be > studied without having to know anything about TSF. On the other hand, it is now > difficult to reason about what happens when DoPeekMessage and ProcessMessage are > called. Just reading MessagePumpWin code isn't enough anymore. Hmm... I add some comments. How about this?
http://codereview.chromium.org/10826223/diff/39012/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/39012/base/message_pump_win.cc#n... base/message_pump_win.cc:120: bool init = message_filter_->Init(); On 2012/09/06 03:56:55, darin wrote: > here, you know that Init() is a no-op. Done. http://codereview.chromium.org/10826223/diff/39012/base/message_pump_win.cc#n... base/message_pump_win.cc:208: MSG message; On 2012/09/06 03:56:55, darin wrote: > nit: gratuitous variable re-naming. elsewhere in this file it is common to use > "msg" instead of "message" for variables of type MSG. consider cleanup changes > in a separate CL? I'm not sure this is a good change considering that the > structure has a member named message. It is a bit jarring to see > "message.message" in the code. Also, if we are going to make this change, then > we should do it uniformly. There are still some "MSG msg" left in this .cc > file. Done. http://codereview.chromium.org/10826223/diff/39012/base/message_pump_win.h File base/message_pump_win.h (right): http://codereview.chromium.org/10826223/diff/39012/base/message_pump_win.h#ne... base/message_pump_win.h:135: virtual bool Init() = 0; On 2012/09/06 03:56:55, darin wrote: > nit: This "Init" method doesn't seem to be necessary here. It can just live on > the subclass. Done. http://codereview.chromium.org/10826223/diff/39012/base/message_pump_win.h#ne... base/message_pump_win.h:162: void SetMessageFilter(MessageFilter* message_filter); On 2012/09/06 03:56:55, darin wrote: > nit: argument type should be "scoped_ptr<MessageFilter>" since this function > takes ownership of the given object. caller should use .Pass() to pass > ownership to SetMessageFilter. Done.
On 2012/09/06 03:56:55, darin wrote: > I'm curious why you didn't just add a mode toggle on MessagePumpWin to > optionally engage this feature. Will the MessageFilter abstraction be used for > other things? On one hand, MessageFilter makes it so that MessagePumpWin can be > studied without having to know anything about TSF. On the other hand, it is now > difficult to reason about what happens when DoPeekMessage and ProcessMessage are > called. Just reading MessagePumpWin code isn't enough anymore. Hmm... Integrating TSF code directly into MessagePumpWin felt out of place: lots of details not really related to what the pump is concerned about. The MessageFilter was just a way to keep things relatively independent, and interaction between the two components clearly defined. It is true that reading MessagePumpWin doesn't give a complete picture of what can happen when retrieving and processing messages, but having the TSF code visible there, forwarding the calls to a COM interface doesn't really help either. In a way, now there's no way to really know what's gonna happen :( Things on the ProcessMessage front are not that different either, because today we still have CallMsgFilter and MessagePumpDispatcher to get in the way. And yes, it's not so clean to have three ways to hook that process, but they serve slightly different purposes. The last thing to consider, is that TSF is for the first time introducing COM into the MessageLoop. If we leave all implementation hidden inside the pump itself, now the code has to make sure that it is OK to call COM on that thread, and that is a decision that is better to leave to the embedder of base. Sure we could have said something like "make sure to enable COM before switching this to use TSF", but it felt cleaner this way.
OK, makes sense. On Thu, Sep 6, 2012 at 11:10 AM, <rvargas@chromium.org> wrote: > On 2012/09/06 03:56:55, darin wrote: > >> I'm curious why you didn't just add a mode toggle on MessagePumpWin to >> optionally engage this feature. Will the MessageFilter abstraction be >> used >> > for > >> other things? On one hand, MessageFilter makes it so that MessagePumpWin >> can >> > be > >> studied without having to know anything about TSF. On the other hand, it >> is >> > now > >> difficult to reason about what happens when DoPeekMessage and >> ProcessMessage >> > are > >> called. Just reading MessagePumpWin code isn't enough anymore. Hmm... >> > > Integrating TSF code directly into MessagePumpWin felt out of place: lots > of > details not really related to what the pump is concerned about. The > MessageFilter was just a way to keep things relatively independent, and > interaction between the two components clearly defined. > > It is true that reading MessagePumpWin doesn't give a complete picture of > what > can happen when retrieving and processing messages, but having the TSF code > visible there, forwarding the calls to a COM interface doesn't really help > either. In a way, now there's no way to really know what's gonna happen :( > > Things on the ProcessMessage front are not that different either, because > today > we still have CallMsgFilter and MessagePumpDispatcher to get in the way. > And > yes, it's not so clean to have three ways to hook that process, but they > serve > slightly different purposes. > > The last thing to consider, is that TSF is for the first time introducing > COM > into the MessageLoop. If we leave all implementation hidden inside the pump > itself, now the code has to make sure that it is OK to call COM on that > thread, > and that is a decision that is better to leave to the embedder of base. > Sure we > could have said something like "make sure to enable COM before switching > this to > use TSF", but it felt cleaner this way. > > http://codereview.chromium.**org/10826223/<http://codereview.chromium.org/108... >
Darin, thank you for review. When no any other points, I'm appreciate if you give LGTM.
OK, LGTM http://codereview.chromium.org/10826223/diff/38010/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/38010/base/message_pump_win.cc#n... base/message_pump_win.cc:451: scoped_ptr<MessageFilter> message_filter) { nit: indent by 4 spaces for line continuations
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/10826223/47015
Thank you all! http://codereview.chromium.org/10826223/diff/38010/base/message_pump_win.cc File base/message_pump_win.cc (right): http://codereview.chromium.org/10826223/diff/38010/base/message_pump_win.cc#n... base/message_pump_win.cc:451: scoped_ptr<MessageFilter> message_filter) { On 2012/09/07 07:33:51, darin wrote: > nit: indent by 4 spaces for line continuations Done.
Try job failure for 10826223-47015 (retry) on mac_rel for step "sync_integration_tests". It's a second try, previously, step "sync_integration_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/10826223/47015
Try job failure for 10826223-47015 (retry) (retry) (retry) on mac_rel for step "sync_integration_tests". It's a second try, previously, step "sync_integration_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/10826223/47015
Change committed as 155589 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
