Add ManagePasswordsBubbleController and unit tests.

Add ManagePasswordsBubbleController and unit tests.

BUG=328847
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291399

[dconnelly, 2014-08-04 15:54:06]

-rohitrao
+shess

[Scott Hess - ex-Googler, 2014-08-07 23:54:14]

https://codereview.chromium.org/422233009/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.h
(right):

https://codereview.chromium.org/422233009/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.h:25: bool
neverSave_;
Cocoa-style would be BOOL.

https://codereview.chromium.org/422233009/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.mm
(right):

https://codereview.chromium.org/422233009/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.mm:34:
neverSave_ = false;
And "NO" for false.

https://codereview.chromium.org/422233009/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.mm:82:
animate:[[self window] isVisible]];
[self window] => window.

https://codereview.chromium.org/422233009/diff/60001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/422233009/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller_unittest.mm:56:
[window setAllowedAnimations:info_bubble::kAnimateNone];
Wait ... the CocoaTest fixture should already be setting animation durations to
zero globally.  Hmm.  Maybe I need to look into that.

[dconnelly, 2014-08-08 09:03:15]

https://codereview.chromium.org/422233009/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.h
(right):

https://codereview.chromium.org/422233009/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.h:25: bool
neverSave_;
On 2014/08/07 23:54:14, Scott Hess wrote:
> Cocoa-style would be BOOL.

Done.

https://codereview.chromium.org/422233009/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.mm
(right):

https://codereview.chromium.org/422233009/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.mm:34:
neverSave_ = false;
On 2014/08/07 23:54:14, Scott Hess wrote:
> And "NO" for false.

Done.

https://codereview.chromium.org/422233009/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.mm:82:
animate:[[self window] isVisible]];
On 2014/08/07 23:54:14, Scott Hess wrote:
> [self window] => window.

Done.

[dconnelly, 2014-08-20 11:21:26]

Scott: this is the next leaf CL.

[Scott Hess - ex-Googler, 2014-08-20 20:04:41]

Minor comments.

https://codereview.chromium.org/422233009/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.mm
(right):

https://codereview.chromium.org/422233009/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.mm:49: if
(password_manager::ui::IsPendingState(model_->state()) && !neverSave_) {
I have had complaints in the past about things like double-negative conditions
(!neverSave_).  Is there an alternative naming which was a positive condition
without being insanely wordy?

Also, is there any possibility that this could be called when already in the
indicated state?

https://codereview.chromium.org/422233009/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.mm:87:
[self performLayout];
Is there a case where -updateState shouldn't lead to -performLayout?

https://codereview.chromium.org/422233009/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.mm:90: //
ManagePasswordsBubbleContentViewDelegate
Generally this is indicated by:

#pragma mark ManagePasswordsBubbleContentViewDelegate

Personally I don't care, but apparently it integrates with Xcode or something,
and it covers what the comment is doing too.

https://codereview.chromium.org/422233009/diff/100001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/422233009/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller_unittest.mm:21:
class ManagePasswordsBubbleControllerTest
Hmm, forgot to ask about previous reviews: Will all of this work in an anonymous
namespace?  Sometimes it won't because of <reasons>, and I've never found it
worthwhile to learn to decode it by inspection, but if it can be anonymous
that's an improvement.

https://codereview.chromium.org/422233009/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller_unittest.mm:63:
EXPECT_FALSE([[strongController window] isVisible]);
This depends on the controller keeping a reference to the window, which means it
could succeed incorrectly if the controller drops that reference for some reason
{EXPECT_FALSE([nil isVisible]) will succeed}.  Maybe keep a ref to the window
you want to test instead?

[dconnelly, 2014-08-21 08:48:00]

https://codereview.chromium.org/422233009/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.mm
(right):

https://codereview.chromium.org/422233009/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.mm:49: if
(password_manager::ui::IsPendingState(model_->state()) && !neverSave_) {
On 2014/08/20 20:04:40, Scott Hess wrote:
> I have had complaints in the past about things like double-negative conditions
> (!neverSave_).  Is there an alternative naming which was a positive condition
> without being insanely wordy?
> 
> Also, is there any possibility that this could be called when already in the
> indicated state?

Nope. And to address that and the first comment I've managed to eliminate the
member variable completely. In the CL that adds the NeverSave view the delegate
method can just update the controller directly.

https://codereview.chromium.org/422233009/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.mm:87:
[self performLayout];
On 2014/08/20 20:04:40, Scott Hess wrote:
> Is there a case where -updateState shouldn't lead to -performLayout?

Nope. Removed.

https://codereview.chromium.org/422233009/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller.mm:90: //
ManagePasswordsBubbleContentViewDelegate
On 2014/08/20 20:04:40, Scott Hess wrote:
> Generally this is indicated by:
> 
> #pragma mark ManagePasswordsBubbleContentViewDelegate
> 
> Personally I don't care, but apparently it integrates with Xcode or something,
> and it covers what the comment is doing too.

Done.

https://codereview.chromium.org/422233009/diff/100001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller_unittest.mm
(right):

https://codereview.chromium.org/422233009/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller_unittest.mm:21:
class ManagePasswordsBubbleControllerTest
On 2014/08/20 20:04:41, Scott Hess wrote:
> Hmm, forgot to ask about previous reviews: Will all of this work in an
anonymous
> namespace?  Sometimes it won't because of <reasons>, and I've never found it
> worthwhile to learn to decode it by inspection, but if it can be anonymous
> that's an improvement.

Apparently it will. I've updated all the unit tests.

https://codereview.chromium.org/422233009/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_controller_unittest.mm:63:
EXPECT_FALSE([[strongController window] isVisible]);
On 2014/08/20 20:04:40, Scott Hess wrote:
> This depends on the controller keeping a reference to the window, which means
it
> could succeed incorrectly if the controller drops that reference for some
reason
> {EXPECT_FALSE([nil isVisible]) will succeed}.  Maybe keep a ref to the window
> you want to test instead?

Done.

[Scott Hess - ex-Googler, 2014-08-21 23:21:51]

lgtm

[dconnelly, 2014-08-22 11:25:56]

The CQ bit was checked by dconnelly@chromium.org

[commit-bot: I haz the power, 2014-08-22 11:26:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dconnelly@chromium.org/422233009/180001

[commit-bot: I haz the power, 2014-08-22 13:04:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-22 13:08:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-22 13:08:16]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[dconnelly, 2014-08-22 13:10:49]

The CQ bit was checked by dconnelly@chromium.org

[commit-bot: I haz the power, 2014-08-22 13:11:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dconnelly@chromium.org/422233009/180001

[commit-bot: I haz the power, 2014-08-22 14:16:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[commit-bot: I haz the power, 2014-08-22 14:23:49]

Committed patchset #8 (180001) as 291399