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

Issue 2808823002: MacViews: Allows the toolkit-views Manage Passwords Dialog to be used (Closed)

Created:
3 years, 8 months ago by varkha
Modified:
3 years, 8 months ago
CC:
chromium-reviews, tfarina, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org, mac-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

MacViews: Allows the toolkit-views Manage Passwords Dialog to be used Both chrome/browser/ui/views/passwords/credentials_selection_view.* and chrome/browser/ui/cocoa/passwords/credentials_selection_view.* are now included in the same build target so the cocoa version is renamed. This CL also enables fake full keyboard access by default on MacOS for the tests based on InProcessBrowserTest, so that tests don't depend on system setting of the test machine. This makes those tests more like on other platforms, similar to how it is done in views_unittests. BUG=654115 TEST=interactive_ui_test --gtest_filter=ManagePasswordsBubbleViewTest* browser_tests --gtest_filter=ManagePasswordsBubbleDialogViewTest* Review-Url: https://codereview.chromium.org/2808823002 Cr-Commit-Position: refs/heads/master@{#466285} Committed: https://chromium.googlesource.com/chromium/src/+/58cb30d4f4a8cd26893f614658f752c9cefb30dd

Patch Set 1 #

Patch Set 2 : MacViews: Allows the toolkit-views Manage Passwords Dialog to be used #

Total comments: 18

Patch Set 3 : MacViews: Allows the toolkit-views Manage Passwords Dialog to be used (comments) #

Patch Set 4 : MacViews: Allows the toolkit-views Manage Passwords Dialog to be used (tests compile) #

Total comments: 16

Patch Set 5 : MacViews: Allows the toolkit-views Manage Passwords Dialog to be used (comments) #

Total comments: 1

Patch Set 6 : MacViews: Allows the toolkit-views Manage Passwords Dialog to be used (tests) #

Patch Set 7 : MacViews: Allows the toolkit-views Manage Passwords Dialog to be used (more tests) #

Patch Set 8 : MacViews: Allows the toolkit-views Manage Passwords Dialog to be used (rebase) #

Total comments: 30

Patch Set 9 : MacViews: Allows the toolkit-views Manage Passwords Dialog to be used (comments) #

Total comments: 1

Patch Set 10 : MacViews: Allows the toolkit-views Manage Passwords Dialog to be used (comments) #

Total comments: 18

Patch Set 11 : MacViews: Allows the toolkit-views Manage Passwords Dialog to be used (comments) #

Total comments: 2

Patch Set 12 : MacViews: Allows the toolkit-views Manage Passwords Dialog to be used (browser_tests) #

Total comments: 2

Patch Set 13 : MacViews: Allows the toolkit-views Manage Passwords Dialog to be used (rebase) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+206 lines, -193 lines) Patch
M chrome/browser/ui/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +14 lines, -14 lines 0 comments Download
D chrome/browser/ui/cocoa/passwords/credentials_selection_view.h View 1 2 1 chunk +0 lines, -27 lines 0 comments Download
D chrome/browser/ui/cocoa/passwords/credentials_selection_view.mm View 1 2 1 chunk +0 lines, -117 lines 0 comments Download
A + chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.h View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -3 lines 0 comments Download
A + chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.mm View 1 2 3 4 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller.mm View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller_unittest.mm View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/cocoa/tab_dialogs_views_mac.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/tab_dialogs_views_mac.mm View 1 2 3 4 5 6 7 8 9 2 chunks +64 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +9 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc View 1 2 3 4 5 6 7 8 2 chunks +10 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h View 1 2 3 4 5 6 7 8 9 10 4 chunks +8 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc View 1 2 3 4 5 6 7 8 9 10 8 chunks +24 lines, -7 lines 0 comments Download
M chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc View 1 2 3 4 5 6 7 8 9 10 8 chunks +46 lines, -15 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +2 lines, -2 lines 0 comments Download
M chrome/test/base/in_process_browser_test.h View 1 2 3 4 5 6 7 8 9 2 chunks +10 lines, -0 lines 0 comments Download

Messages

Total messages: 162 (136 generated)
varkha
tapted@, can you please take a first look? In particular I wasn't sure about the ...
3 years, 8 months ago (2017-04-11 07:03:24 UTC) #8
tapted
BUG=654115 is probably a good one - I've started putting screenshots there. https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.h File chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.h ...
3 years, 8 months ago (2017-04-11 07:48:59 UTC) #15
varkha
I will take a look at the browser tests for this next. https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.h File chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.h ...
3 years, 8 months ago (2017-04-12 01:57:28 UTC) #27
tapted
we should be able to move manage_passwords_bubble_view_interactive_uitest.cc around in chrome/test/BUILD.gn so that we get test ...
3 years, 8 months ago (2017-04-12 05:08:42 UTC) #34
varkha
Makes a few things much simpler, thanks. Thanks for the testing pointers also! https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.mm File ...
3 years, 8 months ago (2017-04-12 09:16:43 UTC) #36
varkha
> we should be able to move manage_passwords_bubble_view_interactive_uitest.cc > around in chrome/test/BUILD.gn so that we ...
3 years, 8 months ago (2017-04-13 10:04:58 UTC) #47
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2808823002/320001
3 years, 8 months ago (2017-04-13 10:54:21 UTC) #57
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the ...
3 years, 8 months ago (2017-04-13 10:54:24 UTC) #59
varkha
tapted@, I think this is ready for another round of comments. The tests now run ...
3 years, 8 months ago (2017-04-19 02:45:10 UTC) #102
tapted
https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.h File chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.h (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.h#newcode4 chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.h:4: #ifndef CHROME_BROWSER_UI_COCOA_PASSWORDS_CREDENTIALS_SELECTION_VIEW_H_ nit: update include guard https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/cocoa/tab_dialogs_cocoa.mm File chrome/browser/ui/cocoa/tab_dialogs_cocoa.mm ...
3 years, 8 months ago (2017-04-19 03:31:07 UTC) #106
varkha
Thanks. This again simplifies a few thing. PTAL. https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.h File chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.h (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.h#newcode4 chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.h:4: #ifndef ...
3 years, 8 months ago (2017-04-19 08:34:17 UTC) #111
tapted
lgtm just a cl description nit: wrap at 80-chars
3 years, 8 months ago (2017-04-19 08:46:17 UTC) #112
tapted
https://codereview.chromium.org/2808823002/diff/660001/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/2808823002/diff/660001/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h#newcode39 chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:39: static bool IsBubbleShown(); ooh - I think this is ...
3 years, 8 months ago (2017-04-19 08:49:35 UTC) #113
varkha
On 2017/04/19 08:49:35, tapted wrote: > https://codereview.chromium.org/2808823002/diff/660001/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h > File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): > > https://codereview.chromium.org/2808823002/diff/660001/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h#newcode39 > ...
3 years, 8 months ago (2017-04-19 08:54:10 UTC) #116
varkha
On 2017/04/19 08:46:17, tapted wrote: > lgtm > > just a cl description nit: wrap ...
3 years, 8 months ago (2017-04-19 08:55:44 UTC) #118
varkha
+phajdan.jr@chromium.org: Please review changes in chrome/test/base/ +msw@chromium.org: Please review changes in chrome/browser/ui/views/ Thanks!
3 years, 8 months ago (2017-04-19 08:58:27 UTC) #123
msw
https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h#newcode37 chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h:37: LocationBarBubbleDelegateView(views::View* anchor_view, nit: add a comment https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc ...
3 years, 8 months ago (2017-04-19 18:23:18 UTC) #126
tapted
https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/BUILD.gn#newcode1544 chrome/browser/ui/BUILD.gn:1544: "views/passwords/manage_passwords_icon_views.cc", (let's try removing manage_passwords_icon_views.cc from the build on ...
3 years, 8 months ago (2017-04-19 23:56:29 UTC) #127
varkha
https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/BUILD.gn#newcode1544 chrome/browser/ui/BUILD.gn:1544: "views/passwords/manage_passwords_icon_views.cc", On 2017/04/19 23:56:29, tapted wrote: > (let's try ...
3 years, 8 months ago (2017-04-20 04:14:20 UTC) #139
tapted
still lgtm - just one new thing https://codereview.chromium.org/2808823002/diff/780001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2808823002/diff/780001/chrome/test/BUILD.gn#newcode2192 chrome/test/BUILD.gn:2192: "../browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc", this ...
3 years, 8 months ago (2017-04-20 04:45:40 UTC) #141
varkha
https://codereview.chromium.org/2808823002/diff/780001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2808823002/diff/780001/chrome/test/BUILD.gn#newcode2192 chrome/test/BUILD.gn:2192: "../browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc", On 2017/04/20 04:45:40, tapted wrote: > this is ...
3 years, 8 months ago (2017-04-20 05:25:14 UTC) #145
msw
lgtm with a q https://codereview.chromium.org/2808823002/diff/800001/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc File chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc (right): https://codereview.chromium.org/2808823002/diff/800001/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc#newcode357 chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:357: focused_window->window()->Close(); Why isn't BringBrowserWindowToFront sufficient? ...
3 years, 8 months ago (2017-04-20 17:41:16 UTC) #150
varkha
Thanks! https://codereview.chromium.org/2808823002/diff/800001/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc File chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc (right): https://codereview.chromium.org/2808823002/diff/800001/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc#newcode357 chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:357: focused_window->window()->Close(); On 2017/04/20 17:41:16, msw wrote: > Why ...
3 years, 8 months ago (2017-04-20 21:21:24 UTC) #151
Paweł Hajdan Jr.
chrome/test LGTM
3 years, 8 months ago (2017-04-21 07:30:48 UTC) #156
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2808823002/820001
3 years, 8 months ago (2017-04-21 07:32:11 UTC) #159
commit-bot: I haz the power
3 years, 8 months ago (2017-04-21 07:38:20 UTC) #162
Message was sent while issue was closed.
Committed patchset #13 (id:820001) as
https://chromium.googlesource.com/chromium/src/+/58cb30d4f4a8cd26893f614658f7...

Powered by Google App Engine
This is Rietveld 408576698