PasswordFormManager: Clean up PasswordForm pointer ownership.

PasswordFormManager: Clean up PasswordForm pointer ownership.

In the PasswordFormManager::OnRequestDone method, I found some usage of
raw pointers to elements of a ScopedVector with confusing ownership over
a lot of code. Rewrote the code to use scoped_ptrs so that the ownership
is always clear, and nothing needs to be manually deleted.

BUG=451018
Committed: https://crrev.com/f83db42d42fe8b672e13316c54110d4cf2315828
Cr-Commit-Position: refs/heads/master@{#326745}

[Matt Giuca, 2015-04-20 06:02:37]

mgiuca@chromium.org changed reviewers:
+ vabr@chromium.org

[Matt Giuca, 2015-04-20 06:02:37]

Hi Vaclav,

I was recently doing some cleanup involving scoped pointers and found this code
which I thought I could make less confusing. (I know you recently upgraded it
from "a bowl of spaghetti" [1] ;)).

[1] http://crrev.com/317575

[vabr (Chromium), 2015-04-22 09:43:27]

Thanks, Matt!

This LGTM, although I have a comment below.

Cheers,
Vaclav

https://codereview.chromium.org/1094013002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1094013002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:424: const
base::string16& username = login->username_value;
After this change, we get a lot of key-searching in the map instead of just one
(lines 425, 429 and 430 in the new code as opposed to just 423 in the old one).
While that change might not be important alone, similar changes could add up and
affect performance. My rule of thumb here would be only risk that if the
readability is improved.
That does not seem to be a clear case here: subjectively, I see no readability
gain in coming to |best_matches_[username]| from |best_match|; I understand that
you have the opposite view.
I suggest to keep |best_match| unless you can hint at a more evident readability
gain.

https://codereview.chromium.org/1094013002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:443: const
base::string16& username = protege->username_value;
Please see my comment at line 424, applies here as well.

[vabr (Chromium), 2015-04-22 09:44:08]

Also, could you please add
BUG=451018
to the CL description to help tracking related changes?

Thanks!
Vaclav

[Matt Giuca, 2015-04-23 01:17:36]

https://codereview.chromium.org/1094013002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1094013002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:424: const
base::string16& username = login->username_value;
Hi, yeah this is a good point: I don't want to introduce new map lookups if I
can help it.

I find the use of a non-const reference to update a map to be quite confusing
(since what looks like an assignment to a variable is actually an assignment to
a map), whereas "best_matches_[username] = ..." is clearly a map assignment. But
I don't want to change things just for the sake of it. Partially reverted.
(However, I've replaced "auto" with the name of the type, if that's OK with you:
I think it's at least clearer to know what type we're dealing with and that this
is a reference to a pointer.)

FYI, the reason I'm dabbling in this is that I'm planning to replace raw pointer
maps with a ScopedMap (which you suggested in a TODO in the header file). I have
a CL prepared for it but not sent out:
https://codereview.chromium.org/1096983002/ (see the changes to
password_form_manager). In that context, I don't think we want to continue
directly updating a reference (since I don't want to have to manually delete
pointers), which means we will have to do one or two extra map lookups). But
I'll defer that discussion for the next CL, and I'll definitely loop you in on
the review there if you have feedback. (Also, I am planning to add a set()
method to ScopedMap so that we can do a single set() rather than erase() and
insert() which is what I currently have there.)

https://codereview.chromium.org/1094013002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:443: const
base::string16& username = protege->username_value;
On 2015/04/22 09:43:27, vabr (back by 23 April) wrote:
> Please see my comment at line 424, applies here as well.

Done.

[vabr (Chromium), 2015-04-23 09:22:20]

This LGTM, thanks for improving this code!

Vaclav

https://codereview.chromium.org/1094013002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1094013002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:424: const
base::string16& username = login->username_value;
On 2015/04/23 01:17:36, Matt Giuca wrote:
> Hi, yeah this is a good point: I don't want to introduce new map lookups if I
> can help it.
> 
> I find the use of a non-const reference to update a map to be quite confusing
> (since what looks like an assignment to a variable is actually an assignment
to
> a map), whereas "best_matches_[username] = ..." is clearly a map assignment.
But
> I don't want to change things just for the sake of it. Partially reverted.
> (However, I've replaced "auto" with the name of the type, if that's OK with
you:
> I think it's at least clearer to know what type we're dealing with and that
this
> is a reference to a pointer.)
Definitely happier with the explicit type than with auto, thanks!
I also understand your point with making it clearer that the map is actually
modified, not just a local variable. But given the coming changes you mention
below, I think what you did here in patch set 3 is just the right balance for
the moment.

> 
> FYI, the reason I'm dabbling in this is that I'm planning to replace raw
pointer
> maps with a ScopedMap (which you suggested in a TODO in the header file). I
have
> a CL prepared for it but not sent out:
> https://codereview.chromium.org/1096983002/ (see the changes to
> password_form_manager).
Awesome! Also, happy to review that CL once you need a look from
password_manager OWNERS.

> In that context, I don't think we want to continue
> directly updating a reference (since I don't want to have to manually delete
> pointers), which means we will have to do one or two extra map lookups). But
> I'll defer that discussion for the next CL, and I'll definitely loop you in on
> the review there if you have feedback. (Also, I am planning to add a set()
> method to ScopedMap so that we can do a single set() rather than erase() and
> insert() which is what I currently have there.)
In the context of ScopedMap this might be different story, and the set()
reducing two lookups to one will help. Let's talk on the other CL when it is
time, but your plans sound reasonable.

[Matt Giuca, 2015-04-24 04:32:09]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2015-04-24 04:32:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1094013002/40001

[commit-bot: I haz the power, 2015-04-24 05:39:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-24 05:40:21]

Patchset 3 (id:??) landed as
https://crrev.com/f83db42d42fe8b672e13316c54110d4cf2315828
Cr-Commit-Position: refs/heads/master@{#326745}