Eliminate PasswordAutofillAgentConstructed() in mojo interface PasswordManagerDriver

Eliminate PasswordAutofillAgentConstructed() in mojo interface PasswordManagerDriver

PasswordAutofillAgentConstructed() is just for autofill::PasswordAutofillAgent to
get logging state from browser side when PasswordAutofillAgent is created.
It's useful before due to lack of pending mechanism for legacy IPCs,
but now in mojo world we can achieve this goal without it.

This CL lets browser side just send logging state as soon as it's ready,
even at that timing point renderer process has not started or has not created
PasswordAutofillAgent, the mojo call
autofill::mojom::PasswordAutofillAgent::SetLoggingState() will be cached
inside mojo message pipe and be executed immediately once PasswordAutofillAgent
instantiates and binds itself with the pending mojo connection.

This keeps exactly the same behavior with before:
autofill::mojom::PasswordAutofillAgent::SetLoggingState() is always the first
mojo call arriving at autofill::PasswordAutofillAgent from browser side.

BUG=676814
TEST=components_unittests

Review-Url: https://codereview.chromium.org/2680163006
Cr-Commit-Position: refs/heads/master@{#450171}
Committed: https://chromium.googlesource.com/chromium/src/+/2364c1b8adb850f0d1b1e29f16ecfd75679280d5

[leonhsl(Using Gerrit), 2017-02-09 09:41:35]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 09:42:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-02-09 09:45:17]

leon.han@intel.com changed reviewers:
+ kolos@chromium.org, vabr@chromium.org

[leonhsl(Using Gerrit), 2017-02-09 09:45:18]

Hi, vabr@, kolos@, I've confirmed locally that this CL does fix bug 676814,
PTAL, Thanks.

[leonhsl(Using Gerrit), 2017-02-09 10:14:57]

Description was changed from

==========
Eliminate PasswordAutofillAgentConstructed() in mojo interface
PasswordManagerDriver

BUG=676814
TEST=components_unittests
==========

to

==========
Eliminate PasswordAutofillAgentConstructed() in mojo interface
PasswordManagerDriver

PasswordAutofillAgentConstructed() is just for autofill::PasswordAutofillAgent
to
get logging state from browser side when PasswordAutofillAgent is created.
It's useful before due to lack of pending mechanism for legacy IPCs,
but now in mojo world we can achieve this goal without it.

This CL lets browser side just send logging state as soon as it's ready,
even at that timing point renderer process has not started or has not created
PasswordAutofillAgent, the mojo call
autofill::mojom::PasswordAutofillAgent::SetLoggingState() will be cached
inside mojo message pipe and be executed immediately once PasswordAutofillAgent
instantiates and binds itself with the pending mojo connection.

This keeps exactly the same behavior with before:
autofill::mojom::PasswordAutofillAgent::SetLoggingState() is always the first
mojo call arriving at autofill::PasswordAutofillAgent from browser side.

BUG=676814
TEST=components_unittests
==========

[commit-bot: I haz the power, 2017-02-09 10:24:13]

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

[commit-bot: I haz the power, 2017-02-09 10:24:13]

Dry run: This issue passed the CQ dry run.

[kolos1, 2017-02-09 13:51:01]

The password manager works in custom tabs even after "warmup". 

Could you please explain what happened before this CL? The CL removes code
mostly. If the log manager is called before its creation, it sets
"encountered_error_" in mojo internals and it brakes mojo pipeline. Is that
correct?

[kolos1, 2017-02-09 13:57:37]

LGTM 

Great thanks for fixing that!

[leonhsl(Using Gerrit), 2017-02-10 06:24:26]

On 2017/02/09 13:51:01, kolos1 wrote:
> The password manager works in custom tabs even after "warmup". 
> 
> Could you please explain what happened before this CL? The CL removes code
> mostly. If the log manager is called before its creation, it sets
> "encountered_error_" in mojo internals and it brakes mojo pipeline. Is that
> correct?

Before this CL, autofill::PasswordAutofillAgent would try to connect
autofill.mojom.PasswordManagerDriver impl in browser side in order to call the
mojo call PasswordAutofillAgentConstructed() in its ctor, and in browser side
the connect request would be handled by static function
ContentPasswordManagerDriverFactory::BindPasswordManagerDriver(), which would
normally find the corresponding password_manager::ContentPasswordManagerDriver
instance to bind the request.
But, in case of spare WebContents,
ContentPasswordManagerDriverFactory::BindPasswordManagerDriver() would find that
the ContentPasswordManagerDriverFactory itself for the spare WebContents is not
available yet at [1], then it have to drop the connect request silently, which
causes the underlying message pipe be closed, then the renderer side
autofill.mojom.PasswordManagerDriverPtr will be aware of this disconnection and
set its internal |encountered_error_| as true. After that,
autofill.mojom.PasswordManagerDriverPtr keeps invalid and will fail silently for
any mojo calls on it. So this CL removes the root cause
PasswordAutofillAgentConstructed() :) but still keeps exactly identical behavior
with before.

[1]
https://cs.chromium.org/chromium/src/components/password_manager/content/brow...

[vabr (Chromium), 2017-02-12 18:48:13]

Thanks for the fix and for the helpful explanations both in the CL description
and in the review discussion.

LGTM.

Cheers,
Vaclav

https://codereview.chromium.org/2680163006/diff/1/components/password_manager...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/2680163006/diff/1/components/password_manager...
components/password_manager/content/browser/content_password_manager_driver.cc:54:
GetPasswordAutofillAgent()->SetLoggingState(
I acknowledge that calling the SetLoggingState here instead of calling
SendLoggingAvailability is the right thing to do, because the latter is a
virtual function, and those should not be called from a constructor (to avoid
issues with unexpected vtable values).
Optionally you can add a comment to that extent:
// Do not call the virtual method SendLoggingAvailability from a constructor
here, inline its steps instead.

[leonhsl(Using Gerrit), 2017-02-13 00:14:32]

Thanks all! Sending to CQ now.

https://codereview.chromium.org/2680163006/diff/1/components/password_manager...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/2680163006/diff/1/components/password_manager...
components/password_manager/content/browser/content_password_manager_driver.cc:54:
GetPasswordAutofillAgent()->SetLoggingState(
On 2017/02/12 18:48:12, vabr (OOO until late Feb) wrote:
> I acknowledge that calling the SetLoggingState here instead of calling
> SendLoggingAvailability is the right thing to do, because the latter is a
> virtual function, and those should not be called from a constructor (to avoid
> issues with unexpected vtable values).
> Optionally you can add a comment to that extent:
> // Do not call the virtual method SendLoggingAvailability from a constructor
> here, inline its steps instead.

Done.

[leonhsl(Using Gerrit), 2017-02-13 00:14:49]

The CQ bit was checked by leon.han@intel.com

[leonhsl(Using Gerrit), 2017-02-13 00:14:49]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
kolos@chromium.org
Link to the patchset: https://codereview.chromium.org/2680163006/#ps20001
(title: "Address comments from vabr@")

[commit-bot: I haz the power, 2017-02-13 00:15:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 00:20:30]

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

[commit-bot: I haz the power, 2017-02-13 00:20:32]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[leonhsl(Using Gerrit), 2017-02-13 01:53:54]

leon.han@intel.com changed reviewers:
+ tsepez@chromium.org

[leonhsl(Using Gerrit), 2017-02-13 01:53:55]

+Tom for OWNER review of
components/autofill/content/common/autofill_driver.mojom, Thanks.

[Tom Sepez, 2017-02-13 17:55:56]

LGTM on removing method.

[leonhsl(Using Gerrit), 2017-02-14 00:15:02]

The CQ bit was checked by leon.han@intel.com

[commit-bot: I haz the power, 2017-02-14 00:16:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 00:30:34]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487031302719730,
"parent_rev": "123a93ea122581fd74520d7c526cded29ccb81ae", "commit_rev":
"2364c1b8adb850f0d1b1e29f16ecfd75679280d5"}

[commit-bot: I haz the power, 2017-02-14 00:31:21]

Description was changed from

==========
Eliminate PasswordAutofillAgentConstructed() in mojo interface
PasswordManagerDriver

PasswordAutofillAgentConstructed() is just for autofill::PasswordAutofillAgent
to
get logging state from browser side when PasswordAutofillAgent is created.
It's useful before due to lack of pending mechanism for legacy IPCs,
but now in mojo world we can achieve this goal without it.

This CL lets browser side just send logging state as soon as it's ready,
even at that timing point renderer process has not started or has not created
PasswordAutofillAgent, the mojo call
autofill::mojom::PasswordAutofillAgent::SetLoggingState() will be cached
inside mojo message pipe and be executed immediately once PasswordAutofillAgent
instantiates and binds itself with the pending mojo connection.

This keeps exactly the same behavior with before:
autofill::mojom::PasswordAutofillAgent::SetLoggingState() is always the first
mojo call arriving at autofill::PasswordAutofillAgent from browser side.

BUG=676814
TEST=components_unittests
==========

to

==========
Eliminate PasswordAutofillAgentConstructed() in mojo interface
PasswordManagerDriver

PasswordAutofillAgentConstructed() is just for autofill::PasswordAutofillAgent
to
get logging state from browser side when PasswordAutofillAgent is created.
It's useful before due to lack of pending mechanism for legacy IPCs,
but now in mojo world we can achieve this goal without it.

This CL lets browser side just send logging state as soon as it's ready,
even at that timing point renderer process has not started or has not created
PasswordAutofillAgent, the mojo call
autofill::mojom::PasswordAutofillAgent::SetLoggingState() will be cached
inside mojo message pipe and be executed immediately once PasswordAutofillAgent
instantiates and binds itself with the pending mojo connection.

This keeps exactly the same behavior with before:
autofill::mojom::PasswordAutofillAgent::SetLoggingState() is always the first
mojo call arriving at autofill::PasswordAutofillAgent from browser side.

BUG=676814
TEST=components_unittests

Review-Url: https://codereview.chromium.org/2680163006
Cr-Commit-Position: refs/heads/master@{#450171}
Committed:
https://chromium.googlesource.com/chromium/src/+/2364c1b8adb850f0d1b1e29f16ec...
==========

[commit-bot: I haz the power, 2017-02-14 00:31:24]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/2364c1b8adb850f0d1b1e29f16ec...