Fix a memory leak in PasswordStoreMac

Fix a memory leak in PasswordStoreMac.

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

[vasilii, 2014-08-08 17:04:52]

Hi Ilya,

please review.

[vasilii, 2014-08-11 08:51:51]

Hi Garrett,

could you review this one?

[Ilya Sherman, 2014-08-12 00:31:51]

Thanks for the fix.  Please file a bug for this issue, and update the CL
description to reference the bug.

https://codereview.chromium.org/454083002/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/454083002/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_store_mac.cc:945: if
(owned_keychain_adapter.PasswordExactlyMatchingForm(form)) {
Hmm, could the method be changed to instead just return a boolean value, rather
than returning a pointer to the form?

[vasilii, 2014-08-12 09:17:37]

I filed the bug issue.

https://codereview.chromium.org/454083002/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/454083002/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_store_mac.cc:945: if
(owned_keychain_adapter.PasswordExactlyMatchingForm(form)) {
On 2014/08/12 00:31:51, Ilya Sherman wrote:
> Hmm, could the method be changed to instead just return a boolean value,
rather
> than returning a pointer to the form?

The returned value is used in unit tests. With boolean it'll be more difficult
to test.

[Ilya Sherman, 2014-08-12 17:21:26]

https://codereview.chromium.org/454083002/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/454083002/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_store_mac.cc:945: if
(owned_keychain_adapter.PasswordExactlyMatchingForm(form)) {
On 2014/08/12 09:17:37, vasilii wrote:
> On 2014/08/12 00:31:51, Ilya Sherman wrote:
> > Hmm, could the method be changed to instead just return a boolean value,
> rather
> > than returning a pointer to the form?
> 
> The returned value is used in unit tests. With boolean it'll be more difficult
> to test.

Hmm, why do the tests need more information that the production code?  In
general, it's better to simplify production code and to test public APIs.

[vasilii, 2014-08-13 09:56:46]

https://codereview.chromium.org/454083002/diff/1/chrome/browser/password_mana...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/454083002/diff/1/chrome/browser/password_mana...
chrome/browser/password_manager/password_store_mac.cc:945: if
(owned_keychain_adapter.PasswordExactlyMatchingForm(form)) {
On 2014/08/12 17:21:25, Ilya Sherman wrote:
> On 2014/08/12 09:17:37, vasilii wrote:
> > On 2014/08/12 00:31:51, Ilya Sherman wrote:
> > > Hmm, could the method be changed to instead just return a boolean value,
> > rather
> > > than returning a pointer to the form?
> > 
> > The returned value is used in unit tests. With boolean it'll be more
difficult
> > to test.
> 
> Hmm, why do the tests need more information that the production code?  In
> general, it's better to simplify production code and to test public APIs.

Done.

[Ilya Sherman, 2014-08-13 17:18:11]

LGTM % a nit.  Thanks!

https://codereview.chromium.org/454083002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_mac_internal.h (right):

https://codereview.chromium.org/454083002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_mac_internal.h:35: bool
PasswordExactlyMatchingForm(const autofill::PasswordForm& query_form);
nit: Perhaps rename this to "HasPasswordExactlyMatchingForm()" or something like
that?

[vasilii, 2014-08-14 08:06:33]

https://codereview.chromium.org/454083002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_mac_internal.h (right):

https://codereview.chromium.org/454083002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_mac_internal.h:35: bool
PasswordExactlyMatchingForm(const autofill::PasswordForm& query_form);
On 2014/08/13 17:18:10, Ilya Sherman wrote:
> nit: Perhaps rename this to "HasPasswordExactlyMatchingForm()" or something
like
> that?

Done.

[vasilii, 2014-08-14 08:07:07]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-08-14 08:12:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/454083002/40001

[commit-bot: I haz the power, 2014-08-14 15:17:58]

Committed patchset #3 (40001) as 289593