Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2303963002:
cros/ash: If the partial magnifier is active, do not consume input events if over palette views. (Closed)
|
Created:
4 years, 3 months ago by jdufault Modified:
4 years, 3 months ago Reviewers:
James Cook CC:
chromium-reviews, kalyank, sadrul, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros/ash: If the partial magnifier is active, do not consume input events if over palette views.
This allows the user to disable the magnifier and use the palette while the magnifier is active.
TEST=ash_unittests
BUG=642535
Committed: https://crrev.com/d8d4991f93a15d1d2c388f05e9b93c3a8eb2fa76
Cr-Commit-Position: refs/heads/master@{#417124}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address comments #
Total comments: 6
Patch Set 3 : Address comments #
Messages
Total messages: 35 (25 generated)
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
Description was changed from ========== If the partial magnifier is active, do not consume input events if over palette views. BUG=642535 ========== to ========== cros/ash: If the partial magnifier is active, do not consume input events if over palette views. TEST=ash_unittests BUG=642535 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jdufault@chromium.org changed reviewers: + jamescook@chromium.org
jamescook@ PTAL. Thanks! https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/palette/palette_utils.cc (right): https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/palette/palette_utils.cc:34: for (ash::WmWindow* window : ash::WmShell::Get()->GetAllRootWindows()) { Is there a better way to figure out if an event is targeting a given view?
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...)
Looks like windows might need some ifdefs https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/palette/palette_tray.cc (right): https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/palette/palette_tray.cc:223: bool PaletteTray::ContainsPointInScreen(const gfx::Point& p) { nit: p -> point or similar https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/palette/palette_tray.cc:224: return (icon_ && icon_->GetBoundsInScreen().Contains(p.x(), p.y())) || nit: This might be more readable as multiple early returns, like if (icon_ && icon_->blah) return true; return bubble_ && bubble_->blah https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/palette/palette_utils.cc (right): https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/palette/palette_utils.cc:32: ash::WmWindow* primary = ash::WmShell::Get()->GetPrimaryRootWindow(); no need for ash:: in this function https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/palette/palette_utils.cc:34: for (ash::WmWindow* window : ash::WmShell::Get()->GetAllRootWindows()) { On 2016/09/01 22:31:31, jdufault wrote: > Is there a better way to figure out if an event is targeting a given view? This seems reasonable to me. https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/palette/palette_utils.cc:41: ash::WmShelf* shelf = window->GetRootWindowController()->GetShelf(); WmShelf::ForWindow() is useful here, lets you skip wm_root_window_controller.h include https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/palette/palette_utils.cc:43: shelf->shelf_widget()->status_area_widget()->palette_tray(); nit: shelf->GetStatusAreaWidget() will let you skip a shelf_widget.h include https://codereview.chromium.org/2303963002/diff/1/ash/magnifier/partial_magni... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2303963002/diff/1/ash/magnifier/partial_magni... ash/magnifier/partial_magnification_controller.cc:206: aura::Window* event_root = target->GetRootWindow(); I would compute point_in_screen here (using ui/wm/core/coordinate_conversion.h) and pass the screen location into PaletteContainsEvent (and maybe rename that to PaletteContainsPoint / PaletteContainsPointInScreen). It would simplify the other function.
Oh, the CL description could use a "why", like "this lets you turn off the magnifier by tapping on the palette icon"
The CQ bit was checked by jdufault@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...
Description was changed from ========== cros/ash: If the partial magnifier is active, do not consume input events if over palette views. TEST=ash_unittests BUG=642535 ========== to ========== cros/ash: If the partial magnifier is active, do not consume input events if over palette views. This allows the user to disable the magnifier and use the palette while the magnifier is active. TEST=ash_unittests BUG=642535 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jdufault@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 jdufault@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
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.
https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/palette/palette_tray.cc (right): https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/palette/palette_tray.cc:223: bool PaletteTray::ContainsPointInScreen(const gfx::Point& p) { On 2016/09/01 23:16:02, James Cook wrote: > nit: p -> point or similar Using p cleaned up some of the formatting to avoid line breaks. I've reverted back to point. https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/palette/palette_tray.cc:224: return (icon_ && icon_->GetBoundsInScreen().Contains(p.x(), p.y())) || On 2016/09/01 23:16:02, James Cook wrote: > nit: This might be more readable as multiple early returns, like > > if (icon_ && icon_->blah) > return true; > return bubble_ && bubble_->blah Also something I was toying with. Done. https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/palette/palette_utils.cc (right): https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/palette/palette_utils.cc:32: ash::WmWindow* primary = ash::WmShell::Get()->GetPrimaryRootWindow(); On 2016/09/01 23:16:02, James Cook wrote: > no need for ash:: in this function Done. https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/palette/palette_utils.cc:41: ash::WmShelf* shelf = window->GetRootWindowController()->GetShelf(); On 2016/09/01 23:16:02, James Cook wrote: > WmShelf::ForWindow() is useful here, lets you skip wm_root_window_controller.h > include Done. https://codereview.chromium.org/2303963002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/palette/palette_utils.cc:43: shelf->shelf_widget()->status_area_widget()->palette_tray(); On 2016/09/01 23:16:02, James Cook wrote: > nit: shelf->GetStatusAreaWidget() will let you skip a shelf_widget.h include Done. https://codereview.chromium.org/2303963002/diff/1/ash/magnifier/partial_magni... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2303963002/diff/1/ash/magnifier/partial_magni... ash/magnifier/partial_magnification_controller.cc:206: aura::Window* event_root = target->GetRootWindow(); On 2016/09/01 23:16:02, James Cook wrote: > I would compute point_in_screen here (using ui/wm/core/coordinate_conversion.h) > and pass the screen location into PaletteContainsEvent (and maybe rename that to > PaletteContainsPoint / PaletteContainsPointInScreen). It would simplify the > other function. Done.
https://codereview.chromium.org/2303963002/diff/60001/ash/magnifier/partial_m... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2303963002/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:64: #if defined(OS_CHROMEOS) I'm leaning towards making this entire file cros only, since it is only used on cros and only works with pen events.
LGTM with nits https://codereview.chromium.org/2303963002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tray.cc (right): https://codereview.chromium.org/2303963002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tray.cc:224: if (icon_ && icon_->GetBoundsInScreen().Contains(point.x(), point.y())) nit: You can just do Contains(point). Sorry I didn't notice before. https://codereview.chromium.org/2303963002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_utils.h (right): https://codereview.chromium.org/2303963002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_utils.h:25: ASH_EXPORT bool PaletteContainsPointInScreen(const gfx::Point& point); optional nit: Do you anticipate using this function in code other than PartialMagnificationController? If not, I would move the function there. (I find these util files hurt readability. In particular, since there is no class or namespace declaration here, it's hard to figure out which file contains PaletteContainsPointInScreen by just looking at the call site.) https://codereview.chromium.org/2303963002/diff/60001/ash/magnifier/partial_m... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2303963002/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:64: #if defined(OS_CHROMEOS) On 2016/09/02 20:22:32, jdufault wrote: > I'm leaning towards making this entire file cros only, since it is only used on > cros and only works with pen events. That's fine with me.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2303963002/#ps80001 (title: "Address comments")
https://codereview.chromium.org/2303963002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tray.cc (right): https://codereview.chromium.org/2303963002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tray.cc:224: if (icon_ && icon_->GetBoundsInScreen().Contains(point.x(), point.y())) On 2016/09/06 16:16:31, James Cook wrote: > nit: You can just do Contains(point). Sorry I didn't notice before. Done; I was looking for that override but somehow missed it. https://codereview.chromium.org/2303963002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_utils.h (right): https://codereview.chromium.org/2303963002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_utils.h:25: ASH_EXPORT bool PaletteContainsPointInScreen(const gfx::Point& point); On 2016/09/06 16:16:32, James Cook wrote: > optional nit: Do you anticipate using this function in code other than > PartialMagnificationController? If not, I would move the function there. > > (I find these util files hurt readability. In particular, since there is no > class or namespace declaration here, it's hard to figure out which file contains > PaletteContainsPointInScreen by just looking at the call site.) Yep, it will be used by the laser pointer.
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 ========== cros/ash: If the partial magnifier is active, do not consume input events if over palette views. This allows the user to disable the magnifier and use the palette while the magnifier is active. TEST=ash_unittests BUG=642535 ========== to ========== cros/ash: If the partial magnifier is active, do not consume input events if over palette views. This allows the user to disable the magnifier and use the palette while the magnifier is active. TEST=ash_unittests BUG=642535 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== cros/ash: If the partial magnifier is active, do not consume input events if over palette views. This allows the user to disable the magnifier and use the palette while the magnifier is active. TEST=ash_unittests BUG=642535 ========== to ========== cros/ash: If the partial magnifier is active, do not consume input events if over palette views. This allows the user to disable the magnifier and use the palette while the magnifier is active. TEST=ash_unittests BUG=642535 Committed: https://crrev.com/d8d4991f93a15d1d2c388f05e9b93c3a8eb2fa76 Cr-Commit-Position: refs/heads/master@{#417124} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d8d4991f93a15d1d2c388f05e9b93c3a8eb2fa76 Cr-Commit-Position: refs/heads/master@{#417124} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
