Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(119)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 9958152: Consolidate win/x dispatchers (Closed)

Created:
8 years, 8 months ago by oshima
Modified:
8 years, 8 months ago
CC:
chromium-reviews, erikwright (departed), brettw-cc_chromium.org, sadrul, tfarina, ben+watch_chromium.org
Visibility:
Public.

Description

Consolidate win/x dispatchers BUG=116282 TEST=no functional change. All tests should pass. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=131412

Patch Set 1 #

Patch Set 2 : 130816 #

Patch Set 3 : sync #

Patch Set 4 : win fix #

Patch Set 5 : conflict,sync #

Patch Set 6 : cleanup #

Total comments: 1

Patch Set 7 : bool, cleanup #

Patch Set 8 : sync #

Total comments: 6

Patch Set 9 : addressed comments #

Patch Set 10 : sync #

Total comments: 8

Patch Set 11 : sync, addressed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+179 lines, -307 lines) Patch
M ash/accelerators/accelerator_dispatcher.h View 1 2 3 4 5 6 1 chunk +2 lines, -6 lines 0 comments Download
M ash/accelerators/accelerator_dispatcher.cc View 1 2 3 4 5 6 7 8 2 chunks +66 lines, -0 lines 0 comments Download
M ash/accelerators/accelerator_dispatcher_linux.cc View 1 1 chunk +0 lines, -58 lines 0 comments Download
D ash/accelerators/accelerator_dispatcher_win.cc View 1 1 chunk +0 lines, -51 lines 0 comments Download
M ash/accelerators/nested_dispatcher_controller_unittest.cc View 1 2 3 4 5 6 2 chunks +6 lines, -13 lines 0 comments Download
M ash/ash.gyp View 1 2 3 4 5 6 7 1 chunk +0 lines, -2 lines 0 comments Download
M base/message_loop.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -4 lines 0 comments Download
M base/message_loop_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
A base/message_pump_dispatcher.h View 1 2 3 4 5 6 1 chunk +34 lines, -0 lines 0 comments Download
M base/message_pump_win.h View 1 2 3 4 5 5 chunks +4 lines, -20 lines 0 comments Download
M base/message_pump_win.cc View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download
M base/message_pump_x.h View 2 chunks +1 line, -20 lines 0 comments Download
M base/message_pump_x.cc View 1 2 3 4 5 6 1 chunk +1 line, -6 lines 0 comments Download
M chrome/browser/ui/views/simple_message_box_views.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -6 lines 0 comments Download
M chrome/browser/ui/views/simple_message_box_views.cc View 1 2 3 4 5 6 1 chunk +6 lines, -14 lines 0 comments Download
M chrome/browser/ui/views/user_data_dir_dialog.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/user_data_dir_dialog.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M ui/aura/dispatcher_linux.h View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M ui/aura/dispatcher_linux.cc View 1 2 3 4 5 6 2 chunks +4 lines, -5 lines 0 comments Download
M ui/aura/dispatcher_win.cc View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M ui/aura/monitor_change_observer_x11.h View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M ui/aura/monitor_change_observer_x11.cc View 1 2 3 4 5 6 1 chunk +2 lines, -4 lines 0 comments Download
M ui/aura/root_window_host_linux.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -2 lines 0 comments Download
M ui/aura/root_window_host_linux.cc View 1 2 3 4 5 6 7 8 9 9 chunks +10 lines, -19 lines 0 comments Download
M ui/gfx/compositor/test/test_compositor_host_linux.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +3 lines, -15 lines 0 comments Download
M ui/gfx/compositor/test/test_compositor_host_win.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M ui/views/controls/menu/menu_controller.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -5 lines 0 comments Download
M ui/views/controls/menu/menu_controller.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +10 lines, -20 lines 0 comments Download
M ui/views/focus/accelerator_handler.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -5 lines 0 comments Download
M ui/views/focus/accelerator_handler_aura.cc View 1 2 3 4 5 6 2 chunks +7 lines, -10 lines 0 comments Download
M ui/views/focus/accelerator_handler_win.cc View 1 2 3 4 5 6 3 chunks +5 lines, -10 lines 0 comments Download

Messages

Total messages: 15 (0 generated)
oshima
Darin, I'd like to consolidate Win/X's Dispatchers to simplify Win/Aura code. Could you please take ...
8 years, 8 months ago (2012-04-05 18:15:49 UTC) #1
darin (slow to review)
http://codereview.chromium.org/9958152/diff/23063/base/message_pump_dispatcher.h File base/message_pump_dispatcher.h (right): http://codereview.chromium.org/9958152/diff/23063/base/message_pump_dispatcher.h#newcode42 base/message_pump_dispatcher.h:42: virtual DispatchStatus Dispatch(const NativeEvent& event) = 0; it seems ...
8 years, 8 months ago (2012-04-05 20:21:09 UTC) #2
oshima
I talked to sadrul and confirmed that EVENT_IGNORED is no longer necessary, so I'll just ...
8 years, 8 months ago (2012-04-05 22:08:18 UTC) #3
oshima
Uploaded new patch. The return value of Dispatch is back to bool. Darin, Sadrul, PTAL.
8 years, 8 months ago (2012-04-05 23:18:38 UTC) #4
sadrul
This is very nice. Thanks for cleaning it up. LGTM with nits/notes below: http://codereview.chromium.org/9958152/diff/26067/ash/accelerators/accelerator_dispatcher.cc File ...
8 years, 8 months ago (2012-04-06 00:16:57 UTC) #5
oshima
http://codereview.chromium.org/9958152/diff/26067/ash/accelerators/accelerator_dispatcher.cc File ash/accelerators/accelerator_dispatcher.cc (right): http://codereview.chromium.org/9958152/diff/26067/ash/accelerators/accelerator_dispatcher.cc#newcode41 ash/accelerators/accelerator_dispatcher.cc:41: #if defined(USE_X11) On 2012/04/06 00:16:57, sadrul wrote: > #elif ...
8 years, 8 months ago (2012-04-06 00:34:35 UTC) #6
oshima
Darin, friendly ping.
8 years, 8 months ago (2012-04-07 02:32:43 UTC) #7
darin (slow to review)
base/ changes LGTM
8 years, 8 months ago (2012-04-07 05:54:33 UTC) #8
oshima
+sky for OWNERS reviews for ui/ash/chrome/browser/ui
8 years, 8 months ago (2012-04-07 08:03:03 UTC) #9
sky
LGTM http://codereview.chromium.org/9958152/diff/29001/chrome/browser/ui/views/simple_message_box_views.h File chrome/browser/ui/views/simple_message_box_views.h (right): http://codereview.chromium.org/9958152/diff/29001/chrome/browser/ui/views/simple_message_box_views.h#newcode79 chrome/browser/ui/views/simple_message_box_views.h:79: virtual bool Dispatch(const base::NativeEvent& xevent) OVERRIDE; xevent -> ...
8 years, 8 months ago (2012-04-09 14:38:58 UTC) #10
oshima
http://codereview.chromium.org/9958152/diff/29001/chrome/browser/ui/views/simple_message_box_views.h File chrome/browser/ui/views/simple_message_box_views.h (right): http://codereview.chromium.org/9958152/diff/29001/chrome/browser/ui/views/simple_message_box_views.h#newcode79 chrome/browser/ui/views/simple_message_box_views.h:79: virtual bool Dispatch(const base::NativeEvent& xevent) OVERRIDE; On 2012/04/09 14:38:59, ...
8 years, 8 months ago (2012-04-09 17:56:09 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/9958152/37003
8 years, 8 months ago (2012-04-09 17:56:23 UTC) #12
commit-bot: I haz the power
Change committed as 131412
8 years, 8 months ago (2012-04-09 19:50:13 UTC) #13
jamesr
I believe the change to test_compositor_host_linux.cc broke the build of compositor_unittests on linux clang: ../../ui/gfx/compositor/test/test_compositor_host_linux.cc:41:16: ...
8 years, 8 months ago (2012-04-19 03:20:06 UTC) #14
oshima
8 years, 8 months ago (2012-04-19 17:03:05 UTC) #15 
compositor is not used on linux/gtk build and this target should be removed from
linux build.
If you want to build this without chromeos sutff, please use linux_aura
(use_aura=1).

On 2012/04/19 03:20:06, jamesr wrote:
> I believe the change to test_compositor_host_linux.cc broke the build of
> compositor_unittests on linux clang:
> 
> 
> ../../ui/gfx/compositor/test/test_compositor_host_linux.cc:41:16: error:
> 'Dispatch' marked 'override' but does not override any member functions
>   virtual bool Dispatch(const base::NativeEvent& event) OVERRIDE;
>                ^
> ../../ui/gfx/compositor/test/test_compositor_host_linux.cc:41:16: error:
> 'ui::TestCompositorHostLinux::Dispatch' hides overloaded virtual function
> [-Werror,-Woverloaded-virtual]
> ../../base/message_pump_gtk.h:37:16: note: hidden overloaded virtual function
> 'base::MessagePumpDispatcher::Dispatch' declared here
>   virtual bool Dispatch(GdkEvent* event) = 0;
>                ^
> ../../ui/gfx/compositor/test/test_compositor_host_linux.cc:113:14: error:
> allocating an object of abstract class type 'TestCompositorHostLinux'
>   return new TestCompositorHostLinux(bounds);
>              ^
> ../../base/message_pump_gtk.h:37:16: note: unimplemented pure virtual method
> 'Dispatch' in 'TestCompositorHostLinux'
>   virtual bool Dispatch(GdkEvent* event) = 0;
> 
> 
> the linux_clang tryjob for this patch didn't attempt to compile this target,
and
> I guess there aren't any other bots on the tree that do this or developers who
> regularly test this target on linux clang.  But I suspect the test code is in
> fact broken

Powered by Google App Engine
This is Rietveld 408576698