Switch components/password_manager code from IPC messages to Mojo.

Switch components/password_manager code from IPC messages to Mojo.

Replace credential_manager_messages.h IPC to Mojo service.

BUG=582391
Committed: https://crrev.com/d20fb918841354a75546fa38b5307aaba117598b
Cr-Commit-Position: refs/heads/master@{#385369}

[leonhsl(Using Gerrit), 2016-03-03 07:29:35]

leon.han@intel.com changed reviewers:
+ amistry@chromium.org, rockot@chromium.org, sammc@chromium.org,
vabr@chromium.org

[leonhsl(Using Gerrit), 2016-03-03 07:29:35]

Hi, would you PTAL? Thanks all~

+Vaclav as OWNER of password manager.
+Ken, Anand, Sam as mojo experts.

This is the initial CL to act as a discussion basis:
  * Old password_manager IPC messages send/receive via
RenderViewHost/RenderView, while this CL will build mojo connection via service
registry pair of the main RenderFrameHost/RenderFrame. #As RVH/RV will
eventually be eliminated via out-of-process iframe project, transferring the IPC
to its main frame should make sense.#

  * 1:1:1 instance mapping and life cycle tied together: 
     password_manager::CredentialManagerImpl (mojo interface impl) <-->
ChromePasswordManagerClient <-->  content::WebContents

  * The password_manager::mojom::CredentialManager mojo service will be
registered into service registry of the main RFH via ChromeContentBrowserClient
in //chrome.

CL current status:
  * gn build OK.
  * TODO: some detail impl such as mojo TypeConverter not ready.
  * TODO: gyp build not ready.
  * TODO: unit_tests, browser_tests not ready.

[vabr (Chromium), 2016-03-07 13:54:21]

Thanks for the CL. So far it looks reasonable, although I have only skimmed it,
given there are still TODOs left (I also ignored formatting and nits at this
point).

To avoid misunderstanding, we should maybe refer to credential manager code
rather than password manager, though. While it is true, that this removes IPC
messages defined in the password_manager component, for some reasons much of the
password manager renderer code sits in the autofill component, so all the IPCs
used for common password manager tasks are defined in the autofill component.
What you are removing here are IPCs for the Credential Management API
implementation, a new and experimental part of the password manager.

Cheers,
Vaclav

[leonhsl(Using Gerrit), 2016-03-08 03:30:45]

On 2016/03/07 13:54:21, vabr (Chromium) wrote:
> Thanks for the CL. So far it looks reasonable, although I have only skimmed
it,
> given there are still TODOs left (I also ignored formatting and nits at this
> point).
> 
> To avoid misunderstanding, we should maybe refer to credential manager code
> rather than password manager, though. While it is true, that this removes IPC
> messages defined in the password_manager component, for some reasons much of
the
> password manager renderer code sits in the autofill component, so all the IPCs
> used for common password manager tasks are defined in the autofill component.
> What you are removing here are IPCs for the Credential Management API
> implementation, a new and experimental part of the password manager.
> 
> Cheers,
> Vaclav

Oh.. Understood. Sorry it's my misunderstanding! I noticed the autofill IPC
messages have deep relationship with password manager component but I kept
focusing on credential_manager_messages.h transformation...
Now I see that our main target should be autofill component, I'll dip into it to
accomplish this work. (Actually these days I'm wondering what's the
non-deterministic ordering problem between legacy navigation IPCs and
CredentialManager mojo IPC, in fact it should be for autofill component..)
About this CL, should I continue to accomplish it even its priority is not so
high, WDYT? Thanks~

[vabr (Chromium), 2016-03-08 09:36:40]

On 2016/03/08 03:30:45, leonhsl wrote:
> On 2016/03/07 13:54:21, vabr (Chromium) wrote:
> > Thanks for the CL. So far it looks reasonable, although I have only skimmed
> it,
> > given there are still TODOs left (I also ignored formatting and nits at this
> > point).
> > 
> > To avoid misunderstanding, we should maybe refer to credential manager code
> > rather than password manager, though. While it is true, that this removes
IPC
> > messages defined in the password_manager component, for some reasons much of
> the
> > password manager renderer code sits in the autofill component, so all the
IPCs
> > used for common password manager tasks are defined in the autofill
component.
> > What you are removing here are IPCs for the Credential Management API
> > implementation, a new and experimental part of the password manager.
> > 
> > Cheers,
> > Vaclav
> 
> Oh.. Understood. Sorry it's my misunderstanding! I noticed the autofill IPC
> messages have deep relationship with password manager component but I kept
> focusing on credential_manager_messages.h transformation...
> Now I see that our main target should be autofill component, I'll dip into it
to
> accomplish this work. (Actually these days I'm wondering what's the
> non-deterministic ordering problem between legacy navigation IPCs and
> CredentialManager mojo IPC, in fact it should be for autofill component..)
> About this CL, should I continue to accomplish it even its priority is not so
> high, WDYT? Thanks~

I definitely think switching this part to Mojo has value. I also like that you
started with it (it is smaller than the autofill part), and would definitely
suggest to stick to just the password_manager component in this CL.

We just need to watch for the potential bugs coming from the non-deterministic
ordering between the Mojo calls in password_manager and the legacy IPC in
autofill.

Cheers,
Vaclav

[leonhsl(Using Gerrit), 2016-03-09 02:51:40]

On 2016/03/08 09:36:40, vabr (Chromium) wrote:
> On 2016/03/08 03:30:45, leonhsl wrote:
> > On 2016/03/07 13:54:21, vabr (Chromium) wrote:
> > > Thanks for the CL. So far it looks reasonable, although I have only
skimmed
> > it,
> > > given there are still TODOs left (I also ignored formatting and nits at
this
> > > point).
> > > 
> > > To avoid misunderstanding, we should maybe refer to credential manager
code
> > > rather than password manager, though. While it is true, that this removes
> IPC
> > > messages defined in the password_manager component, for some reasons much
of
> > the
> > > password manager renderer code sits in the autofill component, so all the
> IPCs
> > > used for common password manager tasks are defined in the autofill
> component.
> > > What you are removing here are IPCs for the Credential Management API
> > > implementation, a new and experimental part of the password manager.
> > > 
> > > Cheers,
> > > Vaclav
> > 
> > Oh.. Understood. Sorry it's my misunderstanding! I noticed the autofill IPC
> > messages have deep relationship with password manager component but I kept
> > focusing on credential_manager_messages.h transformation...
> > Now I see that our main target should be autofill component, I'll dip into
it
> to
> > accomplish this work. (Actually these days I'm wondering what's the
> > non-deterministic ordering problem between legacy navigation IPCs and
> > CredentialManager mojo IPC, in fact it should be for autofill component..)
> > About this CL, should I continue to accomplish it even its priority is not
so
> > high, WDYT? Thanks~
> 
> I definitely think switching this part to Mojo has value. I also like that you
> started with it (it is smaller than the autofill part), and would definitely
> suggest to stick to just the password_manager component in this CL.
> 
> We just need to watch for the potential bugs coming from the non-deterministic
> ordering between the Mojo calls in password_manager and the legacy IPC in
> autofill.
> 
> Cheers,
> Vaclav

Thanks a lot for kindly explanations~ Will address all the TODOs and update this
CL soon.

[leonhsl(Using Gerrit), 2016-03-09 11:00:54]

Description was changed from

==========
Switch password manager code from IPC messages to Mojo.

Replace credential_manager_messages.h IPC to Mojo service.

This CL is a prototype to start, will update continuously.

BUG=582391
==========

to

==========
Switch components/password_manager code from IPC messages to Mojo.

Replace credential_manager_messages.h IPC to Mojo service.

BUG=582391
==========

[leonhsl(Using Gerrit), 2016-03-09 11:29:25]

Uploaded patchset #2, mojo conversion impl done, but unit_tests and
browser_tests are still ongoing and will come soon. Thanks~

[leonhsl(Using Gerrit), 2016-03-09 11:39:27]

On 2016/03/09 11:29:25, leonhsl wrote:
> Uploaded patchset #2, mojo conversion impl done, but unit_tests and
> browser_tests are still ongoing and will come soon. Thanks~

Note for the change of url/origin.{h,cc}:
As for the bellowing generated class, mojom::Credentialinfo::Clone() will be
generated and its impl will make a call to url::Origin::Clone() so I have to add
one url::Origin::Clone() impl.

namespace mojom {

class Credentialinfo {
 // ...
 url::Origin federation;
};

}

[vabr (Chromium), 2016-03-09 16:26:30]

Thank you, the password manager bits look good to me (with comments), I'm
looking forward to the tests.

Just to make it clear, I have zero experience with Mojo, so I rely on you having
at most one person with Mojo experience besides yourself as a reviewer as well.

Thanks,
Vaclav

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

https://codereview.chromium.org/1762603002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:629: return;
The code style says: "do not handle DCHECK() failures" [1]. Please replace lines
627-630 with DCHECK(web_contents).

[1]
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

https://codereview.chromium.org/1762603002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:634: //
ChromePasswordManagerClient instance should have been created already.
nit: You can drop the comment, the DCHECK says essentially the same thing.

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

https://codereview.chromium.org/1762603002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:172: // Mojo
CredentialManager impl instance,
nit: I'm not sure I understand the first sentence, but it seems redundant. Line
178 states the type, and it is clear that this is 1:1 with the owning client and
lives as long as the client lives.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.cc:56:
DCHECK(info.type != CredentialType::CREDENTIAL_TYPE_EMPTY);
nit: Use
DCHECK_NE(CredentialType::CREDENTIAL_TYPE_EMPTY, info.type);
instead, it has better error reporting.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File components/password_manager/content/browser/credential_manager_impl.h
(right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.h:38: //
This class implements the Mojo interface mojom::CredentialManager.
optional: Drop this comment, line 40 says the same thing.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.h:69: //
Called in response to the request from the renderer, triggered by a page's
nit: Actually, the comments explaining the methods should be removed, they
should only be present in the interface definition. Having a single source of
them avoids diverging in the future.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.h:70: //
call
nit: Please indent the comment block properly, do not leave unnecessary
horizontal space. You can, e.g., put the whole comment on one line and let git
cl format break it for you.

Also below.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.h:83: // to
'navigator.credentials.request'.
"request" or "get"? The mojom file says "get". The method name should match the
API call name.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File
components/password_manager/content/public/interfaces/credential_manager.mojom
(right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/public/interfaces/credential_manager.mojom:40:
// The result callback will return a non-null and valid CredentialInfo if
succeeded,
I'm not sure about the indentation rules for .mojom, but in general lines should
be max. 80 characters long.
(Also on line 44.)

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File components/password_manager/content/public/type_converters.cc (right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/public/type_converters.cc:29: default:
Could you leave out the default clause completely? Without the default clause,
the compiler will automatically flag this place as needing an update in case
CredentialType is changed.

(Similarly on line 43 and 121.)

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/public/type_converters.cc:32: return
mojom::CredentialType::EMPTY;
nit: Put a NOTREACHED() between the current lines 31 and 32 instead?

(Similarly for lines 45-46.)

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/public/type_converters.cc:105:
BlinkWebCredentialPtr output = nullptr;
nit: scoped_ptr is automatically null on creation, not need to assign nullptr.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File components/password_manager/content/public/type_converters.h (right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/public/type_converters.h:19: using
BlinkWebCredentialPtr = scoped_ptr<blink::WebCredential>;
Is there any special reason to use this alias?
If not, I suggest just inlining scoped_ptr<blink::WebCredential> -- it is not
much longer than the alias, and one knows immediately what that type means
(unlike the alias).

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File components/password_manager/content/renderer/credential_manager_client.cc
(right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:31:
DCHECK(error != mojom::CredentialManagerError::SUCCESS);
nit: DCHECK_NE(mojom::CredentialManagerError::SUCCESS, error);

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:44:
default:
Please drop the default clause if possible to have the compiler flag the
potential need for update. Also, move NOTREACHED() just before the dummy return
on line 48.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:52: //
pointer. When the wrapper is destroied, if callbacks is still alive
typo: destroyed

Also below.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:52: //
pointer. When the wrapper is destroied, if callbacks is still alive
nit: Add || around "callbacks" to signal you mean the argument.

Also below.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:58: :
callbacks_(callbacks){};
nit: There should be a space between {}, did you run git cl format?

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:59:
~NotificationCallbacksWrapper() {
nit: blank line above

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:59:
~NotificationCallbacksWrapper() {
Please do not inline methods, the style forbids it [1].

Also below.

[1] http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:60: //
Force to call onError() for hanged callbacks.
nit: Just drop the comment, the code is clear without it.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:77: };
Did you consider DISALLOW_COPY_AND_ASSIGN for the classes defined here? Unless
you need to copy or assign between instances of the wrappers, you should use
DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:87:
~RequestCallbacksWrapper() {
nit: blank line above

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:88: //
Force to call onError() for hanged callbacks.
nit: Drop the comment.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File components/password_manager/content/renderer/credential_manager_client.h
(right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.h:42: //
For now still inherit RenderViewObserver here just to keep liftcycle with
nit: Frankly, nobody will periodically check on this TODO to see if the time is
ripe to move this to Blink. So I suggest either removing the comment completely,
or filing a bug with a detailed description of the alternative to moving to
blink, and pointing to that bug here with
// TODO(crbug.com/XXX): Consider moving this to Blink.

It would be helpful to know what are the benefits of the move and what are
particular blockers (if any). That will help us schedule that work in our
refactoring plans.

[leonhsl(Using Gerrit), 2016-03-10 06:42:42]

Hi, Vaclav, Thanks a lot for kindly review~ Uploaded patchset #3 to address all
comments, would you please take another look for this while I'm fighting for
unit_tests and browser_tests?

Hi, Ken, Anand, Sam, would you please take a look for this when you have time?
Thanks~ Especially for the change of url/origin.{h,cc}, I'm not certain it's
proper or not.

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

https://codereview.chromium.org/1762603002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:629: return;
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> The code style says: "do not handle DCHECK() failures" [1]. Please replace
lines
> 627-630 with DCHECK(web_contents).
> 
> [1]
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

Done.

https://codereview.chromium.org/1762603002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:634: //
ChromePasswordManagerClient instance should have been created already.
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> nit: You can drop the comment, the DCHECK says essentially the same thing.

Done.

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

https://codereview.chromium.org/1762603002/diff/20001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:172: // Mojo
CredentialManager impl instance,
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> nit: I'm not sure I understand the first sentence, but it seems redundant.
Line
> 178 states the type, and it is clear that this is 1:1 with the owning client
and
> lives as long as the client lives.

Done. Removed the redundant sentence.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.cc:56:
DCHECK(info.type != CredentialType::CREDENTIAL_TYPE_EMPTY);
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> nit: Use
> DCHECK_NE(CredentialType::CREDENTIAL_TYPE_EMPTY, info.type);
> instead, it has better error reporting.

Build will fail because seems DCHECK_NE can't find proper comparison impl for
'enum class CredentialType'..

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File components/password_manager/content/browser/credential_manager_impl.h
(right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.h:38: //
This class implements the Mojo interface mojom::CredentialManager.
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> optional: Drop this comment, line 40 says the same thing.

Done.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.h:69: //
Called in response to the request from the renderer, triggered by a page's
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> nit: Actually, the comments explaining the methods should be removed, they
> should only be present in the interface definition. Having a single source of
> them avoids diverging in the future.

Done. Only leave comments in the interface definition.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.h:70: //
call
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> nit: Please indent the comment block properly, do not leave unnecessary
> horizontal space. You can, e.g., put the whole comment on one line and let git
> cl format break it for you.
> 
> Also below.

Done.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.h:83: // to
'navigator.credentials.request'.
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> "request" or "get"? The mojom file says "get". The method name should match
the
> API call name.

Done. It should be 'navigator.credentials.get()'.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File
components/password_manager/content/public/interfaces/credential_manager.mojom
(right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/public/interfaces/credential_manager.mojom:40:
// The result callback will return a non-null and valid CredentialInfo if
succeeded,
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> I'm not sure about the indentation rules for .mojom, but in general lines
should
> be max. 80 characters long.
> (Also on line 44.)

Done. Keep lines length within 80 characters and mimic indentation rules from
other .mojom files.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File components/password_manager/content/public/type_converters.cc (right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/public/type_converters.cc:29: default:
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> Could you leave out the default clause completely? Without the default clause,
> the compiler will automatically flag this place as needing an update in case
> CredentialType is changed.
> 
> (Similarly on line 43 and 121.)

Done. Thanks a lot for the knowledge sharing! Will keep in mind.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/public/type_converters.cc:32: return
mojom::CredentialType::EMPTY;
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> nit: Put a NOTREACHED() between the current lines 31 and 32 instead?
> 
> (Similarly for lines 45-46.)

Done.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/public/type_converters.cc:105:
BlinkWebCredentialPtr output = nullptr;
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> nit: scoped_ptr is automatically null on creation, not need to assign nullptr.

Done.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File components/password_manager/content/public/type_converters.h (right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/public/type_converters.h:19: using
BlinkWebCredentialPtr = scoped_ptr<blink::WebCredential>;
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> Is there any special reason to use this alias?
> If not, I suggest just inlining scoped_ptr<blink::WebCredential> -- it is not
> much longer than the alias, and one knows immediately what that type means
> (unlike the alias).

Done.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File components/password_manager/content/renderer/credential_manager_client.cc
(right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:31:
DCHECK(error != mojom::CredentialManagerError::SUCCESS);
On 2016/03/09 16:26:30, vabr (Chromium) wrote:
> nit: DCHECK_NE(mojom::CredentialManagerError::SUCCESS, error);

Done. Moved the check into the switch sentence bellow.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:44:
default:
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> Please drop the default clause if possible to have the compiler flag the
> potential need for update. Also, move NOTREACHED() just before the dummy
return
> on line 48.

Done.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:52: //
pointer. When the wrapper is destroied, if callbacks is still alive
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> typo: destroyed
> 
> Also below.

Done.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:52: //
pointer. When the wrapper is destroied, if callbacks is still alive
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> typo: destroyed
> 
> Also below.

Done.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:58: :
callbacks_(callbacks){};
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> nit: There should be a space between {}, did you run git cl format?

Done. Yes I did run 'git cl format', eh, but here it's because of the extra ';'
at the end of line. Removed it.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:59:
~NotificationCallbacksWrapper() {
On 2016/03/09 16:26:30, vabr (Chromium) wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:59:
~NotificationCallbacksWrapper() {
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> Please do not inline methods, the style forbids it [1].
> 
> Also below.
> 
> [1] http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

Done.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:60: //
Force to call onError() for hanged callbacks.
On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> nit: Just drop the comment, the code is clear without it.

Done.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:77: };
On 2016/03/09 16:26:30, vabr (Chromium) wrote:
> Did you consider DISALLOW_COPY_AND_ASSIGN for the classes defined here? Unless
> you need to copy or assign between instances of the wrappers, you should use
> DISALLOW_COPY_AND_ASSIGN.

Done. Will keep in mind!

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:87:
~RequestCallbacksWrapper() {
On 2016/03/09 16:26:30, vabr (Chromium) wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:88: //
Force to call onError() for hanged callbacks.
On 2016/03/09 16:26:30, vabr (Chromium) wrote:
> nit: Drop the comment.

Done.

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File components/password_manager/content/renderer/credential_manager_client.h
(right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.h:42: //
For now still inherit RenderViewObserver here just to keep liftcycle with
On 2016/03/09 16:26:30, vabr (Chromium) wrote:
> nit: Frankly, nobody will periodically check on this TODO to see if the time
is
> ripe to move this to Blink. So I suggest either removing the comment
completely,
> or filing a bug with a detailed description of the alternative to moving to
> blink, and pointing to that bug here with
> // TODO(crbug.com/XXX): Consider moving this to Blink.
> 
> It would be helpful to know what are the benefits of the move and what are
> particular blockers (if any). That will help us schedule that work in our
> refactoring plans.

Done. Removed the comments. Actually I noticed that //content/renderer is
planned to be migrated to Blink so I considered that maybe here we can also do
the similar migration, well, this idea just came across into my mind and it
should be part of //components wholesale migration even if it's necessary. I
think currently just the mojofication work is enough here. Thanks a lot for hint
about how to proceed work and cheers~ ;-)

[vabr (Chromium), 2016-03-10 12:57:11]

Thank you, password manager production code looks good with a handful of
comments.

Cheers,
Vaclav

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.cc:56:
DCHECK(info.type != CredentialType::CREDENTIAL_TYPE_EMPTY);
On 2016/03/10 06:42:41, leonhsl wrote:
> On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> > nit: Use
> > DCHECK_NE(CredentialType::CREDENTIAL_TYPE_EMPTY, info.type);
> > instead, it has better error reporting.
> 
> Build will fail because seems DCHECK_NE can't find proper comparison impl for
> 'enum class CredentialType'..

The comparison is OK (otherwise != would not work either). The problem is
missing operator<<, I just filed http://crbug.com/593705 for that.

I won't block your CL on this, but my suggestion would be to define operator<<
for this enum class and use DCHECK_NE. You may define the oneliner operator <<
by casting to int, or you may do a human-readable switch-case printing the enum
constants as strings for bonus points. :)

https://codereview.chromium.org/1762603002/diff/40001/components/password_man...
File components/password_manager/content/renderer/credential_manager_client.cc
(right):

https://codereview.chromium.org/1762603002/diff/40001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:44: //
Should not get here.
nit: Just use NOTREACHED() here as well (in addition to the one on line 48)-- it
has the same commenting function, and allows the additional check in debug
builds.

https://codereview.chromium.org/1762603002/diff/40001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:59: :
callbacks_(callbacks) {}
Also this constructor should not be inlined. The difficulty with inlined
constructors is that it is not clear how much code they represent -- they
involve constructors of all parent classes and data members in addition to what
is between { and }. So please also just declare it here and define it below the
class definition.

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

Patchset #5 (id:80001) has been deleted

[leonhsl(Using Gerrit), 2016-03-11 09:52:26]

Uploaded patchset #5. 
Addressed comments, added unit_tests code and component_unittests PASS on my
local machine. I'm preparing the browser_tests code. Thanks~

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.cc:56:
DCHECK(info.type != CredentialType::CREDENTIAL_TYPE_EMPTY);
On 2016/03/10 12:57:11, vabr (Chromium) wrote:
> On 2016/03/10 06:42:41, leonhsl wrote:
> > On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> > > nit: Use
> > > DCHECK_NE(CredentialType::CREDENTIAL_TYPE_EMPTY, info.type);
> > > instead, it has better error reporting.
> > 
> > Build will fail because seems DCHECK_NE can't find proper comparison impl
for
> > 'enum class CredentialType'..
> 
> The comparison is OK (otherwise != would not work either). The problem is
> missing operator<<, I just filed http://crbug.com/593705 for that.
> 
> I won't block your CL on this, but my suggestion would be to define operator<<
> for this enum class and use DCHECK_NE. You may define the oneliner operator <<
> by casting to int, or you may do a human-readable switch-case printing the
enum
> constants as strings for bonus points. :)

Thank you~ Yeah it is! I'd like to get the bonus points. ;-) Done.

https://codereview.chromium.org/1762603002/diff/40001/components/password_man...
File components/password_manager/content/renderer/credential_manager_client.cc
(right):

https://codereview.chromium.org/1762603002/diff/40001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:44: //
Should not get here.
On 2016/03/10 12:57:11, vabr (Chromium) wrote:
> nit: Just use NOTREACHED() here as well (in addition to the one on line 48)--
it
> has the same commenting function, and allows the additional check in debug
> builds.

Done.

https://codereview.chromium.org/1762603002/diff/40001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:59: :
callbacks_(callbacks) {}
On 2016/03/10 12:57:11, vabr (Chromium) wrote:
> Also this constructor should not be inlined. The difficulty with inlined
> constructors is that it is not clear how much code they represent -- they
> involve constructors of all parent classes and data members in addition to
what
> is between { and }. So please also just declare it here and define it below
the
> class definition.

Done.

[leonhsl(Using Gerrit), 2016-03-13 13:28:53]

Uploaded patchset#6 to add browsertests impl, components_browsertests PASS on my
local machine.

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

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

[commit-bot: I haz the power, 2016-03-14 09:58:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/160001

[commit-bot: I haz the power, 2016-03-14 10:02:30]

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

[commit-bot: I haz the power, 2016-03-14 10:02:31]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vabr (Chromium), 2016-03-14 15:40:26]

Thank you, password manager code LGTM, with some nits and comments.

Cheers,
Vaclav

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/1762603002/diff/20001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.cc:56:
DCHECK(info.type != CredentialType::CREDENTIAL_TYPE_EMPTY);
On 2016/03/11 09:52:26, leonhsl wrote:
> On 2016/03/10 12:57:11, vabr (Chromium) wrote:
> > On 2016/03/10 06:42:41, leonhsl wrote:
> > > On 2016/03/09 16:26:29, vabr (Chromium) wrote:
> > > > nit: Use
> > > > DCHECK_NE(CredentialType::CREDENTIAL_TYPE_EMPTY, info.type);
> > > > instead, it has better error reporting.
> > > 
> > > Build will fail because seems DCHECK_NE can't find proper comparison impl
> for
> > > 'enum class CredentialType'..
> > 
> > The comparison is OK (otherwise != would not work either). The problem is
> > missing operator<<, I just filed http://crbug.com/593705 for that.
> > 
> > I won't block your CL on this, but my suggestion would be to define
operator<<
> > for this enum class and use DCHECK_NE. You may define the oneliner operator
<<
> > by casting to int, or you may do a human-readable switch-case printing the
> enum
> > constants as strings for bonus points. :)
> 
> Thank you~ Yeah it is! I'd like to get the bonus points. ;-) Done.

Thanks, I appreciate the care you put into your code!

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
File components/password_manager/content/browser/credential_manager_impl.h
(right):

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl.h:67: friend
class CredentialManagerImplTest;
What exactly is this needed for? It is usually a warning sign if the test needs
anything else than the public interface of the tested class.

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
File components/password_manager/content/renderer/DEPS (right):

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
components/password_manager/content/renderer/DEPS:9: specific_include_rules = {
Why is this restricted only to tests?
(I'm thinking  of the situation when someone needs this dependency in the
production code later, and will be unsure if it's OK to remove the test-only
restriction. Unless there is a good reason, I suggest just adding the dependency
unconditionally.)

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
File
components/password_manager/content/renderer/credential_manager_client_browsertest.cc
(right):

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
components/password_manager/content/renderer/credential_manager_client_browsertest.cc:186:
client_->dispatchStore(*(credential_.get()), callbacks.release());
nit: You should not need the .get(), scoped_ptr has operator* defined.

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
components/password_manager/content/renderer/credential_manager_client_browsertest.cc:215:
EXPECT_TRUE(credential_.get());
nit: No need for .get(), scoped_ptr is implicitly converted to a boolean value
here.

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
components/password_manager/content/renderer/credential_manager_client_browsertest.cc:243:
EXPECT_TRUE(error_ == blink::WebCredentialManagerError::
EXPECT_EQ(blink::WebCredentialManagerError::WebCredentialManagerPasswordStoreUnavailableError,
error_);

will make the log message more informative.

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
File components/password_manager/core/common/credential_manager_types.h (right):

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
components/password_manager/core/common/credential_manager_types.h:35: inline
std::ostream& operator<<(std::ostream& os, CredentialType value) {
Please do not inline this, create a .cc file and put the definition there. The
style guide is clear on this:
http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
components/password_manager/core/common/credential_manager_types.h:43: default:
nit: Move this from the default clause to below line 46. This will again use the
compiler check in case somebody extends this enum. But I agree with your code
that there should not be a NOTREACHED anywhere in this method, because it is not
useful to crash in the middle of supplying info to another DCHECK.

[leonhsl(Using Gerrit), 2016-03-16 07:02:14]

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

[commit-bot: I haz the power, 2016-03-16 07:02:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/180001

[leonhsl(Using Gerrit), 2016-03-16 07:07:28]

Hi, Vaclav, Thanks a lot for kindly approval! Uploaded patchset #9 to address
comments and fix some trybots failures, PTAL again, Thanks~ 
  # ios build still can't succeed, I'm working for this.

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
File components/password_manager/content/browser/credential_manager_impl.h
(right):

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl.h:67: friend
class CredentialManagerImplTest;
On 2016/03/14 15:40:26, vabr (Chromium) wrote:
> What exactly is this needed for? It is usually a warning sign if the test
needs
> anything else than the public interface of the tested class.

Done. Removed this line.
This was to enable CredentialManagerImplTest call
CredentialManagerImpl::Store/RequireUserMediation/Get private member functions.
But I realized that these member functions are declared as public in parent
class mojom::CredentialManager, so we can just keep them public and avoid using
the 'friend class' declaration here.

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
File components/password_manager/content/renderer/DEPS (right):

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
components/password_manager/content/renderer/DEPS:9: specific_include_rules = {
On 2016/03/14 15:40:26, vabr (Chromium) wrote:
> Why is this restricted only to tests?
> (I'm thinking  of the situation when someone needs this dependency in the
> production code later, and will be unsure if it's OK to remove the test-only
> restriction. Unless there is a good reason, I suggest just adding the
dependency
> unconditionally.)

Done.

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
File
components/password_manager/content/renderer/credential_manager_client_browsertest.cc
(right):

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
components/password_manager/content/renderer/credential_manager_client_browsertest.cc:186:
client_->dispatchStore(*(credential_.get()), callbacks.release());
On 2016/03/14 15:40:26, vabr (Chromium) wrote:
> nit: You should not need the .get(), scoped_ptr has operator* defined.

Done.

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
components/password_manager/content/renderer/credential_manager_client_browsertest.cc:215:
EXPECT_TRUE(credential_.get());
On 2016/03/14 15:40:26, vabr (Chromium) wrote:
> nit: No need for .get(), scoped_ptr is implicitly converted to a boolean value
> here.

Done and solved all codes like this. Thanks!

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
components/password_manager/content/renderer/credential_manager_client_browsertest.cc:243:
EXPECT_TRUE(error_ == blink::WebCredentialManagerError::
On 2016/03/14 15:40:26, vabr (Chromium) wrote:
>
EXPECT_EQ(blink::WebCredentialManagerError::WebCredentialManagerPasswordStoreUnavailableError,
> error_);
> 
> will make the log message more informative.

Done.

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
File components/password_manager/core/common/credential_manager_types.h (right):

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
components/password_manager/core/common/credential_manager_types.h:35: inline
std::ostream& operator<<(std::ostream& os, CredentialType value) {
On 2016/03/14 15:40:26, vabr (Chromium) wrote:
> Please do not inline this, create a .cc file and put the definition there. The
> style guide is clear on this:
> http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

Done. Moved the definition to credential_manager_types.cc. Thanks!

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
components/password_manager/core/common/credential_manager_types.h:43: default:
On 2016/03/14 15:40:26, vabr (Chromium) wrote:
> nit: Move this from the default clause to below line 46. This will again use
the
> compiler check in case somebody extends this enum. But I agree with your code
> that there should not be a NOTREACHED anywhere in this method, because it is
not
> useful to crash in the middle of supplying info to another DCHECK.

Done.

[commit-bot: I haz the power, 2016-03-16 07:07:59]

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

[commit-bot: I haz the power, 2016-03-16 07:08:00]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[vabr (Chromium), 2016-03-16 07:16:20]

Thanks for addressing the comments, still LGTM.

Let me know if you need help with the iOS compilation.

Cheers,
Vaclav

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
File components/password_manager/content/browser/credential_manager_impl.h
(right):

https://codereview.chromium.org/1762603002/diff/120001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl.h:67: friend
class CredentialManagerImplTest;
On 2016/03/16 07:07:28, leonhsl wrote:
> On 2016/03/14 15:40:26, vabr (Chromium) wrote:
> > What exactly is this needed for? It is usually a warning sign if the test
> needs
> > anything else than the public interface of the tested class.
> 
> Done. Removed this line.
> This was to enable CredentialManagerImplTest call
> CredentialManagerImpl::Store/RequireUserMediation/Get private member
functions.
> But I realized that these member functions are declared as public in parent
> class mojom::CredentialManager, so we can just keep them public and avoid
using
> the 'friend class' declaration here.

Acknowledged, SGTM.

[leonhsl(Using Gerrit), 2016-03-16 11:51:49]

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

[commit-bot: I haz the power, 2016-03-16 11:51:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/200001

[commit-bot: I haz the power, 2016-03-16 11:57:54]

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

[commit-bot: I haz the power, 2016-03-16 11:57:56]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[leonhsl(Using Gerrit), 2016-03-17 05:57:47]

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

[commit-bot: I haz the power, 2016-03-17 05:57:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/220001

[commit-bot: I haz the power, 2016-03-17 06:07:27]

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

[commit-bot: I haz the power, 2016-03-17 06:07:28]

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

[leonhsl(Using Gerrit), 2016-03-17 06:26:25]

Hi, Vaclav, Thanks a lot for kindly help~ Uploaded patchset #11 to fix iOS
trybot failure, PTAL, Thanks~

[leonhsl(Using Gerrit), 2016-03-17 06:39:39]

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

[commit-bot: I haz the power, 2016-03-17 06:39:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/240001

[commit-bot: I haz the power, 2016-03-17 06:48:38]

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

[commit-bot: I haz the power, 2016-03-17 06:48:40]

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

[leonhsl(Using Gerrit), 2016-03-17 08:31:53]

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

[commit-bot: I haz the power, 2016-03-17 08:32:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/260001

[commit-bot: I haz the power, 2016-03-17 08:34:40]

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

[commit-bot: I haz the power, 2016-03-17 08:34:41]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

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

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

[commit-bot: I haz the power, 2016-03-17 09:08:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/280001

[commit-bot: I haz the power, 2016-03-17 09:17:56]

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

[commit-bot: I haz the power, 2016-03-17 09:17:59]

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

[vabr (Chromium), 2016-03-17 09:34:38]

On 2016/03/17 06:26:25, leonhsl wrote:
> Hi, Vaclav, Thanks a lot for kindly help~ Uploaded patchset #11 to fix iOS
> trybot failure, PTAL, Thanks~

The iOS changes LGTM, thanks!
Vaclav

[leonhsl(Using Gerrit), 2016-03-17 14:16:44]

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

[commit-bot: I haz the power, 2016-03-17 14:17:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/300001

[commit-bot: I haz the power, 2016-03-17 14:26:24]

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

[commit-bot: I haz the power, 2016-03-17 14:26:26]

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

[leonhsl(Using Gerrit), 2016-03-17 16:09:44]

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

[commit-bot: I haz the power, 2016-03-17 16:10:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/320001

[leonhsl(Using Gerrit), 2016-03-17 16:19:07]

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

[leonhsl(Using Gerrit), 2016-03-17 16:19:10]

Hi, Ken, Anand, Sam, this CL is ready now except some gyp build failures which
I'm fixing, would you PTAL? Thanks~

+jam@ for OWNER review of added dependencies in
components/password_manager/content/renderer/DEPS, Thanks~

[commit-bot: I haz the power, 2016-03-17 16:20:49]

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

[commit-bot: I haz the power, 2016-03-17 16:20:51]

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

[leonhsl(Using Gerrit), 2016-03-17 16:22:18]

Hi, Ken, Anand, Sam, this CL is ready now except some gyp build failures which
I'm fixing, would you PTAL? Thanks~

+jam@ for OWNER review of added dependencies in
components/password_manager/content/renderer/DEPS, Thanks~

[Ken Rockot(use gerrit already), 2016-03-17 17:28:21]

Generally LG to me

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File components/password_manager.gypi (right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager.gypi:252: # GN version:
//components/password_manager/content/public
The GN version is //components/password_manager/content/public/interfaces

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File components/password_manager/content/browser/BUILD.gn (right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/browser/BUILD.gn:25:
"//components/password_manager/content/public",
Please also add an explicit public dependency on
//components/password_manager/content/public/interfaces

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl.cc:99: //
Send acknowledge response back.
nit: unnecessary comment

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl.cc:107:
base::Bind(&CredentialManagerImpl::ScheduleRequireMediationTask,
It looks like this callback will be passed to a background thread while work is
being done, and this callback in turn owns a copy of the Mojo response callback.
It is in general not safe to pass Mojo response callbacks to other threads
because they may be destroyed there where they're not allowed to be destroyed.

This is a subtle issue that we need to address at the bindings layer, but we
don't have a solution yet.

I think it may be acceptable to leave this as-is, if and only if
CredentialManagerImpl always lives on the main thread. Otherwise we'd need to do
some unfortunate callback tracking in this class.

I will let Sam or Anand weigh in on this opinion. :)

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File components/password_manager/content/public/BUILD.gn (right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/public/BUILD.gn:1: # Copyright 2016 The
Chromium Authors. All rights reserved.
nit: I'd prefer we stick to the common convention and move this target into
public/cpp

[Ken Rockot(use gerrit already), 2016-03-17 18:03:42]

On 2016/03/17 at 17:28:21, Ken Rockot wrote:
> Generally LG to me
> 
>
https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
> File components/password_manager.gypi (right):
> 
>
https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
> components/password_manager.gypi:252: # GN version:
//components/password_manager/content/public
> The GN version is //components/password_manager/content/public/interfaces
> 
>
https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
> File components/password_manager/content/browser/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
> components/password_manager/content/browser/BUILD.gn:25:
"//components/password_manager/content/public",
> Please also add an explicit public dependency on
//components/password_manager/content/public/interfaces
> 
>
https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
> File components/password_manager/content/browser/credential_manager_impl.cc
(right):
> 
>
https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
> components/password_manager/content/browser/credential_manager_impl.cc:99: //
Send acknowledge response back.
> nit: unnecessary comment
> 
>
https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
> components/password_manager/content/browser/credential_manager_impl.cc:107:
base::Bind(&CredentialManagerImpl::ScheduleRequireMediationTask,
> It looks like this callback will be passed to a background thread while work
is being done, and this callback in turn owns a copy of the Mojo response
callback. It is in general not safe to pass Mojo response callbacks to other
threads because they may be destroyed there where they're not allowed to be
destroyed.
> 
> This is a subtle issue that we need to address at the bindings layer, but we
don't have a solution yet.
> 
> I think it may be acceptable to leave this as-is, if and only if
CredentialManagerImpl always lives on the main thread. Otherwise we'd need to do
some unfortunate callback tracking in this class.

Actually I take that back. This doesn't make it any more safe. Unfortunately I
think we'll need to retain the pending callback on this thread somehow. Let me
give this some more thought.

> 
> I will let Sam or Anand weigh in on this opinion. :)
> 
>
https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
> File components/password_manager/content/public/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
> components/password_manager/content/public/BUILD.gn:1: # Copyright 2016 The
Chromium Authors. All rights reserved.
> nit: I'd prefer we stick to the common convention and move this target into
public/cpp

[jam, 2016-03-17 23:02:08]

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File
components/password_manager/content/renderer/credential_manager_client_browsertest.cc
(right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/renderer/credential_manager_client_browsertest.cc:88:
registry->AddServiceOverrideForTesting(
code outside of content/ can only include code in content/public.

promote this to the ServiceRegistry public interface instead

[Anand Mistry (off Chromium), 2016-03-18 01:24:13]

Mostly LG with a few small issues, and one temporary blocker (you can follow at
crbug.com/595939).

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl.cc:99: //
Send acknowledge response back.
On 2016/03/17 17:28:21, Ken Rockot wrote:
> nit: unnecessary comment

I should point out there are a few of these. You only need to comment
exceptional cases that aren't clear.

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl.cc:107:
base::Bind(&CredentialManagerImpl::ScheduleRequireMediationTask,
On 2016/03/17 17:28:21, Ken Rockot wrote:
> It looks like this callback will be passed to a background thread while work
is
> being done, and this callback in turn owns a copy of the Mojo response
callback.
> It is in general not safe to pass Mojo response callbacks to other threads
> because they may be destroyed there where they're not allowed to be destroyed.
> 
> This is a subtle issue that we need to address at the bindings layer, but we
> don't have a solution yet.
> 
> I think it may be acceptable to leave this as-is, if and only if
> CredentialManagerImpl always lives on the main thread. Otherwise we'd need to
do
> some unfortunate callback tracking in this class.
> 
> I will let Sam or Anand weigh in on this opinion. :)

It looks like this is an issue. Unfortunately, the problem is so rare in
practice that it almost never gets hit (but I have seen it and know it exists).

But, we're actively working on this problem and have solutions in mind. So
you'll need to wait a few more days.

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File components/password_manager/content/browser/credential_manager_impl.h
(right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl.h:54: 
nit: We generally don't have blank lines between functions of the same
interface.

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File
components/password_manager/content/browser/credential_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:209:

Don't introduce new whitespace unnecessarily.

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File components/password_manager/content/renderer/credential_manager_client.cc
(right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/renderer/credential_manager_client.cc:177:
base::Passed(make_scoped_ptr(
base::Owned does what you need.

[leonhsl(Using Gerrit), 2016-03-18 09:51:57]

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

[commit-bot: I haz the power, 2016-03-18 09:52:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/330045
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/330045

[commit-bot: I haz the power, 2016-03-18 10:01:47]

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

[commit-bot: I haz the power, 2016-03-18 10:01:49]

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

[leonhsl(Using Gerrit), 2016-03-18 10:02:16]

Big thanks to all for kindly review! Uploaded patchset#17 to address all
comments except the Mojo response callback thread safety issue. Would you PTAL
again? Thanks~

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File components/password_manager.gypi (right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager.gypi:252: # GN version:
//components/password_manager/content/public
On 2016/03/17 17:28:21, Ken Rockot wrote:
> The GN version is //components/password_manager/content/public/interfaces

Done.
This target contains both .mojom and typeConverters impl so I think its matching
GN version is //components/password_manager/content/public/cpp  and
//components/password_manager/content/public/interfaces?

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File components/password_manager/content/browser/BUILD.gn (right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/browser/BUILD.gn:25:
"//components/password_manager/content/public",
On 2016/03/17 17:28:21, Ken Rockot wrote:
> Please also add an explicit public dependency on
> //components/password_manager/content/public/interfaces

Done.

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl.cc:99: //
Send acknowledge response back.
On 2016/03/17 17:28:21, Ken Rockot wrote:
> nit: unnecessary comment

Done.

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl.cc:99: //
Send acknowledge response back.
On 2016/03/18 01:24:12, Anand Mistry wrote:
> On 2016/03/17 17:28:21, Ken Rockot wrote:
> > nit: unnecessary comment
> 
> I should point out there are a few of these. You only need to comment
> exceptional cases that aren't clear.

Looked through all added comments and removed unnecessary ones.

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File components/password_manager/content/browser/credential_manager_impl.h
(right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl.h:54: 
On 2016/03/18 01:24:13, Anand Mistry wrote:
> nit: We generally don't have blank lines between functions of the same
> interface.

Done.

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File
components/password_manager/content/browser/credential_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:209:

On 2016/03/18 01:24:13, Anand Mistry wrote:
> Don't introduce new whitespace unnecessarily.

Done.

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File components/password_manager/content/public/BUILD.gn (right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/public/BUILD.gn:1: # Copyright 2016 The
Chromium Authors. All rights reserved.
On 2016/03/17 17:28:21, Ken Rockot wrote:
> nit: I'd prefer we stick to the common convention and move this target into
> public/cpp

Done.

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File components/password_manager/content/renderer/credential_manager_client.cc
(right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/renderer/credential_manager_client.cc:177:
base::Passed(make_scoped_ptr(
On 2016/03/18 01:24:13, Anand Mistry wrote:
> base::Owned does what you need.

Done.

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File
components/password_manager/content/renderer/credential_manager_client_browsertest.cc
(right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/renderer/credential_manager_client_browsertest.cc:88:
registry->AddServiceOverrideForTesting(
On 2016/03/17 23:02:07, jam wrote:
> code outside of content/ can only include code in content/public.
> 
> promote this to the ServiceRegistry public interface instead

Thanks a lot! Actually I was also wondering why we did not put these into
ServiceRegistry and not sure whether there're some concern or not. 
OK, I think it's better to prepare a separate CL to land firstly to enable
others having such need start to employ them, otherwise we have to wait for this
CL landing, which has a temporary blocker right now. The new CL is
https://codereview.chromium.org/1806843005/, PTAL, Thanks~

[leonhsl(Using Gerrit), 2016-03-18 10:34:45]

About the Mojo response callback thread safety issue. Thanks~

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl.cc:107:
base::Bind(&CredentialManagerImpl::ScheduleRequireMediationTask,
On 2016/03/18 01:24:13, Anand Mistry wrote:
> On 2016/03/17 17:28:21, Ken Rockot wrote:
> > It looks like this callback will be passed to a background thread while work
> is
> > being done, and this callback in turn owns a copy of the Mojo response
> callback.
> > It is in general not safe to pass Mojo response callbacks to other threads
> > because they may be destroyed there where they're not allowed to be
destroyed.
> > 
> > This is a subtle issue that we need to address at the bindings layer, but we
> > don't have a solution yet.
> > 
> > I think it may be acceptable to leave this as-is, if and only if
> > CredentialManagerImpl always lives on the main thread. Otherwise we'd need
to
> do
> > some unfortunate callback tracking in this class.
> > 
> > I will let Sam or Anand weigh in on this opinion. :)
> 
> It looks like this is an issue. Unfortunately, the problem is so rare in
> practice that it almost never gets hit (but I have seen it and know it
exists).
> 
> But, we're actively working on this problem and have solutions in mind. So
> you'll need to wait a few more days.

Hi, Ken, Anand, I understand that Mojo is thread hostile and the Mojo response
callback should never run or be destroyed on any other thread except the binding
thread. But, I checked around all CredentialManagerImpl related codes and I
think the current CL is safe.
It is true that here the Mojo response callback is bound as a parameter to the
CredentialManagerImpl::ScheduleRequireMediationTask callback, which is passed to
background thread, but, after several passing around, eventually the
CredentialManagerImpl::ScheduleRequireMediationTask callback will be
*definitely* postback to current thread to execute, even FacetManager got
destroyed accidentally. # FacetManager::~FacetManager() will do the postback.
And, even CrendentialManagerImpl instance destroyed firstly, the callback will
still be postback to current thread, it will notice the null
CredentialManagerImpl weak pointer then the callback get destroyed, still on the
current thread, and the same to the bound parameter(our Mojo response callback).
WDYT? Thanks~

[Anand Mistry (off Chromium), 2016-03-18 14:10:17]

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl.cc:107:
base::Bind(&CredentialManagerImpl::ScheduleRequireMediationTask,
On 2016/03/18 10:34:45, leonhsl wrote:
> On 2016/03/18 01:24:13, Anand Mistry wrote:
> > On 2016/03/17 17:28:21, Ken Rockot wrote:
> > > It looks like this callback will be passed to a background thread while
work
> > is
> > > being done, and this callback in turn owns a copy of the Mojo response
> > callback.
> > > It is in general not safe to pass Mojo response callbacks to other threads
> > > because they may be destroyed there where they're not allowed to be
> destroyed.
> > > 
> > > This is a subtle issue that we need to address at the bindings layer, but
we
> > > don't have a solution yet.
> > > 
> > > I think it may be acceptable to leave this as-is, if and only if
> > > CredentialManagerImpl always lives on the main thread. Otherwise we'd need
> to
> > do
> > > some unfortunate callback tracking in this class.
> > > 
> > > I will let Sam or Anand weigh in on this opinion. :)
> > 
> > It looks like this is an issue. Unfortunately, the problem is so rare in
> > practice that it almost never gets hit (but I have seen it and know it
> exists).
> > 
> > But, we're actively working on this problem and have solutions in mind. So
> > you'll need to wait a few more days.
> 
> Hi, Ken, Anand, I understand that Mojo is thread hostile and the Mojo response
> callback should never run or be destroyed on any other thread except the
binding
> thread. But, I checked around all CredentialManagerImpl related codes and I
> think the current CL is safe.
> It is true that here the Mojo response callback is bound as a parameter to the
> CredentialManagerImpl::ScheduleRequireMediationTask callback, which is passed
to
> background thread, but, after several passing around, eventually the
> CredentialManagerImpl::ScheduleRequireMediationTask callback will be
> *definitely* postback to current thread to execute, even FacetManager got
> destroyed accidentally. # FacetManager::~FacetManager() will do the postback.
> And, even CrendentialManagerImpl instance destroyed firstly, the callback will
> still be postback to current thread, it will notice the null
> CredentialManagerImpl weak pointer then the callback get destroyed, still on
the
> current thread, and the same to the bound parameter(our Mojo response
callback).
> WDYT? Thanks~

Well... that's not entirely true. The subtle problem with callbacks is that when
you post them to a task runner, you have no guarantee of which thread holds the
last remaining reference to the callback's internal state. In 99.99999% of cases
(I made that number up), what you described is what happens. In the remaining
0.00001% of cases, the callback is posted and the thread immediately switches to
the task runner thread, the task runner runs and completes the callback and
releases its reference, then the original thread continues with its reference
and releases it, thereby destroying the callback's state and hence its bound
parameters.

[leonhsl(Using Gerrit), 2016-03-18 15:25:38]

On 2016/03/18 14:10:17, Anand Mistry wrote:
>
https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
> File components/password_manager/content/browser/credential_manager_impl.cc
> (right):
> 
>
https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
> components/password_manager/content/browser/credential_manager_impl.cc:107:
> base::Bind(&CredentialManagerImpl::ScheduleRequireMediationTask,
> On 2016/03/18 10:34:45, leonhsl wrote:
> > On 2016/03/18 01:24:13, Anand Mistry wrote:
> > > On 2016/03/17 17:28:21, Ken Rockot wrote:
> > > > It looks like this callback will be passed to a background thread while
> work
> > > is
> > > > being done, and this callback in turn owns a copy of the Mojo response
> > > callback.
> > > > It is in general not safe to pass Mojo response callbacks to other
threads
> > > > because they may be destroyed there where they're not allowed to be
> > destroyed.
> > > > 
> > > > This is a subtle issue that we need to address at the bindings layer,
but
> we
> > > > don't have a solution yet.
> > > > 
> > > > I think it may be acceptable to leave this as-is, if and only if
> > > > CredentialManagerImpl always lives on the main thread. Otherwise we'd
need
> > to
> > > do
> > > > some unfortunate callback tracking in this class.
> > > > 
> > > > I will let Sam or Anand weigh in on this opinion. :)
> > > 
> > > It looks like this is an issue. Unfortunately, the problem is so rare in
> > > practice that it almost never gets hit (but I have seen it and know it
> > exists).
> > > 
> > > But, we're actively working on this problem and have solutions in mind. So
> > > you'll need to wait a few more days.
> > 
> > Hi, Ken, Anand, I understand that Mojo is thread hostile and the Mojo
response
> > callback should never run or be destroyed on any other thread except the
> binding
> > thread. But, I checked around all CredentialManagerImpl related codes and I
> > think the current CL is safe.
> > It is true that here the Mojo response callback is bound as a parameter to
the
> > CredentialManagerImpl::ScheduleRequireMediationTask callback, which is
passed
> to
> > background thread, but, after several passing around, eventually the
> > CredentialManagerImpl::ScheduleRequireMediationTask callback will be
> > *definitely* postback to current thread to execute, even FacetManager got
> > destroyed accidentally. # FacetManager::~FacetManager() will do the
postback.
> > And, even CrendentialManagerImpl instance destroyed firstly, the callback
will
> > still be postback to current thread, it will notice the null
> > CredentialManagerImpl weak pointer then the callback get destroyed, still on
> the
> > current thread, and the same to the bound parameter(our Mojo response
> callback).
> > WDYT? Thanks~
> 
> Well... that's not entirely true. The subtle problem with callbacks is that
when
> you post them to a task runner, you have no guarantee of which thread holds
the
> last remaining reference to the callback's internal state. In 99.99999% of
cases
> (I made that number up), what you described is what happens. In the remaining
> 0.00001% of cases, the callback is posted and the thread immediately switches
to
> the task runner thread, the task runner runs and completes the callback and
> releases its reference, then the original thread continues with its reference
> and releases it, thereby destroying the callback's state and hence its bound
> parameters.

Oh, I see, this is an issue caused by subtle thread switch timing, 
it is very rare case but it is certainly possible! Thanks for kindly
explanations!
Actually this is a general problem for any callbacks who care about which thread
they will destroy.. Looking forward to the solution~

[leonhsl(Using Gerrit), 2016-03-21 09:26:36]

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

[commit-bot: I haz the power, 2016-03-21 09:26:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/370001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/370001

[commit-bot: I haz the power, 2016-03-21 09:39:24]

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

[commit-bot: I haz the power, 2016-03-21 09:39:26]

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

[leonhsl(Using Gerrit), 2016-03-21 11:14:00]

As the promoting CL https://codereview.chromium.org/1806843005/ has been landed,
uploaded patchset #20 to remove dependency on //content/common/mojo.

And, there are still 2 trybots can't build well which I'm fighting for,, would
you please give some hints about it when you have time? Thanks in advance~

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
File
components/password_manager/content/renderer/credential_manager_client_browsertest.cc
(right):

https://codereview.chromium.org/1762603002/diff/320001/components/password_ma...
components/password_manager/content/renderer/credential_manager_client_browsertest.cc:88:
registry->AddServiceOverrideForTesting(
On 2016/03/18 10:02:16, leonhsl wrote:
> On 2016/03/17 23:02:07, jam wrote:
> > code outside of content/ can only include code in content/public.
> > 
> > promote this to the ServiceRegistry public interface instead
> 
> Thanks a lot! Actually I was also wondering why we did not put these into
> ServiceRegistry and not sure whether there're some concern or not. 
> OK, I think it's better to prepare a separate CL to land firstly to enable
> others having such need start to employ them, otherwise we have to wait for
this
> CL landing, which has a temporary blocker right now. The new CL is
> https://codereview.chromium.org/1806843005/, PTAL, Thanks~

Done.

[leonhsl(Using Gerrit), 2016-03-21 11:14:20]

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

[commit-bot: I haz the power, 2016-03-21 11:14:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/390001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/390001

[commit-bot: I haz the power, 2016-03-21 11:23:48]

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

[commit-bot: I haz the power, 2016-03-21 11:23:49]

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

[vabr (Chromium), 2016-03-21 13:21:02]

On 2016/03/21 11:14:00, leonhsl wrote:
> And, there are still 2 trybots can't build well which I'm fighting for,, would
> you please give some hints about it when you have time? Thanks in advance~

Looking at the Android compilation errors, this looks like
components/password_manager/content/public/interfaces/credential_manager.mojom
causes the Origin-related bindings to be re-defined, instead of just
re-declared. I have no experience with .mojom files and the associated build
rules, so just from the top of my head I have no idea how to fix those.

Cheers,
Vaclav

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

On 2016/03/21 13:21:02, vabr (Chromium) wrote:
> On 2016/03/21 11:14:00, leonhsl wrote:
> > And, there are still 2 trybots can't build well which I'm fighting for,,
would
> > you please give some hints about it when you have time? Thanks in advance~
> 
> Looking at the Android compilation errors, this looks like
> components/password_manager/content/public/interfaces/credential_manager.mojom
> causes the Origin-related bindings to be re-defined, instead of just
> re-declared. I have no experience with .mojom files and the associated build
> rules, so just from the top of my head I have no idea how to fix those.
> 
> Cheers,
> Vaclav

Hi, Vaclav, Thanks a lot for analytics. 
Yeah the link error happens because of multiple definition of
url/mojo/origin.mojom bindings:
1,
out/Release/obj/content/gen/url/mojo/content_common_mojo_bindings.origin.mojom.o,
   created by content_common_mojo_bindings_mojom, as it uses typemap origin.
2,
out/Release/obj/components/gen/url/mojo/password_manager_content_mojo_bindings.origin.mojom.o,
   created by our password_manager_content_mojo_bindings_mojom, as we also use
the typemap origin.

I investigated around but can't find any clue to solve this,
but I think it's not correct that any one target depending typemap origin will 
create origin.mojom bindings as part of their own..
Hi, Ken, John, I noticed you have some CLs about this, WDYT? Thanks~

[leonhsl(Using Gerrit), 2016-03-25 10:26:54]

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

[commit-bot: I haz the power, 2016-03-25 10:27:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/410001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/410001

[commit-bot: I haz the power, 2016-03-25 10:38:34]

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

[commit-bot: I haz the power, 2016-03-25 10:38:36]

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

[leonhsl(Using Gerrit), 2016-03-25 14:52:20]

Hi, as crbug.com/595939 has been solved, the only blocker for this CL is the
linux_android_rel_ng trybot failure, which is caused by duplicated definition of
origin.mojom bindings generated by content_common_mojo_bindings_mojom and
password_manager_content_mojo_bindings_mojom. 
I suppose the root cause is that the behavior of gyp rule '--typemap' is
incorrect, it will always redefine the mojo bindings instead of just declaring
them, but I have no idea how to fix it.
And I think this is probably a general problem for gyp typemap bindings, would
you please give some hints? Thanks~

[Ken Rockot(use gerrit already), 2016-03-25 15:42:20]

https://codereview.chromium.org/1762603002/diff/410001/components/password_ma...
File components/password_manager.gypi (right):

https://codereview.chromium.org/1762603002/diff/410001/components/password_ma...
components/password_manager.gypi:263: 'dependencies': [
Remove this and the export_depenedent_settings value. If you have these settings
on the generation target (this one), the net result (because the GYP rules are
awful) is that anyone who depends on password_manager_content_mojo_bindings will
inherit their own dependency on the url bindings static library, hence the
multiple definitions.

https://codereview.chromium.org/1762603002/diff/410001/components/password_ma...
components/password_manager.gypi:278: 'target_name':
'password_manager_content_mojo_bindings',
Instead you want to add dependencies and export_dependent_settings in this
target, and you should use the url_mojom target here rather than
url_interfaces_mojom.

This should make the linker happy.

[leonhsl(Using Gerrit), 2016-03-28 05:07:28]

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

[commit-bot: I haz the power, 2016-03-28 05:07:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/450001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/450001

[commit-bot: I haz the power, 2016-03-28 05:23:10]

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

[commit-bot: I haz the power, 2016-03-28 05:23:12]

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

[leonhsl(Using Gerrit), 2016-03-28 06:43:56]

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

[leonhsl(Using Gerrit), 2016-03-28 06:43:57]

Sorry for my lacking of knowledge and big thanks for Ken's advice to solve the
gyp build failure! I updated both //components/password_manager.gypi and
//content/content_common_mojo_bindings.gyp at patchset #23, it works well ;-)

As all trybots got green now, would you PTAL again? Thanks~
  Ken, Anand, Sam --> for all mojo implementations.
  John            --> for //content and overall.
  Jochen          --> for //chrome.
  Tom             --> for //ipc.

[Tom Sepez, 2016-03-28 16:06:13]

RS LGTM for deleting IPC constant.

[Ken Rockot(use gerrit already), 2016-03-28 16:12:40]

lgtm

[leonhsl(Using Gerrit), 2016-03-29 02:07:18]

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

[commit-bot: I haz the power, 2016-03-29 02:07:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/450001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/450001

[leonhsl(Using Gerrit), 2016-03-29 02:26:30]

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

[commit-bot: I haz the power, 2016-03-29 02:26:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/490001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/490001

[commit-bot: I haz the power, 2016-03-29 03:40:41]

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

[commit-bot: I haz the power, 2016-03-29 03:40:43]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-03-29 20:36:01]

lgtm!

[Ken Rockot(use gerrit already), 2016-03-29 20:36:20]

On 2016/03/29 at 20:36:01, Ken Rockot wrote:
> lgtm!

doh... i already said that. :)

[vabr (Chromium), 2016-03-30 08:10:18]

Han Leon,
As heads-up: if you rebase, you will likely hit a conflict or a compilation
error due to https://codereview.chromium.org/1832933002/ which landed yesterday
and added some IPC handling in
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc.

Cheers,
Vaclav

[leonhsl(Using Gerrit), 2016-03-30 09:13:30]

On 2016/03/30 08:10:18, vabr (Chromium) wrote:
> Han Leon,
> As heads-up: if you rebase, you will likely hit a conflict or a compilation
> error due to https://codereview.chromium.org/1832933002/ which landed
yesterday
> and added some IPC handling in
>
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc.
> 
> Cheers,
> Vaclav

Hi, Vaclav,
Got it~ Thanks a lot for the information sharing!

[jochen (gone - plz use gerrit), 2016-03-30 16:36:54]

lgtm

[leonhsl(Using Gerrit), 2016-03-31 07:08:10]

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

[commit-bot: I haz the power, 2016-03-31 07:08:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/510001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/510001

[commit-bot: I haz the power, 2016-03-31 07:14:20]

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

[commit-bot: I haz the power, 2016-03-31 07:14:22]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[leonhsl(Using Gerrit), 2016-03-31 07:28:57]

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

[commit-bot: I haz the power, 2016-03-31 07:29:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/530001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/530001

[commit-bot: I haz the power, 2016-03-31 09:47:18]

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

[commit-bot: I haz the power, 2016-03-31 09:47:21]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-03-31 10:00:16]

Hi, Vaclav, rebase done and all trybots got green, would you PTAL again as the
rebase changed some more password manager codes. Thanks~

[leonhsl(Using Gerrit), 2016-03-31 10:02:06]

Hi, John, Anand, Sam, ping~ Eager to your feedback. Thanks~

John         --> for //content and overall.
Anand, Sam   --> for all mojo related impl.

[vabr (Chromium), 2016-03-31 11:43:44]

On 2016/03/31 10:00:16, leonhsl wrote:
> Hi, Vaclav, rebase done and all trybots got green, would you PTAL again as the
> rebase changed some more password manager codes. Thanks~

The rebase and your adaptation of
components/password_manager/content/browser/credential_manager_impl_unittest.cc
in particular LGTM.

Thanks,
Vaclav

[leonhsl(Using Gerrit), 2016-04-05 05:50:40]

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

[commit-bot: I haz the power, 2016-04-05 05:51:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762603002/550001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762603002/550001

[commit-bot: I haz the power, 2016-04-05 07:00:25]

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

[commit-bot: I haz the power, 2016-04-05 07:00:27]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-04-05 07:27:01]

Hi, Vaclav, would you PTAL at patch set #28? If it's OK I'd like to land this CL
soon ;-) Thanks~

patch set #28 changes:
Did rebase and changed all scoped_ptr --> std::unique_ptr

[Anand Mistry (off Chromium), 2016-04-05 07:42:33]

LGTM

I did a once-over on patchset 27 and it all looks good. Submit it!

[vabr (Chromium), 2016-04-05 11:50:26]

The diff of 28 vs. 27 seems to be just a rebase, no? In that case rubber-stamp
LGTM.

Thanks for doing this change!

Vaclav

[leonhsl(Using Gerrit), 2016-04-06 01:59:44]

On 2016/04/05 11:50:26, vabr (Chromium) wrote:
> The diff of 28 vs. 27 seems to be just a rebase, no? In that case rubber-stamp
> LGTM.
> 
> Thanks for doing this change!
> 
> Vaclav

Yes #28 vs. #27 is just a rebase. Land this CL now. Thanks a lot for so many
helps, I'll observe any issue caused by this CL possibly and continue the
autofill IPC conversion.

[leonhsl(Using Gerrit), 2016-04-06 02:00:01]

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

[leonhsl(Using Gerrit), 2016-04-06 02:00:02]

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

[commit-bot: I haz the power, 2016-04-06 02:00:27]

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

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

Committed patchset #28 (id:550001)

[commit-bot: I haz the power, 2016-04-06 02:09:44]

Description was changed from

==========
Switch components/password_manager code from IPC messages to Mojo.

Replace credential_manager_messages.h IPC to Mojo service.

BUG=582391
==========

to

==========
Switch components/password_manager code from IPC messages to Mojo.

Replace credential_manager_messages.h IPC to Mojo service.

BUG=582391

Committed: https://crrev.com/d20fb918841354a75546fa38b5307aaba117598b
Cr-Commit-Position: refs/heads/master@{#385369}
==========

[commit-bot: I haz the power, 2016-04-06 02:09:47]

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

[loyso (OOO), 2016-04-06 02:34:44]

Is this CL related to Android GN build failure?
https://build.chromium.org/p/chromium.linux/builders/Android%20GN%20%28dbg%29...

/tmp/tmpZ5Cfbk/java/org/chromium/mojom/password_manager/mojom/CredentialInfo.java:37:
error: package org.chromium.mojom.url.mojom does not exist
    public org.chromium.mojom.url.mojom.Origin federation;
                                       ^
/tmp/tmpZ5Cfbk/java/org/chromium/mojom/password_manager/mojom/CredentialInfo.java:205:
error: package org.chromium.mojom.url.mojom does not exist
            result.federation =
org.chromium.mojom.url.mojom.Origin.decode(decoder1);
                                                            ^
2 errors

[loyso (OOO), 2016-04-06 02:49:00]

A revert of this CL (patchset #28 id:550001) has been created in
https://codereview.chromium.org/1861973002/ by loyso@chromium.org.

The reason for reverting is: Breaks Android GN (dbg) build. (Which is strange)
https://build.chromium.org/p/chromium.linux/builders/Android%20GN%20%28dbg%29...

util.build_utils.CalledProcessError: Command failed: ( cd
/b/build/slave/Android_GN__dbg_/build/src/out/Debug; javac -g -encoding UTF-8
-classpath
lib.java/base/base_java.interface.jar:lib.java/mojo/public/java/bindings.interface.jar:lib.java/mojo/public/java/system.interface.jar
-sourcepath '' -bootclasspath lib.java/android.interface.jar -source 1.7 -target
1.7 -Xlint:unchecked -Xlint:deprecation -d /tmp/tmpZ5Cfbk/classes
/tmp/tmpZ5Cfbk/java/org/chromium/mojom/password_manager/mojom/CredentialManager.java
/tmp/tmpZ5Cfbk/java/org/chromium/mojom/password_manager/mojom/CredentialManager_Internal.java
/tmp/tmpZ5Cfbk/java/org/chromium/mojom/password_manager/mojom/CredentialType.java
/tmp/tmpZ5Cfbk/java/org/chromium/mojom/password_manager/mojom/CredentialManagerError.java
/tmp/tmpZ5Cfbk/java/org/chromium/mojom/password_manager/mojom/CredentialInfo.java
)
/tmp/tmpZ5Cfbk/java/org/chromium/mojom/password_manager/mojom/CredentialInfo.java:37:
error: package org.chromium.mojom.url.mojom does not exist
    public org.chromium.mojom.url.mojom.Origin federation;
                                       ^
/tmp/tmpZ5Cfbk/java/org/chromium/mojom/password_manager/mojom/CredentialInfo.java:205:
error: package org.chromium.mojom.url.mojom does not exist
            result.federation =
org.chromium.mojom.url.mojom.Origin.decode(decoder1);
                                                            ^
2 errors
.

[loyso (OOO), 2016-04-06 02:55:27]

Also:
https://build.chromium.org/p/chromium.linux/builders/Linux%20Builder%20%28dbg...