Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3748002:
Add a message pump for touchui=1 (Closed)
|
Created:
10 years, 2 months ago by sadrul Modified:
9 years, 6 months ago CC:
chromium-reviews, ben+cc_chromium.org, arv (Not doing code reviews), brettw-cc_chromium.org Base URL:
http://src.chromium.org/git/chromium.git Visibility:
Public. |
DescriptionAdd a message pump for touchui=1
The message pump reads events directly from X. For most events, it passes them
on to GDK for normal processing. It consumes some events (e.g. keypress events)
to demonstrate how it's intended to work. This, of course, makes chrome mostly
unusable since you can only use the mouse to do things. But this is a small
first step towards capturing events through MPX (e.g. touch etc.) and processing
them as chrome pleases.
glib message pump: Slightly change architecture
This changeset breaks down the glib message pump a little so that it can be
easily subclassed. The next set of commits will introduce a subclass that still
uses GTK and GDK widgets, but reads events directly from X instead of through
GTK/GDK.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=63397
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=63519
Patch Set 1 #
Total comments: 18
Patch Set 2 : Renamed to MessagePumpGlibX, corrected style, added comments. #
Total comments: 17
Patch Set 3 : Remove the file that keeps getting added. #Patch Set 4 : Addressed the issues. #
Total comments: 4
Patch Set 5 : Use bitset and use X11 header cleanly. #
Total comments: 19
Patch Set 6 : Follow styles, add comments. #
Total comments: 4
Patch Set 7 : Move code around a little bit. #Patch Set 8 : Merge fix. #Patch Set 9 : Trying to fix the build failures for the trybots. #Patch Set 10 : GTK+ version check to pass compile in trybots. #
Total comments: 3
Patch Set 11 : Simplifying code for older GTK+ versions. #Patch Set 12 : gyp magic to fix build failure (hopefully). #
Messages
Total messages: 24 (0 generated)
http://codereview.chromium.org/3748002/diff/1/8 File chrome/browser/resources/network_menu.js (right): http://codereview.chromium.org/3748002/diff/1/8#newcode21 chrome/browser/resources/network_menu.js:21: initMenuItem_: function() { The changes in the file are from revision beb1bcb6ef299e302a5d877fd006686710a2bc4e. I am not entirely sure how this managed to sneak in for review (or how to get rid of it).
http://codereview.chromium.org/3748002/diff/1/4 File base/message_pump_glib.cc (right): http://codereview.chromium.org/3748002/diff/1/4#newcode234 base/message_pump_glib.cc:234: bool MessagePumpForUI::RunOnce(GMainContext *context, bool block) { in Chrome land, it's always type* var as sky has patiently pointed out to me. :-) http://codereview.chromium.org/3748002/diff/1/5 File base/message_pump_glib.h (right): http://codereview.chromium.org/3748002/diff/1/5#newcode83 base/message_pump_glib.h:83: virtual void DispatchEvents(GdkEvent *event); Add a comment saying what it's for? http://codereview.chromium.org/3748002/diff/1/6 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/3748002/diff/1/6#newcode12 base/message_pump_glib_x.cc:12: bool captureXEvents[LASTEvent] = {false}; Could these be made members? And explain what they're for. http://codereview.chromium.org/3748002/diff/1/6#newcode20 base/message_pump_glib_x.cc:20: static gboolean type on same line in google style I think http://codereview.chromium.org/3748002/diff/1/6#newcode40 base/message_pump_glib_x.cc:40: for (d = gdk_display_manager_list_displays(NULL); check the google style for this too. I think the lower lines need to line up with the d, not the (. http://codereview.chromium.org/3748002/diff/1/6#newcode53 base/message_pump_glib_x.cc:53: fprintf(stderr, "nom noming event\n"); use the chrome logging facilities http://codereview.chromium.org/3748002/diff/1/7 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/3748002/diff/1/7#newcode5 base/message_pump_glib_x.h:5: #ifndef BASE_MESSAGE_PUMP_GLIB_X_H the name of this file, class MessagePumpX and the #define are not in alignment. see the style guide. http://codereview.chromium.org/3748002/diff/1/7#newcode29 base/message_pump_glib_x.h:29: you need DISALLOW_COPY_AND_ASSIGN(MessagePumpX); here. And probably a virtual destructor to be defined. http://codereview.chromium.org/3748002/diff/1/8 File chrome/browser/resources/network_menu.js (right): http://codereview.chromium.org/3748002/diff/1/8#newcode21 chrome/browser/resources/network_menu.js:21: initMenuItem_: function() { On 2010/10/13 15:50:07, sadrul wrote: > The changes in the file are from revision > beb1bcb6ef299e302a5d877fd006686710a2bc4e. I am not entirely sure how this > managed to sneak in for review (or how to get rid of it). Upload a new patch set.
http://codereview.chromium.org/3748002/diff/1/4 File base/message_pump_glib.cc (right): http://codereview.chromium.org/3748002/diff/1/4#newcode234 base/message_pump_glib.cc:234: bool MessagePumpForUI::RunOnce(GMainContext *context, bool block) { On 2010/10/13 18:24:50, rjkroege wrote: > in Chrome land, it's always type* var as sky has patiently pointed out to me. > :-) Done. http://codereview.chromium.org/3748002/diff/1/5 File base/message_pump_glib.h (right): http://codereview.chromium.org/3748002/diff/1/5#newcode83 base/message_pump_glib.h:83: virtual void DispatchEvents(GdkEvent *event); On 2010/10/13 18:24:50, rjkroege wrote: > Add a comment saying what it's for? Done. http://codereview.chromium.org/3748002/diff/1/6 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/3748002/diff/1/6#newcode12 base/message_pump_glib_x.cc:12: bool captureXEvents[LASTEvent] = {false}; On 2010/10/13 18:24:50, rjkroege wrote: > Could these be made members? And explain what they're for. Done. http://codereview.chromium.org/3748002/diff/1/6#newcode20 base/message_pump_glib_x.cc:20: static gboolean On 2010/10/13 18:24:50, rjkroege wrote: > type on same line in google style I think Done. http://codereview.chromium.org/3748002/diff/1/6#newcode40 base/message_pump_glib_x.cc:40: for (d = gdk_display_manager_list_displays(NULL); On 2010/10/13 18:24:50, rjkroege wrote: > check the google style for this too. I think the lower lines need to line up > with the d, not the (. Done. http://codereview.chromium.org/3748002/diff/1/6#newcode53 base/message_pump_glib_x.cc:53: fprintf(stderr, "nom noming event\n"); On 2010/10/13 18:24:50, rjkroege wrote: > use the chrome logging facilities Done. http://codereview.chromium.org/3748002/diff/1/7 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/3748002/diff/1/7#newcode5 base/message_pump_glib_x.h:5: #ifndef BASE_MESSAGE_PUMP_GLIB_X_H Done (renamed to MessagePumpGlibX) http://codereview.chromium.org/3748002/diff/1/7#newcode29 base/message_pump_glib_x.h:29: On 2010/10/13 18:24:50, rjkroege wrote: > you need DISALLOW_COPY_AND_ASSIGN(MessagePumpX); here. And probably a virtual > destructor to be defined. Done.
> > beb1bcb6ef299e302a5d877fd006686710a2bc4e. I am not entirely sure how this > > managed to sneak in for review (or how to get rid of it). > > Upload a new patch set. It still seems to hang around in here. Did you mean create a new branch for the changeset and upload from there?
http://codereview.chromium.org/3748002/diff/10001/6004 File base/message_pump_glib.cc (right): http://codereview.chromium.org/3748002/diff/10001/6004#newcode234 base/message_pump_glib.cc:234: bool MessagePumpForUI::RunOnce(GMainContext *context, bool block) { GMainContext *context -> GMainContext* context and everywhere else. http://codereview.chromium.org/3748002/diff/10001/6005 File base/message_pump_glib.h (right): http://codereview.chromium.org/3748002/diff/10001/6005#newcode59 base/message_pump_glib.h:59: virtual bool RunOnce(GMainContext *context, bool block); say what this one does too please. http://codereview.chromium.org/3748002/diff/10001/6006 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/3748002/diff/10001/6006#newcode23 base/message_pump_glib_x.cc:23: DCHECK(COUNT_EVENTS >= LASTEvent); so, it won't fail mysteriously per my other comment but failing at compile time would be better than runtime? http://codereview.chromium.org/3748002/diff/10001/6006#newcode42 base/message_pump_glib_x.cc:42: DLOG(INFO) << "nom noming event" << std::endl; I think that this is the core of where the code connects with wyck's right? http://codereview.chromium.org/3748002/diff/10001/6006#newcode57 base/message_pump_glib_x.cc:57: (gdksource_->source_funcs->dispatch); indent +2 http://codereview.chromium.org/3748002/diff/10001/6006#newcode73 base/message_pump_glib_x.cc:73: captureXEvents[KeyPress] = true; this needs to be expanded for the other event types that we are processing specially yes? If so, can you add a TODO here? http://codereview.chromium.org/3748002/diff/10001/6006#newcode86 base/message_pump_glib_x.cc:86: DLOG(INFO) << "GDK ruined it!!" << std::endl; I think that DLOG adds a endl implicitly. Also, we should avoid unnecessary DLOG(?) so it might be worth noting that this (and the DLOG above) will be replaced when the TODO is implemented in a subsequent CL. http://codereview.chromium.org/3748002/diff/10001/6007 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/3748002/diff/10001/6007#newcode15 base/message_pump_glib_x.h:15: #define COUNT_EVENTS 48 Can we get this # out of gtk/gdk? otherwise, this code could become unhappy when we roll gdk/gtk?
http://codereview.chromium.org/3748002/diff/10001/6006 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/3748002/diff/10001/6006#newcode23 base/message_pump_glib_x.cc:23: DCHECK(COUNT_EVENTS >= LASTEvent); On 2010/10/13 22:04:42, rjkroege wrote: > so, it won't fail mysteriously per my other comment but failing at compile time > would be better than runtime? I originally had captureXEvents[LASTEvent] and captureGdkEvents[GDK_EVENT_LAST]. For LASTEvent, I have to include <X11/X.h> in the header file, which brings in 'typedef unsigned long Time', which in turn conflicts with base::Time in message_loop.cc and causes compilation errors. I can use GDK_EVENT_LAST for both, and DCHECK that GDK_EVENT_LAST >= LASTEvent. I cannot use GDK_EVENT_LAST in a preprocessor check since it's an enum, it seems.
http://codereview.chromium.org/3748002/diff/10001/6004 File base/message_pump_glib.cc (right): http://codereview.chromium.org/3748002/diff/10001/6004#newcode234 base/message_pump_glib.cc:234: bool MessagePumpForUI::RunOnce(GMainContext *context, bool block) { On 2010/10/13 22:04:42, rjkroege wrote: > GMainContext *context -> GMainContext* context > > and everywhere else. Done. http://codereview.chromium.org/3748002/diff/10001/6005 File base/message_pump_glib.h (right): http://codereview.chromium.org/3748002/diff/10001/6005#newcode59 base/message_pump_glib.h:59: virtual bool RunOnce(GMainContext *context, bool block); On 2010/10/13 22:04:42, rjkroege wrote: > say what this one does too please. Done. http://codereview.chromium.org/3748002/diff/10001/6006 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/3748002/diff/10001/6006#newcode23 base/message_pump_glib_x.cc:23: DCHECK(COUNT_EVENTS >= LASTEvent); On 2010/10/14 09:53:10, sadrul wrote: > On 2010/10/13 22:04:42, rjkroege wrote: > > so, it won't fail mysteriously per my other comment but failing at compile > time > > would be better than runtime? > > I originally had captureXEvents[LASTEvent] and captureGdkEvents[GDK_EVENT_LAST]. > For LASTEvent, I have to include <X11/X.h> in the header file, which brings in > 'typedef unsigned long Time', which in turn conflicts with base::Time in > message_loop.cc and causes compilation errors. I can use GDK_EVENT_LAST for > both, and DCHECK that GDK_EVENT_LAST >= LASTEvent. I cannot use GDK_EVENT_LAST > in a preprocessor check since it's an enum, it seems. > Done. http://codereview.chromium.org/3748002/diff/10001/6006#newcode42 base/message_pump_glib_x.cc:42: DLOG(INFO) << "nom noming event" << std::endl; On 2010/10/13 22:04:42, rjkroege wrote: > I think that this is the core of where the code connects with wyck's right? That is correct. I have added a note accordingly here (in the comment just below). http://codereview.chromium.org/3748002/diff/10001/6006#newcode57 base/message_pump_glib_x.cc:57: (gdksource_->source_funcs->dispatch); On 2010/10/13 22:04:42, rjkroege wrote: > indent +2 Done. http://codereview.chromium.org/3748002/diff/10001/6006#newcode73 base/message_pump_glib_x.cc:73: captureXEvents[KeyPress] = true; On 2010/10/13 22:04:42, rjkroege wrote: > this needs to be expanded for the other event types that we are processing > specially yes? If so, can you add a TODO here? Done. http://codereview.chromium.org/3748002/diff/10001/6006#newcode86 base/message_pump_glib_x.cc:86: DLOG(INFO) << "GDK ruined it!!" << std::endl; On 2010/10/13 22:04:42, rjkroege wrote: > I think that DLOG adds a endl implicitly. > > Also, we should avoid unnecessary DLOG(?) so it might be worth noting that this > (and the DLOG above) will be replaced when the TODO is implemented in a > subsequent CL. Done. http://codereview.chromium.org/3748002/diff/10001/6007 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/3748002/diff/10001/6007#newcode15 base/message_pump_glib_x.h:15: #define COUNT_EVENTS 48 On 2010/10/13 22:04:42, rjkroege wrote: > Can we get this # out of gtk/gdk? otherwise, this code could become unhappy when > we roll gdk/gtk? Using GDK_EVENT_LAST
http://codereview.chromium.org/3748002/diff/19001/20005 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/3748002/diff/19001/20005#newcode25 base/message_pump_glib_x.cc:25: DCHECK(GDK_EVENT_LAST >= LASTEvent); I'm still not happy about this and I'm glad that it's gotten fixed. http://codereview.chromium.org/3748002/diff/19001/20006 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/3748002/diff/19001/20006#newcode36 base/message_pump_glib_x.h:36: bool captureXEvents[GDK_EVENT_LAST]; how big do these end up? Would a bit vector be smaller?
Uploaded a new patch that uses bitset and includes header files correctly and cleanly. http://codereview.chromium.org/3748002/diff/19001/20005 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/3748002/diff/19001/20005#newcode25 base/message_pump_glib_x.cc:25: DCHECK(GDK_EVENT_LAST >= LASTEvent); On 2010/10/14 15:08:12, rjkroege wrote: > I'm still not happy about this and I'm glad that it's gotten fixed. Fixed, thanks to Chad (wyck). http://codereview.chromium.org/3748002/diff/19001/20006 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/3748002/diff/19001/20006#newcode36 base/message_pump_glib_x.h:36: bool captureXEvents[GDK_EVENT_LAST]; On 2010/10/14 15:08:12, rjkroege wrote: > how big do these end up? Would a bit vector be smaller? In my debug build, each bool seems to end up taking 1byte. The newer patchset uses a bitset instead.
http://codereview.chromium.org/3748002/diff/29001/18003 File base/message_loop.cc (right): http://codereview.chromium.org/3748002/diff/29001/18003#newcode542 base/message_loop.cc:542: bool MessageLoop::DoDelayedWork(base::Time* next_delayed_work_time) { It was necessary to disambiguate base::Time from Time typedef'ed in X11/X.h (here and in line 544)
You need to get someone that knows about the messageloop on X to review this. Maybe Evan can suggest a good reviewer.
I don't understand your underlying design. I feel like we've discussed this before, so maybe you can refresh my memory. What is the problem you're trying to address -- that you have a new type of X event? I worry that there is information you're going to lose by pumping X events yourself -- for example, won't gtk get confused by the missing serial numbers? How about the way GTK handles X errors? I wonder if something like http://library.gnome.org/devel/gdk/2.22/gdk-X-Window-System-Interaction.html#... might be a better approach. PS: random nit, variable names like captureXEvents are not Chrome style
On Thu, Oct 14, 2010 at 12:26 PM, <evan@chromium.org> wrote: > I don't understand your underlying design. I feel like we've discussed this > before, so maybe you can refresh my memory. What is the problem you're > trying > to address -- that you have a new type of X event? Since Sadrul is a noogler, I'll chime in here with some background. This CL is one work-in-progress step of what's necessary to not depend on Gtk/Gdk for event delivery in the touchui=1 version of ChromeOS/Chrome and instead distribute events via the views hierarchy. Doing the event distribution via views will let us more easily support touch events in Chrome on ChromeOS including correcting them for orientation and also simplifies distributing synthetic events (like from gestures). If you want more background, we should probably move the discussion out of the CL review. > > I worry that there is information you're going to lose by pumping X events > yourself -- for example, won't gtk get confused by the missing serial > numbers? I hadn't considered this. My (perhaps insufficient) hacking on this with xtest and manual Gdk event injection taught me that out-of-order is VeryBad (tm) but that filtering event categories was safe -- i.e. button ups without downs bad, all button changes filtered fine. > How about the way GTK handles X errors? My understanding is that Chrome uses raw Xlib error handling and responds to an X error by saving the user's profile and then shutting down. So I don't think our event routing would alter this. Rob. > > I wonder if something like > http://library.gnome.org/devel/gdk/2.22/gdk-X-Window-System-Interaction.html#... > might be a better approach. > > > PS: random nit, variable names like captureXEvents are not Chrome style > > http://codereview.chromium.org/3748002/show >
I've said my piece (that I am skeptical this route will work, and that you may run into subtle GTK-related bugs), so I will continue with the review leaving you to worry about that. Just one last note, a grep for "serial" in GTK source indicates it does stuff with matching serials. http://codesearch.google.com/codesearch/p?hl=en#ErvFMsc8kPE/pub/GNOME/sources... Regarding the code itself: please do read the C++ style guide. The only reason the Chrome code is as easy to follow as it is (I know it's not perfect, but it's a lot better than a lot of other code I've read) is because we are really anal about comments and naming. In particular, don't be afraid to put lots of comments in the header file, and avoid abbreviations for variable names (you should configure your editor to help you if you have trouble typing them out). http://codereview.chromium.org/3748002/diff/29001/18004 File base/message_pump_glib.cc (right): http://codereview.chromium.org/3748002/diff/29001/18004#newcode340 base/message_pump_glib.cc:340: EventDispatcher(event, this); I don't quite get this. DispatchEvents doesn't seem to be used in this class, so how does adding this help? http://codereview.chromium.org/3748002/diff/29001/18006 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/3748002/diff/29001/18006#newcode12 base/message_pump_glib_x.cc:12: static gboolean placeholder_dispatch(GSource* source, Don't need "static", anon namespaces have static linkage PlaceholderDispatch http://codereview.chromium.org/3748002/diff/29001/18006#newcode36 base/message_pump_glib_x.cc:36: bool retvalue; You can declare this nearer to where it's used. http://codereview.chromium.org/3748002/diff/29001/18006#newcode38 base/message_pump_glib_x.cc:38: Display* xd = GDK_DISPLAY_XDISPLAY(gdisp); display http://codereview.chromium.org/3748002/diff/29001/18007 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/3748002/diff/29001/18007#newcode15 base/message_pump_glib_x.h:15: #include <X11/X.h> We normally try pretty hard to keep Xlib headers out of other headers, because (as you've observed) they tend to ruin the namespace. It seems this is unused anyway? http://codereview.chromium.org/3748002/diff/29001/18007#newcode31 base/message_pump_glib_x.h:31: void InitializeEventsToCapture(void); Can you add docs on all of these functions and member variables? Stuff like 'dispatching_event_' in particular is non-obvious. http://codereview.chromium.org/3748002/diff/29001/18007#newcode39 base/message_pump_glib_x.h:39: std::bitset<LASTEvent> captureXEvents; capture_x_events_ http://codereview.chromium.org/3748002/diff/29001/18007#newcode40 base/message_pump_glib_x.h:40: std::bitset<GDK_EVENT_LAST> captureGdkEvents; capture_gdk_events_ http://codereview.chromium.org/3748002/diff/29001/18007#newcode45 base/message_pump_glib_x.h:45: } } // namespace base
Thanks! I have addressed your concerns. It's taking me a little time to get used to the Chrome coding style :-) http://codereview.chromium.org/3748002/diff/29001/18004 File base/message_pump_glib.cc (right): http://codereview.chromium.org/3748002/diff/29001/18004#newcode340 base/message_pump_glib.cc:340: EventDispatcher(event, this); On 2010/10/15 16:28:32, Evan Martin wrote: > I don't quite get this. DispatchEvents doesn't seem to be used in this class, > so how does adding this help? This is called from MessagePumpGlibX::EventDispatcherX, since we want most of the GDK events to be handled normally. Alternatively, MessagePumpForUI::EventDispatcher could be made public so that it can be called directly. http://codereview.chromium.org/3748002/diff/29001/18006 File base/message_pump_glib_x.cc (right): http://codereview.chromium.org/3748002/diff/29001/18006#newcode12 base/message_pump_glib_x.cc:12: static gboolean placeholder_dispatch(GSource* source, On 2010/10/15 16:28:32, Evan Martin wrote: > Don't need "static", anon namespaces have static linkage > > PlaceholderDispatch Done. http://codereview.chromium.org/3748002/diff/29001/18006#newcode36 base/message_pump_glib_x.cc:36: bool retvalue; On 2010/10/15 16:28:32, Evan Martin wrote: > You can declare this nearer to where it's used. Done. http://codereview.chromium.org/3748002/diff/29001/18006#newcode38 base/message_pump_glib_x.cc:38: Display* xd = GDK_DISPLAY_XDISPLAY(gdisp); On 2010/10/15 16:28:32, Evan Martin wrote: > display Done. http://codereview.chromium.org/3748002/diff/29001/18007 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/3748002/diff/29001/18007#newcode15 base/message_pump_glib_x.h:15: #include <X11/X.h> On 2010/10/15 16:28:32, Evan Martin wrote: > We normally try pretty hard to keep Xlib headers out of other headers, because > (as you've observed) they tend to ruin the namespace. It seems this is unused > anyway? This particular header is required in this case for the LASTEvent define for capture_x_events_ bitset's size. http://codereview.chromium.org/3748002/diff/29001/18007#newcode31 base/message_pump_glib_x.h:31: void InitializeEventsToCapture(void); On 2010/10/15 16:28:32, Evan Martin wrote: > Can you add docs on all of these functions and member variables? Stuff like > 'dispatching_event_' in particular is non-obvious. Done. http://codereview.chromium.org/3748002/diff/29001/18007#newcode39 base/message_pump_glib_x.h:39: std::bitset<LASTEvent> captureXEvents; On 2010/10/15 16:28:32, Evan Martin wrote: > capture_x_events_ Done. http://codereview.chromium.org/3748002/diff/29001/18007#newcode40 base/message_pump_glib_x.h:40: std::bitset<GDK_EVENT_LAST> captureGdkEvents; On 2010/10/15 16:28:32, Evan Martin wrote: > capture_gdk_events_ Done. http://codereview.chromium.org/3748002/diff/29001/18007#newcode45 base/message_pump_glib_x.h:45: } On 2010/10/15 16:28:32, Evan Martin wrote: > } // namespace base Done.
Looks great, just one more thought on DispatchEvents() http://codereview.chromium.org/3748002/diff/9/17004 File base/message_pump_glib.cc (right): http://codereview.chromium.org/3748002/diff/9/17004#newcode345 base/message_pump_glib.cc:345: MessagePumpForUI* message_pump = reinterpret_cast<MessagePumpForUI*>(data); How about: Have DispatchEvents() be a public and not-static, and have the body of this code in it (without all the message_pump-> bits in it, since that with just be 'this'). Then EventDispatcher can be a static that just calls message_pump->DispatchEvents(event); http://codereview.chromium.org/3748002/diff/9/17007 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/3748002/diff/9/17007#newcode48 base/message_pump_glib_x.h:48: std::bitset<LASTEvent> capture_x_events_; I guess you could avoid depending on the X header by making this a set<int> and only note the events you want. Your call though.
http://codereview.chromium.org/3748002/diff/9/17004 File base/message_pump_glib.cc (right): http://codereview.chromium.org/3748002/diff/9/17004#newcode345 base/message_pump_glib.cc:345: MessagePumpForUI* message_pump = reinterpret_cast<MessagePumpForUI*>(data); On 2010/10/15 17:37:55, Evan Martin wrote: > How about: > Have DispatchEvents() be a public and not-static, and have the body of this code > in it (without all the message_pump-> bits in it, since that with just be > 'this'). > > Then EventDispatcher can be a static that just calls > message_pump->DispatchEvents(event); That sounds like a good idea. Done. http://codereview.chromium.org/3748002/diff/9/17007 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/3748002/diff/9/17007#newcode48 base/message_pump_glib_x.h:48: std::bitset<LASTEvent> capture_x_events_; On 2010/10/15 17:37:55, Evan Martin wrote: > I guess you could avoid depending on the X header by making this a set<int> and > only note the events you want. Your call though. Since numerous X events go off pretty often, I think it'd be good to maintain a lookup table instead of checking in a set for each event.
LGTM
http://codereview.chromium.org/3748002/diff/48001/49006 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/3748002/diff/48001/49006#newcode54 base/message_pump_glib_x.h:54: Older versions of GTK+ (GDK) does not have GDK_EVENT_LAST, which was causing the compile errors in the trybots. I have made changes here so that it compiles and works correctly in all versions of GTK+.
LGTM, consider simplifying http://codereview.chromium.org/3748002/diff/48001/49006 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/3748002/diff/48001/49006#newcode48 base/message_pump_glib_x.h:48: #elif GTK_CHECK_VERSION(2,8,0) I think we require 2.12 minimum anyway elsewhere, but it's good of you to think of it. Since these constants are part of the GTK ABI they're not going to change, so it might be worth just copying the appropriate number out of the header. (Since this is just used for the bitset size, it might be worth just making it the largest value ever seen in any GTK release. The difference between these constants you have here is likely under a byte or two in terms of memory cost below.)
http://codereview.chromium.org/3748002/diff/48001/49006 File base/message_pump_glib_x.h (right): http://codereview.chromium.org/3748002/diff/48001/49006#newcode48 base/message_pump_glib_x.h:48: #elif GTK_CHECK_VERSION(2,8,0) On 2010/10/20 16:08:34, Evan Martin wrote: > I think we require 2.12 minimum anyway elsewhere, but it's good of you to think > of it. Since these constants are part of the GTK ABI they're not going to > change, so it might be worth just copying the appropriate number out of the > header. > > (Since this is just used for the bitset size, it might be worth just making it > the largest value ever seen in any GTK release. The difference between these > constants you have here is likely under a byte or two in terms of memory cost > below.) Done. Thanks!
LGTM++
The build failure caused by this patch shows up only in some cases. I could not reproduce the problem. But Matt Mueller (mattm_g) could and he tested the fix for the build failure. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
