Fix password manager internals renderer reporting

Fix password manager internals renderer reporting

Background: Logs for chrome://password-manager-internals are collected both from renderer and from browser code. While browser code passes the data directly, renderer code needs to send it via IPC. IPC is also used to signal whether the internals page is open or not (so that logs are not generated if not needed).

Issue: The IPC communication browser->renderer was done via RenderViewHost. Due to the switch from RenderViewHost to RenderFrameHost, this silently broke, the renderer code never got the message "internals page open, start sending logs", and hence renderer logs have been missing on the internals page for some time.

This CL: Fixes the situation by sending IPCs through RenderFrameHost. However, while there has been a single RVH per WebContent, there is a single RFH per frame, of which there is a number per WebContents. In password manager, PasswordManagerClient corresponds to a WebContent, while PasswordManagerDriver corresponds to a frame. This CL thus needs to move the reporting methods from the client to the drivers. It also improves the abstraction of the reporting framework (see below).

New reporting framework: Consists of 3 abstract interfaces: LogRouter, LogReceiver and LogManager.
  LogReceiver: Is an object able to receive a string to log, and display it to the user. This is implemented by the chrome://password-manager-internals WebUI. Potentially many instances (e.g., multiple internals tabs open).
  LogManager: Is an object able to gather logs from the code and tell whether logging is active (receivers ready) or not. After this CL, this is implemented by PasswordManager. The interface did not exist before, but its function used to be fulfilled by PasswordManagerClient. One per tab.
  LogRouter: This coordinates the managers and receivers. Both managers and receivers register with the router. The router keeps track of the "availability of logging" -- whether there are any receivers to receive logs or not, and distributes that information to the managers. It also routes the logs from the managers to the receivers. This is implemented by PasswordManagerInternalsService. One per profile.

Old framework + introduced change:
  * There was no formal LogManager, although that function was fulfilled by PasswordManagerClient. The client also took care of the IPC communication.
  * The "manager-part" was shifted from the client to PasswordManager, while the IPC part was shifted to PasswordManagerDriver instances (of which the client owns a number).
  * The full chain of command when sending IPC messages is now:
    1. LogReceiver instances register with LogRouter.
    2. LogRouter notifies LogManager=PasswordManager.
    3. PasswordManager asks PasswordManagerClient to notify the renderer.
    4. PMC delegates to its ContentPasswordManagerDriverFactory.
    5. The factory notifies each of the drivers.
    6. Each driver sends the IPC.
    The steps 3-6 are new, they used to be a single step "PMC sends the IPC".

FAQ:
Why is LoginManager implemented by PasswordManager and not by PMC, as it used to be?
-> The PMC should be a thin abstraction of embedder-dependent code. There is nothing embedder-dependent about the LoginManager.

Notes: This has not been implemented on iOS yet, so the only client and driver implementations for this are ChromePMC and ContentPMD.

BUG=549167
Committed: https://crrev.com/cb4ed16a9881ec356b26e9ffd250a5e018a7a18a
Cr-Commit-Position: refs/heads/master@{#360313}

[vabr (Chromium), 2015-10-30 17:30:46]

Patchset #1 (id:1) has been deleted

[vabr (Chromium), 2015-11-04 15:58:52]

Patchset #1 (id:20001) has been deleted

[vabr (Chromium), 2015-11-11 12:58:38]

Patchset #1 (id:40001) has been deleted

[vabr (Chromium), 2015-11-11 15:59:49]

Patchset #1 (id:60001) has been deleted

[vabr (Chromium), 2015-11-11 15:59:57]

Patchset #1 (id:80001) has been deleted

[vabr (Chromium), 2015-11-11 20:23:48]

Description was changed from

==========
Fix password manager internals renderer reporting

Switching on logging for the internals page should be signalled through
RenderFrameHost, not RenderViewHost.

This is just a backup for now.
COMMIT=false
WIP:

* Some logging still in place, and browser internals logs blocked -- revert
before landing.
* implementing bullet point 1 from http://crbug.com/549167#c2, introduced
LogManager
* TODO:
  - Add the header for LogManager and make PasswordManager inherit from that
  - All other bullet points in http://crbug.com/549167#c2
  - Tests!

BUG=549167
==========

to

==========
Fix password manager internals renderer reporting

Background: Logs for chrome://password-manager-internals are collected both from
renderer and from browser code. While browser code passes the data directly,
renderer code needs to send it via IPC. IPC is also used to signal whether the
internals page is open or not (so that logs are not generated if not needed).

Issue: The IPC communication browser->renderer was done via RenderViewHost. Due
to the switch from RenderViewHost to RenderFrameHost, this silently broke, the
renderer code never got the message "internals page open, start sending logs",
and hence renderer logs have been missing on the internals page for some time.

This CL: Fixes the situation by sending IPCs through RenderFrameHost. However,
while there has been a single RVH per WebContent, there is a single RFH per
frame, of which there is a number per WebContents. In password manager,
PasswordManagerClient corresponds to a WebContent, while PasswordManagerDriver
corresponds to a frame. This CL thus needs to move the reporting methods from
the client to the drivers. It also improves the abstraction of the reporting
framework.

TODO: Describe the structure.
TODO: Integration test.

BUG=549167
==========

[vabr (Chromium), 2015-11-11 20:32:15]

Description was changed from

==========
Fix password manager internals renderer reporting

Background: Logs for chrome://password-manager-internals are collected both from
renderer and from browser code. While browser code passes the data directly,
renderer code needs to send it via IPC. IPC is also used to signal whether the
internals page is open or not (so that logs are not generated if not needed).

Issue: The IPC communication browser->renderer was done via RenderViewHost. Due
to the switch from RenderViewHost to RenderFrameHost, this silently broke, the
renderer code never got the message "internals page open, start sending logs",
and hence renderer logs have been missing on the internals page for some time.

This CL: Fixes the situation by sending IPCs through RenderFrameHost. However,
while there has been a single RVH per WebContent, there is a single RFH per
frame, of which there is a number per WebContents. In password manager,
PasswordManagerClient corresponds to a WebContent, while PasswordManagerDriver
corresponds to a frame. This CL thus needs to move the reporting methods from
the client to the drivers. It also improves the abstraction of the reporting
framework.

TODO: Describe the structure.
TODO: Integration test.

BUG=549167
==========

to

==========
Fix password manager internals renderer reporting

Background: Logs for chrome://password-manager-internals are collected both from
renderer and from browser code. While browser code passes the data directly,
renderer code needs to send it via IPC. IPC is also used to signal whether the
internals page is open or not (so that logs are not generated if not needed).

Issue: The IPC communication browser->renderer was done via RenderViewHost. Due
to the switch from RenderViewHost to RenderFrameHost, this silently broke, the
renderer code never got the message "internals page open, start sending logs",
and hence renderer logs have been missing on the internals page for some time.

This CL: Fixes the situation by sending IPCs through RenderFrameHost. However,
while there has been a single RVH per WebContent, there is a single RFH per
frame, of which there is a number per WebContents. In password manager,
PasswordManagerClient corresponds to a WebContent, while PasswordManagerDriver
corresponds to a frame. This CL thus needs to move the reporting methods from
the client to the drivers. It also improves the abstraction of the reporting
framework (see below).

New reporting framework: Consists of 3 abstract interfaces: LogRouter,
LogReceiver and LogManager.
  LogReceiver: Is an object able to receive a string to log, and display it to
the user. This is implemented by the chrome://password-manager-internals WebUI.
Potentially many instances (e.g., multiple internals tabs open).
  LogManager: Is an object able to gather logs from the code. After this CL,
this is implemented by PasswordManager. The interface did not exist before, but
its function used to be fulfilled by PasswordManagerClient. One per tab.
  LogRouter: This coordinates the managers and receivers. Both managers and
receivers register with the router. The router keeps track of the "availability
of logging" -- whether there are any receivers to receive logs or not, and
distributes that information to the managers. It also routes the logs from the
managers to the receivers. This is implemented by
PasswordManagerInternalsService. One per profile.

TODO: Integration test.

BUG=549167
==========

[vabr (Chromium), 2015-11-11 20:40:33]

Description was changed from

==========
Fix password manager internals renderer reporting

Background: Logs for chrome://password-manager-internals are collected both from
renderer and from browser code. While browser code passes the data directly,
renderer code needs to send it via IPC. IPC is also used to signal whether the
internals page is open or not (so that logs are not generated if not needed).

Issue: The IPC communication browser->renderer was done via RenderViewHost. Due
to the switch from RenderViewHost to RenderFrameHost, this silently broke, the
renderer code never got the message "internals page open, start sending logs",
and hence renderer logs have been missing on the internals page for some time.

This CL: Fixes the situation by sending IPCs through RenderFrameHost. However,
while there has been a single RVH per WebContent, there is a single RFH per
frame, of which there is a number per WebContents. In password manager,
PasswordManagerClient corresponds to a WebContent, while PasswordManagerDriver
corresponds to a frame. This CL thus needs to move the reporting methods from
the client to the drivers. It also improves the abstraction of the reporting
framework (see below).

New reporting framework: Consists of 3 abstract interfaces: LogRouter,
LogReceiver and LogManager.
  LogReceiver: Is an object able to receive a string to log, and display it to
the user. This is implemented by the chrome://password-manager-internals WebUI.
Potentially many instances (e.g., multiple internals tabs open).
  LogManager: Is an object able to gather logs from the code. After this CL,
this is implemented by PasswordManager. The interface did not exist before, but
its function used to be fulfilled by PasswordManagerClient. One per tab.
  LogRouter: This coordinates the managers and receivers. Both managers and
receivers register with the router. The router keeps track of the "availability
of logging" -- whether there are any receivers to receive logs or not, and
distributes that information to the managers. It also routes the logs from the
managers to the receivers. This is implemented by
PasswordManagerInternalsService. One per profile.

TODO: Integration test.

BUG=549167
==========

to

==========
Fix password manager internals renderer reporting

Background: Logs for chrome://password-manager-internals are collected both from
renderer and from browser code. While browser code passes the data directly,
renderer code needs to send it via IPC. IPC is also used to signal whether the
internals page is open or not (so that logs are not generated if not needed).

Issue: The IPC communication browser->renderer was done via RenderViewHost. Due
to the switch from RenderViewHost to RenderFrameHost, this silently broke, the
renderer code never got the message "internals page open, start sending logs",
and hence renderer logs have been missing on the internals page for some time.

This CL: Fixes the situation by sending IPCs through RenderFrameHost. However,
while there has been a single RVH per WebContent, there is a single RFH per
frame, of which there is a number per WebContents. In password manager,
PasswordManagerClient corresponds to a WebContent, while PasswordManagerDriver
corresponds to a frame. This CL thus needs to move the reporting methods from
the client to the drivers. It also improves the abstraction of the reporting
framework (see below).

New reporting framework: Consists of 3 abstract interfaces: LogRouter,
LogReceiver and LogManager.
  LogReceiver: Is an object able to receive a string to log, and display it to
the user. This is implemented by the chrome://password-manager-internals WebUI.
Potentially many instances (e.g., multiple internals tabs open).
  LogManager: Is an object able to gather logs from the code. After this CL,
this is implemented by PasswordManager. The interface did not exist before, but
its function used to be fulfilled by PasswordManagerClient. One per tab.
  LogRouter: This coordinates the managers and receivers. Both managers and
receivers register with the router. The router keeps track of the "availability
of logging" -- whether there are any receivers to receive logs or not, and
distributes that information to the managers. It also routes the logs from the
managers to the receivers. This is implemented by
PasswordManagerInternalsService. One per profile.

Old framework + introduced change:
  * There was no formal LogManager, although that function was fulfilled by
PasswordManagerClient. The client also took care of the IPC communication.
  * The "manager-part" was shifted from the client to PasswordManager, while the
IPC part was shifted to PasswordManagerDriver instances (of which the client
owns a number).
  * The full chain of command when sending IPC messages is now:
    1. LogReceiver instances register with LogRouter.
    2. LogRouter notifies LogManager=PasswordManager.
    3. PasswordManager asks PasswordManagerClient to notify the renderer.
    4. PMC delegates to its ContentPasswordManagerDriverFactory.
    5. The factory notifies each of the drivers.
    6. Each driver sends the IPC.
    The steps 3-6 are new, they used to be a single step "PMC sends the IPC".

FAQ:
Why is LoginManager implemented by PasswordManager and not by PMC, as it used to
be?
-> The PMC should be a thin abstraction of embedder-dependent code. There is
nothing embedder-dependent about the LoginManager.

Notes: This has not been implemented on iOS yet, so the only client and driver
implementations for this are ChromePMC and ContentPMD.

TODO: Integration test.

BUG=549167
==========

[vabr (Chromium), 2015-11-11 22:23:39]

Description was changed from

==========
Fix password manager internals renderer reporting

Background: Logs for chrome://password-manager-internals are collected both from
renderer and from browser code. While browser code passes the data directly,
renderer code needs to send it via IPC. IPC is also used to signal whether the
internals page is open or not (so that logs are not generated if not needed).

Issue: The IPC communication browser->renderer was done via RenderViewHost. Due
to the switch from RenderViewHost to RenderFrameHost, this silently broke, the
renderer code never got the message "internals page open, start sending logs",
and hence renderer logs have been missing on the internals page for some time.

This CL: Fixes the situation by sending IPCs through RenderFrameHost. However,
while there has been a single RVH per WebContent, there is a single RFH per
frame, of which there is a number per WebContents. In password manager,
PasswordManagerClient corresponds to a WebContent, while PasswordManagerDriver
corresponds to a frame. This CL thus needs to move the reporting methods from
the client to the drivers. It also improves the abstraction of the reporting
framework (see below).

New reporting framework: Consists of 3 abstract interfaces: LogRouter,
LogReceiver and LogManager.
  LogReceiver: Is an object able to receive a string to log, and display it to
the user. This is implemented by the chrome://password-manager-internals WebUI.
Potentially many instances (e.g., multiple internals tabs open).
  LogManager: Is an object able to gather logs from the code. After this CL,
this is implemented by PasswordManager. The interface did not exist before, but
its function used to be fulfilled by PasswordManagerClient. One per tab.
  LogRouter: This coordinates the managers and receivers. Both managers and
receivers register with the router. The router keeps track of the "availability
of logging" -- whether there are any receivers to receive logs or not, and
distributes that information to the managers. It also routes the logs from the
managers to the receivers. This is implemented by
PasswordManagerInternalsService. One per profile.

Old framework + introduced change:
  * There was no formal LogManager, although that function was fulfilled by
PasswordManagerClient. The client also took care of the IPC communication.
  * The "manager-part" was shifted from the client to PasswordManager, while the
IPC part was shifted to PasswordManagerDriver instances (of which the client
owns a number).
  * The full chain of command when sending IPC messages is now:
    1. LogReceiver instances register with LogRouter.
    2. LogRouter notifies LogManager=PasswordManager.
    3. PasswordManager asks PasswordManagerClient to notify the renderer.
    4. PMC delegates to its ContentPasswordManagerDriverFactory.
    5. The factory notifies each of the drivers.
    6. Each driver sends the IPC.
    The steps 3-6 are new, they used to be a single step "PMC sends the IPC".

FAQ:
Why is LoginManager implemented by PasswordManager and not by PMC, as it used to
be?
-> The PMC should be a thin abstraction of embedder-dependent code. There is
nothing embedder-dependent about the LoginManager.

Notes: This has not been implemented on iOS yet, so the only client and driver
implementations for this are ChromePMC and ContentPMD.

TODO: Integration test.

BUG=549167
==========

to

==========
Fix password manager internals renderer reporting

Background: Logs for chrome://password-manager-internals are collected both from
renderer and from browser code. While browser code passes the data directly,
renderer code needs to send it via IPC. IPC is also used to signal whether the
internals page is open or not (so that logs are not generated if not needed).

Issue: The IPC communication browser->renderer was done via RenderViewHost. Due
to the switch from RenderViewHost to RenderFrameHost, this silently broke, the
renderer code never got the message "internals page open, start sending logs",
and hence renderer logs have been missing on the internals page for some time.

This CL: Fixes the situation by sending IPCs through RenderFrameHost. However,
while there has been a single RVH per WebContent, there is a single RFH per
frame, of which there is a number per WebContents. In password manager,
PasswordManagerClient corresponds to a WebContent, while PasswordManagerDriver
corresponds to a frame. This CL thus needs to move the reporting methods from
the client to the drivers. It also improves the abstraction of the reporting
framework (see below).

New reporting framework: Consists of 3 abstract interfaces: LogRouter,
LogReceiver and LogManager.
  LogReceiver: Is an object able to receive a string to log, and display it to
the user. This is implemented by the chrome://password-manager-internals WebUI.
Potentially many instances (e.g., multiple internals tabs open).
  LogManager: Is an object able to gather logs from the code. After this CL,
this is implemented by PasswordManager. The interface did not exist before, but
its function used to be fulfilled by PasswordManagerClient. One per tab.
  LogRouter: This coordinates the managers and receivers. Both managers and
receivers register with the router. The router keeps track of the "availability
of logging" -- whether there are any receivers to receive logs or not, and
distributes that information to the managers. It also routes the logs from the
managers to the receivers. This is implemented by
PasswordManagerInternalsService. One per profile.

Old framework + introduced change:
  * There was no formal LogManager, although that function was fulfilled by
PasswordManagerClient. The client also took care of the IPC communication.
  * The "manager-part" was shifted from the client to PasswordManager, while the
IPC part was shifted to PasswordManagerDriver instances (of which the client
owns a number).
  * The full chain of command when sending IPC messages is now:
    1. LogReceiver instances register with LogRouter.
    2. LogRouter notifies LogManager=PasswordManager.
    3. PasswordManager asks PasswordManagerClient to notify the renderer.
    4. PMC delegates to its ContentPasswordManagerDriverFactory.
    5. The factory notifies each of the drivers.
    6. Each driver sends the IPC.
    The steps 3-6 are new, they used to be a single step "PMC sends the IPC".

FAQ:
Why is LoginManager implemented by PasswordManager and not by PMC, as it used to
be?
-> The PMC should be a thin abstraction of embedder-dependent code. There is
nothing embedder-dependent about the LoginManager.

Notes: This has not been implemented on iOS yet, so the only client and driver
implementations for this are ChromePMC and ContentPMD.

BUG=549167
==========

[vabr (Chromium), 2015-11-11 22:51:32]

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

[vabr (Chromium), 2015-11-11 22:51:33]

Hi Vasilii,

Could you PTAL?

Sorry that it is a big CL, I did shave off a couple of smaller ones of it, but
the rest seems to be hard to separate.

Cheers,
Vaclav

[vasilii, 2015-11-12 16:27:35]

What is the benefit of moving the logic from the client to PasswordManager?
Wouldn't it be better to have the logic in a stand-alone LogManager class
(probably owned by the client)? So those who want to log something would request
that object and don't even need to depend on PasswordManager.

https://codereview.chromium.org/1415533013/diff/180001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (left):

https://codereview.chromium.org/1415533013/diff/180001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:161:
service->UnregisterClient(this);
How is it related to the changes?

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver_factory.cc
(right):

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver_factory.cc:57:
new_driver->NotifyAboutLoggingAvailability(logging_available_);
|logging_available_| is false here by default. Can't it be false by default in
ContentPasswordManagerDriver*?

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver_unittest.cc
(right):

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver_unittest.cc:71:
EXPECT_EQ(logging_activated, logging_activated);
That's a very reliable check :-)

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
File components/password_manager/core/browser/log_manager.h (right):

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/log_manager.h:17: LogManager() {}
Inspired by the C++11 class, I recommend to get rid of the unnecessary
constructor. Please declare the destructor as default.

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/log_manager.h:31:
DISALLOW_COPY_AND_ASSIGN(LogManager);
I'm not sure that it's the interface's business to say that the descendants are
noncopyable.

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

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/log_router.cc:52:
accumulated_logs_.clear();
This string can be really long. I'd recommend to clear it by swapping
std::string().swap(accumulated_logs_);

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

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/log_router.h:34: // and the router only
runs on a single thread.
I don't understand this comment, especially the threading part. It's quite
normal to register/unregister in constructor/destructor respectively.

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/log_router.h:47: std::string
RegisterReceiver(LogReceiver* receiver);
It seems that you care a lot about a receiver cashing the previous log. Then you
can use WARN_UNUSED_RESULT.

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
File components/password_manager/core/browser/log_router_unittest.cc (right):

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/log_router_unittest.cc:24:
MOCK_METHOD1(LogSavePasswordProgress, void(const std::string&));
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/log_router_unittest.cc:30: void
LogSavePasswordProgress(const std::string& text) const override {}
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:317: new
BrowserSavePasswordProgressLogger(client_->GetPasswordManager()));
Why not simply |password_manager_|?

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:178:
can_use_log_router_(log_router_ ? log_router_->RegisterManager(this)
Can't you just add a method to LogRouter to request it's status?

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1323:
EXPECT_CALL(receiver_, LogSavePasswordProgress(kTestText)).Times(0);
LogSavePasswordProgress(_)

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1348:
EXPECT_CALL(receiver_, LogSavePasswordProgress(kTestText)).Times(0);
LogSavePasswordProgress(_)

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1352:
TEST_F(PasswordManagerTest, LogToAReceiver) {
I don't see any difference with  LogSavePasswordProgressAttachReceiver

[vabr (Chromium), 2015-11-12 21:55:44]

Hi Vasilii, PTAL.

Guide:
Diff patch set 8 versus 6 gives you how I addressed your in-code comments.
Diff patch set 11 against 9 gives you how I addressed the one comment you added
outside of the code.
The rest are rebases and broken stuff, sorry about that.

Cheers,
Vaclav

https://codereview.chromium.org/1415533013/diff/180001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (left):

https://codereview.chromium.org/1415533013/diff/180001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:161:
service->UnregisterClient(this);
On 2015/11/12 16:27:34, vasilii wrote:
> How is it related to the changes?

The client no longer registers with the service=LogRouter, so it does not need
to unregister. This moved to ~PasswordManager.

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver_factory.cc
(right):

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver_factory.cc:57:
new_driver->NotifyAboutLoggingAvailability(logging_available_);
On 2015/11/12 16:27:34, vasilii wrote:
> |logging_available_| is false here by default. Can't it be false by default in
> ContentPasswordManagerDriver*?

Fair point. It is off by default in the driver as well as in the renderer code,
so I am dropping the notification here.

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver_unittest.cc
(right):

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver_unittest.cc:71:
EXPECT_EQ(logging_activated, logging_activated);
On 2015/11/12 16:27:34, vasilii wrote:
> That's a very reliable check :-)

:D
Good catch!
Fixed.

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
File components/password_manager/core/browser/log_manager.h (right):

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/log_manager.h:17: LogManager() {}
On 2015/11/12 16:27:34, vasilii wrote:
> Inspired by the C++11 class, I recommend to get rid of the unnecessary
> constructor. Please declare the destructor as default.

Done.

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/log_manager.h:31:
DISALLOW_COPY_AND_ASSIGN(LogManager);
On 2015/11/12 16:27:34, vasilii wrote:
> I'm not sure that it's the interface's business to say that the descendants
are
> noncopyable.

Yeah, I don't see a strong reason why it should be. So I'm dropping it.

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

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/log_router.cc:52:
accumulated_logs_.clear();
On 2015/11/12 16:27:34, vasilii wrote:
> This string can be really long. I'd recommend to clear it by swapping
> std::string().swap(accumulated_logs_);

Done.

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

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/log_router.h:34: // and the router only
runs on a single thread.
On 2015/11/12 16:27:34, vasilii wrote:
> I don't understand this comment, especially the threading part. It's quite
> normal to register/unregister in constructor/destructor respectively.

The dangerous situation would be when the object would register
mid-construction, and the router would attempt to use it from another thread
before the construction finished. If you find the comment more confusing than
its absence, I'm fine with removing it. Please indicate if that's the case.

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/log_router.h:47: std::string
RegisterReceiver(LogReceiver* receiver);
On 2015/11/12 16:27:34, vasilii wrote:
> It seems that you care a lot about a receiver cashing the previous log. Then
you
> can use WARN_UNUSED_RESULT.

Done.

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
File components/password_manager/core/browser/log_router_unittest.cc (right):

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/log_router_unittest.cc:24:
MOCK_METHOD1(LogSavePasswordProgress, void(const std::string&));
On 2015/11/12 16:27:34, vasilii wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/log_router_unittest.cc:30: void
LogSavePasswordProgress(const std::string& text) const override {}
On 2015/11/12 16:27:34, vasilii wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:317: new
BrowserSavePasswordProgressLogger(client_->GetPasswordManager()));
On 2015/11/12 16:27:34, vasilii wrote:
> Why not simply |password_manager_|?

I apparently cannot see two lines up. :)
Fixed.

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:178:
can_use_log_router_(log_router_ ? log_router_->RegisterManager(this)
On 2015/11/12 16:27:34, vasilii wrote:
> Can't you just add a method to LogRouter to request it's status?

I could, but what would be the advantage?

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1323:
EXPECT_CALL(receiver_, LogSavePasswordProgress(kTestText)).Times(0);
On 2015/11/12 16:27:34, vasilii wrote:
> LogSavePasswordProgress(_)

Done.

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1348:
EXPECT_CALL(receiver_, LogSavePasswordProgress(kTestText)).Times(0);
On 2015/11/12 16:27:35, vasilii wrote:
> LogSavePasswordProgress(_)

Done.

https://codereview.chromium.org/1415533013/diff/180001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1352:
TEST_F(PasswordManagerTest, LogToAReceiver) {
On 2015/11/12 16:27:34, vasilii wrote:
> I don't see any difference with  LogSavePasswordProgressAttachReceiver

Good call, removed.

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File components/password_manager/core/browser/log_manager.cc (right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/core/browser/log_manager.cc:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
In response to:
> What is the benefit of moving the logic from the client to
> PasswordManager? Wouldn't it be better to have the logic in a
> stand-alone LogManager class (probably owned by the client)? So
> those who want to log something would request that object and
> don't even need to depend on PasswordManager.

Yes, good idea! I separated the LogManager implementation into a LogManagerImpl
(keeping the interface for easy testing).

[vasilii, 2015-11-13 12:17:31]

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (left):

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:432:
IPC_MESSAGE_HANDLER(AutofillHostMsg_PasswordAutofillAgentConstructed,
What happened to this message?

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.h:96: void
NotifyDriversAboutLoggingAvailability(bool is_available);
Should be private.

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_state_unittest.cc (right):

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_state_unittest.cc:35:
MOCK_CONST_METHOD0(GetLogManager, const password_manager::LogManager*());
Should you move this logic with DummyLogManager to StubPasswordManagerClient? I
see this code much more than once in this CL.

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File components/password_manager/content/browser/DEPS (right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/content/browser/DEPS:2:
"+components/autofill/core/browser",
Where do you need it?

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver.cc:13:
#include "components/password_manager/core/browser/password_manager.h"
Why do you need this guy here?

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver.cc:126:
void ContentPasswordManagerDriver::SendLoggingAvailability() {
Declaration order should match the definition order.

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver.h
(right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver.h:110:
bool logging_available_ = false;
Why can't you just ask the client's LoginManager instead of duplicating these
booleans here and in ContentPasswordManagerDriverFactory?

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File components/password_manager/core/browser/dummy_log_manager.h (right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/core/browser/dummy_log_manager.h:13: 
A comment?

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File components/password_manager/core/browser/log_manager.cc (right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/core/browser/log_manager.cc:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2015/11/12 21:55:44, vabr (Chromium) wrote:
> In response to:
> > What is the benefit of moving the logic from the client to
> > PasswordManager? Wouldn't it be better to have the logic in a
> > stand-alone LogManager class (probably owned by the client)? So
> > those who want to log something would request that object and
> > don't even need to depend on PasswordManager.
> 
> Yes, good idea! I separated the LogManager implementation into a
LogManagerImpl
> (keeping the interface for easy testing).

I think the CL got better after this change :-)

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/core/browser/log_manager.cc:57:
notification_callback_.Run(can_use_log_router_);
Can the callback be NULL?

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/core/browser/log_manager.cc:72: //  static
one space

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File components/password_manager/core/browser/log_manager_unittest.cc (right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/core/browser/log_manager_unittest.cc:50: .Pass();
Not needed anymore

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File components/password_manager/core/browser/log_router.cc (right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/core/browser/log_router.cc:13:
LogRouter::LogRouter() {}
I'd use default. It's up to you.

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/core/browser/password_manager_client.h:171: virtual
LogRouter* GetLogRouter() const;
Where is it used?

[vabr (Chromium), 2015-11-13 18:17:48]

Description was changed from

==========
Fix password manager internals renderer reporting

Background: Logs for chrome://password-manager-internals are collected both from
renderer and from browser code. While browser code passes the data directly,
renderer code needs to send it via IPC. IPC is also used to signal whether the
internals page is open or not (so that logs are not generated if not needed).

Issue: The IPC communication browser->renderer was done via RenderViewHost. Due
to the switch from RenderViewHost to RenderFrameHost, this silently broke, the
renderer code never got the message "internals page open, start sending logs",
and hence renderer logs have been missing on the internals page for some time.

This CL: Fixes the situation by sending IPCs through RenderFrameHost. However,
while there has been a single RVH per WebContent, there is a single RFH per
frame, of which there is a number per WebContents. In password manager,
PasswordManagerClient corresponds to a WebContent, while PasswordManagerDriver
corresponds to a frame. This CL thus needs to move the reporting methods from
the client to the drivers. It also improves the abstraction of the reporting
framework (see below).

New reporting framework: Consists of 3 abstract interfaces: LogRouter,
LogReceiver and LogManager.
  LogReceiver: Is an object able to receive a string to log, and display it to
the user. This is implemented by the chrome://password-manager-internals WebUI.
Potentially many instances (e.g., multiple internals tabs open).
  LogManager: Is an object able to gather logs from the code. After this CL,
this is implemented by PasswordManager. The interface did not exist before, but
its function used to be fulfilled by PasswordManagerClient. One per tab.
  LogRouter: This coordinates the managers and receivers. Both managers and
receivers register with the router. The router keeps track of the "availability
of logging" -- whether there are any receivers to receive logs or not, and
distributes that information to the managers. It also routes the logs from the
managers to the receivers. This is implemented by
PasswordManagerInternalsService. One per profile.

Old framework + introduced change:
  * There was no formal LogManager, although that function was fulfilled by
PasswordManagerClient. The client also took care of the IPC communication.
  * The "manager-part" was shifted from the client to PasswordManager, while the
IPC part was shifted to PasswordManagerDriver instances (of which the client
owns a number).
  * The full chain of command when sending IPC messages is now:
    1. LogReceiver instances register with LogRouter.
    2. LogRouter notifies LogManager=PasswordManager.
    3. PasswordManager asks PasswordManagerClient to notify the renderer.
    4. PMC delegates to its ContentPasswordManagerDriverFactory.
    5. The factory notifies each of the drivers.
    6. Each driver sends the IPC.
    The steps 3-6 are new, they used to be a single step "PMC sends the IPC".

FAQ:
Why is LoginManager implemented by PasswordManager and not by PMC, as it used to
be?
-> The PMC should be a thin abstraction of embedder-dependent code. There is
nothing embedder-dependent about the LoginManager.

Notes: This has not been implemented on iOS yet, so the only client and driver
implementations for this are ChromePMC and ContentPMD.

BUG=549167
==========

to

==========
Fix password manager internals renderer reporting

Background: Logs for chrome://password-manager-internals are collected both from
renderer and from browser code. While browser code passes the data directly,
renderer code needs to send it via IPC. IPC is also used to signal whether the
internals page is open or not (so that logs are not generated if not needed).

Issue: The IPC communication browser->renderer was done via RenderViewHost. Due
to the switch from RenderViewHost to RenderFrameHost, this silently broke, the
renderer code never got the message "internals page open, start sending logs",
and hence renderer logs have been missing on the internals page for some time.

This CL: Fixes the situation by sending IPCs through RenderFrameHost. However,
while there has been a single RVH per WebContent, there is a single RFH per
frame, of which there is a number per WebContents. In password manager,
PasswordManagerClient corresponds to a WebContent, while PasswordManagerDriver
corresponds to a frame. This CL thus needs to move the reporting methods from
the client to the drivers. It also improves the abstraction of the reporting
framework (see below).

New reporting framework: Consists of 3 abstract interfaces: LogRouter,
LogReceiver and LogManager.
  LogReceiver: Is an object able to receive a string to log, and display it to
the user. This is implemented by the chrome://password-manager-internals WebUI.
Potentially many instances (e.g., multiple internals tabs open).
  LogManager: Is an object able to gather logs from the code and tell whether
logging is active (receivers ready) or not. After this CL, this is implemented
by PasswordManager. The interface did not exist before, but its function used to
be fulfilled by PasswordManagerClient. One per tab.
  LogRouter: This coordinates the managers and receivers. Both managers and
receivers register with the router. The router keeps track of the "availability
of logging" -- whether there are any receivers to receive logs or not, and
distributes that information to the managers. It also routes the logs from the
managers to the receivers. This is implemented by
PasswordManagerInternalsService. One per profile.

Old framework + introduced change:
  * There was no formal LogManager, although that function was fulfilled by
PasswordManagerClient. The client also took care of the IPC communication.
  * The "manager-part" was shifted from the client to PasswordManager, while the
IPC part was shifted to PasswordManagerDriver instances (of which the client
owns a number).
  * The full chain of command when sending IPC messages is now:
    1. LogReceiver instances register with LogRouter.
    2. LogRouter notifies LogManager=PasswordManager.
    3. PasswordManager asks PasswordManagerClient to notify the renderer.
    4. PMC delegates to its ContentPasswordManagerDriverFactory.
    5. The factory notifies each of the drivers.
    6. Each driver sends the IPC.
    The steps 3-6 are new, they used to be a single step "PMC sends the IPC".

FAQ:
Why is LoginManager implemented by PasswordManager and not by PMC, as it used to
be?
-> The PMC should be a thin abstraction of embedder-dependent code. There is
nothing embedder-dependent about the LoginManager.

Notes: This has not been implemented on iOS yet, so the only client and driver
implementations for this are ChromePMC and ContentPMD.

BUG=549167
==========

[vabr (Chromium), 2015-11-13 20:48:19]

Thanks, Vasilii!

The saga continues. :)

Sorry for renaming the Dummy to a Stub, I realised too late that I should be
consistent with the other Stub* names.

Cheers,
Vaclav

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (left):

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:432:
IPC_MESSAGE_HANDLER(AutofillHostMsg_PasswordAutofillAgentConstructed,
On 2015/11/13 12:17:31, vasilii wrote:
> What happened to this message?

It is handled by the driver. This is the very core of the fix -- I believe that
because of the switch from RenderVievHost to RenderFrameHost, the client was no
longer ever receiving this message, although it was sent from the renderer.

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.h:96: void
NotifyDriversAboutLoggingAvailability(bool is_available);
On 2015/11/13 12:17:31, vasilii wrote:
> Should be private.

Done.

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_state_unittest.cc (right):

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_state_unittest.cc:35:
MOCK_CONST_METHOD0(GetLogManager, const password_manager::LogManager*());
On 2015/11/13 12:17:31, vasilii wrote:
> Should you move this logic with DummyLogManager to StubPasswordManagerClient?
I
> see this code much more than once in this CL.

I should move it, but not in this CL. The proper fix is tracked in
http://crbug.com/554886, and will likely result in removing the stubs and just
providing the mocks.

Why not in this CL -- if I were to introduce the mock client, I would need to
mock all the virtual methods (I do not want to assume where such a centrally
distributed mock client can be used). But then I would also need to provide a
good way to provide default return values, and also switch all the existing
places which mock the client. This CL is large already, I'd prefer to separate
that work.

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File components/password_manager/content/browser/DEPS (right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/content/browser/DEPS:2:
"+components/autofill/core/browser",
On 2015/11/13 12:17:31, vasilii wrote:
> Where do you need it?

content_password_manager_driver_unittest.cc:#include
"components/autofill/core/browser/test_autofill_client.h"

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver.cc:13:
#include "components/password_manager/core/browser/password_manager.h"
On 2015/11/13 12:17:31, vasilii wrote:
> Why do you need this guy here?

Line 184 (existing code) calls a method of the PasswordManager. I guess it
compiled because of some indirect #includes.

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver.cc:126:
void ContentPasswordManagerDriver::SendLoggingAvailability() {
On 2015/11/13 12:17:31, vasilii wrote:
> Declaration order should match the definition order.

Done.

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver.h
(right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver.h:110:
bool logging_available_ = false;
On 2015/11/13 12:17:31, vasilii wrote:
> Why can't you just ask the client's LoginManager instead of duplicating these
> booleans here and in ContentPasswordManagerDriverFactory?

Fair enough, I realize the API provides two ways to pass the availability state,
and I need to keep IsLoggingActive. To cut redundancy, I'll get rid of passing
the argument and caching it locally.

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File components/password_manager/core/browser/dummy_log_manager.h (right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/core/browser/dummy_log_manager.h:13: 
On 2015/11/13 12:17:31, vasilii wrote:
> A comment?

Done.

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File components/password_manager/core/browser/log_manager.cc (right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/core/browser/log_manager.cc:57:
notification_callback_.Run(can_use_log_router_);
On 2015/11/13 12:17:31, vasilii wrote:
> Can the callback be NULL?

Good question! I think it makes sense to allow it being null, because then there
is still IsLoggingActive for some usecases. I made it work with null callbacks
now.

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/core/browser/log_manager.cc:72: //  static
On 2015/11/13 12:17:31, vasilii wrote:
> one space

Done.

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File components/password_manager/core/browser/log_manager_unittest.cc (right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/core/browser/log_manager_unittest.cc:50: .Pass();
On 2015/11/13 12:17:31, vasilii wrote:
> Not needed anymore

Done.

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File components/password_manager/core/browser/log_router.cc (right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/core/browser/log_router.cc:13:
LogRouter::LogRouter() {}
On 2015/11/13 12:17:31, vasilii wrote:
> I'd use default. It's up to you.

Done.

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/1415533013/diff/300001/components/password_ma...
components/password_manager/core/browser/password_manager_client.h:171: virtual
LogRouter* GetLogRouter() const;
On 2015/11/13 12:17:31, vasilii wrote:
> Where is it used?

Nowehere. Good catch!
Removed completely.

[vasilii, 2015-11-16 14:12:25]

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_state_unittest.cc (right):

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_state_unittest.cc:35:
MOCK_CONST_METHOD0(GetLogManager, const password_manager::LogManager*());
On 2015/11/13 20:48:18, vabr (Chromium) wrote:
> On 2015/11/13 12:17:31, vasilii wrote:
> > Should you move this logic with DummyLogManager to
StubPasswordManagerClient?
> I
> > see this code much more than once in this CL.
> 
> I should move it, but not in this CL. The proper fix is tracked in
> http://crbug.com/554886, and will likely result in removing the stubs and just
> providing the mocks.
> 
> Why not in this CL -- if I were to introduce the mock client, I would need to
> mock all the virtual methods (I do not want to assume where such a centrally
> distributed mock client can be used). But then I would also need to provide a
> good way to provide default return values, and also switch all the existing
> places which mock the client. This CL is large already, I'd prefer to separate
> that work.

I don't buy this argument. If you add GetLogManager to StubPasswordManagerClient
that returns a   new member variable log_manager_, you'll make this CL smaller
and the code neater regardless of the upcoming refactoring.

https://codereview.chromium.org/1415533013/diff/360001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/1415533013/diff/360001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:417:
log_manager_->SetSuspended(web_contents()->GetWebUI() != nullptr);
Why is this a special case? Do people often share a polluted log?

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver_unittest.cc
(right):

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver_unittest.cc:87:
logging_activated);
I think you want to test 2 values separately.

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
File components/password_manager/core/browser/log_manager_unittest.cc (right):

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
components/password_manager/core/browser/log_manager_unittest.cc:101: 
You need to test |suspended|

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
File components/password_manager/core/browser/log_router_unittest.cc (right):

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
components/password_manager/core/browser/log_router_unittest.cc:33: };
I didn't check but I'm sure that the class has some members now. You need
DISALLOW_COPY_AND_ASSIGN to be on the safe side.

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
components/password_manager/core/browser/password_manager_client.h:24: class
LogRouter;
unused

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
File components/password_manager/core/browser/password_manager_driver.h (right):

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
components/password_manager/core/browser/password_manager_driver.h:86: virtual
void SendLoggingAvailability() {}
Do you want to make it pure and update StubPasswordManagerDriver instead?

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:71:
MOCK_CONST_METHOD0(GetLogManager, const LogManager*());
Here is an example of what you save if you update StubPasswordManagerClient.
This file will stay intact.

[vabr (Chromium), 2015-11-16 16:08:48]

Thanks, Vasilii!

Some comments addressed, on the rest I pushed back. :)

PTAL.

Cheers,
Vaclav

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_state_unittest.cc (right):

https://codereview.chromium.org/1415533013/diff/300001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_state_unittest.cc:35:
MOCK_CONST_METHOD0(GetLogManager, const password_manager::LogManager*());
On 2015/11/16 14:12:25, vasilii wrote:
> On 2015/11/13 20:48:18, vabr (Chromium) wrote:
> > On 2015/11/13 12:17:31, vasilii wrote:
> > > Should you move this logic with DummyLogManager to
> StubPasswordManagerClient?
> > I
> > > see this code much more than once in this CL.
> > 
> > I should move it, but not in this CL. The proper fix is tracked in
> > http://crbug.com/554886, and will likely result in removing the stubs and
just
> > providing the mocks.
> > 
> > Why not in this CL -- if I were to introduce the mock client, I would need
to
> > mock all the virtual methods (I do not want to assume where such a centrally
> > distributed mock client can be used). But then I would also need to provide
a
> > good way to provide default return values, and also switch all the existing
> > places which mock the client. This CL is large already, I'd prefer to
separate
> > that work.
> 
> I don't buy this argument. If you add GetLogManager to
StubPasswordManagerClient
> that returns a   new member variable log_manager_, you'll make this CL smaller
> and the code neater regardless of the upcoming refactoring.

Yes. I was going to argue that StubPasswordManagerClient should not return
anything but nulls, but I see I already added the stub filter object to it in
the past :), so let's add the StubLogManager. I still need to keep the
StubLogManager separate, for tests where I actually do want to derive a mock
from it, like in
components/password_manager/content/browser/content_password_manager_driver_unittest.cc.

https://codereview.chromium.org/1415533013/diff/360001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/1415533013/diff/360001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.cc:417:
log_manager_->SetSuspended(web_contents()->GetWebUI() != nullptr);
On 2015/11/16 14:12:25, vasilii wrote:
> Why is this a special case? Do people often share a polluted log?

Well, because this protection has been there since beginning, people do not
share polluted logs. :)

The logs can already be quite verbose, especially if the users have more than 2
tabs open, so the design has been to limit the inputs if possible. WebUI will
never be interesting, as long as PasswordManager does not work on it.

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver_unittest.cc
(right):

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver_unittest.cc:87:
logging_activated);
On 2015/11/16 14:12:25, vasilii wrote:
> I think you want to test 2 values separately. 

I'm not sure. I want to test that either no message was sent (so the default
state -- off -- is kept) or that the message sent contained the "off" state. The
test should not discriminate the two, the distinction is an implementation
detail. The important thing is the result (logging remains off).

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
File components/password_manager/core/browser/log_manager_unittest.cc (right):

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
components/password_manager/core/browser/log_manager_unittest.cc:101: 
On 2015/11/16 14:12:25, vasilii wrote:
> You need to test |suspended|

Done.

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
File components/password_manager/core/browser/log_router_unittest.cc (right):

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
components/password_manager/core/browser/log_router_unittest.cc:33: };
On 2015/11/16 14:12:25, vasilii wrote:
> I didn't check but I'm sure that the class has some members now. You need
> DISALLOW_COPY_AND_ASSIGN to be on the safe side.

I would argue that if those members come from gmock, then gmock probably took
care of not introducing copying if the members are not copyable.

But I don't object to DISALLOW_COPY_AND_ASSIGN here in any case, so done.

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
components/password_manager/core/browser/password_manager_client.h:24: class
LogRouter;
On 2015/11/16 14:12:25, vasilii wrote:
> unused

Done.

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
File components/password_manager/core/browser/password_manager_driver.h (right):

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
components/password_manager/core/browser/password_manager_driver.h:86: virtual
void SendLoggingAvailability() {}
On 2015/11/16 14:12:25, vasilii wrote:
> Do you want to make it pure and update StubPasswordManagerDriver instead?

I don't, until we implement this on iOS.

While it would also be possible to have it pure here and update the iOS
implementation, it has been deemed impractical to require iOS downstream changes
for features not present there.

Therefore I'd like to keep it as is.

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:71:
MOCK_CONST_METHOD0(GetLogManager, const LogManager*());
On 2015/11/16 14:12:25, vasilii wrote:
> Here is an example of what you save if you update StubPasswordManagerClient.
> This file will stay intact.

Acknowledged.

[vasilii, 2015-11-17 11:12:05]

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver_unittest.cc
(right):

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver_unittest.cc:87:
logging_activated);
On 2015/11/16 16:08:48, vabr (Chromium) wrote:
> On 2015/11/16 14:12:25, vasilii wrote:
> > I think you want to test 2 values separately. 
> 
> I'm not sure. I want to test that either no message was sent (so the default
> state -- off -- is kept) or that the message sent contained the "off" state.
The
> test should not discriminate the two, the distinction is an implementation
> detail. The important thing is the result (logging remains off).

Your implementation sends the message always. What do you mean by the default
state? The driver doesn't have any state and it has no method to request it.
Therefore, how can you assume that it's off?

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
File components/password_manager/core/browser/log_router_unittest.cc (right):

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
components/password_manager/core/browser/log_router_unittest.cc:33: };
On 2015/11/16 16:08:48, vabr (Chromium) wrote:
> On 2015/11/16 14:12:25, vasilii wrote:
> > I didn't check but I'm sure that the class has some members now. You need
> > DISALLOW_COPY_AND_ASSIGN to be on the safe side.
> 
> I would argue that if those members come from gmock, then gmock probably took
> care of not introducing copying if the members are not copyable.
> 
> But I don't object to DISALLOW_COPY_AND_ASSIGN here in any case, so done.

Acknowledged.

https://codereview.chromium.org/1415533013/diff/400001/components/password_ma...
File components/password_manager/core/browser/log_manager.cc (right):

https://codereview.chromium.org/1415533013/diff/400001/components/password_ma...
components/password_manager/core/browser/log_manager.cc:59:
notification_callback_.Run();
By the way, this callback is called when the manger is notified by the router.
Unfortunately, the logging can be suspended at the moment and IsLoggingActive()
returns false for the driver. When the logging turns |suspended| off, the
callback isn't called. The driver will keep thinking that it shouldn't send
logs..

https://codereview.chromium.org/1415533013/diff/400001/components/password_ma...
File components/password_manager/core/browser/stub_password_manager_client.cc
(right):

https://codereview.chromium.org/1415533013/diff/400001/components/password_ma...
components/password_manager/core/browser/stub_password_manager_client.cc:29: //
forbids us to have a static object, so let's hush the compiler.
Or you drop 'const'.

[vabr (Chromium), 2015-11-17 17:23:47]

All addressed, PTAL!

Thanks,
Vaclav

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver_unittest.cc
(right):

https://codereview.chromium.org/1415533013/diff/360001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver_unittest.cc:87:
logging_activated);
On 2015/11/17 11:12:05, vasilii wrote:
> On 2015/11/16 16:08:48, vabr (Chromium) wrote:
> > On 2015/11/16 14:12:25, vasilii wrote:
> > > I think you want to test 2 values separately. 
> > 
> > I'm not sure. I want to test that either no message was sent (so the default
> > state -- off -- is kept) or that the message sent contained the "off" state.
> The
> > test should not discriminate the two, the distinction is an implementation
> > detail. The important thing is the result (logging remains off).
> 
> Your implementation sends the message always. What do you mean by the default
> state? The driver doesn't have any state and it has no method to request it.
> Therefore, how can you assume that it's off?

I stand corrected, you are right. The default off state is in renderer, but
that's not tested here. The driver is indeed stateless now. Fixed, thanks for
your patience!

https://codereview.chromium.org/1415533013/diff/400001/components/password_ma...
File components/password_manager/core/browser/log_manager.cc (right):

https://codereview.chromium.org/1415533013/diff/400001/components/password_ma...
components/password_manager/core/browser/log_manager.cc:59:
notification_callback_.Run();
On 2015/11/17 11:12:05, vasilii wrote:
> By the way, this callback is called when the manger is notified by the router.
> Unfortunately, the logging can be suspended at the moment and
IsLoggingActive()
> returns false for the driver. When the logging turns |suspended| off, the
> callback isn't called. The driver will keep thinking that it shouldn't send
> logs..

Good catch!
Fixed & added tests for this as well.

https://codereview.chromium.org/1415533013/diff/400001/components/password_ma...
File components/password_manager/core/browser/stub_password_manager_client.cc
(right):

https://codereview.chromium.org/1415533013/diff/400001/components/password_ma...
components/password_manager/core/browser/stub_password_manager_client.cc:29: //
forbids us to have a static object, so let's hush the compiler.
On 2015/11/17 11:12:05, vasilii wrote:
> Or you drop 'const'.

TL;DR: Indeed, done.

(I did not want to drop const, because I meant it to be const, and the only
accessor for it returns a pointer to const. But then, this is just a bloody test
code, and 5-line paragraph saying just "ignore a word on the next line" is more
time wasted than can be spared by any possibly more optimised code thanks to
const.)

[vasilii, 2015-11-17 19:05:24]

lgtm

[vabr (Chromium), 2015-11-18 08:11:03]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2015-11-18 08:11:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415533013/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415533013/440001

[commit-bot: I haz the power, 2015-11-18 09:49:35]

Committed patchset #18 (id:440001)

[commit-bot: I haz the power, 2015-11-18 09:50:35]

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