Password internals page: notify renderer about logging state on client construction

Password internals page: notify renderer about logging state on client construction

Currently, ChromePasswordManagerClient informs the PasswordAutofillAgent almost every time about changed availability of the internals page. The only exception is when the client is created. As a result, new tabs opened after a chrome://password-manager-internals tab will only cause browser-side logs.

When the client is created, the agent on the renderer side may not exist yet. So it does not help to send messages to the agent during constructing the client.

Instead, this CL introduces a new "ping" IPC message from the renderer to the browser, which the agent uses to explicitly request logging state update from the client at the time the agent is constructed.

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

[vabr (Chromium), 2014-06-13 10:28:05]

Hi,
PTAL.
Thanks!
Vaclav

[vabr (Chromium), 2014-06-13 10:42:08]

On 2014/06/13 10:28:05, vabr (Chromium) wrote:
> Hi,
> PTAL.
> Thanks!
> Vaclav

Actually, please hold on, the fix is sadly not complete yet. :(
Sorry for the noise.
I'll ping you when I'm done again.

Cheers,
Vaclav

[vabr (Chromium), 2014-06-13 15:17:50]

Ilya,
Could you please review the whole CL, in particular the components/autofill
bits?
Do you think I should also send the logging state update from the
ChromePasswordManagerClient constructor to PasswordAutofillAgent, to make sure
the case when the agent would be created before the client is covered? Or is
there a guarantee that the agent on the renderer side is always created after
the client on the browser side?

Tom,
Could you please review components/autofill/content/common/autofill_messages.h?

Balazs,
I already need to ask Ilya for OWNERS approval of the autofill bits, and it does
not make sense to look at that without looking at the rest. So your review is
not necessary, but of course I'll be more than happy to hear any comments you
may have.

Thanks to you all.
Vaclav

[Tom Sepez, 2014-06-13 18:56:11]

Messages LGTM.

[Ilya Sherman, 2014-06-14 00:50:39]

LGTM % nits.  Thanks, Václav.

On 2014/06/13 15:17:50, vabr (Chromium) wrote:
> Do you think I should also send the logging state update from the
> ChromePasswordManagerClient constructor to PasswordAutofillAgent, to make sure
> the case when the agent would be created before the client is covered? Or is
> there a guarantee that the agent on the renderer side is always created after
> the client on the browser side?

You should double-check me on this, but I don't think that it's possible to have
a PasswordAutofillAgent instance without having a corresponding
ChromePasswordManagerClient instance live.

https://codereview.chromium.org/336763002/diff/60001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/336763002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:305: void
ChromePasswordManagerClient::NotifyRendererAboutLoggingActivityState() {
Optional nit: "About" -> "Of" for a slightly shorter method name.

https://codereview.chromium.org/336763002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:309: // Also
inform the renderer process to start or stop logging.
nit: Please update this comment.

https://codereview.chromium.org/336763002/diff/60001/components/autofill/cont...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/336763002/diff/60001/components/autofill/cont...
components/autofill/content/common/autofill_messages.h:202: // A ping to the
browser to send AutofillMsg_ChangeLoggingState with the current
nit: Perhaps we should update the IPC message to be named "SetLoggingState"
rather than "ChangeLoggingState", since the state might not actually change as a
result of calling this method?

[engedy, 2014-06-16 09:13:38]

I have added some comments.

Given the yet limited understanding I have of this code, I fear the comments may
very well not be meaningful contributions to this CL, so please feel ignore
them. :-)

https://codereview.chromium.org/336763002/diff/60001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/336763002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.h:114: void
NotifyRendererAboutLoggingActivityState();
nit: I was wondering, isn't this more about "logging availability" rather than
"logging activity"?

https://codereview.chromium.org/336763002/diff/60001/chrome/renderer/autofill...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/336763002/diff/60001/chrome/renderer/autofill...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1332: // Thest
that the agent sends an IPC call to get the current activity state of
nit: s/Thest/Test/.

https://codereview.chromium.org/336763002/diff/60001/components/autofill/cont...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/336763002/diff/60001/components/autofill/cont...
components/autofill/content/common/autofill_messages.h:205:
IPC_MESSAGE_ROUTED0(AutofillHostMsg_UpdateLoggingState);
I think this name is somewhat misleading: it suggests that there might be some
side effects involved when this message is received.

Tentatively, how about "NeedLoggingState" or "RequestLoggingState" (or even drop
mention of the use-case from the name, and just say "AgentConstructed").

[vabr (Chromium), 2014-06-20 10:47:04]

Thanks all!

Comments addressed, sending to the CQ.

Cheers,
Vaclav

https://codereview.chromium.org/336763002/diff/60001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/336763002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:305: void
ChromePasswordManagerClient::NotifyRendererAboutLoggingActivityState() {
On 2014/06/14 00:50:39, Ilya Sherman wrote:
> Optional nit: "About" -> "Of" for a slightly shorter method name.

Done.

https://codereview.chromium.org/336763002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:309: // Also
inform the renderer process to start or stop logging.
On 2014/06/14 00:50:39, Ilya Sherman wrote:
> nit: Please update this comment.

Done. (Removed, since the whole method is just for notifying the renderer.)

https://codereview.chromium.org/336763002/diff/60001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/336763002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.h:114: void
NotifyRendererAboutLoggingActivityState();
On 2014/06/16 09:13:38, engedy wrote:
> nit: I was wondering, isn't this more about "logging availability" rather than
> "logging activity"?

Done.

https://codereview.chromium.org/336763002/diff/60001/chrome/renderer/autofill...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/336763002/diff/60001/chrome/renderer/autofill...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1332: // Thest
that the agent sends an IPC call to get the current activity state of
On 2014/06/16 09:13:38, engedy wrote:
> nit: s/Thest/Test/.

Done.

https://codereview.chromium.org/336763002/diff/60001/components/autofill/cont...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/336763002/diff/60001/components/autofill/cont...
components/autofill/content/common/autofill_messages.h:202: // A ping to the
browser to send AutofillMsg_ChangeLoggingState with the current
On 2014/06/14 00:50:39, Ilya Sherman wrote:
> nit: Perhaps we should update the IPC message to be named "SetLoggingState"
> rather than "ChangeLoggingState", since the state might not actually change as
a
> result of calling this method?

Done.

https://codereview.chromium.org/336763002/diff/60001/components/autofill/cont...
components/autofill/content/common/autofill_messages.h:205:
IPC_MESSAGE_ROUTED0(AutofillHostMsg_UpdateLoggingState);
On 2014/06/16 09:13:38, engedy wrote:
> I think this name is somewhat misleading: it suggests that there might be some
> side effects involved when this message is received.
> 
> Tentatively, how about "NeedLoggingState" or "RequestLoggingState" (or even
drop
> mention of the use-case from the name, and just say "AgentConstructed").

Done.

[vabr (Chromium), 2014-06-20 10:47:11]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-06-20 10:49:50]

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

[engedy, 2014-06-20 10:51:37]

LGTM.

[commit-bot: I haz the power, 2014-06-20 14:38:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-20 15:19:53]

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

[commit-bot: I haz the power, 2014-06-20 15:19:54]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[vabr (Chromium), 2014-06-20 15:27:55]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-06-20 15:30:29]

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

[commit-bot: I haz the power, 2014-06-20 17:19:36]

Change committed as 278738