[Autofill] Migrate ChromePasswordManagerClient<-->PasswordGenerationAgent IPCs to mojo.

[Autofill] Migrate ChromePasswordManagerClient<-->PasswordGenerationAgent IPCs to mojo.

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

TEST=browser_tests
BUG=582391
Committed: https://crrev.com/471b67d4c3111179e8d1babb3219a62f0c2a8faa
Cr-Commit-Position: refs/heads/master@{#424351}

[leonhsl(Using Gerrit), 2016-09-20 12:54:58]

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

[commit-bot: I haz the power, 2016-09-20 12:55: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-09-20 13:09:32]

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

[commit-bot: I haz the power, 2016-09-20 13:09:33]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[leonhsl(Using Gerrit), 2016-09-20 15:06:30]

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

[leonhsl(Using Gerrit), 2016-09-20 15:06:31]

Hi, Ken, this CL is using content::AssociatedInterfaceProvider invented by you
;-) To write test codes, I tried to enable registering local callbacks into
content::AssociatedInterfaceProvider, just as shell::InterfaceProvider did. 

The changes are in //content/* and the use case is in
chrome/renderer/autofill/password_autofill_agent_browsertest.cc. For Win/Linux
trybots the compilation is OK but the test case
PasswordAutofillAgentTest.PasswordGenerationSupersedesAutofill is failing, seems
the injected local callback is not working properly, but I can't figure out the
root reason.. Would you please help to take a look to see whether my solution is
reasonable or is there any big miss on it? Thanks~

[Ken Rockot(use gerrit already), 2016-09-20 18:26:20]

Looking pretty good!

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

https://codereview.chromium.org/2352533003/diff/20001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:144:
password_manager_client_bindings_(web_contents, this),
Looks like you're hitting DCHECKs on construction here in some cases. These
indicate that there is more than one ChromePasswordManagerClient being added to
a WebContents, which seems undesirable. Any idea how that's happening?

https://codereview.chromium.org/2352533003/diff/20001/chrome/renderer/autofil...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2352533003/diff/20001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:551:
base::RunLoop().RunUntilIdle();
RunUntilIdle() is generally a red flag for flaky behavior. Please don't use it
unless you have to.

In this case, you could add something like a
set_show_password_generation_callback to the fake impl, and pass a RunLoop's
QuitClosure() in for that. Then you just run the RunLoop, and if the test is
functioning properly the loop will exit once ShowPasswordGeneration is called.

https://codereview.chromium.org/2352533003/diff/20001/components/autofill/con...
File components/autofill/content/common/OWNERS (left):

https://codereview.chromium.org/2352533003/diff/20001/components/autofill/con...
components/autofill/content/common/OWNERS:3: per-file *_messages*.h=set noparent
nit: I don't think these lines should be deleted. They should actually be
*_message*.h so that foo_message_generators.h is included, and files still exist
which match that.

https://codereview.chromium.org/2352533003/diff/20001/content/common/associat...
File content/common/associated_interface_provider_impl.h (right):

https://codereview.chromium.org/2352533003/diff/20001/content/common/associat...
content/common/associated_interface_provider_impl.h:17: class LocalProvider;
Never use anonymous namespaces in headers.

LocalProvider can be declared privately inside the the
AssociatedIntercaceProviderImpl definition.

https://codereview.chromium.org/2352533003/diff/20001/content/public/common/a...
File content/public/common/associated_interface_provider.h (right):

https://codereview.chromium.org/2352533003/diff/20001/content/public/common/a...
content/public/common/associated_interface_provider.h:34: class TestApi {
I don't think we need to emulate what's done in InterfaceRegistry. And there's
no use of ClearBinders() in this CL anyway.

Can you just add a public OverrideBinderForTesting(name, callback) method on
AssociatedInterfaceProvider/Impl?

[yzshen1, 2016-09-20 18:50:25]

yzshen@chromium.org changed reviewers:
+ yzshen@chromium.org

[yzshen1, 2016-09-20 18:50:26]

Some drive-by comments.
I didn't review the whole CL.

https://codereview.chromium.org/2352533003/diff/20001/chrome/renderer/autofil...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2352533003/diff/20001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:551:
base::RunLoop().RunUntilIdle();
On 2016/09/20 18:26:19, Ken Rockot wrote:
> RunUntilIdle() is generally a red flag for flaky behavior. Please don't use it
> unless you have to.
> 
> In this case, you could add something like a
> set_show_password_generation_callback to the fake impl, and pass a RunLoop's
> QuitClosure() in for that. Then you just run the RunLoop, and if the test is
> functioning properly the loop will exit once ShowPasswordGeneration is called.

+1 for what Ken said.
Besides, you might also find the FlushForTesting() method useful. This method is
defined on [Associated]InterfacePtr/Binding. It sends a control message and
waits for a response, so when you receive the response, you can be sure that any
message sent prior to the control message have been received by the other side.

https://codereview.chromium.org/2352533003/diff/20001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2352533003/diff/20001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:1817:
#"../renderer/autofill/password_generation_agent_browsertest.cc",
Is this change intentional?

[leonhsl(Using Gerrit), 2016-09-21 11:46:57]

Uploaded ps#3 to address comments, would you PTAnL? Thanks!

Points:
1,
Revised AssociatedInterfaceProvider related impl, now browser_tests can pass. #
TODO: Will modify password_generation_agent_browsertest.cc later.
2,
How to handle duplicated ChromePasswordManagerClient(Mock) in unit tests.

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

https://codereview.chromium.org/2352533003/diff/20001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:144:
password_manager_client_bindings_(web_contents, this),
On 2016/09/20 18:26:19, Ken Rockot wrote:
> Looks like you're hitting DCHECKs on construction here in some cases. These
> indicate that there is more than one ChromePasswordManagerClient being added
to
> a WebContents, which seems undesirable. Any idea how that's happening?

Yeah this is because another mock ChromePasswordManagerClient has been created
here:
https://cs.chromium.org/chromium/src/chrome/browser/password_manager/chrome_p...
I'm not sure how can we remove the old ChromePasswordManagerClient gracefully
before creating the mock one, as it is a content::WebContentsUserData supposed
having the same lifetime with web contents. Would you please help to give some
hints about this? Thanks~

https://codereview.chromium.org/2352533003/diff/20001/chrome/renderer/autofil...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2352533003/diff/20001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:551:
base::RunLoop().RunUntilIdle();
On 2016/09/20 18:50:25, yzshen1 wrote:
> On 2016/09/20 18:26:19, Ken Rockot wrote:
> > RunUntilIdle() is generally a red flag for flaky behavior. Please don't use
it
> > unless you have to.
> > 
> > In this case, you could add something like a
> > set_show_password_generation_callback to the fake impl, and pass a RunLoop's
> > QuitClosure() in for that. Then you just run the RunLoop, and if the test is
> > functioning properly the loop will exit once ShowPasswordGeneration is
called.
> 
> +1 for what Ken said.
> Besides, you might also find the FlushForTesting() method useful. This method
is
> defined on [Associated]InterfacePtr/Binding. It sends a control message and
> waits for a response, so when you receive the response, you can be sure that
any
> message sent prior to the control message have been received by the other
side.

OK.. Thanks! I had no idea of FlushForTesting() before, change to use it,
because for some test cases maybe there has no any ShowPasswordGenerationPopup
request. 
Also, I found the root cause for the failure here and I've changed
AssociatedInterfaceProvider::LocalProvider implementation, now this test case
PASS.

https://codereview.chromium.org/2352533003/diff/20001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2352533003/diff/20001/chrome/test/BUILD.gn#ne...
chrome/test/BUILD.gn:1817:
#"../renderer/autofill/password_generation_agent_browsertest.cc",
On 2016/09/20 18:50:25, yzshen1 wrote:
> Is this change intentional?

Yeah I have not changed the old IPCs related test codes in
password_generation_agent_browsertest.cc so just exclude it from build
temporally. Will touch it after verified that my changes of
AssociatedInterfaceProvider have no problem.

https://codereview.chromium.org/2352533003/diff/20001/components/autofill/con...
File components/autofill/content/common/OWNERS (left):

https://codereview.chromium.org/2352533003/diff/20001/components/autofill/con...
components/autofill/content/common/OWNERS:3: per-file *_messages*.h=set noparent
On 2016/09/20 18:26:19, Ken Rockot wrote:
> nit: I don't think these lines should be deleted. They should actually be
> *_message*.h so that foo_message_generators.h is included, and files still
exist
> which match that.

Done.

https://codereview.chromium.org/2352533003/diff/20001/content/common/associat...
File content/common/associated_interface_provider_impl.h (right):

https://codereview.chromium.org/2352533003/diff/20001/content/common/associat...
content/common/associated_interface_provider_impl.h:17: class LocalProvider;
On 2016/09/20 18:26:19, Ken Rockot wrote:
> Never use anonymous namespaces in headers.
> 
> LocalProvider can be declared privately inside the the
> AssociatedIntercaceProviderImpl definition.

Done.

https://codereview.chromium.org/2352533003/diff/20001/content/public/common/a...
File content/public/common/associated_interface_provider.h (right):

https://codereview.chromium.org/2352533003/diff/20001/content/public/common/a...
content/public/common/associated_interface_provider.h:34: class TestApi {
On 2016/09/20 18:26:20, Ken Rockot wrote:
> I don't think we need to emulate what's done in InterfaceRegistry. And there's
> no use of ClearBinders() in this CL anyway.
> 
> Can you just add a public OverrideBinderForTesting(name, callback) method on
> AssociatedInterfaceProvider/Impl?

Done and thanks!

[leonhsl(Using Gerrit), 2016-09-22 09:54:54]

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

[commit-bot: I haz the power, 2016-09-22 09:55:09]

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-09-22 09:57:31]

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

[commit-bot: I haz the power, 2016-09-22 09:57:38]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  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-09-22 10:05:15]

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

[commit-bot: I haz the power, 2016-09-22 10:05:23]

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-09-22 10:53:33]

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

[commit-bot: I haz the power, 2016-09-22 10:53:34]

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

[leonhsl(Using Gerrit), 2016-09-23 15:33:47]

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

[commit-bot: I haz the power, 2016-09-23 15:33:58]

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-09-23 15:38:37]

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

[leonhsl(Using Gerrit), 2016-09-23 15:38:38]

Uploaded ps#6 to fix browser_tests, now the only problem left is duplicated
ChromePasswordManagerClient instance caused by
https://cs.chromium.org/chromium/src/chrome/browser/password_manager/chrome_p...
, do you have any idea how to solve this gracefully? Thanks~

[commit-bot: I haz the power, 2016-09-23 16:17:24]

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

[commit-bot: I haz the power, 2016-09-23 16:17:26]

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_...)

[vabr (Chromium), 2016-09-27 15:06:12]

LGTM!

As for the issue with
ChromePasswordManagerClientTest.SavingAndFillingEnabledConditionsTest, you could
supply a new WebContents to the mocked client:

WebContents::CreateParams params(web_contents()->GetBrowserContext());
auto test_web_contents = MakeUnique<WebContents>(params);
auto client =
MakeUnique<MockChromePasswordManagerClient>(test_web_contents.get());

Cheers,
Vaclav

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

https://codereview.chromium.org/2352533003/diff/100001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.h:101: // Causes
the password generation UI to be shown for the specified form.
nit: Please move these comments to interface PasswordManagerClient instead.
Overridden interface methods do not usually have comments, because the interface
specifies the purpose of the method and the meaning of the arguments.

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

https://codereview.chromium.org/2352533003/diff/100001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:338: // We only
use the fake password client for main frame
optional nit: Pronouns like "we" and "our" are ambiguous. Consider rephrasing
like:
"Because the test cases only involve the main frame in this test, the fake
password client is only used for the main frame."

Also in chrome/renderer/autofill/password_generation_agent_browsertest.cc.

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

https://codereview.chromium.org/2352533003/diff/100001/components/autofill/co...
components/autofill/content/common/autofill_message_generator.h:1: // Copyright
2013 The Chromium Authors. All rights reserved.
Is this file still used? Where? Could we delete it instead?

https://codereview.chromium.org/2352533003/diff/100001/components/autofill/co...
File components/autofill/content/public/interfaces/autofill_driver.mojom
(right):

https://codereview.chromium.org/2352533003/diff/100001/components/autofill/co...
components/autofill/content/public/interfaces/autofill_driver.mojom:125:
gfx.mojom.RectF bounds, int32 max_length,
nit: |max_length| and other arguments are missing from the description. Please
improve, feel free to reuse parts of the existing comment in
ChromePasswordManagerClient (which can be deleted afterwards).

[leonhsl(Using Gerrit), 2016-09-28 01:16:45]

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

[commit-bot: I haz the power, 2016-09-28 01:17:13]

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-09-28 02:22:17]

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

[commit-bot: I haz the power, 2016-09-28 02:22:19]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-09-28 02:40:39]

Thanks Vaclav a lot for help! Uploaded ps#7 to address comments, PTAnL, Thanks.

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

https://codereview.chromium.org/2352533003/diff/100001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client.h:101: // Causes
the password generation UI to be shown for the specified form.
On 2016/09/27 15:06:11, vabr (Chromium) wrote:
> nit: Please move these comments to interface PasswordManagerClient instead.
> Overridden interface methods do not usually have comments, because the
interface
> specifies the purpose of the method and the meaning of the arguments.

Done. Moved these comments to interface definition in autofill_driver.mojom, and
removed some redundant comments.

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

https://codereview.chromium.org/2352533003/diff/100001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:338: // We only
use the fake password client for main frame
On 2016/09/27 15:06:11, vabr (Chromium) wrote:
> optional nit: Pronouns like "we" and "our" are ambiguous. Consider rephrasing
> like:
> "Because the test cases only involve the main frame in this test, the fake
> password client is only used for the main frame."
> 
> Also in chrome/renderer/autofill/password_generation_agent_browsertest.cc.

Done.

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

https://codereview.chromium.org/2352533003/diff/100001/components/autofill/co...
components/autofill/content/common/autofill_message_generator.h:1: // Copyright
2013 The Chromium Authors. All rights reserved.
On 2016/09/27 15:06:11, vabr (Chromium) wrote:
> Is this file still used? Where? Could we delete it instead?

Yeah, it's still in use, we can delete all autofill legacy IPC traits here after
chrome/common/importer/profile_import_process_messages.h is converted to Mojo
interface.
https://cs.chromium.org/chromium/src/chrome/common/importer/profile_import_pr...
still needs the IPC struct traits for autofill::PasswordForm, which then needs
the struct traits of autofill::FormFieldData defined here.

https://codereview.chromium.org/2352533003/diff/100001/components/autofill/co...
File components/autofill/content/public/interfaces/autofill_driver.mojom
(right):

https://codereview.chromium.org/2352533003/diff/100001/components/autofill/co...
components/autofill/content/public/interfaces/autofill_driver.mojom:125:
gfx.mojom.RectF bounds, int32 max_length,
On 2016/09/27 15:06:12, vabr (Chromium) wrote:
> nit: |max_length| and other arguments are missing from the description. Please
> improve, feel free to reuse parts of the existing comment in
> ChromePasswordManagerClient (which can be deleted afterwards).

Done.

[leonhsl(Using Gerrit), 2016-09-28 02:52:36]

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

[leonhsl(Using Gerrit), 2016-09-28 02:52:37]

Hi, would you PTAL? Thanks.

Ken: Overall review, especially AssociatedInterfaceProvider changes in
//content.

Tom: Security review for .mojom file, owner review for //ipc and
//tools/ipc_fuzzer.

John: Owner review for //chrome except {autofill, password_manager}, owner
review for //content.

[vabr (Chromium), 2016-09-28 08:59:55]

Still LGTM, thanks for addressing the comments!
Vaclav

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

https://codereview.chromium.org/2352533003/diff/100001/components/autofill/co...
components/autofill/content/common/autofill_message_generator.h:1: // Copyright
2013 The Chromium Authors. All rights reserved.
On 2016/09/28 02:40:39, leonhsl (OOO 29.Sep--05.Oct) wrote:
> On 2016/09/27 15:06:11, vabr (Chromium) wrote:
> > Is this file still used? Where? Could we delete it instead?
> 
> Yeah, it's still in use, we can delete all autofill legacy IPC traits here
after
> chrome/common/importer/profile_import_process_messages.h is converted to Mojo
> interface.
>
https://cs.chromium.org/chromium/src/chrome/common/importer/profile_import_pr...
> still needs the IPC struct traits for autofill::PasswordForm, which then needs
> the struct traits of autofill::FormFieldData defined here.

Acknowledged.

[Ken Rockot(use gerrit already), 2016-09-28 14:42:48]

lgtm

https://codereview.chromium.org/2352533003/diff/120001/content/common/associa...
File content/common/associated_interface_provider_impl.h (right):

https://codereview.chromium.org/2352533003/diff/120001/content/common/associa...
content/common/associated_interface_provider_impl.h:26: // mojom::RouteProvider
and mojom::AssociatedInterfaceProvider.
nit: We can reduce this documentation a bit since it's revealing a bunch of
details the reader probably doesn't need. How about

  // Constructs a local provider with no remote interfaces. This is useful in
  // conjunction with OverrideBinderForTesting(), in test environments where
there may
  // not be a remote |mojom::AssociatedInterfaceProvider| available.

https://codereview.chromium.org/2352533003/diff/120001/content/common/associa...
content/common/associated_interface_provider_impl.h:40: class LocalProvider :
public mojom::RouteProvider,
Please move the class definition into the cc file. You only need to declare it
here:


  ...
 private:
  class LocalProvider;

  ...

  std::unique_ptr<LocalProvider> local_provider_;
}


In the cc you can define

class AssociatedInterfaceProviderImpl::LocalProvider {
  ...
};

[Tom Sepez, 2016-09-28 16:38:25]

Code is fine, just a small issue about how to structure it.  Thanks.

https://codereview.chromium.org/2352533003/diff/120001/components/autofill/co...
File components/autofill/content/common/autofill_message_generator.h (right):

https://codereview.chromium.org/2352533003/diff/120001/components/autofill/co...
components/autofill/content/common/autofill_message_generator.h:12:
IPC_ENUM_TRAITS_MAX_VALUE(autofill::FormFieldData::CheckStatus,
It feels like these definitions all belong in
components/autofill/content/common/autofill_param_traits_macros.h now that there
are no messages being defined, just traits.

[jam, 2016-09-29 20:09:12]

which parts do you want me to look at?

[leonhsl(Using Gerrit), 2016-10-02 13:18:00]

On 2016/09/29 20:09:12, jam wrote:
> which parts do you want me to look at?

Owner review for //chrome except {autofill, password_manager}, owner review for
//content.

After I finished addressing comments from ken and Tom, I'll ping you to look at
the latest code ;-) Thanks.

[leonhsl(Using Gerrit), 2016-10-07 03:54:19]

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

[commit-bot: I haz the power, 2016-10-07 03:54: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-10-07 05:42:49]

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

[commit-bot: I haz the power, 2016-10-07 05:42:50]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[leonhsl(Using Gerrit), 2016-10-07 05:45:25]

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

[commit-bot: I haz the power, 2016-10-07 05:45:46]

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-10-07 06:26:58]

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

[commit-bot: I haz the power, 2016-10-07 06:26:59]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-10-07 06:53:17]

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

[commit-bot: I haz the power, 2016-10-07 06:53:24]

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-10-07 07:19:28]

Patchset #9 (id:160001) has been deleted

[leonhsl(Using Gerrit), 2016-10-07 07:22:19]

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

[commit-bot: I haz the power, 2016-10-07 07:22:29]

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-10-07 09:20:24]

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

[commit-bot: I haz the power, 2016-10-07 09:20:25]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-10-07 09:42:53]

Uploaded ps#9 to address comments from Ken and Tom, would you PTAnL? Thanks!

https://codereview.chromium.org/2352533003/diff/120001/components/autofill/co...
File components/autofill/content/common/autofill_message_generator.h (right):

https://codereview.chromium.org/2352533003/diff/120001/components/autofill/co...
components/autofill/content/common/autofill_message_generator.h:12:
IPC_ENUM_TRAITS_MAX_VALUE(autofill::FormFieldData::CheckStatus,
On 2016/09/28 16:38:25, Tom Sepez wrote:
> It feels like these definitions all belong in
> components/autofill/content/common/autofill_param_traits_macros.h now that
there
> are no messages being defined, just traits.

Yeah there are only the traits now, which are still used by the IPC message at
https://cs.chromium.org/chromium/src/chrome/common/importer/profile_import_pr...

Moved all contents of autofill_message_generator.h into
autofill_param_traits_macros.h, and deleted autofill_message_generator.h itself.

https://codereview.chromium.org/2352533003/diff/120001/content/common/associa...
File content/common/associated_interface_provider_impl.h (right):

https://codereview.chromium.org/2352533003/diff/120001/content/common/associa...
content/common/associated_interface_provider_impl.h:26: // mojom::RouteProvider
and mojom::AssociatedInterfaceProvider.
On 2016/09/28 14:42:48, Ken Rockot wrote:
> nit: We can reduce this documentation a bit since it's revealing a bunch of
> details the reader probably doesn't need. How about
> 
>   // Constructs a local provider with no remote interfaces. This is useful in
>   // conjunction with OverrideBinderForTesting(), in test environments where
> there may
>   // not be a remote |mojom::AssociatedInterfaceProvider| available.

Done and Thanks!

https://codereview.chromium.org/2352533003/diff/120001/content/common/associa...
content/common/associated_interface_provider_impl.h:40: class LocalProvider :
public mojom::RouteProvider,
On 2016/09/28 14:42:48, Ken Rockot wrote:
> Please move the class definition into the cc file. You only need to declare it
> here:
> 
> 
>   ...
>  private:
>   class LocalProvider;
> 
>   ...
> 
>   std::unique_ptr<LocalProvider> local_provider_;
> }
> 
> 
> In the cc you can define
> 
> class AssociatedInterfaceProviderImpl::LocalProvider {
>   ...
> };

Done.

[leonhsl(Using Gerrit), 2016-10-07 09:54:56]

Ping jam@, I think now the code is ready for your review. Thanks.
Owner review for //chrome except {autofill, password_manager}, owner review for
//content.

[Tom Sepez, 2016-10-07 17:50:04]

messages/mojom LGTM.

[jam, 2016-10-10 15:00:28]

lgtm

[leonhsl(Using Gerrit), 2016-10-11 00:38:12]

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

[leonhsl(Using Gerrit), 2016-10-11 00:38:12]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2352533003/#ps180001
(title: "Address comments from Ken and Tom")

[commit-bot: I haz the power, 2016-10-11 00:38:43]

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-10-11 00:42:10]

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

[commit-bot: I haz the power, 2016-10-11 00:42:12]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[leonhsl(Using Gerrit), 2016-10-11 01:04:06]

Did rebase and will land now. Thanks all~

[leonhsl(Using Gerrit), 2016-10-11 01:04:15]

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

[leonhsl(Using Gerrit), 2016-10-11 01:04:15]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
rockot@chromium.org, jam@chromium.org, vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2352533003/#ps190001
(title: "Rebase only")

[commit-bot: I haz the power, 2016-10-11 01:04:59]

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-10-11 02:46:32]

Committed patchset #10 (id:190001)

[commit-bot: I haz the power, 2016-10-11 02:50:52]

Description was changed from

==========
[Autofill] Migrate ChromePasswordManagerClient<-->PasswordGenerationAgent IPCs
to mojo.

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

TEST=browser_tests
BUG=582391
==========

to

==========
[Autofill] Migrate ChromePasswordManagerClient<-->PasswordGenerationAgent IPCs
to mojo.

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

TEST=browser_tests
BUG=582391

Committed: https://crrev.com/471b67d4c3111179e8d1babb3219a62f0c2a8faa
Cr-Commit-Position: refs/heads/master@{#424351}
==========

[commit-bot: I haz the power, 2016-10-11 02:50:56]

Patchset 10 (id:??) landed as
https://crrev.com/471b67d4c3111179e8d1babb3219a62f0c2a8faa
Cr-Commit-Position: refs/heads/master@{#424351}