Simplify GetLoginsRequest, and let PasswordStore::GetLoginsImpl take scoped_ptr<GetLoginsRequest>

Simplify GetLoginsRequest, and let PasswordStore::GetLoginsImpl take scoped_ptr<GetLoginsRequest>.

After this change, the signature of GetLoginsImpl() will be aligned with GetAutofillableLoginsImpl() and GetBlacklistedLoginsImpl().

BUG=437865
NOTRY=true # Unrelated chromevox test failures.

Committed: https://crrev.com/a9a28c8cd244a42fb2f77e08c8cf35cddc558620
Cr-Commit-Position: refs/heads/master@{#316235}

[engedy, 2015-02-11 16:33:15]

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

[engedy, 2015-02-11 16:39:41]

@Vaclav, please take a look.

Unfortunately, the solution we have devised before (notifying the consumer in
the destructor of GetLoginsRequest) is not feasible, as the request may be
cancelled through a CancelableTaskTracker. In that case, of course, the
destructor of the owned GetLoginsRequest instance is called regardless.

The problem is that I do not see a way to tell apart the following two
scenarios:
  1.) The GetLoginsImpl() call was cancelled successfully, so the |results| are
empty, but this is not the "truth", so the consumer must not be notified with
the empty set.
  2.) The request completed successfully with no results, in which case the
consumer must be called back with the empty set.

This would also make it very difficult to use scoped_refptr, or to assemble
results in multiple steps.

Please do let me know if you see a nice workaround.

[vabr (Chromium), 2015-02-12 08:57:50]

This LGTM, with comments.

Your CL is definitely an improvement in the current state.

Once you add the affiliation login requests to be called in a sequence with the
"normal" ones, I agree that we should add histograms to monitor the potential
performance hit. If we need to deal with it, then the options we have include an
explicit refcounting of successfully obtained result batches, in addition to the
implicit refcounting of the holders of the request (we would only notify if both
refcounts reach 0). Or we can revisit the restriction that the consumer must not
get empty results when there was an error.

Cheers,
Vaclav

https://codereview.chromium.org/911323003/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_win.cc (right):

https://codereview.chromium.org/911323003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_win.cc:34: // Requests the IE7
login for |form|. This is async. |callback_runner| will be
Please update the comment with the new arg name.

https://codereview.chromium.org/911323003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_win.cc:44: const
PasswordStoreWin::ConsumerCallbackRunner& result_callback)
Change the type to ResultCallback.

(Interestingly, the PasswordStoreWin::ConsumerCallbackRunner has not been
defined even before, only PasswordStore::ConsumerCallbackRunner. I wonder where
it got it into the code, and why the compilers did not speak up. :))

https://codereview.chromium.org/911323003/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/911323003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_store.cc:57:
remaining_logins.swap(results);
You can drop the swap() and pass remaining_logins instead.

https://codereview.chromium.org/911323003/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/911323003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_store.h:195:
ConsumerCallbackRunner;
Is this typedef used any more?

[engedy, 2015-02-12 09:34:42]

New patchsets have been uploaded after l-g-t-m from vabr@chromium.org

[engedy, 2015-02-12 09:35:51]

Thanks for the nice observations!

https://codereview.chromium.org/911323003/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_win.cc (right):

https://codereview.chromium.org/911323003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_win.cc:34: // Requests the IE7
login for |form|. This is async. |callback_runner| will be
On 2015/02/12 08:57:49, vabr (Chromium) wrote:
> Please update the comment with the new arg name.

Done.

https://codereview.chromium.org/911323003/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_store_win.cc:44: const
PasswordStoreWin::ConsumerCallbackRunner& result_callback)
On 2015/02/12 08:57:50, vabr (Chromium) wrote:
> Change the type to ResultCallback.
> 
> (Interestingly, the PasswordStoreWin::ConsumerCallbackRunner has not been
> defined even before, only PasswordStore::ConsumerCallbackRunner. I wonder
where
> it got it into the code, and why the compilers did not speak up. :))

Done.

https://codereview.chromium.org/911323003/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/911323003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_store.cc:57:
remaining_logins.swap(results);
On 2015/02/12 08:57:50, vabr (Chromium) wrote:
> You can drop the swap() and pass remaining_logins instead.

Done.

https://codereview.chromium.org/911323003/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/911323003/diff/20001/components/password_mana...
components/password_manager/core/browser/password_store.h:195:
ConsumerCallbackRunner;
On 2015/02/12 08:57:50, vabr (Chromium) wrote:
> Is this typedef used any more?

Nope, nice catch.

[engedy, 2015-02-12 09:36:07]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2015-02-12 09:36:20]

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

[commit-bot: I haz the power, 2015-02-12 10:13:36]

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

[commit-bot: I haz the power, 2015-02-12 10:13:36]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[engedy, 2015-02-13 13:35:03]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2015-02-13 13:35:50]

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

[commit-bot: I haz the power, 2015-02-13 14:20:30]

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

[commit-bot: I haz the power, 2015-02-13 14:20:31]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[engedy, 2015-02-13 15:59:13]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2015-02-13 16:00:07]

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

[commit-bot: I haz the power, 2015-02-13 17:05:07]

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

[commit-bot: I haz the power, 2015-02-13 17:05:08]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[engedy, 2015-02-13 17:09:22]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2015-02-13 17:10:27]

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

[commit-bot: I haz the power, 2015-02-13 17:38:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-13 17:38:31]

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