|
|
DescriptionAuto open and close the palette on an eject event.
BUG=616112
TBR=oshima@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/d57fbda9be63adc83e769c52759d9a7e55b65225
Cr-Commit-Position: refs/heads/master@{#414592}
Patch Set 1 : Initial upload #
Total comments: 6
Patch Set 2 : Rename method in parent CL #Patch Set 3 : Call bubble_.reset() directly #Patch Set 4 : Rebase #Patch Set 5 : Fix compile #Patch Set 6 : Rebase onto master #Patch Set 7 : Rebase on master #
Total comments: 8
Patch Set 8 : Address comments #Patch Set 9 : Fix mash #Patch Set 10 : Make PaletteDelegate pure virtual once more #
Total comments: 2
Patch Set 11 : Use weak ptr #Messages
Total messages: 96 (62 generated)
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: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 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 ========== Auto open the palette on an eject event. BUG=616112 ========== to ========== Auto open the palette on an eject event. BUG=616112 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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 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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Auto open the palette on an eject event. BUG=616112 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Auto open and close the palette on an eject event. BUG=616112 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
jdufault@chromium.org changed reviewers: + oshima@chromium.org, stevenjb@chromium.org
oshima@, PTAL at ash/* stevenjb@, PTAL at chrome/* Thanks!
stevenjb@, please also take a look at ash/*. Thanks!
https://codereview.chromium.org/2234203002/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/eject_controller.cc (right): https://codereview.chromium.org/2234203002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/eject_controller.cc:22: bool ejected = ui::InputDeviceManager::GetInstance()->StylusEject(); I can't find the CL that introduces StylusEject, but it seems like it should be StylusEjected() or GetStylusState(). 'Eject' makes sense as an event name, but not as a state query. https://codereview.chromium.org/2234203002/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tray.cc (right): https://codereview.chromium.org/2234203002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tray.cc:273: void PaletteTray::HidePalette() { The name of this is extra confusing, it should probably be 'HideBubble()', at least for now.
https://codereview.chromium.org/2234203002/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/eject_controller.cc (right): https://codereview.chromium.org/2234203002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/eject_controller.cc:22: bool ejected = ui::InputDeviceManager::GetInstance()->StylusEject(); On 2016/08/15 20:18:25, stevenjb wrote: > I can't find the CL that introduces StylusEject, but it seems like it should be > StylusEjected() or GetStylusState(). 'Eject' makes sense as an event name, but > not as a state query. Done. https://codereview.chromium.org/2234203002/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tray.cc (right): https://codereview.chromium.org/2234203002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tray.cc:273: void PaletteTray::HidePalette() { On 2016/08/15 20:18:25, stevenjb wrote: > The name of this is extra confusing, it should probably be 'HideBubble()', at > least for now. There's already a HideBubble(...) from views::TrayBubbleView::Delegate. The PaletteToolManager override doesn't take a parameter, so they need different names. :/ I'm happy to do something else if you have a good idea.
https://codereview.chromium.org/2234203002/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tray.cc (right): https://codereview.chromium.org/2234203002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tray.cc:273: void PaletteTray::HidePalette() { On 2016/08/15 21:55:04, jdufault wrote: > On 2016/08/15 20:18:25, stevenjb wrote: > > The name of this is extra confusing, it should probably be 'HideBubble()', at > > least for now. > > There's already a HideBubble(...) from views::TrayBubbleView::Delegate. The > PaletteToolManager override doesn't take a parameter, so they need different > names. :/ > > I'm happy to do something else if you have a good idea. Oh, so there is. I didn't realize that this was a PaletteToolManager::Delegate method. Calling it from OnEject() is confusing, especially with the OpenBubble vs HidePalette asymetry. Instead we should either just call bubble_.reset() directly, or create a private method like CloseBubble or ResetBubble that we call here and in OnEject (but I wouldn't do that unless we anticipate additional logic beyond bubble_.reset().
https://codereview.chromium.org/2234203002/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tray.cc (right): https://codereview.chromium.org/2234203002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tray.cc:273: void PaletteTray::HidePalette() { On 2016/08/15 22:41:29, stevenjb wrote: > On 2016/08/15 21:55:04, jdufault wrote: > > On 2016/08/15 20:18:25, stevenjb wrote: > > > The name of this is extra confusing, it should probably be 'HideBubble()', > at > > > least for now. > > > > There's already a HideBubble(...) from views::TrayBubbleView::Delegate. The > > PaletteToolManager override doesn't take a parameter, so they need different > > names. :/ > > > > I'm happy to do something else if you have a good idea. > > Oh, so there is. I didn't realize that this was a PaletteToolManager::Delegate > method. Calling it from OnEject() is confusing, especially with the OpenBubble > vs HidePalette asymetry. > > Instead we should either just call bubble_.reset() directly, or create a private > method like CloseBubble or ResetBubble that we call here and in OnEject (but I > wouldn't do that unless we anticipate additional logic beyond bubble_.reset(). I've changed all HidePalette calls to just call bubble_.reset() directly.
lgtm
Description was changed from ========== Auto open and close the palette on an eject event. BUG=616112 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Auto open and close the palette on an eject event. BUG=616112 TBR=oshima@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
TBR oshima@ for trivial changes on ash/common/palette_delegate.h and ash/shell/shell_delegate_impl.cc.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2234203002/#ps140001 (title: "Rebase")
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2234203002/#ps160001 (title: "Fix compile")
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: 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 jdufault@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2234203002/#ps180001 (title: "Rebase onto master")
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jdufault@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) 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 jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2234203002/#ps200001 (title: "Rebase on master")
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm with nits https://codereview.chromium.org/2234203002/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/eject_controller.h (right): https://codereview.chromium.org/2234203002/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/eject_controller.h:21: EjectController(const OnEjectCallback& on_eject); nit: explicit https://codereview.chromium.org/2234203002/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/palette_tray.cc (right): https://codereview.chromium.org/2234203002/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/palette_tray.cc:331: if (stylus_state == ui::StylusState::INSERTED && bubble_) nit: else if? You may keep it if you prefer this way though. https://codereview.chromium.org/2234203002/diff/200001/ash/common/test/test_p... File ash/common/test/test_palette_delegate.h (right): https://codereview.chromium.org/2234203002/diff/200001/ash/common/test/test_p... ash/common/test/test_palette_delegate.h:42: bool ShouldAutoOpenPalette() override; optional: can this be const method? https://codereview.chromium.org/2234203002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/2234203002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:262: (*s_whitelist)["settings.launch_palette_on_eject_event "] = nit: extra white-space.
https://codereview.chromium.org/2234203002/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/eject_controller.h (right): https://codereview.chromium.org/2234203002/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/eject_controller.h:21: EjectController(const OnEjectCallback& on_eject); On 2016/08/24 23:38:50, oshima wrote: > nit: explicit Done. https://codereview.chromium.org/2234203002/diff/200001/ash/common/system/chro... File ash/common/system/chromeos/palette/palette_tray.cc (right): https://codereview.chromium.org/2234203002/diff/200001/ash/common/system/chro... ash/common/system/chromeos/palette/palette_tray.cc:331: if (stylus_state == ui::StylusState::INSERTED && bubble_) On 2016/08/24 23:38:50, oshima wrote: > nit: else if? > > You may keep it if you prefer this way though. Done. https://codereview.chromium.org/2234203002/diff/200001/ash/common/test/test_p... File ash/common/test/test_palette_delegate.h (right): https://codereview.chromium.org/2234203002/diff/200001/ash/common/test/test_p... ash/common/test/test_palette_delegate.h:42: bool ShouldAutoOpenPalette() override; On 2016/08/24 23:38:50, oshima wrote: > optional: can this be const method? Leaving as non-const for the time being to stay parallel with HasNoteApp(). HasNoteApp needs to be non-const for TestPaletteDelegate. I want to convert TestPaletteDelegate to be an actual mock class, which should at that point allow for conversion to const methods. https://codereview.chromium.org/2234203002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/2234203002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:262: (*s_whitelist)["settings.launch_palette_on_eject_event "] = On 2016/08/24 23:38:50, oshima wrote: > nit: extra white-space. Done, good catch!
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2234203002/#ps220001 (title: "Address comments")
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: 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 jdufault@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: 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 jdufault@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 jdufault@chromium.org
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 checked by jdufault@chromium.org to run a CQ dry run
Patchset #9 (id:240001) 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...
Sorry the run around. PTAL again. The previous implementation ended up crashing mash - when trying to reproduce locally I the tests were failing for a different reason, but I eventually also found an issue in this patch as well. I've moved the eject_controller logic into palette_delegate. ui/input doesn't look to be supported in mash atm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_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 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.
https://codereview.chromium.org/2234203002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/palette/palette_tray.cc (right): https://codereview.chromium.org/2234203002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/palette/palette_tray.cc:141: base::Bind(&PaletteTray::OnStylusStateChanged, base::Unretained(this))); Need to pass this as a weak_ptr. It's always iffy to rely on destruction order and with views it is especially uncertain.
https://codereview.chromium.org/2234203002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/palette/palette_tray.cc (right): https://codereview.chromium.org/2234203002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/palette/palette_tray.cc:141: base::Bind(&PaletteTray::OnStylusStateChanged, base::Unretained(this))); On 2016/08/25 22:48:56, stevenjb wrote: > Need to pass this as a weak_ptr. It's always iffy to rely on destruction order > and with views it is especially uncertain. Done.
lgtm
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2234203002/#ps300001 (title: "Use weak ptr")
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 ========== Auto open and close the palette on an eject event. BUG=616112 TBR=oshima@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Auto open and close the palette on an eject event. BUG=616112 TBR=oshima@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #11 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Auto open and close the palette on an eject event. BUG=616112 TBR=oshima@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Auto open and close the palette on an eject event. BUG=616112 TBR=oshima@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/d57fbda9be63adc83e769c52759d9a7e55b65225 Cr-Commit-Position: refs/heads/master@{#414592} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/d57fbda9be63adc83e769c52759d9a7e55b65225 Cr-Commit-Position: refs/heads/master@{#414592} |