Avoid creating PasswordsPrivateDelegate when shutting down.

Avoid creating PasswordsPrivateDelegate when shutting down.

BUG=485227
Committed: https://crrev.com/b57eeaf0c280ffe412f75972d1aa2cf89fd030ce
Cr-Commit-Position: refs/heads/master@{#339855}

[mharanczyk, 2015-07-14 12:50:32]

mharanczyk@opera.com changed reviewers:
+ khorimoto@chromium.org, stevenjb@chromium.org

[mharanczyk, 2015-07-14 12:59:01]

stevenjb@ khorimoto@, PTAL, this is continuation of what you reported here:
https://codereview.chromium.org/1142693003/diff/390001/chrome/browser/extensi...
Creation of PasswordsPrivateDelegateFactory causes posting of some tasks and
other stuff to happen, it is pointless to do all this and delay shutdown.

[mharanczyk, 2015-07-20 07:58:40]

mharanczyk@opera.com changed reviewers:
+ asargent@chromium.org, rdevlin.cronin@chromium.org

[mharanczyk, 2015-07-20 07:59:02]

Ping! PTAL.

[Devlin, 2015-07-20 16:20:07]

On 2015/07/20 07:59:02, mharanczyk wrote:
> Ping! PTAL.

You shouldn't need all four of us as reviewers.  As OWNER of the
passwords_private API, stevenjb@ or khorimoto@ should be sufficient.

[stevenjb, 2015-07-20 18:02:56]

https://codereview.chromium.org/1228813006/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_factory.h
(right):

https://codereview.chromium.org/1228813006/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_factory.h:26:
bool create);
We generally discourage having two methods with the same name but different
signatures in Chrome. Instead, I would just add |create| and update any other
existing calls (there shouldn't be very many) to look like
GetForBrowserContext(context, true /* create */).

[mharanczyk, 2015-07-21 08:05:16]

mharanczyk@opera.com changed reviewers:
- asargent@chromium.org, rdevlin.cronin@chromium.org

[mharanczyk, 2015-07-21 08:15:57]

https://codereview.chromium.org/1228813006/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_factory.h
(right):

https://codereview.chromium.org/1228813006/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_factory.h:26:
bool create);
On 2015/07/20 18:02:56, stevenjb wrote:
> We generally discourage having two methods with the same name but different
> signatures in Chrome. Instead, I would just add |create| and update any other
> existing calls (there shouldn't be very many) to look like
> GetForBrowserContext(context, true /* create */).
> 

There is also another way to avoid creating service at shutdown. I can add
PasswordsPrivateDelegate* field to event router, init it with null and only
assign it once in PasswordsPrivateEventRouter::StartOrStopListeningForChanges
then use it. Since both PasswordsPrivateEventRouter and PasswordsPrivateDelegate
are KeyedServices and event router factory depends on delegate factory it is
safe to use bare pointer in this case as it is guaranteed that delegate will
outlive router. Which solution is better for you?

[mharanczyk, 2015-07-21 08:28:37]

Made changes according to stevenjb@ original suggestion. Please take a look
again.

[stevenjb, 2015-07-21 20:38:25]

lgtm

[stevenjb, 2015-07-21 20:39:31]

On 2015/07/21 08:15:57, mharanczyk wrote:
>
https://codereview.chromium.org/1228813006/diff/1/chrome/browser/extensions/a...
> File
>
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_factory.h
> (right):
> 
>
https://codereview.chromium.org/1228813006/diff/1/chrome/browser/extensions/a...
>
chrome/browser/extensions/api/passwords_private/passwords_private_delegate_factory.h:26:
> bool create);
> On 2015/07/20 18:02:56, stevenjb wrote:
> > We generally discourage having two methods with the same name but different
> > signatures in Chrome. Instead, I would just add |create| and update any
other
> > existing calls (there shouldn't be very many) to look like
> > GetForBrowserContext(context, true /* create */).
> > 
> 
> There is also another way to avoid creating service at shutdown. I can add
> PasswordsPrivateDelegate* field to event router, init it with null and only
> assign it once in PasswordsPrivateEventRouter::StartOrStopListeningForChanges
> then use it. Since both PasswordsPrivateEventRouter and
PasswordsPrivateDelegate
> are KeyedServices and event router factory depends on delegate factory it is
> safe to use bare pointer in this case as it is guaranteed that delegate will
> outlive router. Which solution is better for you?

FWIW, the above solution is probably preferable, but this is fine too.

[mharanczyk, 2015-07-22 08:34:10]

The CQ bit was checked by mharanczyk@opera.com

[commit-bot: I haz the power, 2015-07-22 08:34:21]

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

[commit-bot: I haz the power, 2015-07-22 09:28:12]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-07-22 09:31:09]

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