Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(605)

Issue 17127003: Refine DialogClientView button code and unit tests. (Closed)

Created:
7 years, 6 months ago by msw
Modified:
7 years, 6 months ago
Reviewers:
sky
CC:
chromium-reviews, tfarina, rhashimoto, Evan Stade
Visibility:
Public.

Description

Refine DialogClientView button code and unit tests. Add an escape accelerator to the ok button only if there is *not* a cancel button. Move FocusManager/ChangeListener cleanup to ViewHierarchyChanged. Clear ok/cancel/default button pointers on ViewHierarchyChanged removal. Remove view_unittest MockMenuModel and related use. ( these were left unused by http://crrev.com/87490 ) Move tests and helpers to dialog_delegate_unittest, cleanup. Merge, cleanup, and add tests, no loss of coverage. BUG=241451, 166075 TEST=Dialogs with just an [Ok] button will accept on [Esc]; no crashes. R=sky@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206950

Patch Set 1 #

Patch Set 2 : Handle button removal; add and simplify tests. #

Patch Set 3 : Simplify DialogViewTest.ButtonDropDownClickTest, remove dead code. #

Patch Set 4 : Simplify DialogViewTest.ButtonDropDownClickTest, remove dead code. #

Patch Set 5 : Fix DialogClientView::UpdateDialogButtons logic error, more cleanup. #

Patch Set 6 : More cleanup, combine tests. #

Patch Set 7 : Merge dialog tests into dialog_delegate_unittest; simplify. #

Patch Set 8 : Additional cleanup and refinement. #

Patch Set 9 : Remove unnecessary override. #

Total comments: 2

Patch Set 10 : Revert explicit RemoveChildView calls. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+248 lines, -303 lines) Patch
M ui/views/view_unittest.cc View 1 2 3 4 5 6 7 3 chunks +0 lines, -258 lines 0 comments Download
M ui/views/window/dialog_client_view.cc View 1 2 3 4 5 6 7 8 9 3 chunks +12 lines, -4 lines 0 comments Download
M ui/views/window/dialog_client_view_unittest.cc View 1 2 3 4 5 6 7 8 9 5 chunks +60 lines, -33 lines 0 comments Download
M ui/views/window/dialog_delegate_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +176 lines, -8 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
msw
Hey Scott, please take a look; thanks!
7 years, 6 months ago (2013-06-17 19:49:27 UTC) #1
sky
https://codereview.chromium.org/17127003/diff/28001/ui/views/window/dialog_client_view.cc File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/17127003/diff/28001/ui/views/window/dialog_client_view.cc#newcode80 ui/views/window/dialog_client_view.cc:80: RemoveChildView(ok_button_); Is there a particular reason you are adding ...
7 years, 6 months ago (2013-06-17 20:30:14 UTC) #2
msw
Thanks for the tip! Please take another look. https://codereview.chromium.org/17127003/diff/28001/ui/views/window/dialog_client_view.cc File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/17127003/diff/28001/ui/views/window/dialog_client_view.cc#newcode80 ui/views/window/dialog_client_view.cc:80: RemoveChildView(ok_button_); ...
7 years, 6 months ago (2013-06-17 21:10:55 UTC) #3
sky
LGTM
7 years, 6 months ago (2013-06-17 22:59:30 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/17127003/38001
7 years, 6 months ago (2013-06-17 23:06:05 UTC) #5
commit-bot: I haz the power
7 years, 6 months ago (2013-06-18 12:21:52 UTC) #6
Message was sent while issue was closed.
Change committed as 206950

Powered by Google App Engine
This is Rietveld 408576698