Password manager internals page service: wiring it in

Password manager internals page service 2: wiring it in

Start using the PasswordManagerInternalsService introduced in https://codereview.chromium.org/262583007/.

This CL breaks the fragile direct link between PasswordManagerClient and the UI contoller of the internals page, inserting the service between.

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=270372

[vabr (Chromium), 2014-05-09 18:23:18]

Hi Ilya,

This is hopefully the last big piece of work on the internals page. This CL is
based on https://codereview.chromium.org/262583007/ (hence no trybots yet).

I would be grateful, if you could review it.
There is no rush -- I will be sheriffing on Monday and Tuesday, and so unlikely
to work on this anyway.

Thank you!
Vaclav

P.S. After landing this CL and https://codereview.chromium.org/262583007/, the
rought TODO-list is: remove the flag gating the internals page, polish strings
and formatting of the internals page itself, and rename one class
(https://codereview.chromium.org/264793010/).
I don't plan to bother you with those reviews, unless necessary for OWNERS
approval. If you explicitly want to remain involved, just let me know, though.

[Ilya Sherman, 2014-05-13 04:32:17]

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:210: void
ChromePasswordManagerClient::NotifyCanUseLogRouter(
I find this method name pretty confusing.  Perhaps
"OnLogRouterAvailabilityChanged()", or something like that, would be clearer? 
The method does more than just send a notification -- it also caches the boolean
for its own purposes.

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:236: // WebUI
tabs do not need to log password saving progress.
nit: It would probably be useful to mention that this check is included so that
the internals page doesn't log its own page loads.

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:109: Profile*
const profile_;
Hmm, why did you change the profile to be cached, rather than calling
web_contents()->GetProfile()?  Is that change important for this CL?

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:32:
MockLogReceiver() {}
nit: Can this line be omitted?

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:69: }
Optional nit: It's fine to inline these implementations in test files, if you
like.

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:159:
EXPECT_EQ(std::string(), service_->RegisterReceiver(&receiver_));
I'm noticing that all of the tests in this file expect the return value from
RegisterReceiver to be the empty string.  Should there also be test coverage
here of the other case?  If not, maybe the EXPECT_EQ's are not needed at all,
since that doesn't seem to be an integral part of what's being tested in this
file.

(In general, there are lots of things that you can assert within a test.  It's
nice if the only assertions are the ones corresponding specifically to what the
test is aimed at testing.  That tends to make the test code easier to follow.)

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/password_manager_internals/password_manager_internals_ui.cc
(right):

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/password_manager_internals/password_manager_internals_ui.cc:50:
log_buffer_.append(service->RegisterReceiver(this));
Hmm, why do you need a log buffer?  Couldn't you just postpone registering as a
receiver until DidStopLoading() was called?  (Admittedly, you would then need to
keep track of whether or not you'd registered as a receiver, so maybe it's a
wash in terms of bookkeeping.)

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/password_manager_internals/password_manager_internals_ui_browsertest.cc
(right):

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/password_manager_internals/password_manager_internals_ui_browsertest.cc:94:
// service can make sure both internals pages are synchronised.
Would anything go wrong if all the tests were to log via the service, rather
than the controller?  It sounds like that is a more realistic test scenario
anyway.

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

https://codereview.chromium.org/269513003/diff/180001/components/password_man...
components/password_manager/core/browser/log_router.cc:43:
DCHECK(can_use_receivers_);
Hmm, this seems like a lot of work just for the sake of a DCHECK.  I'd omit all
the extra complexity, and just trust that you've verified this correctly.  I'd
expect that tools like ASan would catch any errors anyway.  In general, DCHECKs
are nice; but if they start requiring a bunch of work to maintain, it's probably
not worth the effort unless that work generalizes to many different callsites
(e.g. as ThreadChecker does).

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

https://codereview.chromium.org/269513003/diff/180001/components/password_man...
components/password_manager/core/browser/log_router.h:64: // Only notify
observes from |receivers_| if this is set to true. It will be
nit: "observes" -> "observers"

https://codereview.chromium.org/269513003/diff/180001/components/password_man...
components/password_manager/core/browser/log_router.h:69: #endif
Hmm, why is this code debug-only?

https://codereview.chromium.org/269513003/diff/180001/components/password_man...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/269513003/diff/180001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:76: //
|router_can_be_used| set to false, the client may no more use the
nit: "no more" -> "no longer"

[vabr (Chromium), 2014-05-13 09:27:11]

Thanks, Ilya, for very helpful comments.
I responded to / addressed all of them.
PTAL.
Cheers,
Vaclav

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:210: void
ChromePasswordManagerClient::NotifyCanUseLogRouter(
On 2014/05/13 04:32:18, Ilya Sherman wrote:
> I find this method name pretty confusing.  Perhaps
> "OnLogRouterAvailabilityChanged()", or something like that, would be clearer? 
> The method does more than just send a notification -- it also caches the
boolean
> for its own purposes.

Done.

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:236: // WebUI
tabs do not need to log password saving progress.
On 2014/05/13 04:32:18, Ilya Sherman wrote:
> nit: It would probably be useful to mention that this check is included so
that
> the internals page doesn't log its own page loads.

Done.

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:109: Profile*
const profile_;
On 2014/05/13 04:32:18, Ilya Sherman wrote:
> Hmm, why did you change the profile to be cached, rather than calling
> web_contents()->GetProfile()?  Is that change important for this CL?

Yes. The problem was: I needed the profile in the destructor, to unregister from
the service. But at that time the web_contents was already NULL (whereas the
Profile outlives this class). If there's a good way to access the profile in the
destructor, then I'm happy to use that and not introduce the caching. But I have
not found any.

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:32:
MockLogReceiver() {}
On 2014/05/13 04:32:18, Ilya Sherman wrote:
> nit: Can this line be omitted?

Done.

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:69: }
On 2014/05/13 04:32:18, Ilya Sherman wrote:
> Optional nit: It's fine to inline these implementations in test files, if you
> like.

I actually prefer them separated: the declarations are easier to read for me,
and also I don't have to read test code differently from the prod code.
But I recognise this is subjective, and I don't care too strongly about it.
Since you marked this "optional", I left the code as is. If you want to drop the
"optional", I'm happy to inline it.

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:159:
EXPECT_EQ(std::string(), service_->RegisterReceiver(&receiver_));
On 2014/05/13 04:32:18, Ilya Sherman wrote:
> I'm noticing that all of the tests in this file expect the return value from
> RegisterReceiver to be the empty string.  Should there also be test coverage
> here of the other case?  If not, maybe the EXPECT_EQ's are not needed at all,
> since that doesn't seem to be an integral part of what's being tested in this
> file.
> 
> (In general, there are lots of things that you can assert within a test.  It's
> nice if the only assertions are the ones corresponding specifically to what
the
> test is aimed at testing.  That tends to make the test code easier to follow.)

That's a fair point. Coverage for non-empty strings returned from
RegisterReceiver is in log_router_unittest.cc. I'll remove the EXPECT_EQ from
here to avoid distracting the reader.

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/password_manager_internals/password_manager_internals_ui.cc
(right):

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/password_manager_internals/password_manager_internals_ui.cc:50:
log_buffer_.append(service->RegisterReceiver(this));
On 2014/05/13 04:32:18, Ilya Sherman wrote:
> Hmm, why do you need a log buffer?  Couldn't you just postpone registering as
a
> receiver until DidStopLoading() was called?  (Admittedly, you would then need
to
> keep track of whether or not you'd registered as a receiver, so maybe it's a
> wash in terms of bookkeeping.)

Good idea, done.
I don't think any more bookkeeping is needed -- there already is a flag
|did_stop_loading_|, I can make use of that (and rename it).

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/password_manager_internals/password_manager_internals_ui_browsertest.cc
(right):

https://codereview.chromium.org/269513003/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/password_manager_internals/password_manager_internals_ui_browsertest.cc:94:
// service can make sure both internals pages are synchronised.
On 2014/05/13 04:32:18, Ilya Sherman wrote:
> Would anything go wrong if all the tests were to log via the service, rather
> than the controller?  It sounds like that is a more realistic test scenario
> anyway.

Done.

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

https://codereview.chromium.org/269513003/diff/180001/components/password_man...
components/password_manager/core/browser/log_router.cc:43:
DCHECK(can_use_receivers_);
On 2014/05/13 04:32:18, Ilya Sherman wrote:
> Hmm, this seems like a lot of work just for the sake of a DCHECK.  I'd omit
all
> the extra complexity, and just trust that you've verified this correctly.  I'd
> expect that tools like ASan would catch any errors anyway.  In general,
DCHECKs
> are nice; but if they start requiring a bunch of work to maintain, it's
probably
> not worth the effort unless that work generalizes to many different callsites
> (e.g. as ThreadChecker does).

Done.
I agree. I double-checked that the current code would not trigger the DCHECK. As
long as the comment at PasswordManagerClient::NotifyCanUseLogRouter is honoured,
that should not be a problem.

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

https://codereview.chromium.org/269513003/diff/180001/components/password_man...
components/password_manager/core/browser/log_router.h:64: // Only notify
observes from |receivers_| if this is set to true. It will be
On 2014/05/13 04:32:18, Ilya Sherman wrote:
> nit: "observes" -> "observers"

Gone -> done. :)

https://codereview.chromium.org/269513003/diff/180001/components/password_man...
components/password_manager/core/browser/log_router.h:69: #endif
On 2014/05/13 04:32:18, Ilya Sherman wrote:
> Hmm, why is this code debug-only?

I was overzealous with trying to slim down the release code.
But that's all gone now.

https://codereview.chromium.org/269513003/diff/180001/components/password_man...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/269513003/diff/180001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:76: //
|router_can_be_used| set to false, the client may no more use the
On 2014/05/13 04:32:18, Ilya Sherman wrote:
> nit: "no more" -> "no longer"

Done.

[Ilya Sherman, 2014-05-13 18:40:26]

LGTM.  Thanks, Václav!  Exciting to see all the final pieces starting to come
together :)

[vabr (Chromium), 2014-05-14 07:43:59]

Thanks a lot, Ilya!

In patch set 6 I replaced some forgotten references to NotifyCanUseLogRouter
with OnLogRouterAvailabilityChanged. Since that's just following your original
comment, I'll be sending the whole CL into CQ later today.

Also, many thanks for your lion's share on putting all the pieces for the first
version of the internals page together. Hopefully this will make user debugging
easier. The further versions of this should also involve some Feedback-related
mechanism to allow even easier bug reporting, and we could also extend this to
other fields of password management + autofill as needed. Let's see, how this
works out. :)

Thanks,
Vaclav

[vabr (Chromium), 2014-05-14 07:52:19]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-05-14 07:52:44]

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

[commit-bot: I haz the power, 2014-05-14 09:40:56]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[commit-bot: I haz the power, 2014-05-14 10:04:25]

Change committed as 270372