Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2317333002:
Refactor EventHandler out of RenderWidgetHostViewAura (Closed)
|
Created:
4 years, 3 months ago by jonross Modified:
4 years, 2 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, dtapuska+chromiumwatch_chromium.org, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, James Su Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWe would like to share the event handling logic of RenderWidgetHostViewAura with RenderWidgetHostViewMus. As a first step this change refactors the EventHandler code out of RenderWidgetHostViewAura into its own class
TEST=manually run browser, dry-run of CQ to verify all existing tests.
BUG=631270
Committed: https://crrev.com/2843ae3645cbbdf62b5ab7a16f89842ff40c86a2
Cr-Commit-Position: refs/heads/master@{#426815}
Patch Set 1 : Fix popups #
Total comments: 6
Patch Set 2 : Create Delegate Abstraction for RWHVA #Patch Set 3 : Rebase #Patch Set 4 : Dont Export Delegate in DLL #Patch Set 5 : Docs and Windows compile #
Total comments: 32
Patch Set 6 : Rebase #Patch Set 7 : y #
Total comments: 12
Patch Set 8 : Rebase #Patch Set 9 : Address Review #Patch Set 10 : Fix Windows #Messages
Total messages: 70 (51 generated)
The CQ bit was checked by jonross@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by jonross@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jonross@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jonross@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by jonross@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Description was changed from ========== WIP Refactor EventHandler out of RenderWidgetHostViewAura BUG=631270 ========== to ========== We would like to share the event handling logic of RenderWidgetHostViewAura with RenderWidgetHostViewMus. As a first step this change refactors the EventHandler code out of RenderWidgetHostViewAura into its own class TEST=manually run browser, dry-run of CQ to verify all existing tests. BUG=631270 ==========
jonross@chromium.org changed reviewers: + sadrul@chromium.org
Hey Sadrul, I would like your input on this approach to pulling event handling out of RenderWidgetHostViewAura. Thanks, Jon
I think this looks pretty good. This is similar to what I had in mind. The biggest point is to make sure RenderWidgetHostViewEventHandler does not have any aura specific code. For example, instead of directly talking to a RenderWidgetHostViewAura, it should use a RenderWidgetHostView instead, or a delegate interface (e.g. EventHandlerDelegate). The tooltip-disabler should also stay in RWHVAura for now (at least until we have an equivalent interface in mus ... btw, looks like this is the only user of TooltipDisabler, so maybe file a bug to implement this for mus?). https://codereview.chromium.org/2317333002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (left): https://codereview.chromium.org/2317333002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1296: tooltip_disabler_.reset(new aura::client::ScopedTooltipDisabler(root_window)); It might make sense to keep this tooltip disabler in here for now. |event_handler_| would ideally not deal with anything aura specific. https://codereview.chromium.org/2317333002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.cc (right): https://codereview.chromium.org/2317333002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:442: } This is a very long function. While you are here, let's split the |mouse_locked_| case into a separate HandleMouseEventWhileLocked() function. https://codereview.chromium.org/2317333002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.h (right): https://codereview.chromium.org/2317333002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:48: RenderWidgetHostViewAura* host_view); Use a delegate interface instead?
The CQ bit was checked by jonross@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jonross@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2317333002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (left): https://codereview.chromium.org/2317333002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1296: tooltip_disabler_.reset(new aura::client::ScopedTooltipDisabler(root_window)); On 2016/09/13 15:39:46, sadrul wrote: > It might make sense to keep this tooltip disabler in here for now. > |event_handler_| would ideally not deal with anything aura specific. Moved the tooltip disabler back to RWHVA, and added a method to the delegate to handle the toggling. Mus can similarly implement the delegate. https://codereview.chromium.org/2317333002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.cc (right): https://codereview.chromium.org/2317333002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:442: } On 2016/09/13 15:39:46, sadrul wrote: > This is a very long function. While you are here, let's split the > |mouse_locked_| case into a separate HandleMouseEventWhileLocked() function. Done. https://codereview.chromium.org/2317333002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.h (right): https://codereview.chromium.org/2317333002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:48: RenderWidgetHostViewAura* host_view); On 2016/09/13 15:39:46, sadrul wrote: > Use a delegate interface instead? I've split this into a RWHVBase and a Delegate.
jonross@chromium.org changed reviewers: + tdresser@chromium.org
+tdresser@ (once he's back) for thoughts. The win failure is a flake seen on other changes.
The CQ bit was checked by jonross@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
High level seems good to me. A few nits (some on code you were just moving around, sorry). https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2081: void RenderWidgetHostViewAura::ForwardKeyboardEvent( The exact boundary between RenderWidgetHostViewAura and RenderWidgetHostViewEventHandler isn't completely clear to me. Why is this here? Maybe have a comment in RenderWidgetHostViewEventHandler indicating what logic goes where? https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:597: // While this is an ui::EventHandler for targetting, |event_handler_| actually an -> a https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:425: bool is_fullscreen_; Why did is_fullscreen move here? (Not looking for justification in code, just wondering). https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.cc (right): https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:47: const int kMouseLockBorderPercentage = 15; I am sad that this review forced me to learn that this exists... https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:49: #if defined(OS_WIN) Not in this patch, but I wonder how hard it would be to make a RenderWidgetHostAuraEventHandlerWin subclass... Might be worth filing a Hotlist-InputDev bug. https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:96: // Reset unchanged touch point to StateStationary for touchmove and point -> points https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.h (right): https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:23: Remove newlines https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:48: // RenderWidgetHostViewBase. A delegate is required in order to provide platform Is this comment right? Isn't it for use with RenderWidgetHostViewAura? https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:53: class CONTENT_EXPORT RenderWidgetHostViewEventHandler I'd prefer if this naming indicated that it was Aura / MUS only. Is MUS still Aura? Maybe RenderWidgetHostAuraEventHandler? https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:56: // An interface to provide additional functionality needed for event handling. Mention that the delegate is for platform specific logic. https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:56: // An interface to provide additional functionality needed for event handling. If we're calling this an interface, perhaps we should just have the implementers hold onto the TouchSelectionControllerClientAura, TouchSelectionController and OverscrollController? https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:99: // Set child popups host view, and event handler, in order to redirect input. popup's https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:127: // Provides an implementation for RenderWidgetHostView, locking future mouse This comment is no longer correct. https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:180: // events to the |host_|. This comment isn't correct anymore. https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:239: // Not owned/ Replace trailing / with . https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:239: // Not owned/ For some of these, it might be worth talking about who owns them, or what the lifetime requirements are. e.g., Delegate needs to outlive |this|, I assume?
https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2081: void RenderWidgetHostViewAura::ForwardKeyboardEvent( On 2016/09/23 13:54:09, tdresser wrote: > The exact boundary between RenderWidgetHostViewAura and > RenderWidgetHostViewEventHandler isn't completely clear to me. Why is this here? > Maybe have a comment in RenderWidgetHostViewEventHandler indicating what logic > goes where? Yeah I went with a minimal bounds change. RenderWidgetHostViewEventHandler takes over the ui::EventHandler code RenderWidgetHostViewAura is still a ui::TextInputClient, so I left in methods that were called by both EventHandler and TextInputClient code. I would also consider moving the TextInputClient out, or the methods required by it. And have RWHVA use RWHVEH more https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:597: // While this is an ui::EventHandler for targetting, |event_handler_| actually On 2016/09/23 13:54:09, tdresser wrote: > an -> a Done. https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:425: bool is_fullscreen_; On 2016/09/23 13:54:09, tdresser wrote: > Why did is_fullscreen move here? (Not looking for justification in code, just > wondering). The concept of a RWHVB being fullscreen was already present (InitAsFullscreen) and the knowledge of if it was is now needed outside of child classes. RWHVEventHandler needs to know this state, regardless of the actual class being an RWHVAura or RWHVMus https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.cc (right): https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:47: const int kMouseLockBorderPercentage = 15; On 2016/09/23 13:54:09, tdresser wrote: > I am sad that this review forced me to learn that this exists... :( same https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:96: // Reset unchanged touch point to StateStationary for touchmove and On 2016/09/23 13:54:09, tdresser wrote: > point -> points Done. https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.h (right): https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:23: On 2016/09/23 13:54:09, tdresser wrote: > Remove newlines Done. https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:48: // RenderWidgetHostViewBase. A delegate is required in order to provide platform On 2016/09/23 13:54:09, tdresser wrote: > Is this comment right? Isn't it for use with RenderWidgetHostViewAura? We will also being using this with RenderWidgetHostViewMus in the future. So it will begin to be used by multiple RenderWidgetHostViewBase child classes https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:53: class CONTENT_EXPORT RenderWidgetHostViewEventHandler On 2016/09/23 13:54:09, tdresser wrote: > I'd prefer if this naming indicated that it was Aura / MUS only. > Is MUS still Aura? > > Maybe RenderWidgetHostAuraEventHandler? We have been maintaining a distinction of Aura/Mus. sadrul@ thoughts on the naming? https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:56: // An interface to provide additional functionality needed for event handling. On 2016/09/23 13:54:09, tdresser wrote: > Mention that the delegate is for platform specific logic. Done. https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:56: // An interface to provide additional functionality needed for event handling. On 2016/09/23 13:54:09, tdresser wrote: > If we're calling this an interface, perhaps we should just have the implementers > hold onto the TouchSelectionControllerClientAura, TouchSelectionController and > OverscrollController? I'd also be fine not calling this an interface, as it would allow centralized spot for holding onto those controllers. Thoughts? https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:99: // Set child popups host view, and event handler, in order to redirect input. On 2016/09/23 13:54:09, tdresser wrote: > popup's Done. https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:127: // Provides an implementation for RenderWidgetHostView, locking future mouse On 2016/09/23 13:54:09, tdresser wrote: > This comment is no longer correct. Done. https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:180: // events to the |host_|. On 2016/09/23 13:54:09, tdresser wrote: > This comment isn't correct anymore. Done. https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:239: // Not owned/ On 2016/09/23 13:54:09, tdresser wrote: > For some of these, it might be worth talking about who owns them, or what the > lifetime requirements are. > > e.g., Delegate needs to outlive |this|, I assume? Updated with more info. In general, aside from two optional ones, the rest should outlive this class.
https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.cc (right): https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:49: #if defined(OS_WIN) On 2016/09/23 13:54:09, tdresser wrote: > Not in this patch, but I wonder how hard it would be to make a > RenderWidgetHostAuraEventHandlerWin subclass... > > Might be worth filing a Hotlist-InputDev bug. crbug.com/653126 filed
https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.h (right): https://codereview.chromium.org/2317333002/diff/300001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:53: class CONTENT_EXPORT RenderWidgetHostViewEventHandler On 2016/10/05 15:37:58, jonross wrote: > On 2016/09/23 13:54:09, tdresser wrote: > > I'd prefer if this naming indicated that it was Aura / MUS only. > > Is MUS still Aura? > > > > Maybe RenderWidgetHostAuraEventHandler? > > We have been maintaining a distinction of Aura/Mus. > > sadrul@ thoughts on the naming? How about RenderWidgetHostUIEventHandler: handles ui::Events for a RenderWidgetHost
https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.cc (right): https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:156: host_tracker_.reset(new aura::WindowTracker); Maybe DCHECK that host_tracker_ is null or empty? https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:259: aura::WindowTracker tracker; Actually, can you use host_tracker_ for this? I mean something like: auto local_tracker = std::move(host_tracker_); ... if (!local_tracker.Contains(window)) { ... } https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:269: delegate_->Shutdown(); Should this reset |host_tracker_|? https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:693: GetRenderViewHostDelegateView(); Not a huge fan of this living in here. Can this be something like: context_menu_params->source_type = TOUCH; delegate_->ShowContextMenu(*context_menu_params); And the delegate can then go back to RenderViewHostDelegateView with the focused frame. https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:731: window_->MoveCursorTo(center); Leave a TODO here that we would ideally not have to do this for mus (reference crbug.com/621412) https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.h (right): https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:94: std::unique_ptr<OverscrollController> overscroll_controller_; DISALLOW_COPY_AND_ASSIGN
https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.cc (right): https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:156: host_tracker_.reset(new aura::WindowTracker); On 2016/10/13 16:27:41, sadrul wrote: > Maybe DCHECK that host_tracker_ is null or empty? Done. https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:259: aura::WindowTracker tracker; On 2016/10/13 16:27:41, sadrul wrote: > Actually, can you use host_tracker_ for this? I mean something like: > > auto local_tracker = std::move(host_tracker_); > ... > if (!local_tracker.Contains(window)) { > ... > } Done. https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:269: delegate_->Shutdown(); On 2016/10/13 16:27:41, sadrul wrote: > Should this reset |host_tracker_|? Done. https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:693: GetRenderViewHostDelegateView(); On 2016/10/13 16:27:41, sadrul wrote: > Not a huge fan of this living in here. Can this be something like: > > context_menu_params->source_type = TOUCH; > delegate_->ShowContextMenu(*context_menu_params); > > And the delegate can then go back to RenderViewHostDelegateView with the focused > frame. Done. https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:731: window_->MoveCursorTo(center); On 2016/10/13 16:27:41, sadrul wrote: > Leave a TODO here that we would ideally not have to do this for mus (reference > crbug.com/621412) Done. https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.h (right): https://codereview.chromium.org/2317333002/diff/340001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.h:94: std::unique_ptr<OverscrollController> overscroll_controller_; On 2016/10/13 16:27:41, sadrul wrote: > DISALLOW_COPY_AND_ASSIGN Done.
LGTM
lgtm
jonross@chromium.org changed reviewers: + avi@chromium.org
avi@chromium.org: Please review changes in content/browser/BUILD.gn This change adds some new files to content/browser/renderer_host as a part of a refactoring. Thanks, Jon
content/browser/BUILD.gn lgtm
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by jonross@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, sadrul@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2317333002/#ps400001 (title: "Fix Windows")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== We would like to share the event handling logic of RenderWidgetHostViewAura with RenderWidgetHostViewMus. As a first step this change refactors the EventHandler code out of RenderWidgetHostViewAura into its own class TEST=manually run browser, dry-run of CQ to verify all existing tests. BUG=631270 ========== to ========== We would like to share the event handling logic of RenderWidgetHostViewAura with RenderWidgetHostViewMus. As a first step this change refactors the EventHandler code out of RenderWidgetHostViewAura into its own class TEST=manually run browser, dry-run of CQ to verify all existing tests. BUG=631270 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== We would like to share the event handling logic of RenderWidgetHostViewAura with RenderWidgetHostViewMus. As a first step this change refactors the EventHandler code out of RenderWidgetHostViewAura into its own class TEST=manually run browser, dry-run of CQ to verify all existing tests. BUG=631270 ========== to ========== We would like to share the event handling logic of RenderWidgetHostViewAura with RenderWidgetHostViewMus. As a first step this change refactors the EventHandler code out of RenderWidgetHostViewAura into its own class TEST=manually run browser, dry-run of CQ to verify all existing tests. BUG=631270 Committed: https://crrev.com/2843ae3645cbbdf62b5ab7a16f89842ff40c86a2 Cr-Commit-Position: refs/heads/master@{#426815} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/2843ae3645cbbdf62b5ab7a16f89842ff40c86a2 Cr-Commit-Position: refs/heads/master@{#426815} |
