Chromium Code Reviews
Help | Chromium Project | Sign in
(3)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 5 days ago by varkha
Modified:
1 week, 1 day 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 162 (136 generated)
varkha
tapted@, can you please take a first look? In particular I wasn't sure about the ...
2 weeks, 4 days ago (2017-04-11 07:03:24 UTC) #8
tapted (OOO until 2017-05-01)
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 ...
2 weeks, 4 days 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 ...
2 weeks, 3 days ago (2017-04-12 01:57:28 UTC) #27
tapted (OOO until 2017-05-01)
we should be able to move manage_passwords_bubble_view_interactive_uitest.cc around in chrome/test/BUILD.gn so that we get test ...
2 weeks, 3 days 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 ...
2 weeks, 3 days 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 ...
2 weeks, 2 days 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
2 weeks, 2 days 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 ...
2 weeks, 2 days 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 ...
1 week, 3 days ago (2017-04-19 02:45:10 UTC) #102
tapted (OOO until 2017-05-01)
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 ...
1 week, 3 days 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 ...
1 week, 3 days ago (2017-04-19 08:34:17 UTC) #111
tapted (OOO until 2017-05-01)
lgtm just a cl description nit: wrap at 80-chars
1 week, 3 days ago (2017-04-19 08:46:17 UTC) #112
tapted (OOO until 2017-05-01)
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 ...
1 week, 3 days 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 > ...
1 week, 3 days 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 ...
1 week, 3 days 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!
1 week, 3 days 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 ...
1 week, 3 days ago (2017-04-19 18:23:18 UTC) #126
tapted (OOO until 2017-05-01)
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 ...
1 week, 2 days 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 ...
1 week, 2 days ago (2017-04-20 04:14:20 UTC) #139
tapted (OOO until 2017-05-01)
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 ...
1 week, 2 days 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 ...
1 week, 2 days 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? ...
1 week, 2 days 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 ...
1 week, 1 day ago (2017-04-20 21:21:24 UTC) #151
Paweł Hajdan Jr.
chrome/test LGTM
1 week, 1 day 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
1 week, 1 day ago (2017-04-21 07:32:11 UTC) #159
commit-bot: I haz the power
1 week, 1 day 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46