Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808823002:
MacViews: Allows the toolkit-views Manage Passwords Dialog to be used (Closed)
DescriptionMacViews: 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) #Messages
Total messages: 162 (136 generated)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
varkha@chromium.org changed reviewers: + tapted@chromium.org
tapted@, can you please take a first look? In particular I wasn't sure about the renaming name style. We had credentials_selection_view.* in 2 places so one needs to be renamed. I could instead (or also) rename the other pair to credentials_selection_view_cocoa.mm/h. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
BUG=654115 is probably a good one - I've started putting screenshots there. https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.h (right): https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.h:77: void ShowManagePasswordsBubbleOnCocoaBrowser(NSPoint anchor, It's a mouthful, but perhaps ShowViewsManage... https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm (right): https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm:113: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) { I think it's fine to move this down to line 140 so the next few lines aren't repeated. (sure bubble_ should always be null with --secondary-ui-md, even when showing, but that's OK) https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm:121: NSPoint ns_point = [bwc bookmarkBubblePoint]; nit: ns_point -> anchor? https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/passwords/passwords_bubble_controller_views.mm (right): https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/passwords_bubble_controller_views.mm:44: return; nit: no return https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc (right): https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc:84: LocationBarBubbleDelegateView(anchor_view, gfx::Point(), this looks odd - is this just creating a temporary object and immediately destroying it? https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h (right): https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h:41: LocationBarBubbleDelegateView(views::View* anchor_view, There's probably only 1-2 other callers - is it easy to avoid adding an overload and update callers instead? https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/credentials_selection_view_views.h (right): https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/credentials_selection_view_views.h:4: #ifndef CHROME_BROWSER_UI_VIEWS_PASSWORDS_CREDENTIALS_SELECTION_VIEW_VIEWS_H_ I think this rename was due to a duplicate object file - usually I try to rename the Cocoa file instead, since that's the one that may eventually be simply deleted, so a confusing name there doesn't matter as much. [edit: ah, I see you asked this in the submit message :)] https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:248: // On MacOS the BrowserView should but does not yet exist. The auto-sign-in instead of The auto-sign-in... perhaps Auto-close of the auto-sign-in dialog is not implemented for a Cocoa browser. https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:859: g_bubble_shown_ = false; Does this work just as well in the destructor? -- I think |this| is a WidgetDelegate, so it's just a matter of waiting a tiny bit longer for Widget::OnNativeWidgetDestroyed() to call WidgetDelegate::DeleteDelegate (which does `delete this`)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== MacViews: Allows the toolkit-views Manage Passwords Dialog to be used BUG=TBD ========== to ========== MacViews: Allows the toolkit-views Manage Passwords Dialog to be used BUG= 654115 TEST=TBD ==========
I will take a look at the browser tests for this next. https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.h (right): https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.h:77: void ShowManagePasswordsBubbleOnCocoaBrowser(NSPoint anchor, On 2017/04/11 07:48:58, tapted wrote: > It's a mouthful, but perhaps ShowViewsManage... Done. https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm (right): https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm:113: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) { On 2017/04/11 07:48:58, tapted wrote: > I think it's fine to move this down to line 140 so the next few lines aren't > repeated. (sure bubble_ should always be null with --secondary-ui-md, even when > showing, but that's OK) Done. https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa.mm:121: NSPoint ns_point = [bwc bookmarkBubblePoint]; On 2017/04/11 07:48:58, tapted wrote: > nit: ns_point -> anchor? Done. https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/passwords/passwords_bubble_controller_views.mm (right): https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/passwords/passwords_bubble_controller_views.mm:44: return; On 2017/04/11 07:48:58, tapted wrote: > nit: no return Done. https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc (right): https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc:84: LocationBarBubbleDelegateView(anchor_view, gfx::Point(), On 2017/04/11 07:48:58, tapted wrote: > this looks odd - is this just creating a temporary object and immediately > destroying it? Yes, not really sure what I thought I was doing here. Fixed. https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h (right): https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h:41: LocationBarBubbleDelegateView(views::View* anchor_view, On 2017/04/11 07:48:58, tapted wrote: > There's probably only 1-2 other callers - is it easy to avoid adding an overload > and update callers instead? There are 5, I'd rather leave this to a separate CL when we go over those other bubbles. We need to fix the alignment there in any case. https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/credentials_selection_view_views.h (right): https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/credentials_selection_view_views.h:4: #ifndef CHROME_BROWSER_UI_VIEWS_PASSWORDS_CREDENTIALS_SELECTION_VIEW_VIEWS_H_ On 2017/04/11 07:48:58, tapted wrote: > I think this rename was due to a duplicate object file - usually I try to rename > the Cocoa file instead, since that's the one that may eventually be simply > deleted, so a confusing name there doesn't matter as much. > > [edit: ah, I see you asked this in the submit message :)] Done. https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:248: // On MacOS the BrowserView should but does not yet exist. The auto-sign-in On 2017/04/11 07:48:58, tapted wrote: > instead of The auto-sign-in... perhaps > > Auto-close of the auto-sign-in dialog is not implemented for a Cocoa browser. Done. https://codereview.chromium.org/2808823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:859: g_bubble_shown_ = false; On 2017/04/11 07:48:58, tapted wrote: > Does this work just as well in the destructor? -- I think |this| is a > WidgetDelegate, so it's just a matter of waiting a tiny bit longer for > Widget::OnNativeWidgetDestroyed() to call WidgetDelegate::DeleteDelegate (which > does `delete this`) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
we should be able to move manage_passwords_bubble_view_interactive_uitest.cc around in chrome/test/BUILD.gn so that we get test coverage on Mac. (however, we'll need to replace its calls to ManagePasswordsBubbleView::ShowBubble(..) since that will be a missing symbol). A new manage_passwords_bubble_view_browser_test.cc with a test harness that inherits from SupportsTestDialog<ManagePasswordsTest> should let us add to the dialog testing framework too - see https://chromium.googlesource.com/chromium/src/+/master/docs/testing/test_bro... https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.mm (right): https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.mm:5: #import "chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.h" it's worth mentioning in the CL description body that this rename is occurring (and why) https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/passwords/passwords_bubble_controller_views.mm (right): https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/passwords/passwords_bubble_controller_views.mm:26: if (ManagePasswordsBubbleView::IsBubbleShown()) Can we get into a situation where this does something unexpected? E.g. if there are two browser windows, and one has a password bubble visible. initiating a login in a second browser window shouldn't block the password bubble. But this should only be an issue if the password bubble persists in some scenarios, or fails to "steal" focus when it is shown. https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc (right): https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc:64: : BubbleDialogDelegateView(anchor_view, Can the overload just delegate to the other constructor (c++11 feature)? i.e. initialize LocationBarBubbleDelegateView rather than BubbleDialogDelegateView https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h (right): https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h:39: views::BubbleBorder::Arrow arrow, Can we remove the |arrow| argument by calling bubble_view->set_arrow(views::BubbleBorder::TOP_RIGHT) in ShowViewsManagePasswordsBubbleOnCocoaBrowser? E.g. immediately before/after it sets the parent window. But also, we may actually want to set the anchor to NONE in some cases on Mac. E.g if there's a password request in a popup window (window.open()) there may be no location bar/icon. I think other platforms will center the dialog below the titlebar in that case https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:250: // shown in a non-active browser window. Can we say "This matches the current Mac behavior with Cocoa password bubbles." (i.e. does it match? -- And do we also match on the focus-stealing behavior when the bubble is shown?) E.g. if the bubbles never auto-close on Mac, then moving the #endif down below when the timer starts is probably the right thing to do. https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:265: #if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER) Although it's dead code, I think it's safer if we leave this stuff compiled in. E.g. in case someone adds a partial fix - they won't forget to restore these steps as well. https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:809: if (manage_passwords_bubble_ == this) huh - I just noticed this. Can we implement IsBubbleShown() as simply `!!manage_passwords_bubble_`? https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:39: static bool IsBubbleShown(); nit: maybe `IsBubbleShowing()` (`shown` may imply it has been shown at least once) edit: Or in fact maybe we do not need this at all, since there is already a public accessor for manage_password_bubble() - we check whether it is null. See also bool IsBubbleShowing() { return ManagePasswordsBubbleView::manage_password_bubble() && ManagePasswordsBubbleView::manage_password_bubble()-> GetWidget()->IsVisible(); } in manage_passwords_bubble_view_interactive_uitest.cc
Description was changed from ========== MacViews: Allows the toolkit-views Manage Passwords Dialog to be used BUG= 654115 TEST=TBD ========== to ========== 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. BUG= 654115 TEST=TBD ==========
Makes a few things much simpler, thanks. Thanks for the testing pointers also! https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.mm (right): https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.mm:5: #import "chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.h" On 2017/04/12 05:08:42, tapted wrote: > it's worth mentioning in the CL description body that this rename is occurring > (and why) Done. https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/passwords/passwords_bubble_controller_views.mm (right): https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/passwords/passwords_bubble_controller_views.mm:26: if (ManagePasswordsBubbleView::IsBubbleShown()) On 2017/04/12 05:08:42, tapted wrote: > Can we get into a situation where this does something unexpected? E.g. if there > are two browser windows, and one has a password bubble visible. initiating a > login in a second browser window shouldn't block the password bubble. > > But this should only be an issue if the password bubble persists in some > scenarios, or fails to "steal" focus when it is shown. There shouldn't be non-interactive login, right? And any text input would first deactivate / hide existing bubble I think. https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc (right): https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc:64: : BubbleDialogDelegateView(anchor_view, On 2017/04/12 05:08:42, tapted wrote: > Can the overload just delegate to the other constructor (c++11 feature)? i.e. > initialize LocationBarBubbleDelegateView rather than BubbleDialogDelegateView Done. https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h (right): https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h:39: views::BubbleBorder::Arrow arrow, On 2017/04/12 05:08:42, tapted wrote: > Can we remove the |arrow| argument by calling > > bubble_view->set_arrow(views::BubbleBorder::TOP_RIGHT) in > ShowViewsManagePasswordsBubbleOnCocoaBrowser? E.g. immediately before/after it > sets the parent window. > > But also, we may actually want to set the anchor to NONE in some cases on Mac. > E.g if there's a password request in a popup window (window.open()) there may be > no location bar/icon. I think other platforms will center the dialog below the > titlebar in that case Done. And we don't center it on other platforms (where the bubble shows right-top-aligned in the window. We would do the same here when http://crbug.com/710694 is resolved. https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:250: // shown in a non-active browser window. On 2017/04/12 05:08:42, tapted wrote: > Can we say "This matches the current Mac behavior with Cocoa password bubbles." > > (i.e. does it match? -- And do we also match on the focus-stealing behavior when > the bubble is shown?) > > E.g. if the bubbles never auto-close on Mac, then moving the #endif down below > when the timer starts is probably the right thing to do. I think it is safer to do more of the same that other platforms do. http://crbug.com/468281 was more about the bubble getting dismissed so the solution added IsActive() check and resuming the timer on re-activation. I think lack of automatic re-activation is OK but removing the check for IsActive() could mean the bubble is sometimes not shown when it should. https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:265: #if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER) On 2017/04/12 05:08:42, tapted wrote: > Although it's dead code, I think it's safer if we leave this stuff compiled in. > E.g. in case someone adds a partial fix - they won't forget to restore these > steps as well. Done. https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:809: if (manage_passwords_bubble_ == this) On 2017/04/12 05:08:42, tapted wrote: > huh - I just noticed this. Can we implement IsBubbleShown() as simply > `!!manage_passwords_bubble_`? Done. https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/2808823002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:39: static bool IsBubbleShown(); On 2017/04/12 05:08:42, tapted wrote: > nit: maybe `IsBubbleShowing()` (`shown` may imply it has been shown at least > once) > > edit: Or in fact maybe we do not need this at all, since there is already a > public accessor for manage_password_bubble() - we check whether it is null. See > also > > bool IsBubbleShowing() { > return ManagePasswordsBubbleView::manage_password_bubble() && > ManagePasswordsBubbleView::manage_password_bubble()-> > GetWidget()->IsVisible(); > } > > in manage_passwords_bubble_view_interactive_uitest.cc Done. https://codereview.chromium.org/2808823002/diff/220001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.mm (right): https://codereview.chromium.org/2808823002/diff/220001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.mm:65: retain]); git cl format does this!
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:240001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:260001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
> we should be able to move manage_passwords_bubble_view_interactive_uitest.cc > around in chrome/test/BUILD.gn so that we get test coverage on Mac. > (however, we'll need to replace its calls to > ManagePasswordsBubbleView::ShowBubble(..) since that will be a missing symbol). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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. BUG= 654115 TEST=TBD ========== to ========== 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. BUG= 654115 TEST=ManagePasswordsBubbleViewTest.* ==========
The CQ bit was unchecked by varkha@chromium.org
Patchset #6 (id:280001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by varkha@chromium.org
The CQ bit was checked by varkha@chromium.org
Patchset #6 (id:300001) has been deleted
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #7 (id:340001) has been deleted
Patchset #7 (id:360001) has been deleted
Patchset #7 (id:380001) has been deleted
Patchset #7 (id:400001) has been deleted
Patchset #7 (id:420001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:460001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Patchset #8 (id:480001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #9 (id:520001) has been deleted
Patchset #9 (id:540001) has been deleted
Patchset #9 (id:560001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by varkha@chromium.org
Patchset #9 (id:580001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tapted@, I think this is ready for another round of comments. The tests now run on Mac, had to fix a few flakes around showing and hiding the bubble and waiting for the message pump to process events where necessary. Can you PTAL? Thanks!
Patchset #7 (id:440001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.h (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/coco... 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/coco... File chrome/browser/ui/cocoa/tab_dialogs_cocoa.mm (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/tab_dialogs_cocoa.mm:80: ManagePasswordsBubbleCocoa::CloseCurrentBubble(); I think we can call ManagePasswordsBubbleView::CloseCurrentBubble() directly in tab_dialogs_views_mac.mm - then we don't need ManagePasswordsBubbleCocoa::CloseCurrentBubble() or CloseViewsManagePasswordsBubbleOnCocoaBrowser() https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc:67: return; I think we don't want this early return (i.e. we should be doing that offset, or comment here why we don't want it) https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h:40: LocationBarBubbleDelegateView(views::View* anchor_view, // TODO(..) Delete this override? https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:248: // shown in a non-active browser window. perhaps add "This matches the behaviour on Mac when using a Cocoa bubble." (looking for words that don't suggest we may be regressing this behaviour by switching to Views bubbles) https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:737: new ManagePasswordsBubbleView(web_contents, anchor_view, gfx::Point(), the gfx::Point() here doesn't look right - where does the Cocoa browser pass in the anchor? Do we need an overload of ManagePasswordsBubbleView::ShowBubble() that the Cocoa browser calls? (maybe all the methods Mac needs call the ManagePasswordsBubbleView() constructor directly, and we can NOTREACHED() here) https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:752: // Adjust for fullscreen after creation as it relies on the content size. We can do this still: we just need bool is_fullscreen = browser()->window()->IsFullscreen(); outside the #ifdef https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc (left): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:326: views::test::WidgetActivationWaiter inactive_waiter(widget, false); Note I don't think this |inactive_waiter| was ever doing anything since at this point |focused_window|->window() is guaranteed to have focus by the BringBrowserWindowToFront call above. So here, |widget->IsActive()| == false means the WidgetActivationWaiter bails out early and .Wait() will no-op. https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:85: ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser())); ah, I need to try again on that CL to do this by default - the bots got angry at me https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:125: content::RunAllPendingInMessageLoop(); It's not obvious why adding this was necessary. comment? (e.g. did we block showing the bubble on mac while it is still closing? - does adding an IsVisible check back help?) https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:188: content::RunAllPendingInMessageLoop(); (same comment) https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:344: focused_waiter.WaitForActivation(); BringBrowserWindowToFront does exactly this. I don't think this is necessary https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:358: waiter.WaitForActivation(); I think the 5 lines above can just be replaced by ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser())); https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:360: EXPECT_FALSE(IsBubbleShowing()); Then the comment above the #ifdef can move down here, and we can have // Auto-close of the auto-sign-in dialog is not implemented for a Cocoa // browser. #if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER) EXPECT_TRUE(IsBubbleShowing()); #else EXPECT_FALSE(IsBubbleShowing()); #endif https://codereview.chromium.org/2808823002/diff/600001/chrome/test/base/in_pr... File chrome/test/base/in_process_browser_test.h (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/test/base/in_pr... chrome/test/base/in_process_browser_test.h:22: namespace ui { nit: sort namespaces ("ui" after "base") edit: actually, I would just #include "ui/base/test/scoped_fake_full_keyboard_access.h" between the #ifdef guards and not use a scoped_ptr -- style guide says ~"don't forward declare/unique_ptr just to avoid an #include" Then we don't need to change /in_process_browser_test.cc at all
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks. This again simplifies a few thing. PTAL. https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.h (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/passwords/credentials_selection_view_cocoa.h:4: #ifndef CHROME_BROWSER_UI_COCOA_PASSWORDS_CREDENTIALS_SELECTION_VIEW_H_ On 2017/04/19 03:31:06, tapted wrote: > nit: update include guard Done. https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/tab_dialogs_cocoa.mm (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/tab_dialogs_cocoa.mm:80: ManagePasswordsBubbleCocoa::CloseCurrentBubble(); On 2017/04/19 03:31:06, tapted wrote: > I think we can call ManagePasswordsBubbleView::CloseCurrentBubble() directly in > tab_dialogs_views_mac.mm - then we don't need > ManagePasswordsBubbleCocoa::CloseCurrentBubble() or > CloseViewsManagePasswordsBubbleOnCocoaBrowser() Done (and same for the ShowManagePasswordsBubble - moved the implementation to tab_dialogs_views_mac.mm as well. https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc:67: return; On 2017/04/19 03:31:06, tapted wrote: > I think we don't want this early return (i.e. we should be doing that offset, or > comment here why we don't want it) Done. https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h:40: LocationBarBubbleDelegateView(views::View* anchor_view, On 2017/04/19 03:31:06, tapted wrote: > // TODO(..) Delete this override? Done. https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:248: // shown in a non-active browser window. On 2017/04/19 03:31:06, tapted wrote: > perhaps add "This matches the behaviour on Mac when using a Cocoa bubble." > > (looking for words that don't suggest we may be regressing this behaviour by > switching to Views bubbles) Done. https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:737: new ManagePasswordsBubbleView(web_contents, anchor_view, gfx::Point(), On 2017/04/19 03:31:06, tapted wrote: > the gfx::Point() here doesn't look right - where does the Cocoa browser pass in > the anchor? Do we need an overload of ManagePasswordsBubbleView::ShowBubble() > that the Cocoa browser calls? (maybe all the methods Mac needs call the > ManagePasswordsBubbleView() constructor directly, and we can NOTREACHED() here) This code path is now only used not on Mac views and the |anchor_point| there does not matter. Made the test calling the bubble through TabDialogsViewsMac::ShowManagePasswordsBubble(). https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:752: // Adjust for fullscreen after creation as it relies on the content size. On 2017/04/19 03:31:06, tapted wrote: > We can do this still: we just need > > bool is_fullscreen = browser()->window()->IsFullscreen(); > > outside the #ifdef Talked about this offline, we probably don't want to adjust for fullscreen because we are not using AnchorRect () but we are anchoring to the location bar nonetheless and calling AdjustForFullscreen will do the wrong thing. https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc (left): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:326: views::test::WidgetActivationWaiter inactive_waiter(widget, false); On 2017/04/19 03:31:06, tapted wrote: > Note I don't think this |inactive_waiter| was ever doing anything since at this > point |focused_window|->window() is guaranteed to have focus by the > BringBrowserWindowToFront call above. So here, |widget->IsActive()| == false > means the WidgetActivationWaiter bails out early and .Wait() will no-op. Acknowledged. https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:85: ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser())); On 2017/04/19 03:31:07, tapted wrote: > ah, I need to try again on that CL to do this by default - the bots got angry at > me Acknowledged. https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:125: content::RunAllPendingInMessageLoop(); On 2017/04/19 03:31:06, tapted wrote: > It's not obvious why adding this was necessary. comment? (e.g. did we block > showing the bubble on mac while it is still closing? - does adding an IsVisible > check back help?) Done. Added a comment. https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:188: content::RunAllPendingInMessageLoop(); On 2017/04/19 03:31:06, tapted wrote: > (same comment) Done. Added a comment. https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:344: focused_waiter.WaitForActivation(); On 2017/04/19 03:31:07, tapted wrote: > BringBrowserWindowToFront does exactly this. I don't think this is necessary Done. https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:358: waiter.WaitForActivation(); On 2017/04/19 03:31:06, tapted wrote: > I think the 5 lines above can just be replaced by > > ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser())); Done. https://codereview.chromium.org/2808823002/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:360: EXPECT_FALSE(IsBubbleShowing()); On 2017/04/19 03:31:06, tapted wrote: > Then the comment above the #ifdef can move down here, and we can have > > // Auto-close of the auto-sign-in dialog is not implemented for a Cocoa > // browser. > #if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER) > EXPECT_TRUE(IsBubbleShowing()); > #else > EXPECT_FALSE(IsBubbleShowing()); > #endif Done. https://codereview.chromium.org/2808823002/diff/600001/chrome/test/base/in_pr... File chrome/test/base/in_process_browser_test.h (right): https://codereview.chromium.org/2808823002/diff/600001/chrome/test/base/in_pr... chrome/test/base/in_process_browser_test.h:22: namespace ui { On 2017/04/19 03:31:07, tapted wrote: > nit: sort namespaces ("ui" after "base") > > edit: actually, I would just #include > "ui/base/test/scoped_fake_full_keyboard_access.h" between the #ifdef guards and > not use a scoped_ptr -- style guide says ~"don't forward declare/unique_ptr just > to avoid an #include" > > Then we don't need to change /in_process_browser_test.cc at all Done. https://codereview.chromium.org/2808823002/diff/620001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/passwords/passwords_bubble_controller_views.mm (right): https://codereview.chromium.org/2808823002/diff/620001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/passwords/passwords_bubble_controller_views.mm:14: I think this can be rolled in Mac-views-specific TabDialogsViewsMac::ShowManagePasswordsBubble. See if you like this code moved there.
lgtm just a cl description nit: wrap at 80-chars
https://codereview.chromium.org/2808823002/diff/660001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/2808823002/diff/660001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:39: static bool IsBubbleShown(); ooh - I think this is unused now.
Patchset #11 (id:660001) has been deleted
Patchset #10 (id:640001) has been deleted
On 2017/04/19 08:49:35, tapted wrote: > https://codereview.chromium.org/2808823002/diff/660001/chrome/browser/ui/view... > File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): > > https://codereview.chromium.org/2808823002/diff/660001/chrome/browser/ui/view... > chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:39: static bool > IsBubbleShown(); > ooh - I think this is unused now. Done.
Description was changed from ========== 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. BUG= 654115 TEST=ManagePasswordsBubbleViewTest.* ========== to ========== 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. BUG= 654115 TEST=ManagePasswordsBubbleViewTest.* ==========
On 2017/04/19 08:46:17, tapted wrote: > lgtm > > just a cl description nit: wrap at 80-chars Done.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:680001) has been deleted
varkha@chromium.org changed reviewers: + msw@chromium.org, phajdan.jr@chromium.org
+phajdan.jr@chromium.org: Please review changes in chrome/test/base/ +msw@chromium.org: Please review changes in chrome/browser/ui/views/ Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... 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/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:246: // On MacOS the BrowserView should but does not yet exist. Auto-close of the Perhaps this comment isn't as elucidating as it could be, can you give a more prescriptive explanation, especially if you're citing a closed bug? Should this be something like "Do not automatically close sign-in dialogs [for inactive browser windows] on MacOS, to match existing Cocoa bubble behavior."? https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:249: // the behaviour on Mac when using a Cocoa bubble. nit: consistent behavior/our spelling with line above. https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:29: static void ShowBubble(content::WebContents* web_contents, nit: limit this decl like the definition? (!OS_MACOSX || MAC_VIEWS_BROWSER) https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:32: #include "ui/views/test/widget_test.h" nit: remove if no longer needed https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:361: // Auto-close of the auto-sign-in dialog is not implemented for a Cocoa ditto: should it be implemented or is not auto-closing intentional? cite an open bug if this is really a TODO comment. https://codereview.chromium.org/2808823002/diff/700001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:562: "../browser/ui/views/passwords/manage_passwords_icon_view_interactive_uitest.cc", Should this also be supported for mac views? https://codereview.chromium.org/2808823002/diff/700001/chrome/test/base/in_pr... File chrome/test/base/in_process_browser_test.h (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/test/base/in_pr... chrome/test/base/in_process_browser_test.h:272: // Enable fake full keyboard access by default, so that tests don't depend on This seems like a big change. Is it possible that some tests will want this, while others won't? Call this out in the CL description?
https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/BUIL... File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/BUIL... 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 mac+cocoa - this file should only be depended on by the location bar used by BrowserView, not BrowserWindowController. It might get tripped up by references in the other passwords files, but there might be a clean way to resolve that and it may hint at bugs) https://codereview.chromium.org/2808823002/diff/700001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:562: "../browser/ui/views/passwords/manage_passwords_icon_view_interactive_uitest.cc", On 2017/04/19 18:23:17, msw wrote: > Should this also be supported for mac views? not yet for this one, since this is the icon used in the location bar, which is still uses Cocoa-based decorations under MacViews. But thanks for pointing this out! We should try removing manage_passwords_icon_views.cc from the build - it shouldn't be necessary yet.
Patchset #11 (id:720001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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. BUG= 654115 TEST=ManagePasswordsBubbleViewTest.* ========== to ========== 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=ManagePasswordsBubbleViewTest.* ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #11 (id:740001) has been deleted
https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/BUIL... File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/BUIL... 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 removing manage_passwords_icon_views.cc from the build on mac+cocoa - > this file should only be depended on by the location bar used by BrowserView, > not BrowserWindowController. It might get tripped up by references in the other > passwords files, but there might be a clean way to resolve that and it may hint > at bugs) Done. https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h:37: LocationBarBubbleDelegateView(views::View* anchor_view, On 2017/04/19 18:23:17, msw wrote: > nit: add a comment Done. https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:246: // On MacOS the BrowserView should but does not yet exist. Auto-close of the On 2017/04/19 18:23:17, msw wrote: > Perhaps this comment isn't as elucidating as it could be, can you give a more > prescriptive explanation, especially if you're citing a closed bug? Should this > be something like "Do not automatically close sign-in dialogs [for inactive > browser windows] on MacOS, to match existing Cocoa bubble behavior."? Done. https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:249: // the behaviour on Mac when using a Cocoa bubble. On 2017/04/19 18:23:17, msw wrote: > nit: consistent behavior/our spelling with line above. Acknowledged. https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:29: static void ShowBubble(content::WebContents* web_contents, On 2017/04/19 18:23:17, msw wrote: > nit: limit this decl like the definition? (!OS_MACOSX || MAC_VIEWS_BROWSER) Done. https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:32: #include "ui/views/test/widget_test.h" On 2017/04/19 18:23:17, msw wrote: > nit: remove if no longer needed Done. https://codereview.chromium.org/2808823002/diff/700001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:361: // Auto-close of the auto-sign-in dialog is not implemented for a Cocoa On 2017/04/19 18:23:17, msw wrote: > ditto: should it be implemented or is not auto-closing intentional? cite an open > bug if this is really a TODO comment. Done. https://codereview.chromium.org/2808823002/diff/700001/chrome/test/base/in_pr... File chrome/test/base/in_process_browser_test.h (right): https://codereview.chromium.org/2808823002/diff/700001/chrome/test/base/in_pr... chrome/test/base/in_process_browser_test.h:272: // Enable fake full keyboard access by default, so that tests don't depend on On 2017/04/19 18:23:17, msw wrote: > This seems like a big change. Is it possible that some tests will want this, > while others won't? Call this out in the CL description? This makes tests n Mac more consistent so I think this is a worthwhile change. I'll update the CL description.
Patchset #11 (id:760001) has been deleted
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#n... chrome/test/BUILD.gn:2192: "../browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc", this is a new file since the rebase - it makes sense to move it now that the file it tests is supported on mac (there shouldn't be any surprises... #famouslastwords)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
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#n... 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 a new file since the rebase - it makes sense to move it now that the > file it tests is supported on mac (there shouldn't be any surprises... > #famouslastwords) Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
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=ManagePasswordsBubbleViewTest.*
==========
to
==========
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*
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a q https://codereview.chromium.org/2808823002/diff/800001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc (right): https://codereview.chromium.org/2808823002/diff/800001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:357: focused_window->window()->Close(); Why isn't BringBrowserWindowToFront sufficient? Comment?
Thanks! https://codereview.chromium.org/2808823002/diff/800001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc (right): https://codereview.chromium.org/2808823002/diff/800001/chrome/browser/ui/view... 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 isn't BringBrowserWindowToFront sufficient? Comment? I didn't comment in code here because I ended not changing these lines. BringBrowserWindowToFront() isn't working here because at least on some platforms it fails to bring focus to the browser. I suspect this is because the bubble is open and it is the one getting focused. The code below effectively breaks it into steps and waits after activating the browser window which works everywhere. This suggests that BringBrowserWindowToFront() has some platform idiosyncrasies, not sure if that raises to a level of a bug though.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chrome/test LGTM
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2808823002/#ps820001 (title: "MacViews: Allows the toolkit-views Manage Passwords Dialog to be used (rebase)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 820001, "attempt_start_ts": 1492759907857010,
"parent_rev": "74934b2b5f8665e2d2f5eb77b5494ff06d9df114", "commit_rev":
"58cb30d4f4a8cd26893f614658f752c9cefb30dd"}
Message was sent while issue was closed.
Description was changed from
==========
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*
==========
to
==========
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/+/58cb30d4f4a8cd26893f614658f7...
==========
Message was sent while issue was closed.
Committed patchset #13 (id:820001) as https://chromium.googlesource.com/chromium/src/+/58cb30d4f4a8cd26893f614658f7... |
