Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

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 months, 2 weeks ago by varkha
Modified:
2 months, 1 week 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 months, 2 weeks 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 ...
2 months, 2 weeks 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 months, 2 weeks 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 ...
2 months, 2 weeks 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 months, 2 weeks 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 months, 2 weeks 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 months, 2 weeks 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 months, 2 weeks 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week ago (2017-04-19 08:34:17 UTC) #111
tapted
lgtm just a cl description nit: wrap at 80-chars
2 months, 1 week 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 ...
2 months, 1 week 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 > ...
2 months, 1 week 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 ...
2 months, 1 week 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!
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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? ...
2 months, 1 week 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 ...
2 months, 1 week ago (2017-04-20 21:21:24 UTC) #151
Paweł Hajdan Jr.
chrome/test LGTM
2 months, 1 week 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
2 months, 1 week ago (2017-04-21 07:32:11 UTC) #159
commit-bot: I haz the power
2 months, 1 week 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 23e94e589