Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2201323002:
Simplify VirtualKeyboardTray (Closed)
DescriptionSimplify VirtualKeyboardTray
Despite being an ActionableView, VirtualKeyboardTray unnecessarily hosts
a button and calls its ActionableView::PerformAction() from its
ButtonListener::ButtonPressed(). We can get rid of the button and let
the ActionableView handle the clicks on the item.
This patch also fixes active background of the VirtualKeyboardTray to
match active status of the virtual keyboard correctly.
This is a step forward to enable ink drop ripples on
VirtualKeyboardTray.
BUG=612579
TEST=none
Committed: https://crrev.com/85a36bf855820ffab6e2de766b5b069b5a87496c
Cr-Commit-Position: refs/heads/master@{#410413}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Addressed review comments #Patch Set 3 : Removed HasObserver() #
Total comments: 2
Patch Set 4 : Added some comments #
Messages
Total messages: 33 (19 generated)
The CQ bit was checked by mohsen@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 ========== Simplified VirtualKeyboardTray Despite being an ActionableView, VirtualKeyboardTray unnecessarily hosts a button and calls its ActionableView::PerformAction() from its ButtonListener::ButtonPressed(). We can get rid of the button and let the ActionableView handle the clicks on the item. This patch also fixes active background of the VirtualKeyboardTray to match active status of the virtual keyboard correctly. BUG=612579 ========== to ========== Simplified VirtualKeyboardTray Despite being an ActionableView, VirtualKeyboardTray unnecessarily hosts a button and calls its ActionableView::PerformAction() from its ButtonListener::ButtonPressed(). We can get rid of the button and let the ActionableView handle the clicks on the item. This patch handles its border sizing similar to OverviewButtonTray. This patch also fixes active background of the VirtualKeyboardTray to match active status of the virtual keyboard correctly. BUG=612579 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Simplified VirtualKeyboardTray Despite being an ActionableView, VirtualKeyboardTray unnecessarily hosts a button and calls its ActionableView::PerformAction() from its ButtonListener::ButtonPressed(). We can get rid of the button and let the ActionableView handle the clicks on the item. This patch handles its border sizing similar to OverviewButtonTray. This patch also fixes active background of the VirtualKeyboardTray to match active status of the virtual keyboard correctly. BUG=612579 ========== to ========== Simplify VirtualKeyboardTray Despite being an ActionableView, VirtualKeyboardTray unnecessarily hosts a button and calls its ActionableView::PerformAction() from its ButtonListener::ButtonPressed(). We can get rid of the button and let the ActionableView handle the clicks on the item. This patch handles its border sizing similar to OverviewButtonTray. This patch also fixes active background of the VirtualKeyboardTray to match active status of the virtual keyboard correctly. BUG=612579 ==========
Description was changed from ========== Simplify VirtualKeyboardTray Despite being an ActionableView, VirtualKeyboardTray unnecessarily hosts a button and calls its ActionableView::PerformAction() from its ButtonListener::ButtonPressed(). We can get rid of the button and let the ActionableView handle the clicks on the item. This patch handles its border sizing similar to OverviewButtonTray. This patch also fixes active background of the VirtualKeyboardTray to match active status of the virtual keyboard correctly. BUG=612579 ========== to ========== Simplify VirtualKeyboardTray Despite being an ActionableView, VirtualKeyboardTray unnecessarily hosts a button and calls its ActionableView::PerformAction() from its ButtonListener::ButtonPressed(). We can get rid of the button and let the ActionableView handle the clicks on the item. This patch also fixes active background of the VirtualKeyboardTray to match active status of the virtual keyboard correctly. BUG=612579 ==========
Description was changed from ========== Simplify VirtualKeyboardTray Despite being an ActionableView, VirtualKeyboardTray unnecessarily hosts a button and calls its ActionableView::PerformAction() from its ButtonListener::ButtonPressed(). We can get rid of the button and let the ActionableView handle the clicks on the item. This patch also fixes active background of the VirtualKeyboardTray to match active status of the virtual keyboard correctly. BUG=612579 ========== to ========== Simplify VirtualKeyboardTray Despite being an ActionableView, VirtualKeyboardTray unnecessarily hosts a button and calls its ActionableView::PerformAction() from its ButtonListener::ButtonPressed(). We can get rid of the button and let the ActionableView handle the clicks on the item. This patch also fixes active background of the VirtualKeyboardTray to match active status of the virtual keyboard correctly. This is a step forward to enable ink drop ripples on VirtualKeyboardTray. BUG=612579 ==========
mohsen@chromium.org changed reviewers: + sadrul@chromium.org
Please take a look...
https://codereview.chromium.org/2201323002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/2201323002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:81: void VirtualKeyboardTray::OnKeyboardEnabledStateChanged(bool new_value) { Call this |enabled|, or |new_enabled|. https://codereview.chromium.org/2201323002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:91: SetDrawBackgroundAsActive(!new_bounds.IsEmpty()); It's a bit unfortunate that we don't have anything on KeyboardUIObserver for 'enabled but show/hidden' notifications. https://codereview.chromium.org/2201323002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:95: const gfx::ImageSkia image = icon_->GetImage(); const-ref? (moving existing code, I know, but fix since you are touching it?) https://codereview.chromium.org/2201323002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:114: keyboard_controller->AddObserver(this); Why would you need to check HasObserver() here? (or below in Unobserve?)
https://codereview.chromium.org/2201323002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/2201323002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:81: void VirtualKeyboardTray::OnKeyboardEnabledStateChanged(bool new_value) { On 2016/08/04 at 16:36:37, sadrul wrote: > Call this |enabled|, or |new_enabled|. Done (Renamed to |new_enabled|). https://codereview.chromium.org/2201323002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:95: const gfx::ImageSkia image = icon_->GetImage(); On 2016/08/04 at 16:36:37, sadrul wrote: > const-ref? (moving existing code, I know, but fix since you are touching it?) Done. https://codereview.chromium.org/2201323002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:114: keyboard_controller->AddObserver(this); On 2016/08/04 at 16:36:37, sadrul wrote: > Why would you need to check HasObserver() here? (or below in Unobserve?) OnKeyboardEnabledStateChanged() can be called multiple times. For example, in one run, looking at the backtrace, it is called once when virtual keyboard prefs are loaded and once when magnifier prefs are loaded and apparently KeyboardUIImpl doesn't check that the value has not actually changed (it actually doesn't keep track of the old value). The one in the Unobserve() is not really needed as the RemoveObserver() will ignore the request. Removed.
The CQ bit was checked by mohsen@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/2201323002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/2201323002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:114: keyboard_controller->AddObserver(this); On 2016/08/04 21:02:38, mohsen wrote: > On 2016/08/04 at 16:36:37, sadrul wrote: > > Why would you need to check HasObserver() here? (or below in Unobserve?) > > OnKeyboardEnabledStateChanged() can be called multiple times. For example, in > one run, looking at the backtrace, it is called once when virtual keyboard prefs > are loaded and once when magnifier prefs are loaded and apparently > KeyboardUIImpl doesn't check that the value has not actually changed (it > actually doesn't keep track of the old value). That seems unfortunate. Can we fix that? At the least, can we track that here? > > The one in the Unobserve() is not really needed as the RemoveObserver() will > ignore the request. Removed.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2201323002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/2201323002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:114: keyboard_controller->AddObserver(this); On 2016/08/05 at 05:50:26, sadrul wrote: > On 2016/08/04 21:02:38, mohsen wrote: > > On 2016/08/04 at 16:36:37, sadrul wrote: > > > Why would you need to check HasObserver() here? (or below in Unobserve?) > > > > OnKeyboardEnabledStateChanged() can be called multiple times. For example, in > > one run, looking at the backtrace, it is called once when virtual keyboard prefs > > are loaded and once when magnifier prefs are loaded and apparently > > KeyboardUIImpl doesn't check that the value has not actually changed (it > > actually doesn't keep track of the old value). > > That seems unfortunate. Can we fix that? At the least, can we track that here? On every accessibility pref change, AccessibilityManager notifies observers of a change and the information about the change is thrown away on its way to the observer (in SystemTrayDelegateChromeOS::OnAccessibilityStatusChanged()). Fixing that seems out of the scope of this CL. However, I can track that in KeyboardUIImpl. Done. > > > > > The one in the Unobserve() is not really needed as the RemoveObserver() will > > ignore the request. Removed.
lgtm. thanks
mohsen@chromium.org changed reviewers: + jamescook@chromium.org
jamescook@: Please take a look...
LGTM with nit https://codereview.chromium.org/2201323002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/2201323002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:83: if (new_enabled) I would add a comment here explaining why you need to add/remove observers. (It looks like the keyboard controller is destroyed whenever the virtual keyboard is closed?)
On 2016/08/05 20:24:25, James Cook wrote: > LGTM with nit > > https://codereview.chromium.org/2201323002/diff/60001/ash/common/system/chrom... > File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc > (right): > > https://codereview.chromium.org/2201323002/diff/60001/ash/common/system/chrom... > ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:83: if > (new_enabled) > I would add a comment here explaining why you need to add/remove observers. (It > looks like the keyboard controller is destroyed whenever the virtual keyboard is > closed?) Also, please update the CL description to include a TEST= line, even if that is just ash_unittests, chrome unit_tests, or none
Description was changed from ========== Simplify VirtualKeyboardTray Despite being an ActionableView, VirtualKeyboardTray unnecessarily hosts a button and calls its ActionableView::PerformAction() from its ButtonListener::ButtonPressed(). We can get rid of the button and let the ActionableView handle the clicks on the item. This patch also fixes active background of the VirtualKeyboardTray to match active status of the virtual keyboard correctly. This is a step forward to enable ink drop ripples on VirtualKeyboardTray. BUG=612579 ========== to ========== Simplify VirtualKeyboardTray Despite being an ActionableView, VirtualKeyboardTray unnecessarily hosts a button and calls its ActionableView::PerformAction() from its ButtonListener::ButtonPressed(). We can get rid of the button and let the ActionableView handle the clicks on the item. This patch also fixes active background of the VirtualKeyboardTray to match active status of the virtual keyboard correctly. This is a step forward to enable ink drop ripples on VirtualKeyboardTray. BUG=612579 TEST=none ==========
https://codereview.chromium.org/2201323002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc (right): https://codereview.chromium.org/2201323002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_tray.cc:83: if (new_enabled) On 2016/08/05 at 20:24:25, James Cook wrote: > I would add a comment here explaining why you need to add/remove observers. (It looks like the keyboard controller is destroyed whenever the virtual keyboard is closed?) Yeah, it is actually destroyed when virtual keyboard is disabled in accessibility settings (probably what you meant). So, the unobserve part is not really necessary. But, I would prefer not to make that assumption and keep UnobserveKeyboardController() here in case something changes in the future. Similarly, it seems that ObserveKeyboardController() in the constructor and UnobserveKeyboardController() in the destructor are not necessary at the moment either since VirtualKeyboardTray is created before and destroyed after keyboard controller. But, again, I'd prefer not make any assumption about that in case something is going to change in the future. WDYT? However, I added some comments.
lgtm
The CQ bit was checked by mohsen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2201323002/#ps80001 (title: "Added some comments")
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 #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Simplify VirtualKeyboardTray Despite being an ActionableView, VirtualKeyboardTray unnecessarily hosts a button and calls its ActionableView::PerformAction() from its ButtonListener::ButtonPressed(). We can get rid of the button and let the ActionableView handle the clicks on the item. This patch also fixes active background of the VirtualKeyboardTray to match active status of the virtual keyboard correctly. This is a step forward to enable ink drop ripples on VirtualKeyboardTray. BUG=612579 TEST=none ========== to ========== Simplify VirtualKeyboardTray Despite being an ActionableView, VirtualKeyboardTray unnecessarily hosts a button and calls its ActionableView::PerformAction() from its ButtonListener::ButtonPressed(). We can get rid of the button and let the ActionableView handle the clicks on the item. This patch also fixes active background of the VirtualKeyboardTray to match active status of the virtual keyboard correctly. This is a step forward to enable ink drop ripples on VirtualKeyboardTray. BUG=612579 TEST=none Committed: https://crrev.com/85a36bf855820ffab6e2de766b5b069b5a87496c Cr-Commit-Position: refs/heads/master@{#410413} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/85a36bf855820ffab6e2de766b5b069b5a87496c Cr-Commit-Position: refs/heads/master@{#410413} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
