Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2264503003:
exo: WMHelperMus: Hook up event handlers for mus+ash (Closed)
Descriptionexo: WMHelperMus: Hook up event handlers for mus+ash
BUG=637914
Committed: https://crrev.com/49a0d443aad757be9766762fb8b7bd1bc26bf58e
Cr-Commit-Position: refs/heads/master@{#413796}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Update #Patch Set 3 : Add a TODO #Patch Set 4 : Avoid using auto. #Patch Set 5 : Avoid using auto. #Patch Set 6 : Rebase #Patch Set 7 : Fix build errors #Patch Set 8 : Remove changes in pointer.cc #Messages
Total messages: 21 (11 generated)
penghuang@chromium.org changed reviewers: + reveman@chromium.org
Hi David, PTAL. Thanks.
https://codereview.chromium.org/2264503003/diff/1/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2264503003/diff/1/components/exo/pointer.cc#n... components/exo/pointer.cc:133: if (target == surface_) why is this needed? https://codereview.chromium.org/2264503003/diff/1/components/exo/wm_helper_mu... File components/exo/wm_helper_mus.cc (right): https://codereview.chromium.org/2264503003/diff/1/components/exo/wm_helper_mu... components/exo/wm_helper_mus.cc:36: EventForwarder(WMHelperMus* helper, bool is_post) Can we pass a ui::EventHandlerList reference to this ctor and avoid the callback into the helper class? https://codereview.chromium.org/2264503003/diff/1/components/exo/wm_helper_mu... components/exo/wm_helper_mus.cc:67: if (env) Why this conditional necessary? Would be nice if we could instead DCHECK and require that this instance is destroyed before aura env.. https://codereview.chromium.org/2264503003/diff/1/components/exo/wm_helper_mu... components/exo/wm_helper_mus.cc:181: if (!is_post && event->stopped_propagation()) why is stopped_propagation() only respected in the pre target handler but not the post target handler?
https://codereview.chromium.org/2264503003/diff/1/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2264503003/diff/1/components/exo/pointer.cc#n... components/exo/pointer.cc:133: if (target == surface_) On 2016/08/22 13:41:42, reveman wrote: > why is this needed? I don't why, but on mus-ash, this surface_ will get mouse events. And I think the surface_ is for showing cursor, so we shouldn't care about it, right? https://codereview.chromium.org/2264503003/diff/1/components/exo/wm_helper_mu... File components/exo/wm_helper_mus.cc (right): https://codereview.chromium.org/2264503003/diff/1/components/exo/wm_helper_mu... components/exo/wm_helper_mus.cc:36: EventForwarder(WMHelperMus* helper, bool is_post) On 2016/08/22 13:41:42, reveman wrote: > Can we pass a ui::EventHandlerList reference to this ctor and avoid the callback > into the helper class? Done. https://codereview.chromium.org/2264503003/diff/1/components/exo/wm_helper_mu... components/exo/wm_helper_mus.cc:67: if (env) On 2016/08/22 13:41:42, reveman wrote: > Why this conditional necessary? Would be nice if we could instead DCHECK and > require that this instance is destroyed before aura env.. Done. https://codereview.chromium.org/2264503003/diff/1/components/exo/wm_helper_mu... components/exo/wm_helper_mus.cc:181: if (!is_post && event->stopped_propagation()) On 2016/08/22 13:41:42, reveman wrote: > why is stopped_propagation() only respected in the pre target handler but not > the post target handler? I thought we should always dispatch event to post event handlers. But after checking the code again, I find out I was wrong. Done.
https://codereview.chromium.org/2264503003/diff/1/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2264503003/diff/1/components/exo/pointer.cc#n... components/exo/pointer.cc:133: if (target == surface_) On 2016/08/22 at 14:32:40, Peng wrote: > On 2016/08/22 13:41:42, reveman wrote: > > why is this needed? > > I don't why, but on mus-ash, this surface_ will get mouse events. And I think the surface_ is for showing cursor, so we shouldn't care about it, right? correct this is the surface for the cursor sprite. however, "params.accept_events = false" in Pointer::CreatePointerWidget should prevent this from getting events dispatched to it. is that not the case for mus?
https://codereview.chromium.org/2264503003/diff/1/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2264503003/diff/1/components/exo/pointer.cc#n... components/exo/pointer.cc:133: if (target == surface_) On 2016/08/22 15:44:08, reveman wrote: > On 2016/08/22 at 14:32:40, Peng wrote: > > On 2016/08/22 13:41:42, reveman wrote: > > > why is this needed? > > > > I don't why, but on mus-ash, this surface_ will get mouse events. And I think > the surface_ is for showing cursor, so we shouldn't care about it, right? > > correct this is the surface for the cursor sprite. however, > "params.accept_events = false" in Pointer::CreatePointerWidget should prevent > this from getting events dispatched to it. is that not the case for mus? I think it must be a bug in mus+ash. I filed a bug for it and added a TODO here.
The CQ bit was checked by penghuang@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_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 penghuang@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.
On 2016/08/22 16:15:14, Peng wrote: > https://codereview.chromium.org/2264503003/diff/1/components/exo/pointer.cc > File components/exo/pointer.cc (right): > > https://codereview.chromium.org/2264503003/diff/1/components/exo/pointer.cc#n... > components/exo/pointer.cc:133: if (target == surface_) > On 2016/08/22 15:44:08, reveman wrote: > > On 2016/08/22 at 14:32:40, Peng wrote: > > > On 2016/08/22 13:41:42, reveman wrote: > > > > why is this needed? > > > > > > I don't why, but on mus-ash, this surface_ will get mouse events. And I > think > > the surface_ is for showing cursor, so we shouldn't care about it, right? > > > > correct this is the surface for the cursor sprite. however, > > "params.accept_events = false" in Pointer::CreatePointerWidget should prevent > > this from getting events dispatched to it. is that not the case for mus? > > I think it must be a bug in mus+ash. I filed a bug for it and added a TODO here. I just sent a fixing CL for this issue. https://codereview.chromium.org/2272753002/# And I also removed the change in pointer.cc. PTAL.
lgtm, thanks!
The CQ bit was checked by penghuang@chromium.org
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.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== exo: WMHelperMus: Hook up event handlers for mus+ash BUG=637914 ========== to ========== exo: WMHelperMus: Hook up event handlers for mus+ash BUG=637914 Committed: https://crrev.com/49a0d443aad757be9766762fb8b7bd1bc26bf58e Cr-Commit-Position: refs/heads/master@{#413796} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/49a0d443aad757be9766762fb8b7bd1bc26bf58e Cr-Commit-Position: refs/heads/master@{#413796} |
