[Autofill] Migrate ContentPasswordManagerDriver<-->Password{Autofill,Generation}Agent IPCs to mojo.

[Autofill] Migrate ContentPasswordManagerDriver<-->Password{Autofill,Generation}Agent IPCs to mojo.

This CL
-Migrates all ContentPasswordManagerDriver<-->Password{Autofill,Generation}Agent
 IPC messages into mojo interfaces.
-Revises related unit tests and browser tests.

TEST=components_unittests, browser_tests
BUG=582391
Committed: https://crrev.com/38eab5aeedba6745c0d9f7d7c897c16982299d88
Cr-Commit-Position: refs/heads/master@{#413708}

[leonhsl(Using Gerrit), 2016-08-08 08:11:08]

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

[commit-bot: I haz the power, 2016-08-08 08:11:32]

Dry run: 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, 2016-08-08 08:21:36]

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

[commit-bot: I haz the power, 2016-08-08 08:21:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2016-08-08 09:11:09]

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

[commit-bot: I haz the power, 2016-08-08 09:11:16]

Dry run: 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, 2016-08-08 09:44:26]

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

[commit-bot: I haz the power, 2016-08-08 09:44:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[leonhsl(Using Gerrit), 2016-08-08 10:05:53]

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

[commit-bot: I haz the power, 2016-08-08 10:06:02]

Dry run: 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, 2016-08-08 10:33:35]

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

[commit-bot: I haz the power, 2016-08-08 10:33:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[leonhsl(Using Gerrit), 2016-08-09 05:00:33]

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

[commit-bot: I haz the power, 2016-08-09 05:01:06]

Dry run: 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, 2016-08-09 05:36:22]

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

[commit-bot: I haz the power, 2016-08-09 05:36:23]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[leonhsl(Using Gerrit), 2016-08-09 07:23:19]

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

[commit-bot: I haz the power, 2016-08-09 07:23:44]

Dry run: 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, 2016-08-09 08:32:19]

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

[commit-bot: I haz the power, 2016-08-09 08:32:20]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-08-09 09:03:14]

leon.han@intel.com changed reviewers:
+ mathp@chromium.org, rockot@chromium.org, sky@chromium.org,
tsepez@chromium.org, vabr@chromium.org, yzshen@chromium.org

[leonhsl(Using Gerrit), 2016-08-09 09:03:15]

Would you PTAL at this? Thanks.

Ken, Yuzhu      --> For everything.
Vaclav, Mathieu --> For {autofill,password_manager} OWNER review.
Tom             --> For security review.
Sky             --> For OWNER review of
  chrome/browser/chrome_content_browser_client.cc
  chrome/test/base/chrome_render_view_test.h
  chrome/test/base/chrome_render_view_test.cc

After this CL, all autofill IPCs will be converted into Mojo interfaces, except
the 4 IPCs sent from PasswordGenerationAgent to ChromePasswordManagerClient, you
can find them still left in autofill_messages.h. As for these 4 IPCs, we need to
create another new mojo interface and impl it in chrome/, I'd like to do this
work with a follow-up CL.

https://codereview.chromium.org/2216463002/diff/40001/chrome/test/base/chrome...
File chrome/test/base/chrome_render_view_test.cc (right):

https://codereview.chromium.org/2216463002/diff/40001/chrome/test/base/chrome...
chrome/test/base/chrome_render_view_test.cc:114:
RegisterMainFrameRemoteInterfaces();
Because bellowing password_autofill_agent_ constructor will try to connect to
autofill::mojom::PasswordManagerDriver interface, we must register a local
interface impl before that. So I added this virtual member function. 
One use case is at Line472 in
https://codereview.chromium.org/2216463002/diff/40001/chrome/renderer/autofil...

[Tom Sepez, 2016-08-09 16:51:23]

messages/mojom LGTM

[vabr (Chromium), 2016-08-09 17:36:18]

LGTM with nits.
This is amazing! So near to removing autofill_messages.h! :)

Thanks,
Vaclav

https://codereview.chromium.org/2216463002/diff/100001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2216463002/diff/100001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:661:
ASSERT_TRUE(false);
Let's rephrase this a bit to improve readability. ASSERT_TRUE(false) does not
generate a helpful error message, so instead of writing

...
} else if (condition) {
...
} else {
ASSERT_TRUE(false);
}

let's write

...
} else {
ASSERT_TRUE(condition);
...
}

That way, if the ASSERT fails, the failing condition will be printed out.

https://codereview.chromium.org/2216463002/diff/100001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:996:
EXPECT_TRUE(fake_driver_.password_forms_rendered()->size());
nit: For a container "x", instead of
EXPECT_TRUE(x.size());
please use
EXPECT_FALSE(x.empty());
There is little difference, but the latter is more readable and potentially more
efficient.

This also applies below and to the cases where the truth value is inverted
(EXPECT_FALSE(x.size()) -> EXPECT_TRUE(x.empty())).

https://codereview.chromium.org/2216463002/diff/100001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_generation_agent_browsertest.cc (right):

https://codereview.chromium.org/2216463002/diff/100001/chrome/renderer/autofi...
chrome/renderer/autofill/password_generation_agent_browsertest.cc:39: class
FakeContentPasswordManagerDriver
Would it make sense to separate the definition of the fake class into its own
header, and #include it in the test instead of repeating the code?

https://codereview.chromium.org/2216463002/diff/100001/components/autofill/co...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/2216463002/diff/100001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:14: #include
"components/autofill/core/common/form_data_predictions.h"
Looks like most of the structures #included here and described below are no
longer needed. The only exception seems to be PasswordForm, FormData (included
in PF) and FormFieldData (included in FD) + their enums. Could you slim down the
#includes and the IPC traits definitions below to the necessary minimum?

https://codereview.chromium.org/2216463002/diff/100001/components/autofill/co...
File
components/autofill/content/renderer/renderer_save_password_progress_logger.h
(right):

https://codereview.chromium.org/2216463002/diff/100001/components/autofill/co...
components/autofill/content/renderer/renderer_save_password_progress_logger.h:38:
// Used by SendLog to send the IPC message with logs.
nit: To avoid confusion, I would drop "IPC message with" from the comment, and
add the destination instead:
// Used by SendLog to send the logs to the browser.

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

https://codereview.chromium.org/2216463002/diff/100001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver_unittest.cc:160:
WasLoggingActivationMessageSent(NULL);
nit: nullptr instead of NULL.

[leonhsl(Using Gerrit), 2016-08-10 09:18:33]

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

[commit-bot: I haz the power, 2016-08-10 09:19:03]

Dry run: 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, 2016-08-10 10:24:42]

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

[commit-bot: I haz the power, 2016-08-10 10:24:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2016-08-10 11:13:27]

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

[commit-bot: I haz the power, 2016-08-10 11:13:56]

Dry run: 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, 2016-08-10 12:15:56]

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

[commit-bot: I haz the power, 2016-08-10 12:15:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2016-08-10 13:22:02]

Thanks Vaclav a lot for review, uploaded ps#7 to address comments, PTAnL,
Thanks.

And about the trybot failure(at ps#7) about the browser test
passwordManagerBrowserTestBase.InFrameNavigationDoesNotClearPopupState, I'm
still checking around the root cause,, would you please also help to take a look
there when you have time? Thanks ;-)

https://codereview.chromium.org/2216463002/diff/100001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2216463002/diff/100001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:661:
ASSERT_TRUE(false);
On 2016/08/09 17:36:17, vabr (Chromium) wrote:
> Let's rephrase this a bit to improve readability. ASSERT_TRUE(false) does not
> generate a helpful error message, so instead of writing
> 
> ...
> } else if (condition) {
> ...
> } else {
> ASSERT_TRUE(false);
> }
> 
> let's write
> 
> ...
> } else {
> ASSERT_TRUE(condition);
> ...
> }
> 
> That way, if the ASSERT fails, the failing condition will be printed out.

Done.

https://codereview.chromium.org/2216463002/diff/100001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:996:
EXPECT_TRUE(fake_driver_.password_forms_rendered()->size());
On 2016/08/09 17:36:17, vabr (Chromium) wrote:
> nit: For a container "x", instead of
> EXPECT_TRUE(x.size());
> please use
> EXPECT_FALSE(x.empty());
> There is little difference, but the latter is more readable and potentially
more
> efficient.
> 
> This also applies below and to the cases where the truth value is inverted
> (EXPECT_FALSE(x.size()) -> EXPECT_TRUE(x.empty())).

Done.

https://codereview.chromium.org/2216463002/diff/100001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_generation_agent_browsertest.cc (right):

https://codereview.chromium.org/2216463002/diff/100001/chrome/renderer/autofi...
chrome/renderer/autofill/password_generation_agent_browsertest.cc:39: class
FakeContentPasswordManagerDriver
On 2016/08/09 17:36:17, vabr (Chromium) wrote:
> Would it make sense to separate the definition of the fake class into its own
> header, and #include it in the test instead of repeating the code?

Yeah agree. Put FakeContentPasswordManagerDriver into new files
fake_content_password_manager_driver.h and
fake_content_password_manager_driver.cc.
Done and thanks.

https://codereview.chromium.org/2216463002/diff/100001/components/autofill/co...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/2216463002/diff/100001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:14: #include
"components/autofill/core/common/form_data_predictions.h"
On 2016/08/09 17:36:17, vabr (Chromium) wrote:
> Looks like most of the structures #included here and described below are no
> longer needed. The only exception seems to be PasswordForm, FormData (included
> in PF) and FormFieldData (included in FD) + their enums. Could you slim down
the
> #includes and the IPC traits definitions below to the necessary minimum?

Yeah, Agree. Slim down this file and also cleaned up common/BUILD.gn and
common/DEPS. Done.

https://codereview.chromium.org/2216463002/diff/100001/components/autofill/co...
File
components/autofill/content/renderer/renderer_save_password_progress_logger.h
(right):

https://codereview.chromium.org/2216463002/diff/100001/components/autofill/co...
components/autofill/content/renderer/renderer_save_password_progress_logger.h:38:
// Used by SendLog to send the IPC message with logs.
On 2016/08/09 17:36:17, vabr (Chromium) wrote:
> nit: To avoid confusion, I would drop "IPC message with" from the comment, and
> add the destination instead:
> // Used by SendLog to send the logs to the browser.

Done and thanks.

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

https://codereview.chromium.org/2216463002/diff/100001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver_unittest.cc:160:
WasLoggingActivationMessageSent(NULL);
On 2016/08/09 17:36:18, vabr (Chromium) wrote:
> nit: nullptr instead of NULL.

Done.

[vabr (Chromium), 2016-08-10 14:00:52]

Thank you!

Still a handful of comments, but also still LGTM.

Cheers,
Vaclav

https://codereview.chromium.org/2216463002/diff/120001/chrome/renderer/autofi...
File chrome/renderer/autofill/fake_content_password_manager_driver.h (right):

https://codereview.chromium.org/2216463002/diff/120001/chrome/renderer/autofi...
chrome/renderer/autofill/fake_content_password_manager_driver.h:1: // Copyright
(c) 2016 The Chromium Authors. All rights reserved.
nit: No "(c)" in the copyright message, just the year [1].

[1]
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2216463002/diff/120001/chrome/renderer/autofi...
chrome/renderer/autofill/fake_content_password_manager_driver.h:65: const
base::Optional<std::vector<autofill::PasswordForm>>&
Should you forward-declare PasswordForm?

namespace autofill {
struct PasswordForm;
}  // namespace autofill

https://codereview.chromium.org/2216463002/diff/120001/chrome/renderer/autofi...
chrome/renderer/autofill/fake_content_password_manager_driver.h:138: void
RecordSavePasswordProgress(const std::string& log) override;
Please also #include <string>.

https://codereview.chromium.org/2216463002/diff/120001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2216463002/diff/120001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:515:
ASSERT_TRUE(expected_type == PasswordFormSubmitted ||
This is not what I meant. While it improves the information logged by the
ASSERT, it feels strange to repeat all the conditions, and there is a risk of
the ASSERT going out of sync with the structure of the if-else block. My
suggestion was:

if (expected_type == PasswordFormSubmitted) {
  ...
} else {
  ASSERT_EQ(PasswordFormInPageNavigation, expected_type);
  ASSERT_TRUE(fake_driver_.called_inpage_navigation());
  ASSERT_TRUE(
      static_cast<bool>(fake_driver_.password_form_inpage_navigation()));
  form = *(fake_driver_.password_form_inpage_navigation());
}

Note that the last "else" disappeared completely, and the last "else if" changed
into just "else", with the ASSERT_EQ verifying what used to be the "else
if"-condition. The test will abort at failing ASSERT*, so the rest of the code
in that else branch can safely assume that expected_type ==
PasswordFormInPageNavigation.

(Also note the use of ASSERT_EQ instead of ASSERT_TRUE(...==...), for an even
more informative log message.)

[yzshen1, 2016-08-10 16:44:59]

On 2016/08/09 09:03:15, leonhsl wrote:
> Would you PTAL at this? Thanks.
> 
> Ken, Yuzhu      --> For everything.
> Vaclav, Mathieu --> For {autofill,password_manager} OWNER review.
> Tom             --> For security review.
> Sky             --> For OWNER review of
>   chrome/browser/chrome_content_browser_client.cc
>   chrome/test/base/chrome_render_view_test.h
>   chrome/test/base/chrome_render_view_test.cc
> 
> After this CL, all autofill IPCs will be converted into Mojo interfaces,
except
> the 4 IPCs sent from PasswordGenerationAgent to ChromePasswordManagerClient,
you
> can find them still left in autofill_messages.h. As for these 4 IPCs, we need
to
> create another new mojo interface and impl it in chrome/, I'd like to do this
> work with a follow-up CL.
> 
>
https://codereview.chromium.org/2216463002/diff/40001/chrome/test/base/chrome...
> File chrome/test/base/chrome_render_view_test.cc (right):
> 
>
https://codereview.chromium.org/2216463002/diff/40001/chrome/test/base/chrome...
> chrome/test/base/chrome_render_view_test.cc:114:
> RegisterMainFrameRemoteInterfaces();
> Because bellowing password_autofill_agent_ constructor will try to connect to
> autofill::mojom::PasswordManagerDriver interface, we must register a local
> interface impl before that. So I added this virtual member function. 
> One use case is at Line472 in
>
https://codereview.chromium.org/2216463002/diff/40001/chrome/renderer/autofil...

Is there any specific issue (or a subset of files) that you would like me to
look at?
I am not very familiar with this domain. And I can easily get lost while looking
at a big CL like this.

[leonhsl(Using Gerrit), 2016-08-11 03:15:46]

On 2016/08/10 16:44:59, yzshen1 wrote:
> On 2016/08/09 09:03:15, leonhsl wrote:
> > Would you PTAL at this? Thanks.
> > 
> > Ken, Yuzhu      --> For everything.
> > Vaclav, Mathieu --> For {autofill,password_manager} OWNER review.
> > Tom             --> For security review.
> > Sky             --> For OWNER review of
> >   chrome/browser/chrome_content_browser_client.cc
> >   chrome/test/base/chrome_render_view_test.h
> >   chrome/test/base/chrome_render_view_test.cc
> > 
> > After this CL, all autofill IPCs will be converted into Mojo interfaces,
> except
> > the 4 IPCs sent from PasswordGenerationAgent to ChromePasswordManagerClient,
> you
> > can find them still left in autofill_messages.h. As for these 4 IPCs, we
need
> to
> > create another new mojo interface and impl it in chrome/, I'd like to do
this
> > work with a follow-up CL.
> > 
> >
>
https://codereview.chromium.org/2216463002/diff/40001/chrome/test/base/chrome...
> > File chrome/test/base/chrome_render_view_test.cc (right):
> > 
> >
>
https://codereview.chromium.org/2216463002/diff/40001/chrome/test/base/chrome...
> > chrome/test/base/chrome_render_view_test.cc:114:
> > RegisterMainFrameRemoteInterfaces();
> > Because bellowing password_autofill_agent_ constructor will try to connect
to
> > autofill::mojom::PasswordManagerDriver interface, we must register a local
> > interface impl before that. So I added this virtual member function. 
> > One use case is at Line472 in
> >
>
https://codereview.chromium.org/2216463002/diff/40001/chrome/renderer/autofil...
> 
> Is there any specific issue (or a subset of files) that you would like me to
> look at?
> I am not very familiar with this domain. And I can easily get lost while
looking
> at a big CL like this.

Yeah this CL is big as it contains many test codes... En, firstly I'd like you
to help review the changes in bellowing files and the attached issue ;-) Thanks.

  chrome/browser/chrome_content_browser_client.cc
  chrome/test/base/chrome_render_view_test.h
  chrome/test/base/chrome_render_view_test.cc

[leonhsl(Using Gerrit), 2016-08-11 03:48:48]

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

[commit-bot: I haz the power, 2016-08-11 03:49:06]

Dry run: 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, 2016-08-11 04:33:55]

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

[commit-bot: I haz the power, 2016-08-11 04:33:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2016-08-11 05:50:12]

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

[commit-bot: I haz the power, 2016-08-11 05:50:26]

Dry run: 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, 2016-08-11 06:25:08]

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

[commit-bot: I haz the power, 2016-08-11 06:25:09]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-08-11 06:29:56]

Uploaded ps#8 to address comments from Vaclav, PTAnL, Thanks~

https://codereview.chromium.org/2216463002/diff/120001/chrome/renderer/autofi...
File chrome/renderer/autofill/fake_content_password_manager_driver.h (right):

https://codereview.chromium.org/2216463002/diff/120001/chrome/renderer/autofi...
chrome/renderer/autofill/fake_content_password_manager_driver.h:1: // Copyright
(c) 2016 The Chromium Authors. All rights reserved.
On 2016/08/10 14:00:51, vabr (Chromium) wrote:
> nit: No "(c)" in the copyright message, just the year [1].
> 
> [1]
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Oh,, I have no idea where I copied this from.. ;-) Done.

https://codereview.chromium.org/2216463002/diff/120001/chrome/renderer/autofi...
chrome/renderer/autofill/fake_content_password_manager_driver.h:65: const
base::Optional<std::vector<autofill::PasswordForm>>&
On 2016/08/10 14:00:51, vabr (Chromium) wrote:
> Should you forward-declare PasswordForm?
> 
> namespace autofill {
> struct PasswordForm;
> }  // namespace autofill

As we also uses PasswordForm to define a member variable at line 153, so I
included its header file components/autofill/core/common/password_form.h. Done.

https://codereview.chromium.org/2216463002/diff/120001/chrome/renderer/autofi...
chrome/renderer/autofill/fake_content_password_manager_driver.h:138: void
RecordSavePasswordProgress(const std::string& log) override;
On 2016/08/10 14:00:52, vabr (Chromium) wrote:
> Please also #include <string>.

Done.

https://codereview.chromium.org/2216463002/diff/120001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2216463002/diff/120001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:515:
ASSERT_TRUE(expected_type == PasswordFormSubmitted ||
On 2016/08/10 14:00:52, vabr (Chromium) wrote:
> This is not what I meant. While it improves the information logged by the
> ASSERT, it feels strange to repeat all the conditions, and there is a risk of
> the ASSERT going out of sync with the structure of the if-else block. My
> suggestion was:
> 
> if (expected_type == PasswordFormSubmitted) {
>   ...
> } else {
>   ASSERT_EQ(PasswordFormInPageNavigation, expected_type);
>   ASSERT_TRUE(fake_driver_.called_inpage_navigation());
>   ASSERT_TRUE(
>       static_cast<bool>(fake_driver_.password_form_inpage_navigation()));
>   form = *(fake_driver_.password_form_inpage_navigation());
> }
> 
> Note that the last "else" disappeared completely, and the last "else if"
changed
> into just "else", with the ASSERT_EQ verifying what used to be the "else
> if"-condition. The test will abort at failing ASSERT*, so the rest of the code
> in that else branch can safely assume that expected_type ==
> PasswordFormInPageNavigation.
> 
> (Also note the use of ASSERT_EQ instead of ASSERT_TRUE(...==...), for an even
> more informative log message.)

Oh I see. Thanks a lot for detailed explanation ;-) Done.

[vabr (Chromium), 2016-08-11 07:29:18]

LGTM, thank you! :)
Vaclav

https://codereview.chromium.org/2216463002/diff/120001/chrome/renderer/autofi...
File chrome/renderer/autofill/fake_content_password_manager_driver.h (right):

https://codereview.chromium.org/2216463002/diff/120001/chrome/renderer/autofi...
chrome/renderer/autofill/fake_content_password_manager_driver.h:65: const
base::Optional<std::vector<autofill::PasswordForm>>&
On 2016/08/11 06:29:55, leonhsl wrote:
> On 2016/08/10 14:00:51, vabr (Chromium) wrote:
> > Should you forward-declare PasswordForm?
> > 
> > namespace autofill {
> > struct PasswordForm;
> > }  // namespace autofill
> 
> As we also uses PasswordForm to define a member variable at line 153, so I
> included its header file components/autofill/core/common/password_form.h.
Done.

Acknowledged.

[yzshen1, 2016-08-11 20:01:50]

On 2016/08/11 03:15:46, leonhsl wrote:
> On 2016/08/10 16:44:59, yzshen1 wrote:
> > On 2016/08/09 09:03:15, leonhsl wrote:
> > > Would you PTAL at this? Thanks.
> > > 
> > > Ken, Yuzhu      --> For everything.
> > > Vaclav, Mathieu --> For {autofill,password_manager} OWNER review.
> > > Tom             --> For security review.
> > > Sky             --> For OWNER review of
> > >   chrome/browser/chrome_content_browser_client.cc
> > >   chrome/test/base/chrome_render_view_test.h
> > >   chrome/test/base/chrome_render_view_test.cc
> > > 
> > > After this CL, all autofill IPCs will be converted into Mojo interfaces,
> > except
> > > the 4 IPCs sent from PasswordGenerationAgent to
ChromePasswordManagerClient,
> > you
> > > can find them still left in autofill_messages.h. As for these 4 IPCs, we
> need
> > to
> > > create another new mojo interface and impl it in chrome/, I'd like to do
> this
> > > work with a follow-up CL.
> > > 
> > >
> >
>
https://codereview.chromium.org/2216463002/diff/40001/chrome/test/base/chrome...
> > > File chrome/test/base/chrome_render_view_test.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2216463002/diff/40001/chrome/test/base/chrome...
> > > chrome/test/base/chrome_render_view_test.cc:114:
> > > RegisterMainFrameRemoteInterfaces();
> > > Because bellowing password_autofill_agent_ constructor will try to connect
> to
> > > autofill::mojom::PasswordManagerDriver interface, we must register a local
> > > interface impl before that. So I added this virtual member function. 
> > > One use case is at Line472 in
> > >
> >
>
https://codereview.chromium.org/2216463002/diff/40001/chrome/renderer/autofil...
> > 
> > Is there any specific issue (or a subset of files) that you would like me to
> > look at?
> > I am not very familiar with this domain. And I can easily get lost while
> looking
> > at a big CL like this.
> 
> Yeah this CL is big as it contains many test codes... En, firstly I'd like you
> to help review the changes in bellowing files and the attached issue ;-)
Thanks.
> 
>   chrome/browser/chrome_content_browser_client.cc
>   chrome/test/base/chrome_render_view_test.h
>   chrome/test/base/chrome_render_view_test.cc

The changes to the three files LGTM

[leonhsl(Using Gerrit), 2016-08-17 01:42:41]

Friendly ping sky@, Thanks.

[sky, 2016-08-17 15:28:33]

https://codereview.chromium.org/2216463002/diff/140001/chrome/test/base/chrom...
File chrome/test/base/chrome_render_view_test.h (right):

https://codereview.chromium.org/2216463002/diff/140001/chrome/test/base/chrom...
chrome/test/base/chrome_render_view_test.h:39: virtual void
RegisterMainFrameRemoteInterfaces() {}
Add description and don't inline. Also, can you outline why you need this rather
than overriding Setup?

[leonhsl(Using Gerrit), 2016-08-19 03:16:57]

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

[commit-bot: I haz the power, 2016-08-19 03:17:28]

Dry run: 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, 2016-08-19 04:07:41]

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

[commit-bot: I haz the power, 2016-08-19 04:07:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2016-08-19 05:02:37]

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

[commit-bot: I haz the power, 2016-08-19 05:02:51]

Dry run: 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, 2016-08-19 06:50:09]

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

[commit-bot: I haz the power, 2016-08-19 06:50:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2016-08-19 06:59:00]

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

[commit-bot: I haz the power, 2016-08-19 06:59:26]

Dry run: 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, 2016-08-19 07:40:29]

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

[commit-bot: I haz the power, 2016-08-19 07:40:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2016-08-19 09:08:33]

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

[commit-bot: I haz the power, 2016-08-19 09:08:52]

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), 2016-08-19 10:13:38]

Uploaded ps#9 to address comments from sky, PTAnL, Thanks.

https://codereview.chromium.org/2216463002/diff/140001/chrome/test/base/chrom...
File chrome/test/base/chrome_render_view_test.cc (right):

https://codereview.chromium.org/2216463002/diff/140001/chrome/test/base/chrom...
chrome/test/base/chrome_render_view_test.cc:114:
RegisterMainFrameRemoteInterfaces();
Because bellowing password_autofill_agent_ constructor will try to connect to
autofill::mojom::PasswordManagerDriver interface, we must register a local
interface impl before that. So I added this virtual member function.
One use case is at Line326 in
https://codereview.chromium.org/2216463002/diff/40001/chrome/renderer/autofil...

https://codereview.chromium.org/2216463002/diff/140001/chrome/test/base/chrom...
File chrome/test/base/chrome_render_view_test.h (right):

https://codereview.chromium.org/2216463002/diff/140001/chrome/test/base/chrom...
chrome/test/base/chrome_render_view_test.h:39: virtual void
RegisterMainFrameRemoteInterfaces() {}
On 2016/08/17 15:28:33, sky wrote:
> Add description and don't inline. Also, can you outline why you need this
rather
> than overriding Setup?

Added description and made definition outline. 
The explanation is in another comment in 
https://codereview.chromium.org/2216463002/diff/140001/chrome/test/base/chrom....

[commit-bot: I haz the power, 2016-08-19 10:28:58]

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

[commit-bot: I haz the power, 2016-08-19 10:28:59]

Dry run: This issue passed the CQ dry run.

[sky, 2016-08-19 15:35:50]

https://codereview.chromium.org/2216463002/diff/160001/chrome/test/base/chrom...
File chrome/test/base/chrome_render_view_test.h (right):

https://codereview.chromium.org/2216463002/diff/160001/chrome/test/base/chrom...
chrome/test/base/chrome_render_view_test.h:39: // Is called by SetUp() to ensure
all mojo interfaces impl ready for main
How about:
// Called from SetUp(). Override to register mojo interfaces.

[leonhsl(Using Gerrit), 2016-08-20 11:39:13]

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

[commit-bot: I haz the power, 2016-08-20 11:39:21]

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), 2016-08-20 11:47:08]

Thanks sky a lot for suggestion, uploaded ps#10 to address comments, PTAnL,
Thanks.

https://codereview.chromium.org/2216463002/diff/160001/chrome/test/base/chrom...
File chrome/test/base/chrome_render_view_test.h (right):

https://codereview.chromium.org/2216463002/diff/160001/chrome/test/base/chrom...
chrome/test/base/chrome_render_view_test.h:39: // Is called by SetUp() to ensure
all mojo interfaces impl ready for main
On 2016/08/19 15:35:50, sky wrote:
> How about:
> // Called from SetUp(). Override to register mojo interfaces.

Done and thanks. This got clearer!

[commit-bot: I haz the power, 2016-08-20 12:43:27]

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

[commit-bot: I haz the power, 2016-08-20 12:43:29]

Dry run: Exceeded global retry quota

[leonhsl(Using Gerrit), 2016-08-20 13:10:26]

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

[commit-bot: I haz the power, 2016-08-20 13:10:30]

Dry run: 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, 2016-08-20 13:13:41]

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

[commit-bot: I haz the power, 2016-08-20 13:13:43]

Dry run: This issue passed the CQ dry run.

[sky, 2016-08-22 14:56:12]

LGTM

[leonhsl(Using Gerrit), 2016-08-23 02:03:01]

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

[leonhsl(Using Gerrit), 2016-08-23 02:03:02]

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

[commit-bot: I haz the power, 2016-08-23 02:03:45]

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, 2016-08-23 03:16:04]

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

[commit-bot: I haz the power, 2016-08-23 03:16:06]

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

[leonhsl(Using Gerrit), 2016-08-23 06:08:28]

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

[commit-bot: I haz the power, 2016-08-23 06:08:50]

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

[leonhsl(Using Gerrit), 2016-08-23 06:31:38]

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

[commit-bot: I haz the power, 2016-08-23 06:31:58]

Dry run: 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, 2016-08-23 07:54:55]

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

[commit-bot: I haz the power, 2016-08-23 07:54:56]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-08-23 08:23:49]

Hi, Vaclav, would you please take a look at changes in ps#11?  Thanks. 
Now all the trybots got green, means it has solved the trybots failure on
PasswordManagerBrowserTestBase.InFrameNavigationDoesNotClearPopupState, which
are not flaky test but definitely have some reasons ;-) I added an inline
comment to explain why we need to do so.

https://codereview.chromium.org/2216463002/diff/200001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_browsertest.cc (left):

https://codereview.chromium.org/2216463002/diff/200001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:1640:
driver_factory->TestingSetDriverForFrame(
The mojo connection has already been established between current password
manager driver and the password autofill agent(in renderer), if we reset a new
driver here, current driver will be destroyed and the mojo connection will
break, this would be considered as an connection error by the agent(in renderer)
who will not re-connect after the error, this means the agent(in renderer) can't
do any IPC call to the new driver anymore.
This is different with legacy IPC mechanism.

I think actually we only want to reset the autofill client for the driver here,
so I added a new interface function TestingSetAutofillClient() to driver and
called it here, instead of resetting the driver instance totally.

[vabr (Chromium), 2016-08-23 08:48:50]

Thanks, Han Leon, for the fixed test and the helpful comment.

Your change between patch set 10 and 11 LGTM with a small nit below.

Thanks,
Vaclav

https://codereview.chromium.org/2216463002/diff/200001/components/password_ma...
File components/password_manager/core/browser/password_autofill_manager.h
(right):

https://codereview.chromium.org/2216463002/diff/200001/components/password_ma...
components/password_manager/core/browser/password_autofill_manager.h:69: void
TestingSetAutofillClient(autofill::AutofillClient* autofill_client);
nit: To make it clear that this only sets the autofill_client_, I propose to:
(1) Inline it in the header.
(2) Rename to set_autofill_client()
(3) Move the restriction about use in tests in a comment.

// Only use in tests.
void set_autofill_client(autofill::AutofillClient* autofill_client) {
  autofill_client_ = autofill_client;
}

[leonhsl(Using Gerrit), 2016-08-23 09:03:08]

Thanks Vaclav a lot for confirmation and kindly help! Uploaded ps#12 to address
the nit and will land this CL now. Thanks all.

https://codereview.chromium.org/2216463002/diff/200001/components/password_ma...
File components/password_manager/core/browser/password_autofill_manager.h
(right):

https://codereview.chromium.org/2216463002/diff/200001/components/password_ma...
components/password_manager/core/browser/password_autofill_manager.h:69: void
TestingSetAutofillClient(autofill::AutofillClient* autofill_client);
On 2016/08/23 08:48:50, vabr (Chromium) wrote:
> nit: To make it clear that this only sets the autofill_client_, I propose to:
> (1) Inline it in the header.
> (2) Rename to set_autofill_client()
> (3) Move the restriction about use in tests in a comment.
> 
> // Only use in tests.
> void set_autofill_client(autofill::AutofillClient* autofill_client) {
>   autofill_client_ = autofill_client;
> }

Done.

[leonhsl(Using Gerrit), 2016-08-23 09:04:27]

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

[leonhsl(Using Gerrit), 2016-08-23 09:04:28]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
tsepez@chromium.org, yzshen@chromium.org, vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2216463002/#ps220001
(title: "Address nit from Vaclav")

[commit-bot: I haz the power, 2016-08-23 09:04:43]

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

[vabr (Chromium), 2016-08-23 09:05:04]

\o/ LGTM!

So happy to see this landing! :)

Cheers,
Vaclav

[commit-bot: I haz the power, 2016-08-23 10:39:48]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-08-23 10:41:25]

Description was changed from

==========
[Autofill] Migrate
ContentPasswordManagerDriver<-->Password{Autofill,Generation}Agent IPCs to mojo.

This CL
-Migrates all ContentPasswordManagerDriver<-->Password{Autofill,Generation}Agent
 IPC messages into mojo interfaces.
-Revises related unit tests and browser tests.

TEST=components_unittests, browser_tests
BUG=582391
==========

to

==========
[Autofill] Migrate
ContentPasswordManagerDriver<-->Password{Autofill,Generation}Agent IPCs to mojo.

This CL
-Migrates all ContentPasswordManagerDriver<-->Password{Autofill,Generation}Agent
 IPC messages into mojo interfaces.
-Revises related unit tests and browser tests.

TEST=components_unittests, browser_tests
BUG=582391

Committed: https://crrev.com/38eab5aeedba6745c0d9f7d7c897c16982299d88
Cr-Commit-Position: refs/heads/master@{#413708}
==========

[commit-bot: I haz the power, 2016-08-23 10:41:27]

Patchset 12 (id:??) landed as
https://crrev.com/38eab5aeedba6745c0d9f7d7c897c16982299d88
Cr-Commit-Position: refs/heads/master@{#413708}

[Ken Rockot(use gerrit already), 2016-08-23 21:13:57]

We were just discussing the autofill mojo conversion in a related discussion
about message ordering in general.

There's some concern that converting this to a separate mojo interface (i.e. not
using a mojo interface associated with the IPC Channel) may violate implicit
ordering expecations among autofill events and other WebContents events such as
navigation.

vabr@ and tsepez@ does this sound like something that could be a security
concern?

[jam, 2016-08-23 21:17:22]

I just saw this. the same comment that I made in
https://codereview.chromium.org/2143383002/ stands here: why is there a
components/autofill/content/public directory? this seems unnecessary given that
autofill isn't a service with a public api. the ocntents of
components/autofill/content/public/cpp/ and
components/autofill/content/public/interfaces should just be in
components/autofill/content/common.

[leonhsl(Using Gerrit), 2016-08-24 01:38:56]

On 2016/08/23 21:13:57, Ken Rockot wrote:
> We were just discussing the autofill mojo conversion in a related discussion
> about message ordering in general.
> 
> There's some concern that converting this to a separate mojo interface (i.e.
not
> using a mojo interface associated with the IPC Channel) may violate implicit
> ordering expecations among autofill events and other WebContents events such
as
> navigation.
> 
> vabr@ and tsepez@ does this sound like something that could be a security
> concern?

Oh,, actually I did some investigations and somehow be confident about the
message ordering issue.. Would you please help to give some details(maybe the
discussion email thread) about the implicit ordering expectations which may be
broken by this CL's mojo conversion? I'd like to take a try to find a possible
solution for this.. Thanks ;-)

[leonhsl(Using Gerrit), 2016-08-24 01:44:07]

On 2016/08/23 21:17:22, jam wrote:
> I just saw this. the same comment that I made in
> https://codereview.chromium.org/2143383002/ stands here: why is there a
> components/autofill/content/public directory? this seems unnecessary given
that
> autofill isn't a service with a public api. the ocntents of
> components/autofill/content/public/cpp/ and
> components/autofill/content/public/interfaces should just be in
> components/autofill/content/common.

Got it. As this(public/cpp and public/interfaces) happened before your comments
in https://codereview.chromium.org/2143383002/, so actually I'm planning to put
all of them into common/ once at all later ;-)

[keishi, 2016-08-24 01:49:31]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2274783002/ by keishi@chromium.org.

The reason for reverting is: Broke browser_test
PasswordManagerBrowserTestBase.InternalsPage

https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.10%20Tests....

[leonhsl(Using Gerrit), 2016-08-24 03:21:13]

Patchset #13 (id:240001) has been deleted

[vabr (Chromium), 2016-08-24 08:28:13]

On 2016/08/24 01:38:56, leonhsl wrote:
> On 2016/08/23 21:13:57, Ken Rockot wrote:
> > We were just discussing the autofill mojo conversion in a related discussion
> > about message ordering in general.
> > 
> > There's some concern that converting this to a separate mojo interface (i.e.
> not
> > using a mojo interface associated with the IPC Channel) may violate implicit
> > ordering expecations among autofill events and other WebContents events such
> as
> > navigation.
> > 
> > vabr@ and tsepez@ does this sound like something that could be a security
> > concern?
> 
> Oh,, actually I did some investigations and somehow be confident about the
> message ordering issue.. Would you please help to give some details(maybe the
> discussion email thread) about the implicit ordering expectations which may be
> broken by this CL's mojo conversion? I'd like to take a try to find a possible
> solution for this.. Thanks ;-)

Thanks for bringing up this issue, Ken.

When Han Leon planned this CL, the thing we considered was whether the mix of
old IPC autofill messages and new Mojo autofill messages might break autofill
functionality. The conclusion was that because in autofill the browser-renderer
conversations are ping-pong with each message in one direction being followed by
a response in the other direction, there were not assumptions about preserving
order.

However, I did not check whether there are any assumptions about the order of
the messages and WebContents events.

I'm also afraid I don't know what is the difference between the Mojo interface
associated with the IPC channel and the separate Mojo interface used here.
Pointers to information on that, as well as to the mentioned conversation would
be certainly appreciated!

Thanks,
Vaclav

[vabr (Chromium), 2016-08-29 10:09:08]

Heads-up: I just gave approval to reland this CL on
https://codereview.chromium.org/2270333002/#msg10. Please speak up if you see
any concerns still in need of addressing.

Cheers,
Vaclav