[Mac] Password generation: Add some tests for the password generation UI.

[Mac] Password generation: Add some tests for the password generation UI.

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

[Patrick Dubroy, 2014-05-23 13:50:32]

PTAL. Adding a test as promised :-)

[groby-ooo-7-16, 2014-05-23 17:50:28]

Awesome, thank you!

A few nits/suggestions how you could simplify a bit, but looks otherwise good.

https://codereview.chromium.org/292383005/diff/1/chrome/browser/ui/cocoa/auto...
File
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm
(right):

https://codereview.chromium.org/292383005/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm:28:
: help_text_(l10n_util::GetStringUTF16(IDS_PASSWORD_GENERATION_PROMPT)),
You could initialize the string with some test string - reduces dependencies.
(You can also generate the font list from a description, freeing you from having
to pull in ResourceBundle)

https://codereview.chromium.org/292383005/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm:84:
const gfx::Rect popup_bounds_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/292383005/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm:94:
initWithController:mock_controller_.get()
Shouldn't need get()

https://codereview.chromium.org/292383005/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm:104:
scoped_ptr<MockPasswordGenerationPopupController> mock_controller_;
No need for scoped_ptr, just have an instance? Or does it _have_ to be
instantiated after the test framework is set up?

If you keep it - is it intentional the view outlives the controller?

https://codereview.chromium.org/292383005/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm:113:
Return(base::ASCIIToUTF16("wow! such password")));
Much secure! So wow! ;)

[Patrick Dubroy, 2014-05-26 16:43:14]

https://codereview.chromium.org/292383005/diff/1/chrome/browser/ui/cocoa/auto...
File
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm
(right):

https://codereview.chromium.org/292383005/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm:28:
: help_text_(l10n_util::GetStringUTF16(IDS_PASSWORD_GENERATION_PROMPT)),
On 2014/05/23 17:50:28, groby wrote:
> You could initialize the string with some test string - reduces dependencies.
> (You can also generate the font list from a description, freeing you from
having
> to pull in ResourceBundle)

Done.

https://codereview.chromium.org/292383005/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm:84:
const gfx::Rect popup_bounds_;
On 2014/05/23 17:50:28, groby wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/292383005/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm:94:
initWithController:mock_controller_.get()
On 2014/05/23 17:50:28, groby wrote:
> Shouldn't need get()

It won't compile without it.

https://codereview.chromium.org/292383005/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm:104:
scoped_ptr<MockPasswordGenerationPopupController> mock_controller_;
On 2014/05/23 17:50:28, groby wrote:
> No need for scoped_ptr, just have an instance? Or does it _have_ to be
> instantiated after the test framework is set up?
> 
> If you keep it - is it intentional the view outlives the controller?

Well, the ordering here was not actually intentional, but the view actually
*can* outlive the controller (due to the fact that it gets autoreleased when the
controller is destroyed).

I've switched the declaration order to match the initialization order, and added
an explicit test for the case where the controller is destroyed first.

[groby-ooo-7-16, 2014-05-27 17:38:38]

much LGTM :) (Sorry for the delay - US holiday)

https://codereview.chromium.org/292383005/diff/1/chrome/browser/ui/cocoa/auto...
File
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm
(right):

https://codereview.chromium.org/292383005/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm:94:
initWithController:mock_controller_.get()
On 2014/05/26 16:43:15, Patrick Dubroy wrote:
> On 2014/05/23 17:50:28, groby wrote:
> > Shouldn't need get()
> 
> It won't compile without it.

My bad. get() is only unnecessary for _exact_ types. It doesn't downcast
automatically :(

https://codereview.chromium.org/292383005/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm:104:
scoped_ptr<MockPasswordGenerationPopupController> mock_controller_;
On 2014/05/26 16:43:15, Patrick Dubroy wrote:
> On 2014/05/23 17:50:28, groby wrote:
> > No need for scoped_ptr, just have an instance? Or does it _have_ to be
> > instantiated after the test framework is set up?
> > 
> > If you keep it - is it intentional the view outlives the controller?
> 
> Well, the ordering here was not actually intentional, but the view actually
> *can* outlive the controller (due to the fact that it gets autoreleased when
the
> controller is destroyed).
> 
> I've switched the declaration order to match the initialization order, and
added
> an explicit test for the case where the controller is destroyed first.

Awesome, thank you!

[Patrick Dubroy, 2014-05-27 18:07:24]

The CQ bit was checked by dubroy@chromium.org

[commit-bot: I haz the power, 2014-05-27 18:08:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dubroy@chromium.org/292383005/60001

[commit-bot: I haz the power, 2014-05-27 19:59:25]

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

[commit-bot: I haz the power, 2014-05-27 20:03:07]

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

[commit-bot: I haz the power, 2014-05-27 20:03:09]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[Patrick Dubroy, 2014-05-28 06:03:23]

The CQ bit was checked by dubroy@chromium.org

[commit-bot: I haz the power, 2014-05-28 06:04:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dubroy@chromium.org/292383005/60001

[commit-bot: I haz the power, 2014-05-28 08:29:37]

Change committed as 273176