Password manager internals page service: introduction

Password manager internals page service: introduction

Introduce a BrowserContextKeyedService to collect logs from the PasswordManagerClient instances, and distribute them to PasswordManagerInternalsUI instances for display. The service will also ensure that nothing is logged in Incognito profiles, and that logs are flushed as soon as no one is displaying them.

Except for checking the incognito bit, the whole functionality is implemented in the LogRouter class inside the password_manager component, for easy code sharing across platforms.
The service inherits from LogRouter, and adds nothing except for leveraging the BrowserContextKeyedServiceFactory framework to keep the functionality from OTR profiles.

This CL only introduces the service, it does not put it to use (outside tests) yet.
For that there will be another CL (https://codereview.chromium.org/269513003, but WIP, not fit for (re)viewing yet).
Yet another CL deals with renaming PasswordManagerLogger to LogReceiver: https://codereview.chromium.org/264793010/

Design doc: https://docs.google.com/document/d/1ArDhTo0w-8tOPiTwqM1gG6ZGqODo8UTpXlJjjnCNK4s/edit?usp=sharing#heading=h.cqh4wuj8j4yl

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

[vabr (Chromium), 2014-05-02 11:23:22]

Hi Ilya,

Would you mind taking a look?

(Note: this time I don't need you as an OWNER, so it's fine if you decline to
review. I'd still love your feedback, though, both in general, and also because
you have reviewed the other bits of the internals page.)

Thanks!
Vaclav

[vabr (Chromium), 2014-05-02 12:33:08]

Sorry, Ilya,
Actually please hold off with the review if you have not started yet. I need to
change the interface a bit.
I will ping you once it is finished.

Thanks!
Vaclav

[vabr (Chromium), 2014-05-02 13:48:34]

Hi Ilya,

Patch set 3 is the one to look at: PTAL.

Whenever I add TODO(vabr) in the code, it's meant to be addressed within my next
2 CLs. I postponed some of the changes to make the CL smaller.

Thanks,
Vaclav

[vabr (Chromium), 2014-05-02 15:33:10]

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
File
chrome/browser/password_manager/password_manager_internals_service_unittest.cc
(right):

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service_unittest.cc:35:
->MarkBrowserContextLiveForTesting(context.get());
TODO(vabr): Need to put this inside #if !defined(NDEBUG) #endif, to follow the
definition of that function.

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
File components/password_manager/core/browser/log_router.h (right):

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.h:50: bool
RegisterClient(PasswordManagerClient* client);
TODO(vabr): Remove the failure reporting via return value (turned out that in my
subsequent CL I was always ignoring the value). Make RegisterClient return the
current logging activity state instead. (That will spare some headaches in the
next CL about RegisterClient calling directly back to the client to set the
state via a virtual method, in combination with RegisterClient itself being
called from the client's constructor.)

[Ilya Sherman, 2014-05-02 23:38:38]

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
File
chrome/browser/password_manager/password_manager_internals_service_factory.cc
(right):

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service_factory.cc:35:
content::BrowserContext* /*context*/) const {
nit: Please leave spaces next to the asterisks, i.e. "/* context */".  Please do
this above as well.

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
File
chrome/browser/password_manager/password_manager_internals_service_unittest.cc
(right):

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service_unittest.cc:24:
enum ProfileType { NORMAL_PROFILE, OTR_PROFILE };
nit: "OTR" -> "INCOGNITO"

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service_unittest.cc:30:
scoped_ptr<TestingProfile> context(builder.Build());
nit: "context" -> "profile"

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service_unittest.cc:43:
TEST(PasswordManagerInternalsServiceTest, ServiceActiveNoOTR) {
nit: "OTR" -> "Incognito" throughout.  Likewise, "off-the-record" ->
"Incognito".  "Off the record" was the original feature name, but it's no longer
the one that we use.

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service_unittest.cc:44:
scoped_ptr<TestingProfile> context(CreateProfile(NORMAL_PROFILE));
nit: Please be consistent about "profile" or "context" throughout.  If you just
want to think of this as a context, then please use BrowserContext in place of
TestingProfile, and CreateBrowserContext() rather than CreateProfile().

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
File components/password_manager/core/browser/log_router.cc (right):

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.cc:17: }
nit: DCHECK that all listeners have unregistered?  (Not needed if you use
ObserverLists.)

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.cc:33: return false;
Can this be a DCHECK instead?

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.cc:43: return false;
Can this be a DCHECK instead?

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.cc:53: return false;
Can this be a DCHECK instead?

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.cc:66: return false;
Can this be a DCHECK instead?

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
File components/password_manager/core/browser/log_router.h (right):

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.h:25: // stored logs.
Is this complexity worth it, vs. just making the internals page be a singleton?

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.h:36: enum
RouterActivityState { ROUTER_ACTIVE, ROUTER_NOT_ACTIVE };
nit: "NOT_ACTIVE" -> "INACTIVE"

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.h:43: void ProcessLog(const
std::string& text);
Perhaps it should be an error to call this method when the router is inactive?

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.h:57: typedef
std::set<PasswordManagerLogger*> ReceiverSet;
nit: Did you consider using an ObserverList?

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
File components/password_manager/core/browser/log_router_unittest.cc (right):

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router_unittest.cc:54:
router.ProcessLog(kTestText);
Please split this off to a ZeroReceivers test case.

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router_unittest.cc:58: // Check
that only logs generated after activation are passed.
Please test this in a separate test case as well.  Each TEST_F should ideally
correspond to a single simple test, like the one described in this sentence.

[vabr (Chromium), 2014-05-06 13:16:30]

Hi Ilya,

Thanks for your comments so far.
I addressed them. Could you please take another look?

Cheers,
Vaclav

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
File
chrome/browser/password_manager/password_manager_internals_service_factory.cc
(right):

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service_factory.cc:35:
content::BrowserContext* /*context*/) const {
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> nit: Please leave spaces next to the asterisks, i.e. "/* context */".  Please
do
> this above as well.

Done.

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
File
chrome/browser/password_manager/password_manager_internals_service_unittest.cc
(right):

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service_unittest.cc:24:
enum ProfileType { NORMAL_PROFILE, OTR_PROFILE };
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> nit: "OTR" -> "INCOGNITO"

Done.

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service_unittest.cc:30:
scoped_ptr<TestingProfile> context(builder.Build());
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> nit: "context" -> "profile"

Done.

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service_unittest.cc:43:
TEST(PasswordManagerInternalsServiceTest, ServiceActiveNoOTR) {
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> nit: "OTR" -> "Incognito" throughout.  Likewise, "off-the-record" ->
> "Incognito".  "Off the record" was the original feature name, but it's no
longer
> the one that we use.

Thanks, Ilya, I did not know that OTR is deprecated.
Done.

https://codereview.chromium.org/262583007/diff/150001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service_unittest.cc:44:
scoped_ptr<TestingProfile> context(CreateProfile(NORMAL_PROFILE));
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> nit: Please be consistent about "profile" or "context" throughout.  If you
just
> want to think of this as a context, then please use BrowserContext in place of
> TestingProfile, and CreateBrowserContext() rather than CreateProfile().

That's a fair point. I'll switch to "profile" completely.
In fact, I only need to think about this as a context, but I cannot really
create a TestBrowserContext here (http://goo.gl/QQG5wo + chrome/browser/DEPS:85
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/DEP...).

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
File components/password_manager/core/browser/log_router.cc (right):

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.cc:17: }
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> nit: DCHECK that all listeners have unregistered?  (Not needed if you use
> ObserverLists.)

Done via using ObserverList with the right template argument.

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.cc:33: return false;
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> Can this be a DCHECK instead?

Done via ObserverList.

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.cc:43: return false;
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> Can this be a DCHECK instead?

Switched to ObserverList, which actually does not do this check.
I could add that, but I don't actually have a strong need to.
Let me know if you would like me to restore that check (in which case I will do
it via DCHECK as suggested).

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.cc:53: return false;
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> Can this be a DCHECK instead?

Done via ObserverList.

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.cc:66: return false;
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> Can this be a DCHECK instead?

Dropped. The same as in my response in UnregisterClient applies.

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
File components/password_manager/core/browser/log_router.h (right):

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.h:25: // stored logs.
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> Is this complexity worth it, vs. just making the internals page be a
singleton?

Having the internals page a singleton is only a half of the problem -- another
one is how to discover it from the other tabs. There is an established and easy
way to get hold of a service, but not a one for a UI controller.

Also having the internals page as a singleton would require some non-trivial
code arrangement, IMO. I did not see an easy way to handle things like the user
trying to open multiple tabs with the internals page while keeping the UI
controller a singleton.

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.h:36: enum
RouterActivityState { ROUTER_ACTIVE, ROUTER_NOT_ACTIVE };
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> nit: "NOT_ACTIVE" -> "INACTIVE"

Actually, I dropped the enum. Instead of introducing the notion of
"active"/"inactive" I tried to directly speak about having 0 or more receivers.
In the follow-up CL, where I will introduce the API for state change
notification (switching between 0 and more receivers), if having the enum proves
more readable than sending a bool, I'll reintroduce it with the word "inactive"
for the opposite of active.

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.h:43: void ProcessLog(const
std::string& text);
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> Perhaps it should be an error to call this method when the router is inactive?

I'll give it a try, since I could not actually think of a situation when the
user would close the internals page just at the right time so the client would
still think it's active and call ProcessLog, while the state would already have
changed.

If there turns out to be a situation like this, then I'd like to go back to
dropping the logs silently, because at the time the internals page is being
closed, nobody is interested in them any more.

Done.

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router.h:57: typedef
std::set<PasswordManagerLogger*> ReceiverSet;
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> nit: Did you consider using an ObserverList?

Thanks for pointing me to that.
While I'm not sure I need the guarantee that iterators through the list are not
invalidated during list modification, I appreciate that using the ObserverList
will make the code easier to understand, and also reduce the amount of new code.
Done.

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
File components/password_manager/core/browser/log_router_unittest.cc (right):

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router_unittest.cc:54:
router.ProcessLog(kTestText);
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> Please split this off to a ZeroReceivers test case.

I split that as suggested (+ modified to respect that calling ProcessLog is no
longer allowed before receiver registration).
However, the whole ProcessLog_NoReceiver test is included in
ProcessLog_OneReceiver anyway, and I can't help it (except for removing
EXPECT_EQ(std::string(),...) from router.RegisterReceiver(&receiver_), which I
don't see a good reason for).

In general, thanks for pushing to split the tests to single things to test!
I'll try to work on my tendency to lump all together in the future, to spare you
the effort of repeating this to me. :)

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router_unittest.cc:58: // Check
that only logs generated after activation are passed.
On 2014/05/02 23:38:38, Ilya Sherman wrote:
> Please test this in a separate test case as well.  Each TEST_F should ideally
> correspond to a single simple test, like the one described in this sentence.

After CHECKing against calling ProcessLog without registered receivers, the last
part of this test disappeared, so there is no need to split off the middle part
any more.

https://codereview.chromium.org/262583007/diff/150001/components/password_man...
components/password_manager/core/browser/log_router_unittest.cc:94: // The
following tests cover cases of registration and unregistration not tested
I dropped all of the following tests, as the tested functionality is now covered
by ObserverList.

[Ilya Sherman, 2014-05-07 00:28:39]

LGTM with the remaining comments addressed.  Thanks.

https://codereview.chromium.org/262583007/diff/210001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_internals_service.h
(right):

https://codereview.chromium.org/262583007/diff/210001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service.h:18: // them
to the open tabs with the internals page.
nit: "the open tabs" -> "all open tabs"

https://codereview.chromium.org/262583007/diff/210001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service.h:24: // 2)
No service will be created for incognito BrowserContext.
nit: "for incognito BrowserContext" -> "for an incognito BrowserContext" or "for
incognito BrowserContexts"

https://codereview.chromium.org/262583007/diff/210001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service.h:30: // May
return NULL when |context| is incognito.
nit: "May return" -> "Returns".

https://codereview.chromium.org/262583007/diff/210001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service.h:31: static
PasswordManagerInternalsService* Get(content::BrowserContext* context);
nit: Probably appropriate to name this GetForBrowserContext(), to match the
common pattern.

https://codereview.chromium.org/262583007/diff/210001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/262583007/diff/210001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:1415:
'browser/password_manager/password_manager_internals_service_factory.h',
nit: It looks like these classes don't have any dependencies on //chrome code. 
Does it make sense to move them into the password_manager component?

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
File components/password_manager/core/browser/log_router.cc (right):

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
components/password_manager/core/browser/log_router.cc:22:
CHECK(receivers_.might_have_observers());
nit: DCHECK?

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
components/password_manager/core/browser/log_router.cc:50: DCHECK(receiver);
nit: IMO DCHECK(receivers_.HasObserver(receiver)) would be more meaningful.

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
File components/password_manager/core/browser/log_router.h (right):

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
components/password_manager/core/browser/log_router.h:59: // Logs seen so far
since the first receiver was registered.
nit: "seen so far since" -> "accumulated since"

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
File components/password_manager/core/browser/log_router_unittest.cc (right):

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
components/password_manager/core/browser/log_router_unittest.cc:38: // on the
registration of the first receiver, none are returned.
Did you mean to have a ProcessLog() call somewhere in this test?  Otherwise, the
accumulated logs are empty because nobody has tried to log anything, as well as
because there were no receivers.

[blundell, 2014-05-07 09:05:14]

https://codereview.chromium.org/262583007/diff/210001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_internals_service.h
(right):

https://codereview.chromium.org/262583007/diff/210001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service.h:31: static
PasswordManagerInternalsService* Get(content::BrowserContext* context);
This method will have to be eliminated for the service to live in the core code
of the component.

On 2014/05/07 00:28:39, Ilya Sherman wrote:
> nit: Probably appropriate to name this GetForBrowserContext(), to match the
> common pattern.

[vabr (Chromium), 2014-05-07 13:42:58]

Thanks, Ilya and also Colin!

Ilya, I addressed all your comments, but you might want to review in particular
my response about ProcessLog.

Please let me know what you think.

Cheers,
Vaclav

https://codereview.chromium.org/262583007/diff/210001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_internals_service.h
(right):

https://codereview.chromium.org/262583007/diff/210001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service.h:18: // them
to the open tabs with the internals page.
On 2014/05/07 00:28:39, Ilya Sherman wrote:
> nit: "the open tabs" -> "all open tabs"

Done.

https://codereview.chromium.org/262583007/diff/210001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service.h:24: // 2)
No service will be created for incognito BrowserContext.
On 2014/05/07 00:28:39, Ilya Sherman wrote:
> nit: "for incognito BrowserContext" -> "for an incognito BrowserContext" or
"for
> incognito BrowserContexts"

Done.

https://codereview.chromium.org/262583007/diff/210001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service.h:30: // May
return NULL when |context| is incognito.
On 2014/05/07 00:28:39, Ilya Sherman wrote:
> nit: "May return" -> "Returns".

The whole function was dropped (see Colin's comment).

https://codereview.chromium.org/262583007/diff/210001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service.h:31: static
PasswordManagerInternalsService* Get(content::BrowserContext* context);
On 2014/05/07 00:28:39, Ilya Sherman wrote:
> nit: Probably appropriate to name this GetForBrowserContext(), to match the
> common pattern.

Agreed, but I eventually dropped the function (see below).

https://codereview.chromium.org/262583007/diff/210001/chrome/browser/password...
chrome/browser/password_manager/password_manager_internals_service.h:31: static
PasswordManagerInternalsService* Get(content::BrowserContext* context);
On 2014/05/07 09:05:15, blundell wrote:
> This method will have to be eliminated for the service to live in the core
code
> of the component.
> 
> On 2014/05/07 00:28:39, Ilya Sherman wrote:
> > nit: Probably appropriate to name this GetForBrowserContext(), to match the
> > common pattern.
> 

Done.

https://codereview.chromium.org/262583007/diff/210001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/262583007/diff/210001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:1415:
'browser/password_manager/password_manager_internals_service_factory.h',
On 2014/05/07 00:28:39, Ilya Sherman wrote:
> nit: It looks like these classes don't have any dependencies on //chrome code.

> Does it make sense to move them into the password_manager component?

Done.

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
File components/password_manager/core/browser/log_router.cc (right):

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
components/password_manager/core/browser/log_router.cc:22:
CHECK(receivers_.might_have_observers());
On 2014/05/07 00:28:39, Ilya Sherman wrote:
> nit: DCHECK?

I was thinking about a DCHECK, but there's a catch to that:
If I allow release builds to ProcessLog when there's no receiver, I still need
to not add the logs to accumulated_logs_ (line below).

That's something I need to test, and such tests would need to be Release-only
(in Debug they would crash).

I understood that it is a poor practice to have Release and Debug have different
behaviour up to the point of needing different tests for them. That's why I
chose CHECK.

As long as the LogRouter only lives on a single thread (added ThreadChecker to
guard and document that -- note that guarding only works in Debug), a
cooperating client should be able to 100% avoid calling ProcessLog when there's
no receiver.

If you prefer Release-build-only tests to the danger of release build crashes,
please let me know. Alternatively we can switch back to being permissive and
make ProcessLog a harmless no-op when there is no receiver (with adding the
tests mentioned above, but for both Release and Debug).

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
components/password_manager/core/browser/log_router.cc:50: DCHECK(receiver);
On 2014/05/07 00:28:39, Ilya Sherman wrote:
> nit: IMO DCHECK(receivers_.HasObserver(receiver)) would be more meaningful.

Agreed, also for the UnregisterClient.
Done.

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
File components/password_manager/core/browser/log_router.h (right):

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
components/password_manager/core/browser/log_router.h:59: // Logs seen so far
since the first receiver was registered.
On 2014/05/07 00:28:39, Ilya Sherman wrote:
> nit: "seen so far since" -> "accumulated since"

Done.

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
File components/password_manager/core/browser/log_router_unittest.cc (right):

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
components/password_manager/core/browser/log_router_unittest.cc:38: // on the
registration of the first receiver, none are returned.
On 2014/05/07 00:28:39, Ilya Sherman wrote:
> Did you mean to have a ProcessLog() call somewhere in this test?  Otherwise,
the
> accumulated logs are empty because nobody has tried to log anything, as well
as
> because there were no receivers.

Good point. Since I cannot actually call ProcessLog without receivers, the only
thing to test here is that after receivers are registered and unregistered, all
accumulated logs are flushed. In this sense the test actually subsumes the
following one, though. I don't think I can avoid that.

[Ilya Sherman, 2014-05-08 03:38:52]

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
File components/password_manager/core/browser/log_router.cc (right):

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
components/password_manager/core/browser/log_router.cc:22:
CHECK(receivers_.might_have_observers());
On 2014/05/07 13:42:59, vabr (Chromium) wrote:
> On 2014/05/07 00:28:39, Ilya Sherman wrote:
> > nit: DCHECK?
> 
> I was thinking about a DCHECK, but there's a catch to that:
> If I allow release builds to ProcessLog when there's no receiver, I still need
> to not add the logs to accumulated_logs_ (line below).

A DCHECK doesn't mean that something is disallowed in Debug builds and allowed
in Release builds.  It's an assertion that a certain condition should always
hold.  However, that assertion is only checked in Debug builds, for a variety of
contentious reasons like performance and binary bloat.  Essentially, none of the
reasons apply to an individual assertion; but they're thought to add up across
the whole codebase.

CHECK is generally reserved for exceptional cases: For example to prevent
potential security exploits due to subtle or easy-to-misuse APIs, or to generate
crash dumps to investigate a hard-to-reproduce issue.

[vabr (Chromium), 2014-05-08 17:41:53]

Hi Ilya,

I changed the CHECK to DCHECK (see the comment below).
I also removed the ThreadChecker, as that seemed like an overkill.

PTAL.
Cheers,
Vaclav

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
File components/password_manager/core/browser/log_router.cc (right):

https://codereview.chromium.org/262583007/diff/210001/components/password_man...
components/password_manager/core/browser/log_router.cc:22:
CHECK(receivers_.might_have_observers());
On 2014/05/08 03:38:52, Ilya Sherman wrote:
> On 2014/05/07 13:42:59, vabr (Chromium) wrote:
> > On 2014/05/07 00:28:39, Ilya Sherman wrote:
> > > nit: DCHECK?
> > 
> > I was thinking about a DCHECK, but there's a catch to that:
> > If I allow release builds to ProcessLog when there's no receiver, I still
need
> > to not add the logs to accumulated_logs_ (line below).
> 
> A DCHECK doesn't mean that something is disallowed in Debug builds and allowed
> in Release builds.  It's an assertion that a certain condition should always
> hold.  However, that assertion is only checked in Debug builds, for a variety
of
> contentious reasons like performance and binary bloat.  Essentially, none of
the
> reasons apply to an individual assertion; but they're thought to add up across
> the whole codebase.
> 
> CHECK is generally reserved for exceptional cases: For example to prevent
> potential security exploits due to subtle or easy-to-misuse APIs, or to
generate
> crash dumps to investigate a hard-to-reproduce issue.

Thanks Ilya, for explaining the use-cases for CHECK vs. DCHECK. I should have
read
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
more carefully.

Since the style guide also says not to handle situations after a DCHECK, I did
not try to avoid adding stuff to accumulated_logs_ when there are no receivers.
Instead I put one more accumulated_logs_.clear() at the place when the first
receiver is registered (and would otherwise get the accumulated_logs_). That
seems like a reasonable thing to have anyway. I did not add any new tests,
because there is nothing new to test in Debug. If you want me to add a
Release-only test for not preserving logs passed through ProcessLog without
receivers, just let me know.

[Ilya Sherman, 2014-05-08 17:48:38]

https://codereview.chromium.org/262583007/diff/310001/components/password_man...
File components/password_manager/core/browser/log_router.cc (right):

https://codereview.chromium.org/262583007/diff/310001/components/password_man...
components/password_manager/core/browser/log_router.cc:44:
accumulated_logs_.clear();
There's a DCHECK above that asserts that this code should always be a no-op.  If
the DCHECK is correct, then there is no need to handle this case.  Are you're
worried that the DCHECKs might not be correct?

[vabr (Chromium), 2014-05-09 09:47:19]

Hi Ilya,

I have a couple of ways to deal with the case of accumulating logs when there
are no receivers. I'm not sure what's the best. I summed that up at
https://docs.google.com/a/google.com/document/d/1Uldt2XxS8RFViDN2OSX0tTgzxZtZ...

Any advice or comments from you welcome!

Cheers,
Vaclav

https://codereview.chromium.org/262583007/diff/310001/components/password_man...
File components/password_manager/core/browser/log_router.cc (right):

https://codereview.chromium.org/262583007/diff/310001/components/password_man...
components/password_manager/core/browser/log_router.cc:44:
accumulated_logs_.clear();
On 2014/05/08 17:48:38, Ilya Sherman wrote:
> There's a DCHECK above that asserts that this code should always be a no-op. 
If
> the DCHECK is correct, then there is no need to handle this case.  Are you're
> worried that the DCHECKs might not be correct?

I overlooked the DCHECK above, my bad.
Yes, I'm worried about what happens if the DCHECK assertions would not hold in a
release mode.
Although I'm fairly certain they won't, neither after this CL, nor after the
follow-up which starts using the service, there is always the risk that I'm
wrong.
The current state of the code breaks the "don't deal with a failed DCHECK" rule
no less than guarding the accumulated_logs_.append() by an if (there are some
receivers) would in ProcessLog. :(
So I'll remove the if-block, and ask you some more things in the main e-mail
body.

[vabr (Chromium), 2014-05-09 17:31:26]

One more edit: when moving the service and its factory to
//components/password_manager, I forgot to also put them in the password_manager
namespace.

Patch sets 12 and 13 do that.

Cheers,
Vaclav

[Ilya Sherman, 2014-05-09 22:10:47]

On 2014/05/09 09:47:19, vabr (Chromium) wrote:
> Hi Ilya,
> 
> I have a couple of ways to deal with the case of accumulating logs when there
> are no receivers. I'm not sure what's the best. I summed that up at
>
https://docs.google.com/a/google.com/document/d/1Uldt2XxS8RFViDN2OSX0tTgzxZtZ...
> 
> Any advice or comments from you welcome!

I replied on the doc, but in short: If you're not confident that the DCHECK is
correct, then it's fine to remove the DCHECK and just handle the failure.  If
you're confident that the DCHECK is correct, then we shouldn't try to handle
failures.

[vabr (Chromium), 2014-05-10 14:38:29]

On 2014/05/09 22:10:47, Ilya Sherman wrote:
> On 2014/05/09 09:47:19, vabr (Chromium) wrote:
> > Hi Ilya,
> > 
> > I have a couple of ways to deal with the case of accumulating logs when
there
> > are no receivers. I'm not sure what's the best. I summed that up at
> >
>
https://docs.google.com/a/google.com/document/d/1Uldt2XxS8RFViDN2OSX0tTgzxZtZ...
> > 
> > Any advice or comments from you welcome!
> 
> I replied on the doc, but in short: If you're not confident that the DCHECK is
> correct, then it's fine to remove the DCHECK and just handle the failure.  If
> you're confident that the DCHECK is correct, then we shouldn't try to handle
> failures.

Thank you Ilya.
I'm confident in the DCHECK being correct, hence I won't handle the failure
(true of the current patch set).

You approved the CL before, and expressed agreement with DCHECK + not handling
the failure, so I almost feel like landing :),
but please confirm that also moving stuff in //components/password_manager to
the password_manager namespace sounds fine to you (that's precisely the diff
between patch set 13 and 11).

Thanks for your time on this.

Cheers,
Vaclav

[Ilya Sherman, 2014-05-10 17:45:26]

On 2014/05/10 14:38:29, vabr (Chromium) wrote:
> On 2014/05/09 22:10:47, Ilya Sherman wrote:
> > On 2014/05/09 09:47:19, vabr (Chromium) wrote:
> > > Hi Ilya,
> > > 
> > > I have a couple of ways to deal with the case of accumulating logs when
> there
> > > are no receivers. I'm not sure what's the best. I summed that up at
> > >
> >
>
https://docs.google.com/a/google.com/document/d/1Uldt2XxS8RFViDN2OSX0tTgzxZtZ...
> > > 
> > > Any advice or comments from you welcome!
> > 
> > I replied on the doc, but in short: If you're not confident that the DCHECK
is
> > correct, then it's fine to remove the DCHECK and just handle the failure. 
If
> > you're confident that the DCHECK is correct, then we shouldn't try to handle
> > failures.
> 
> Thank you Ilya.
> I'm confident in the DCHECK being correct, hence I won't handle the failure
> (true of the current patch set).
> 
> You approved the CL before, and expressed agreement with DCHECK + not handling
> the failure, so I almost feel like landing :),
> but please confirm that also moving stuff in //components/password_manager to
> the password_manager namespace sounds fine to you (that's precisely the diff
> between patch set 13 and 11).
> 
> Thanks for your time on this.
> 
> Cheers,
> Vaclav

Yep, that's fine.  LGTM.

[vabr (Chromium), 2014-05-10 19:56:48]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-05-10 19:59:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vabr@chromium.org/262583007/390001

[commit-bot: I haz the power, 2014-05-11 01:48:42]

Change committed as 269686