New issue
Advanced search Search tips

Issue 912658 link

Starred by 1 user
Status: Started
Owner:
qnnguyen@chromium.org
Cc:
jdufault@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Break login_bubble.cc/.h into separate LoginBaseBubbleView subclasses

Project Member Reported by qnnguyen@chromium.org, Dec 6 
There's a lot of redundancy in how we handle the different bubbles in the login screen. Instead of using one wrapper class to launch the different bubble types (which also constantly deletes and re-initializes the bubbles), we should have a separate subclass for each bubble that uses a common Show/Hide method.

This change will involve migrating most of the event handling and widget handling logic to LoginBaseBubbleView, and then moving the bubble types into their own files.
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 7 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a61287973e8f4522ed673ac1a77de0dc56eb1bc8

commit a61287973e8f4522ed673ac1a77de0dc56eb1bc8
Author: Quan Nguyen <qnnguyen@chromium.org>
Date: Fri Dec 07 01:18:53 2018

cros: Move most event/widget handling logic out of LoginBubble

Changes made:
- Move the (keyboard/click/tap) event handling logic into a
  LoginBubbleHandler class.
- Move the widget handling into LoginBaseBubbleView itself

Bug: 912658
Change-Id: Ia1a9bf4b8d847ef74ef1716e73dfe1b16b8d19b2
Reviewed-on: https://chromium-review.googlesource.com/c/1366516
Commit-Queue: Quan Nguyen <qnnguyen@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614544}
[modify] https://crrev.com/a61287973e8f4522ed673ac1a77de0dc56eb1bc8/ash/login/ui/login_base_bubble_view.cc
[modify] https://crrev.com/a61287973e8f4522ed673ac1a77de0dc56eb1bc8/ash/login/ui/login_base_bubble_view.h
[modify] https://crrev.com/a61287973e8f4522ed673ac1a77de0dc56eb1bc8/ash/login/ui/login_bubble.cc
[modify] https://crrev.com/a61287973e8f4522ed673ac1a77de0dc56eb1bc8/ash/login/ui/login_bubble.h
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 7 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bb9ca465cac1a3ec4d0670844865b40efb003c11

commit bb9ca465cac1a3ec4d0670844865b40efb003c11
Author: Takashi Sakamoto <tasak@google.com>
Date: Fri Dec 07 03:49:55 2018

Revert "cros: Move most event/widget handling logic out of LoginBubble"

This reverts commit a61287973e8f4522ed673ac1a77de0dc56eb1bc8.

Reason for revert: Suspect Failure single_process_mash_ash_unittests Failure ash_unittests:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-rel/16943
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8927839167159249024/+/steps/single_process_mash_ash_unittests/0/logs/LoginExpandedPublicAccountViewTest.ChangeMenuSelection/0
---
[ RUN      ] LoginExpandedPublicAccountViewTest.ChangeMenuSelection
Received signal 11 SEGV_MAPERR ffffe03f000008fe
#0 0x5631a3caedbf base::debug::StackTrace::StackTrace()
#1 0x5631a3cae941 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7faa9920f330 <unknown>
#3 0x5631a3aafbbf ash::LoginBubble::~LoginBubble()
#4 0x5631a3ab806c ash::RightPaneView::~RightPaneView()
#5 0x5631a3ab810e ash::RightPaneView::~RightPaneView()
#6 0x5631a40385d4 views::View::~View()
#7 0x5631a3ab6df9 ash::LoginExpandedPublicAccountView::~LoginExpandedPublicAccountView()
#8 0x5631a40385d4 views::View::~View()
#9 0x5631a300afce ash::(anonymous namespace)::DragTestView::~DragTestView()
#10 0x5631a4039b42 views::View::DoRemoveChildView()
#11 0x5631a403a175 views::View::RemoveAllChildViews()
#12 0x5631a4043b81 views::internal::RootView::~RootView()
#13 0x5631a400ef0e views::MenuHostRootView::~MenuHostRootView()
#14 0x5631a404615f views::Widget::~Widget()
#15 0x5631a34d645e exo::(anonymous namespace)::ShellSurfaceWidget::~ShellSurfaceWidget()
#16 0x5631a30c36f0 ash::LoginTestBase::TearDown()

Original change's description:
> cros: Move most event/widget handling logic out of LoginBubble
> 
> Changes made:
> - Move the (keyboard/click/tap) event handling logic into a
>   LoginBubbleHandler class.
> - Move the widget handling into LoginBaseBubbleView itself
> 
> Bug: 912658
> Change-Id: Ia1a9bf4b8d847ef74ef1716e73dfe1b16b8d19b2
> Reviewed-on: https://chromium-review.googlesource.com/c/1366516
> Commit-Queue: Quan Nguyen <qnnguyen@chromium.org>
> Reviewed-by: Jacob Dufault <jdufault@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#614544}

TBR=jdufault@chromium.org,qnnguyen@chromium.org

Change-Id: Ia9b9059f24f6aa6a8551c5c6bc5c76caafe592df
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 912658
Reviewed-on: https://chromium-review.googlesource.com/c/1367070
Reviewed-by: Takashi Sakamoto <tasak@google.com>
Commit-Queue: Takashi Sakamoto <tasak@google.com>
Cr-Commit-Position: refs/heads/master@{#614579}
[modify] https://crrev.com/bb9ca465cac1a3ec4d0670844865b40efb003c11/ash/login/ui/login_base_bubble_view.cc
[modify] https://crrev.com/bb9ca465cac1a3ec4d0670844865b40efb003c11/ash/login/ui/login_base_bubble_view.h
[modify] https://crrev.com/bb9ca465cac1a3ec4d0670844865b40efb003c11/ash/login/ui/login_bubble.cc
[modify] https://crrev.com/bb9ca465cac1a3ec4d0670844865b40efb003c11/ash/login/ui/login_bubble.h
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 15 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/87f297b2473f64fd56724add74f06392257d1d69

commit 87f297b2473f64fd56724add74f06392257d1d69
Author: Quan Nguyen <qnnguyen@chromium.org>
Date: Sat Dec 15 00:31:05 2018

cros: fix a crash that occurs after removing a user

Clicking on the "remove user" button the second time would trigger an animation
and then start tearing down the widget tree for the user to be removed (as well
as the user menu bubble). To prevent these two actions from racing, we
immediately hide the user menu widget before starting the teardown. We also
check the widget state early in key/mouse event handlers to prevent crashes
that could happen due to events on an orphaned view that hasn't been deleted
yet.

Bug: 912658
Change-Id: Iccefd785b5a7c8c97829e973bb4f132c406cf58f
Reviewed-on: https://chromium-review.googlesource.com/c/1379219
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Commit-Queue: Quan Nguyen <qnnguyen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616892}
[modify] https://crrev.com/87f297b2473f64fd56724add74f06392257d1d69/ash/login/ui/login_base_bubble_view.cc
[modify] https://crrev.com/87f297b2473f64fd56724add74f06392257d1d69/ash/login/ui/login_bubble.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 17 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a9cdfa9baf9dc254995520d9bb0feabb35f90920

commit a9cdfa9baf9dc254995520d9bb0feabb35f90920
Author: Quan Nguyen <qnnguyen@chromium.org>
Date: Mon Dec 17 23:28:04 2018

cros: Break error and tooltip bubbles into standalone classes

Bug: 912658
Change-Id: I9d2f6f476cc2b8f9972fa2e7dbb4f4fdbc17ba12
Reviewed-on: https://chromium-review.googlesource.com/c/1373033
Commit-Queue: Quan Nguyen <qnnguyen@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617281}
[modify] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/BUILD.gn
[modify] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/login/ui/lock_contents_view.cc
[modify] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/login/ui/lock_contents_view.h
[modify] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/login/ui/login_base_bubble_view.cc
[modify] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/login/ui/login_base_bubble_view.h
[add] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/login/ui/login_base_bubble_view_unittest.cc
[modify] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/login/ui/login_bubble.cc
[modify] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/login/ui/login_bubble.h
[modify] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/login/ui/login_bubble_unittest.cc
[add] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/login/ui/login_error_bubble.cc
[add] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/login/ui/login_error_bubble.h
[add] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/login/ui/login_error_bubble_unittest.cc
[add] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/login/ui/login_tooltip_view.cc
[add] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/login/ui/login_tooltip_view.h
[modify] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/login/ui/views_utils.cc
[modify] https://crrev.com/a9cdfa9baf9dc254995520d9bb0feabb35f90920/ash/login/ui/views_utils.h
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 11 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2d6270765968844988d3c031b0f3d0867a1f2eda

commit 2d6270765968844988d3c031b0f3d0867a1f2eda
Author: Quan Nguyen <qnnguyen@chromium.org>
Date: Fri Jan 11 21:10:34 2019

cros: Break LoginUserMenuView into a separate class

Bug: 912658
Change-Id: Ib56d677c3ffe5487b7861d0e4a032babd0c4d44d
Reviewed-on: https://chromium-review.googlesource.com/c/1401609
Commit-Queue: Quan Nguyen <qnnguyen@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622146}
[modify] https://crrev.com/2d6270765968844988d3c031b0f3d0867a1f2eda/ash/BUILD.gn
[modify] https://crrev.com/2d6270765968844988d3c031b0f3d0867a1f2eda/ash/login/ui/lock_contents_view_unittest.cc
[modify] https://crrev.com/2d6270765968844988d3c031b0f3d0867a1f2eda/ash/login/ui/lock_screen_sanity_unittest.cc
[modify] https://crrev.com/2d6270765968844988d3c031b0f3d0867a1f2eda/ash/login/ui/login_base_bubble_view.cc
[modify] https://crrev.com/2d6270765968844988d3c031b0f3d0867a1f2eda/ash/login/ui/login_base_bubble_view.h
[modify] https://crrev.com/2d6270765968844988d3c031b0f3d0867a1f2eda/ash/login/ui/login_bubble.cc
[modify] https://crrev.com/2d6270765968844988d3c031b0f3d0867a1f2eda/ash/login/ui/login_bubble.h
[modify] https://crrev.com/2d6270765968844988d3c031b0f3d0867a1f2eda/ash/login/ui/login_bubble_unittest.cc
[add] https://crrev.com/2d6270765968844988d3c031b0f3d0867a1f2eda/ash/login/ui/login_user_menu_view.cc
[add] https://crrev.com/2d6270765968844988d3c031b0f3d0867a1f2eda/ash/login/ui/login_user_menu_view.h
[add] https://crrev.com/2d6270765968844988d3c031b0f3d0867a1f2eda/ash/login/ui/login_user_menu_view_unittest.cc
[modify] https://crrev.com/2d6270765968844988d3c031b0f3d0867a1f2eda/ash/login/ui/login_user_view.cc
[modify] https://crrev.com/2d6270765968844988d3c031b0f3d0867a1f2eda/ash/login/ui/login_user_view.h
[modify] https://crrev.com/2d6270765968844988d3c031b0f3d0867a1f2eda/ash/login/ui/views_utils.cc
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 15 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f01cafd8e288a805e50b434c1e0fd86b3bb08e27

commit f01cafd8e288a805e50b434c1e0fd86b3bb08e27
Author: Quan Nguyen <qnnguyen@chromium.org>
Date: Tue Jan 15 00:18:01 2019

cros: Use LoginMenuView directly in LoginExpandedPublicAccountView

This is the last LoginBaseBubbleView subclass to be removed from LoginBubble.
Therefore, as part of this CL we also remove the LoginBubble class entirely.

Bug: 912658
Change-Id: I48c2e593d5ad550619b28874db9b6edc53f36b11
Reviewed-on: https://chromium-review.googlesource.com/c/1407440
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Commit-Queue: Quan Nguyen <qnnguyen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622646}
[modify] https://crrev.com/f01cafd8e288a805e50b434c1e0fd86b3bb08e27/ash/BUILD.gn
[modify] https://crrev.com/f01cafd8e288a805e50b434c1e0fd86b3bb08e27/ash/login/ui/lock_contents_view_unittest.cc
[modify] https://crrev.com/f01cafd8e288a805e50b434c1e0fd86b3bb08e27/ash/login/ui/lock_screen_sanity_unittest.cc
[delete] https://crrev.com/50bac9035847e7cae6678ec2b34f67366c0ab929/ash/login/ui/login_bubble.cc
[delete] https://crrev.com/50bac9035847e7cae6678ec2b34f67366c0ab929/ash/login/ui/login_bubble.h
[delete] https://crrev.com/50bac9035847e7cae6678ec2b34f67366c0ab929/ash/login/ui/login_bubble_unittest.cc
[modify] https://crrev.com/f01cafd8e288a805e50b434c1e0fd86b3bb08e27/ash/login/ui/login_expanded_public_account_view.cc
[modify] https://crrev.com/f01cafd8e288a805e50b434c1e0fd86b3bb08e27/ash/login/ui/login_expanded_public_account_view.h
[modify] https://crrev.com/f01cafd8e288a805e50b434c1e0fd86b3bb08e27/ash/login/ui/login_expanded_public_account_view_unittest.cc
[modify] https://crrev.com/f01cafd8e288a805e50b434c1e0fd86b3bb08e27/ash/login/ui/login_user_view.cc
►

Sign in to add a comment