Postpone collecting passwords UMA metrics.

Postpone collecting passwords UMA metrics.

BUG=453835
Committed: https://crrev.com/f2d4cd89181eea9a913487b5fe5c9f0a432955cf
Cr-Commit-Position: refs/heads/master@{#314350}

[dvadym, 2015-02-02 13:24:50]

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

[dvadym, 2015-02-02 13:24:50]

Hi Vaclav,

 Could you please review this CL?

Best regards,
Vadym

[vabr (Chromium), 2015-02-02 14:39:14]

LGTM, this sounds reasonable.

I'm still a bit worried about this practice of randomly postponing tasks in
general, instead of having recognised events of "load is low, let's do less
important stuff". There was a promising proposal about runlevels on chromium-dev
in 2013
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/i7nyQOWRUfY/qdJOZ...,
but that somehow sadly died out.

Cheers,
Vaclav

[dvadym, 2015-02-02 14:43:55]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2015-02-02 14:44:36]

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

[commit-bot: I haz the power, 2015-02-02 15:03:01]

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

[commit-bot: I haz the power, 2015-02-02 15:03:02]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[dvadym, 2015-02-03 10:43:24]

Hi Vaclav,

 Could you please take another look at this CL?

Best regards,
Vadym

[vabr (Chromium), 2015-02-03 15:43:54]

Thanks for the fix. I should have noted that delaying all PasswordStore tasks by
30 seconds is not the best idea. :)

LGTM.

Cheers,
Vaclav

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

https://codereview.chromium.org/887043004/diff/20001/components/password_mana...
components/password_manager/core/browser/password_store.cc:165: if
(task_runner.get()) {
nit: You can drop ".get()".

[vabr (Chromium), 2015-02-03 15:44:59]

On 2015/02/03 15:43:54, vabr (Chromium) wrote:
> Thanks for the fix. I should have noted that delaying all PasswordStore tasks
by
> 30 seconds is not the best idea. :)
s/noted/noticed

(In other words, sorry for a crappy review.)

[dvadym, 2015-02-03 15:51:41]

Thanks, Vaclav.

Sorry, it's also my fault, I should have noticed this also.

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

https://codereview.chromium.org/887043004/diff/20001/components/password_mana...
components/password_manager/core/browser/password_store.cc:165: if
(task_runner.get()) {
On 2015/02/03 15:43:54, vabr (Chromium) wrote:
> nit: You can drop ".get()".

Done.

[dvadym, 2015-02-03 15:52:05]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2015-02-03 15:52:56]

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

[commit-bot: I haz the power, 2015-02-03 16:35:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-03 16:36:10]

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

[vasilii, 2015-02-03 17:27:42]

vasilii@chromium.org changed reviewers:
+ vasilii@chromium.org

[vasilii, 2015-02-03 17:27:42]

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

https://codereview.chromium.org/887043004/diff/40001/components/password_mana...
components/password_manager/core/browser/password_store.cc:167:
base::Bind(&PasswordStore::ReportMetricsImpl, this, sync_username,
My concern is that you capture the scoped pointer for 30 seconds. After this 30
seconds the password store may have its Shutdown() called. While it probably
doesn't lead to crash, I don't think that delaying PasswordStore and
LoginDatabase destruction is a good idea.

[dvadym, 2015-02-05 09:54:21]

I checked, even in case when we delete user profile PasswordStore is not
destructed. So it will not cause any crashes. But there is another strange
thing, that even if there are two profiles in use, we still have only report
collection. I'll investigate it later.