Credential Manager: Renderer-side implementation.

Credential Manager: Renderer-side implementation.

This patch does three things:

1. Implements the Blink platform interface WebCredentialManagerClient by
   adding a CredentialManagerClient class to the password manager component.

2. Wires that implementation up to Blink's platform layer by creating an
   instance of the new client, held in ChromeContentRendererClient, and
   setting it as each new RenderView's client.

3. Stubs out IPCs for the renderer to pass messages up to the browser in
   order to do the heavy lifting of actually answering Blink's requests by
   generating and delivering Credential objects.                                                                                                             

BUG=400674
TBR=tkent@chromium.org

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

[Mike West, 2014-08-13 18:46:44]

Ilya, would you mind skimming this WIP patch? Obviously, it's nowhere near ready
to land, but I'd like help evaluating the structure I'm putting together.

This is meant to be the Chromium side of the Blink platform APIs to implement
http://mikewest.github.io/credentialmanagement/spec/. I've gotten as far as
wiring up the Blink platform APIs to render->browser IPC messages. I still need
to put together the browser side listener so we can get started on some UI
experiments. Before I go much further, though, I'd appreciate a sanity check. :)

(Note that this patch depends on https://codereview.chromium.org/464783003/,
which still hasn't rolled into Chromium. *sigh*)

[Mike West, 2014-08-13 18:49:15]

(Also, I'm OOO until Monday, so no rush on this at all; I'd just like to have
some feedback in my inbox to start off next week right :) )

[Ilya Sherman, 2014-08-15 20:42:38]

Looks pretty reasonable.  Thanks :)

I realize that this is a WIP, and that a lot of the nits below you just hadn't
gotten to yet.  Hopefully at least some of them are useful :)

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
File components/password_manager/content/common/credential_manager_messages.h
(right):

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/common/credential_manager_messages.h:39:
IPC_MESSAGE_ROUTED2(CredentialManagerHostMsg_NotifySignedIn,
You'll want to document each of the messages.

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/common/credential_manager_messages.h:40: int
/* request_id */,
nit: alignment (applies throughout)

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/common/credential_manager_messages.h:41:
password_manager::CredentialManagerCredentialInfo)
nit: Please include a variable name for this one too.  (Applies throughout.)

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/common/credential_manager_messages.h:50:
IPC_MESSAGE_ROUTED3(CredentialManagerHostMsg_Request,
nit: "Request" is a little too generic -- perhaps "RequestCredentials" or
something like that?

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/common/credential_manager_messages.h:52:
bool /* zeroClickOnly */,
nit: hacker_case

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/common/credential_manager_messages.h:64:
IPC_MESSAGE_ROUTED2(CredentialManagerMsg_RequestResponse,
nit: This sounds like it is requesting a response, rather than responding to a
request.  Perhaps there's a less generic name we could use that would be clearer
as well?  Something like "SendCredentials"?

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
File components/password_manager/content/common/credential_manager_types.h
(right):

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/common/credential_manager_types.h:24: struct
CredentialManagerCredentialInfo {
nit: This name is a bit repetitive -- perhaps just "CredentialInfo" would
suffice?

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/common/credential_manager_types.h:26:
CredentialManagerCredentialInfo(base::string16 id, base::string16 name, GURL
avatarURL);
nit: Pass by const-ref.

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/common/credential_manager_types.h:38:
base::string16 data;
Hmm, I think I'd prefer to have more fields, where some can be unset, rather
than a single field that's super generic.

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
File
components/password_manager/content/renderer/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:31:
const size_t kMaxNumFederations = 100;
:)

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:91:
signed_out_callbacks_.Remove(request_id);
Seems like you have a bunch of very similar methods.  Perhaps the common parts
could be factored out into a helper function?

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
File
components/password_manager/content/renderer/credential_manager_dispatcher.h
(right):

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/renderer/credential_manager_dispatcher.h:38:
virtual void dispatchFailedSignIn(const blink::WebCredential&,
NotificationCallbacks*) OVERRIDE;
Hmm, what's a NotificationCallbacks?

[Mike West, 2014-08-19 08:37:15]

Thanks Ilya, would you mind taking a closer look at this pass?

-mike

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
File components/password_manager/content/common/credential_manager_messages.h
(right):

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/common/credential_manager_messages.h:39:
IPC_MESSAGE_ROUTED2(CredentialManagerHostMsg_NotifySignedIn,
On 2014/08/15 20:42:37, Ilya Sherman wrote:
> You'll want to document each of the messages.

Indeed!

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/common/credential_manager_messages.h:40: int
/* request_id */,
On 2014/08/15 20:42:37, Ilya Sherman wrote:
> nit: alignment (applies throughout)

Done.

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/common/credential_manager_messages.h:41:
password_manager::CredentialManagerCredentialInfo)
On 2014/08/15 20:42:37, Ilya Sherman wrote:
> nit: Please include a variable name for this one too.  (Applies throughout.)

Done.

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/common/credential_manager_messages.h:50:
IPC_MESSAGE_ROUTED3(CredentialManagerHostMsg_Request,
On 2014/08/15 20:42:37, Ilya Sherman wrote:
> nit: "Request" is a little too generic -- perhaps "RequestCredentials" or
> something like that?

Done.

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/common/credential_manager_messages.h:52:
bool /* zeroClickOnly */,
On 2014/08/15 20:42:37, Ilya Sherman wrote:
> nit: hacker_case

:(

Or ':_(', I guess.

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/common/credential_manager_messages.h:64:
IPC_MESSAGE_ROUTED2(CredentialManagerMsg_RequestResponse,
On 2014/08/15 20:42:37, Ilya Sherman wrote:
> nit: This sounds like it is requesting a response, rather than responding to a
> request.  Perhaps there's a less generic name we could use that would be
clearer
> as well?  Something like "SendCredentials"?

Done.

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
File
components/password_manager/content/renderer/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:91:
signed_out_callbacks_.Remove(request_id);
On 2014/08/15 20:42:38, Ilya Sherman wrote:
> Seems like you have a bunch of very similar methods.  Perhaps the common parts
> could be factored out into a helper function?

Extracted this out to RespondToNotificationCallback.

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
File
components/password_manager/content/renderer/credential_manager_dispatcher.h
(right):

https://codereview.chromium.org/464883002/diff/1/components/password_manager/...
components/password_manager/content/renderer/credential_manager_dispatcher.h:38:
virtual void dispatchFailedSignIn(const blink::WebCredential&,
NotificationCallbacks*) OVERRIDE;
On 2014/08/15 20:42:38, Ilya Sherman wrote:
> Hmm, what's a NotificationCallbacks?

Changed this to blink::WebCredentialManager::NotificationCallbacks for clarity,
as there's no way you could possibly have known this otherwise.

[Mike West, 2014-08-19 11:05:24]

palmer@: Do you have some time to review the IPC additions in this patch?

-mike

[Mike West, 2014-08-19 14:10:02]

jam@: Would you mind taking a look at content/public/renderer/,
content/renderer/, chrome/common, and chrome/renderer/ changes? They're fairly
small, just piping through the Blink platform API to the content layer.

vabr@: I've added you as well, in case you have nothing else to do after your
quasi-vacation. :)

[Mike West, 2014-08-19 14:10:40]

Actually adding jam@ this time: jam@: Would you mind taking a look at
content/public/renderer/, content/renderer/, chrome/common, and chrome/renderer/
changes? They're fairly small, just piping through the Blink platform API to the
content layer.

[palmer, 2014-08-19 18:28:44]

LGTM but note policy enforcement concern which you can handle in the
browser-side implementation (a later CL, I assume).

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File
components/password_manager/content/renderer/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:30:
// something reasonably sane.
Does the browser enforce this limit upon receipt? If so, it probably can't share
this declaration, which means it'll have to be duplicated, which means there is
a risk of drift...

100 seems high.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:31:
const size_t kMaxNumFederations = 100;
Could also be declared static?

[jam, 2014-08-19 19:55:31]

https://codereview.chromium.org/464883002/diff/160001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/464883002/diff/160001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:877: return
GetContentClient()->renderer()->GetCredentialManager();
the pattern we had used for the content refactoring is that interfaces which
blink needs and that are implemented by chrome are set directly by chrome. that
way we dont have to go through content just to ask chrome. as an example, see
ChromeContentRendererClient::RenderViewCreated which creates a bunch of classes.
in their constructors, tehy do something like
render_view->GetWebView()->setFooClient

[Ilya Sherman, 2014-08-19 23:55:54]

nits! (mostly)

https://codereview.chromium.org/464883002/diff/160001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/464883002/diff/160001/chrome/chrome_common.gy...
chrome/chrome_common.gypi:371:
'<(DEPTH)/components/components.gyp:password_manager_core_common',
Hmm, if iOS already doesn't use the password manager, is there any reason to
distinguish between "core" and "content"?

Or, perhaps this line doesn't need to be in the 'OS != "ios"' section?

https://codereview.chromium.org/464883002/diff/160001/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/464883002/diff/160001/chrome/renderer/BUILD.g...
chrome/renderer/BUILD.gn:42: "//components/password_manager/content/renderer",
nit: Alphabetize, plz.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File components/password_manager/content/common/credential_manager_messages.h
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/common/credential_manager_messages.h:69:
bool /* zeroclick_only */,
nit: "zeroclick_only" -> "zero_click_only"

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/common/credential_manager_messages.h:92:
IPC_MESSAGE_ROUTED2(CredentialManagerMsg_SendCredentials,
nit: You use "credentials" here and "credential" below, as the variable name. 
Let's be consistent :)

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File components/password_manager/content/common/credential_manager_types.h
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/common/credential_manager_types.h:21:
CREDENTIAL_MANAGER_CREDENTIAL_TYPE_LAST =
nit: I'd omit the "CREDENTIAL_MANAGER_" prefix for the constant names as well as
for the enum name.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/common/credential_manager_types.h:36: GURL
avatarURL;
Please document all of the member fields.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/common/credential_manager_types.h:36: GURL
avatarURL;
nit: "avatar_url" (hacker_case).  Ditto above and in the .cc file.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/common/credential_manager_types.h:42: GURL
federation;
Optional nit: This is a rare case where a union type might be useful.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/common/credential_manager_types.h:42: GURL
federation;
Please expand the documentation for these two fields.  For example, what sort of
URL is stored in the federation field?

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File components/password_manager/content/renderer/BUILD.gn (right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/BUILD.gn:22: }
Could we avoid introducing size_t to int truncations instead?

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File
components/password_manager/content/renderer/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:22:
"An unknown error occurred.");
Is there a plan to add more error types?  If not, this parameter seems pretty
redundant.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:31:
const size_t kMaxNumFederations = 100;
On 2014/08/19 18:28:44, Chromium Palmer wrote:
> Could also be declared static?

I'm pretty sure that 'static' would have no effect for this declaration, both
because it's an integer constant declaration and because it's in an anonymous
namespace.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:38:
DCHECK(callbacks);
nit: This seems redundant with the call on the line below.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:53:
ClearCallbackErrorMaps();
nit: Perhaps "ClearCallbackMapsWithErrors"?

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:77:
render_thread_->RemoveRoute(GetRoutingID());
nit: Why do you call GetRoutingID() here, rather than using routing_id_
directly, even though you use routing_id_ directly on the line above?

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:90:
OnFailedSignInResponse)
nit: Why not have the method names match the IPC names, i.e.
OnAcknowledgeFailedSignIn in this case?

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:133:
if (render_thread_)
nit: Please add curly braces, since the body spans multiple lines of text.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:163:
std::vector<GURL> federationVector;
nit: hacker_case

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:164:
for (size_t i = 0; i < federations.size() && i < kMaxNumFederations; i++)
nit: ++i

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:164:
for (size_t i = 0; i < federations.size() && i < kMaxNumFederations; i++)
Optional nit: I'd use std::min rather than having two comparisons.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File
components/password_manager/content/renderer/credential_manager_dispatcher.h
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.h:13:
#include "ipc/ipc_sender.h"
nit: Looks like this is unused.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.h:18:
#include "third_party/WebKit/public/platform/WebVector.h"
Are all of these needed in the header / can any of them be forward-declared?

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.h:38:
// When the render thread is shutdown (or the dispatcher is destructed), the
nit: "shutdown" -> "shut down"

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.h:48:
IDMapOwnPointer> NotificationCallbacksMap;
nit: Why are these public?  Can they be protected or private instead?

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.h:68:
WebCredentialManager::NotificationCallbacks*) OVERRIDE;
nit: Please include variable names, throughout.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.h:74:
bool zeroClickOnly,
nit: hacker_case.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.h:88:
RequestCallbacksMap request_callbacks_;
nit: Member variables should always be private.  If subclasses need to access
them, please provide accessors.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File
components/password_manager/content/renderer/credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher_unittest.cc:28:
testing::Test::SetUp();
Is this not empty?  It's surprising to see this called explicitly here, but not
see a corresponding call to the parent class's TearDown() method below.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher_unittest.cc:30:
dispatcher_.reset(new TestCredentialManagerDispatcher());
Why not do this in the constructor?

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher_unittest.cc:35:
render_thread_.reset();
If this happens (automatically) in the destructor, is that too late?

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher_unittest.cc:38:
TestCredentialManagerDispatcher* dispatcher() { return dispatcher_.get(); }
nit: What code is this exposed for?  |dispatcher_| is already protected.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher_unittest.cc:45:
const IPC::Message* GetMessage(uint32 messageID) {
nit: hacker_case

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher_unittest.cc:60:
EXPECT_TRUE(dispatcher()->failed_sign_in_map()->IsEmpty());
Can you avoid accessing the internal state of the class, and instead observing
some outgoing calls from the dispatcher?

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File
components/password_manager/content/renderer/test_credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/test_credential_manager_dispatcher.cc:16:
// Chosen by fair dice roll.
:)

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/test_credential_manager_dispatcher.cc:24:
request_callbacks_.Clear();
Hmm, why do you need custom logic to clear the maps in the test class?

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File
components/password_manager/content/renderer/test_credential_manager_dispatcher.h
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/test_credential_manager_dispatcher.h:29:
RequestCallbacksMap* request_credentials_map() { return &request_callbacks_; }
Can all of these accessors be const and return const maps?

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/test_credential_manager_dispatcher.h:31:
DISALLOW_COPY_AND_ASSIGN(TestCredentialManagerDispatcher);
This needs to be private, else it doesn't work ;)

[vabr (Chromium), 2014-08-20 10:57:48]

Thanks, Mike.
Just a handful of comments, since Ilya seems to have done a thorough review. If
you want me to have a detailed look as well, let me know.
What I saw LGTM.
Cheers,
Vaclav

https://codereview.chromium.org/464883002/diff/160001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/464883002/diff/160001/chrome/chrome_common.gy...
chrome/chrome_common.gypi:371:
'<(DEPTH)/components/components.gyp:password_manager_core_common',
On 2014/08/19 23:55:51, Ilya Sherman wrote:
> Hmm, if iOS already doesn't use the password manager, is there any reason to
> distinguish between "core" and "content"?
> 
> Or, perhaps this line doesn't need to be in the 'OS != "ios"' section?

Yeah, I actually don't know why all the *_core_common targets are excluded from
iOS (saying that while being responsible for this particular one). Looks like a
good idea to move the *_core_common deps up to the all-including section.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File components/password_manager.gypi (right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager.gypi:237: 'password_manager_content_renderer',
Why does the browser code depend on the renderer code? That does not seem right.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File components/password_manager/content/common/credential_manager_types.h
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/common/credential_manager_types.h:42: GURL
federation;
On 2014/08/19 23:55:51, Ilya Sherman wrote:
> Optional nit: This is a rare case where a union type might be useful.

I don't think a union might involve types other than POD.

[Mike West, 2014-08-20 13:17:20]

Thanks to all four of you.

The latest patchset hopefully addresses your concerns. Would you mind taking
another look?

jam@: In particular, note that I've restructured things in
https://codereview.chromium.org/488993003/ to avoid the platform-level //content
dependencies. I hope the implementation I ended up with is what you were
pointing me towards. :)

I'll throw this to the bots as soon as that Blink-side patch lands and rolls
into Chromium. So, in a month or so. *sigh* :)

https://codereview.chromium.org/464883002/diff/160001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/464883002/diff/160001/chrome/chrome_common.gy...
chrome/chrome_common.gypi:371:
'<(DEPTH)/components/components.gyp:password_manager_core_common',
On 2014/08/19 23:55:51, Ilya Sherman wrote:
> Hmm, if iOS already doesn't use the password manager, is there any reason to
> distinguish between "core" and "content"?
> 
> Or, perhaps this line doesn't need to be in the 'OS != "ios"' section?

1. Honestly, it's a little bit unclear how we'd go about implementing the API
I'm prototyping in iOS.

2. The message definitions can't be in 'core/common' because they rely on
'content/public/common/common_param_traits.h' for the 'IPC::ParamTraits<GURL>'
definition, and core can't depend on content. :/

https://codereview.chromium.org/464883002/diff/160001/chrome/chrome_common.gy...
chrome/chrome_common.gypi:371:
'<(DEPTH)/components/components.gyp:password_manager_core_common',
On 2014/08/20 10:57:47, vabr (Chromium) wrote:
> On 2014/08/19 23:55:51, Ilya Sherman wrote:
> > Hmm, if iOS already doesn't use the password manager, is there any reason to
> > distinguish between "core" and "content"?
> > 
> > Or, perhaps this line doesn't need to be in the 'OS != "ios"' section?
> 
> Yeah, I actually don't know why all the *_core_common targets are excluded
from
> iOS (saying that while being responsible for this particular one). Looks like
a
> good idea to move the *_core_common deps up to the all-including section.

Let's do that in a separate CL? :)

https://codereview.chromium.org/464883002/diff/160001/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/464883002/diff/160001/chrome/renderer/BUILD.g...
chrome/renderer/BUILD.gn:42: "//components/password_manager/content/renderer",
On 2014/08/19 23:55:51, Ilya Sherman wrote:
> nit: Alphabetize, plz.

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File components/password_manager.gypi (right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager.gypi:237: 'password_manager_content_renderer',
On 2014/08/20 10:57:47, vabr (Chromium) wrote:
> Why does the browser code depend on the renderer code? That does not seem
right.

It doesn't! Or, shouldn't. Dropping this.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File components/password_manager/content/common/credential_manager_messages.h
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/common/credential_manager_messages.h:69:
bool /* zeroclick_only */,
On 2014/08/19 23:55:51, Ilya Sherman wrote:
> nit: "zeroclick_only" -> "zero_click_only"

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/common/credential_manager_messages.h:92:
IPC_MESSAGE_ROUTED2(CredentialManagerMsg_SendCredentials,
On 2014/08/19 23:55:51, Ilya Sherman wrote:
> nit: You use "credentials" here and "credential" below, as the variable name. 
> Let's be consistent :)

Yeah, "credentials" is one of those words that's colloquially pluralized even
when talking about a single credential. At least, I personally find "a
credential" more awkward than "credentials". :)

Changing to "credential" regardless.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File components/password_manager/content/common/credential_manager_types.h
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/common/credential_manager_types.h:21:
CREDENTIAL_MANAGER_CREDENTIAL_TYPE_LAST =
On 2014/08/19 23:55:51, Ilya Sherman wrote:
> nit: I'd omit the "CREDENTIAL_MANAGER_" prefix for the constant names as well
as
> for the enum name.

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/common/credential_manager_types.h:36: GURL
avatarURL;
On 2014/08/19 23:55:51, Ilya Sherman wrote:
> Please document all of the member fields.

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/common/credential_manager_types.h:42: GURL
federation;
On 2014/08/19 23:55:51, Ilya Sherman wrote:
> Optional nit: This is a rare case where a union type might be useful.

Let's see what LocalCredential and FederatedCredential end up looking like once
the spec matures a bit. For the moment, a union type is just complexity I'd
prefer not to deal with. :)

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/common/credential_manager_types.h:42: GURL
federation;
On 2014/08/19 23:55:51, Ilya Sherman wrote:
> Please expand the documentation for these two fields.  For example, what sort
of
> URL is stored in the federation field?

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File components/password_manager/content/renderer/BUILD.gn (right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/BUILD.gn:22: }
On 2014/08/19 23:55:51, Ilya Sherman wrote:
> Could we avoid introducing size_t to int truncations instead?

I think I can drop this, actually. Thanks for pointing it out.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File
components/password_manager/content/renderer/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:22:
"An unknown error occurred.");
On 2014/08/19 23:55:52, Ilya Sherman wrote:
> Is there a plan to add more error types?  If not, this parameter seems pretty
> redundant.

I put it in for flexibility as the API develops; it's still pretty early.
*shrug* If we never end up using it, then I agree completely that it's overkill,
and I'll remove it.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:30:
// something reasonably sane.
On 2014/08/19 18:28:44, Chromium Palmer wrote:
> Does the browser enforce this limit upon receipt? If so, it probably can't
share
> this declaration, which means it'll have to be duplicated, which means there
is
> a risk of drift...

Makes sense, moving this to 'content/common/credential_manager_types.h'.

> 100 seems high.

How about 50?

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:38:
DCHECK(callbacks);
On 2014/08/19 23:55:53, Ilya Sherman wrote:
> nit: This seems redundant with the call on the line below.

Sure, but it's a nicer error message if it blows up. *shrug* I can drop it, I
suppose.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:53:
ClearCallbackErrorMaps();
On 2014/08/19 23:55:52, Ilya Sherman wrote:
> nit: Perhaps "ClearCallbackMapsWithErrors"?

Dropping this. Earlier, I needed to override this in the test subclass. Now I
don't need to, so I can just inline these calls.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:77:
render_thread_->RemoveRoute(GetRoutingID());
On 2014/08/19 23:55:52, Ilya Sherman wrote:
> nit: Why do you call GetRoutingID() here, rather than using routing_id_
> directly, even though you use routing_id_ directly on the line above?

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:90:
OnFailedSignInResponse)
On 2014/08/19 23:55:52, Ilya Sherman wrote:
> nit: Why not have the method names match the IPC names, i.e.
> OnAcknowledgeFailedSignIn in this case?

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:133:
if (render_thread_)
On 2014/08/19 23:55:52, Ilya Sherman wrote:
> nit: Please add curly braces, since the body spans multiple lines of text.

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:163:
std::vector<GURL> federationVector;
On 2014/08/19 23:55:52, Ilya Sherman wrote:
> nit: hacker_case

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:164:
for (size_t i = 0; i < federations.size() && i < kMaxNumFederations; i++)
On 2014/08/19 23:55:52, Ilya Sherman wrote:
> Optional nit: I'd use std::min rather than having two comparisons.

An excellent idea!

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.cc:164:
for (size_t i = 0; i < federations.size() && i < kMaxNumFederations; i++)
On 2014/08/19 23:55:52, Ilya Sherman wrote:
> nit: ++i

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File
components/password_manager/content/renderer/credential_manager_dispatcher.h
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.h:13:
#include "ipc/ipc_sender.h"
On 2014/08/19 23:55:53, Ilya Sherman wrote:
> nit: Looks like this is unused.

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.h:18:
#include "third_party/WebKit/public/platform/WebVector.h"
On 2014/08/19 23:55:53, Ilya Sherman wrote:
> Are all of these needed in the header / can any of them be forward-declared?

I can drop all except WebVector and WebCredentialManagerClient. Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.h:38:
// When the render thread is shutdown (or the dispatcher is destructed), the
On 2014/08/19 23:55:53, Ilya Sherman wrote:
> nit: "shutdown" -> "shut down"

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.h:48:
IDMapOwnPointer> NotificationCallbacksMap;
On 2014/08/19 23:55:53, Ilya Sherman wrote:
> nit: Why are these public?  Can they be protected or private instead?

They're public so I can use them in an anonymous-namespaced helper function. I
can move that onto the object, I suppose.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.h:68:
WebCredentialManager::NotificationCallbacks*) OVERRIDE;
On 2014/08/19 23:55:53, Ilya Sherman wrote:
> nit: Please include variable names, throughout.

Huh. Ok, that's repetitive, but I guess it's Chromium style. Get me back to
Blink, quickly! :)

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.h:74:
bool zeroClickOnly,
On 2014/08/19 23:55:53, Ilya Sherman wrote:
> nit: hacker_case.

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher.h:88:
RequestCallbacksMap request_callbacks_;
On 2014/08/19 23:55:53, Ilya Sherman wrote:
> nit: Member variables should always be private.  If subclasses need to access
> them, please provide accessors.

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File
components/password_manager/content/renderer/credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher_unittest.cc:28:
testing::Test::SetUp();
On 2014/08/19 23:55:53, Ilya Sherman wrote:
> Is this not empty?  It's surprising to see this called explicitly here, but
not
> see a corresponding call to the parent class's TearDown() method below.

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher_unittest.cc:30:
dispatcher_.reset(new TestCredentialManagerDispatcher());
On 2014/08/19 23:55:53, Ilya Sherman wrote:
> Why not do this in the constructor?

Why not indeed! I think this might cause some problems with the setup/teardown
of Blink, but let's see what the bots say.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher_unittest.cc:35:
render_thread_.reset();
On 2014/08/19 23:55:53, Ilya Sherman wrote:
> If this happens (automatically) in the destructor, is that too late?

Let's find out!

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher_unittest.cc:38:
TestCredentialManagerDispatcher* dispatcher() { return dispatcher_.get(); }
On 2014/08/19 23:55:53, Ilya Sherman wrote:
> nit: What code is this exposed for?  |dispatcher_| is already protected.

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher_unittest.cc:45:
const IPC::Message* GetMessage(uint32 messageID) {
On 2014/08/19 23:55:53, Ilya Sherman wrote:
> nit: hacker_case

Done.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/credential_manager_dispatcher_unittest.cc:60:
EXPECT_TRUE(dispatcher()->failed_sign_in_map()->IsEmpty());
On 2014/08/19 23:55:53, Ilya Sherman wrote:
> Can you avoid accessing the internal state of the class, and instead observing
> some outgoing calls from the dispatcher?

I've restructured this to have the class expose more state for tests. Once the
browser side is in, and things are piped through to Blink, I can verify that the
right promise is being resolved; right now, noting that a promise is queued up
is the best I can do, and seems better than doing nothing.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File
components/password_manager/content/renderer/test_credential_manager_dispatcher.h
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/test_credential_manager_dispatcher.h:29:
RequestCallbacksMap* request_credentials_map() { return &request_callbacks_; }
On 2014/08/19 23:55:54, Ilya Sherman wrote:
> Can all of these accessors be const and return const maps?

Removed entirely in favor of exposing protected methods which return bools.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/renderer/test_credential_manager_dispatcher.h:31:
DISALLOW_COPY_AND_ASSIGN(TestCredentialManagerDispatcher);
On 2014/08/19 23:55:54, Ilya Sherman wrote:
> This needs to be private, else it doesn't work ;)

Done.

https://codereview.chromium.org/464883002/diff/160001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/464883002/diff/160001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:877: return
GetContentClient()->renderer()->GetCredentialManager();
On 2014/08/19 19:55:31, jam wrote:
> the pattern we had used for the content refactoring is that interfaces which
> blink needs and that are implemented by chrome are set directly by chrome.
that
> way we dont have to go through content just to ask chrome. as an example, see
> ChromeContentRendererClient::RenderViewCreated which creates a bunch of
classes.
> in their constructors, tehy do something like
> render_view->GetWebView()->setFooClient

That keeps //content clean at the expense of making Blink a bit more complex,
which is a shame. :( *shrug* Regardless, I've uploaded
https://codereview.chromium.org/488993003/ to prepare for moving things around
in this patch.

https://codereview.chromium.org/464883002/diff/180001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/464883002/diff/180001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:878:
'../components/autofill/content/renderer/test_password_generation_agent.cc',
This looks strange, but it's just reordering the .cc and .h files as a driveby.

[vabr (Chromium), 2014-08-20 14:40:34]

Thanks, Mike.
Still LGTM, although with one more comment.
Cheers,
Vaclav

https://codereview.chromium.org/464883002/diff/160001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/464883002/diff/160001/chrome/chrome_common.gy...
chrome/chrome_common.gypi:371:
'<(DEPTH)/components/components.gyp:password_manager_core_common',
On 2014/08/20 13:17:18, Mike West wrote:
> On 2014/08/20 10:57:47, vabr (Chromium) wrote:
> > On 2014/08/19 23:55:51, Ilya Sherman wrote:
> > > Hmm, if iOS already doesn't use the password manager, is there any reason
to
> > > distinguish between "core" and "content"?
> > > 
> > > Or, perhaps this line doesn't need to be in the 'OS != "ios"' section?
> > 
> > Yeah, I actually don't know why all the *_core_common targets are excluded
> from
> > iOS (saying that while being responsible for this particular one). Looks
like
> a
> > good idea to move the *_core_common deps up to the all-including section.
> 
> Let's do that in a separate CL? :)

SGTM.

https://codereview.chromium.org/464883002/diff/180001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/464883002/diff/180001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:318: //
ChromeRenderViewTest::SetUp() creates its own CredentialManagerClient and
It would be great if the production code did not know about tests at all. Checks
like this might hide a bug later. I'm not blocking this CL on removing this test
dependency (because it's currently just a minor thing, and I don't know how to
fix it), but if there is a chance to deal with the clashing
CredentialManagerClient instances in the test rather than in the prod. code, we
should do that.

[Ilya Sherman, 2014-08-21 00:11:21]

FWIW, I kind of liked "dispatcher", though I guess "client" is good too.  The
other similar classes are both called "agent", for no particularly good reason. 
No need to make everything consistent as part of this CL, but it would be nice
to do so eventually.

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
File components/password_manager/content/common/credential_manager_messages.h
(right):

https://codereview.chromium.org/464883002/diff/160001/components/password_man...
components/password_manager/content/common/credential_manager_messages.h:92:
IPC_MESSAGE_ROUTED2(CredentialManagerMsg_SendCredentials,
On 2014/08/20 13:17:18, Mike West wrote:
> On 2014/08/19 23:55:51, Ilya Sherman wrote:
> > nit: You use "credentials" here and "credential" below, as the variable
name. 
> > Let's be consistent :)
> 
> Yeah, "credentials" is one of those words that's colloquially pluralized even
> when talking about a single credential. At least, I personally find "a
> credential" more awkward than "credentials". :)
> 
> Changing to "credential" regardless.

I don't mind which one you choose -- I just ask that things are named
consistently :)

https://codereview.chromium.org/464883002/diff/180001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/464883002/diff/180001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:527:
'../components/password_manager/content/renderer/test_credential_manager_client.h',
Hmm, why are these needed?  It looks like all of the tests that exercise the
credential manager client live in the components code...

https://codereview.chromium.org/464883002/diff/180001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/464883002/diff/180001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:318: //
ChromeRenderViewTest::SetUp() creates its own CredentialManagerClient and
On 2014/08/20 14:40:34, vabr (Chromium) wrote:
> It would be great if the production code did not know about tests at all.
Checks
> like this might hide a bug later. I'm not blocking this CL on removing this
test
> dependency (because it's currently just a minor thing, and I don't know how to
> fix it), but if there is a chance to deal with the clashing
> CredentialManagerClient instances in the test rather than in the prod. code,
we
> should do that.

+1, especially since it doesn't look like the test version of the client is
actually used in the Chrome layer.

https://codereview.chromium.org/464883002/diff/180001/components/password_man...
File components/password_manager/content/common/credential_manager_types.cc
(right):

https://codereview.chromium.org/464883002/diff/180001/components/password_man...
components/password_manager/content/common/credential_manager_types.cc:10: :
type(CREDENTIAL_TYPE_UNKNOWN) {
nit: No longer a need to wrap.

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

https://codereview.chromium.org/464883002/diff/180001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:158:
if (render_thread_)
nit: Please add curly braces, because the body of the if-stmt spans multiple
lines.  Please do this above and below as well ;)

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

https://codereview.chromium.org/464883002/diff/180001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.h:87:
bool FailedSignInCallbacksWaiting() {
Optional nit: Perhaps preppend "Has" to all of these method names?

https://codereview.chromium.org/464883002/diff/180001/components/password_man...
File
components/password_manager/content/renderer/credential_manager_client_unittest.cc
(right):

https://codereview.chromium.org/464883002/diff/180001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:21:
dispatcher_.reset(new TestCredentialManagerClient());
nit: From the constructor, you can initialize these via the initializer list. 
Even better, you could have direct instances of these objects, rather than
storing (scoped) pointers to them, in which case you wouldn't need to write any
manual initialization code.

https://codereview.chromium.org/464883002/diff/180001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:43:
scoped_ptr<TestCredentialManagerClient> dispatcher_;
nit: Please update this variable name.

https://codereview.chromium.org/464883002/diff/180001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:59:
EXPECT_TRUE(dispatcher_->FailedSignInCallbacksWaiting());
It seems like you could test this more directly by sending the expected IPC
message, and verifying that the callbacks get called.  That would be preferable,
IMO, as it tests the public API of your class.

[Mike West, 2014-08-21 10:20:47]

Thanks for your comments, Ilya. I've removed the injection of the
TestCredentialManagerClient in favor of null-checking the client before asking
it to register itself with a new RenderView. This diverges from the
ExtensionDelegate and Spellcheck objects in the same layer, but until we
actually need to inject the test client this early, there's no point in
complicating the implementation...

The latest iteration also improves the tests as you've suggested.

WDYT?

tkent@: Would you mind stamping the DEPS additions? The component needs to
depend on Blink's public interfaces in order to implement the WebCredential*
bits and pieces.

https://codereview.chromium.org/464883002/diff/180001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/464883002/diff/180001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:527:
'../components/password_manager/content/renderer/test_credential_manager_client.h',
On 2014/08/21 00:11:21, Ilya Sherman wrote:
> Hmm, why are these needed?  It looks like all of the tests that exercise the
> credential manager client live in the components code...

These were added to support the TestCredentialManagerClient injection in
ChromeRenderViewTest. There are a few tests that instantiate a RenderView
without instantiating a RenderThread, which means that '::RenderThreadCreated'
is never called, and therefore the client is never created. More on that when we
get to those comments.

https://codereview.chromium.org/464883002/diff/180001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/464883002/diff/180001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:318: //
ChromeRenderViewTest::SetUp() creates its own CredentialManagerClient and
On 2014/08/21 00:11:21, Ilya Sherman wrote:
> On 2014/08/20 14:40:34, vabr (Chromium) wrote:
> > It would be great if the production code did not know about tests at all.
> Checks
> > like this might hide a bug later. I'm not blocking this CL on removing this
> test
> > dependency (because it's currently just a minor thing, and I don't know how
to
> > fix it), but if there is a chance to deal with the clashing
> > CredentialManagerClient instances in the test rather than in the prod. code,
> we
> > should do that.
> 
> +1, especially since it doesn't look like the test version of the client is
> actually used in the Chrome layer.

This isn't something I invented; I lifted this model from both Spellcheck and
ExtensionDelegate's implementation (lines 290 and 300 above in the same method).
Autofill does something similar in ChromeRenderViewTest, but it's sneakier since
it's implemented as a RenderViewObserver.

I'll try to rearrange things a bit, but there are a number of browser tests that
don't set things up in a way that CredentialManager appreciates. I'm looking for
the least-bad way of enabling them without teaching unrelated tests about things
they shouldn't need to care about.

https://codereview.chromium.org/464883002/diff/180001/components/password_man...
File components/password_manager/content/common/credential_manager_types.cc
(right):

https://codereview.chromium.org/464883002/diff/180001/components/password_man...
components/password_manager/content/common/credential_manager_types.cc:10: :
type(CREDENTIAL_TYPE_UNKNOWN) {
On 2014/08/21 00:11:21, Ilya Sherman wrote:
> nit: No longer a need to wrap.

Done.

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

https://codereview.chromium.org/464883002/diff/180001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:158:
if (render_thread_)
On 2014/08/21 00:11:21, Ilya Sherman wrote:
> nit: Please add curly braces, because the body of the if-stmt spans multiple
> lines.  Please do this above and below as well ;)

Done.

https://codereview.chromium.org/464883002/diff/180001/components/password_man...
File
components/password_manager/content/renderer/credential_manager_client_unittest.cc
(right):

https://codereview.chromium.org/464883002/diff/180001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:43:
scoped_ptr<TestCredentialManagerClient> dispatcher_;
On 2014/08/21 00:11:21, Ilya Sherman wrote:
> nit: Please update this variable name.

Done.

https://codereview.chromium.org/464883002/diff/180001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:59:
EXPECT_TRUE(dispatcher_->FailedSignInCallbacksWaiting());
On 2014/08/21 00:11:21, Ilya Sherman wrote:
> It seems like you could test this more directly by sending the expected IPC
> message, and verifying that the callbacks get called.  That would be
preferable,
> IMO, as it tests the public API of your class.

I'll try to put something like that together.

[jam, 2014-08-21 17:29:32]

lgtm

Ideally the blink side would be per WebFrame instead of WebView. As part of the
site isolation project, before working on the CY I had been moving stuff that
hangs off WebView to WebFrame. So if you introduce something new off WebView,
it'll have to be reworked to be off WebFrame instead.

Can you switch this? It doesn't have to be in this cl, but the sooner the
better. Thanks

https://codereview.chromium.org/464883002/diff/240001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/464883002/diff/240001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.h:212:
scoped_ptr<password_manager::CredentialManagerClient>
nit: put before ifdefs

[Ilya Sherman, 2014-08-21 17:58:16]

Thanks, Mike!  LGTM % a few remaining nits (and one request for a change to the
Blink<->Chromium API interface, though I'm not sure if it's workable).

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

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:36: ,
render_thread_(content::RenderThread::Get()) {
nit: In Chromium style, the comma goes on the previous line.

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
File
components/password_manager/content/renderer/credential_manager_client_unittest.cc
(right):

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:25:
callback_succeeded_ = false;
nit: Perhaps initialize these in the constructor's initializer list?

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:48:
case CredentialManagerHostMsg_NotifyFailedSignIn::ID: {
nit: The case stmts should be indented by two spaces, and their innards by
another two.

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:93:
bool callback_errored_;
nit: Docs, plz

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:95:
DISALLOW_COPY_AND_ASSIGN(CredentialManagerClientTest);
nit: DISALLOW_COPY_AND_ASSIGN is often omitted for test fixture classes; but if
you keep it, it should have private visibility.

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:101:
TestNotificationCallbacks(CredentialManagerClientTest* test)
nit: explicit

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:123:
TestRequestCallbacks(CredentialManagerClientTest* test) : test_(test) { }
nit: explicit

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:139:
};
nit: Please tuck these into an anonymous namespace at the top of the file, and
update the style to use Chromium style rather than Blink style ;)

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:150:
client_.dispatchFailedSignIn(credential, callbacks.release());
Oof, I'm just noticing that the Blink API has the client take ownership of raw
pointers that are passed to it.  Is there any way to pass a scoped_ptr or some
other type of smart pointer instead?  I'm always a little nervous about APIs
that expect some code to take ownership of memory without enforcing that
behavior.

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:212:
EXPECT_FALSE(callback_errored());
Can you also add test coverage for where callback_errored() would be false?

[Mike West, 2014-08-22 09:03:16]

jam: Thanks! I'll move the code to WebFrame in a subsequent patch. I'd like to
get this one in to unblock other folks who want to start experimenting with the
UI that this feature will eventually require.

ilya: Thanks for the helpful review comments!

https://codereview.chromium.org/464883002/diff/240001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/464883002/diff/240001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.h:212:
scoped_ptr<password_manager::CredentialManagerClient>
On 2014/08/21 17:29:32, jam wrote:
> nit: put before ifdefs

Done.

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

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client.cc:36: ,
render_thread_(content::RenderThread::Get()) {
On 2014/08/21 17:58:15, Ilya Sherman wrote:
> nit: In Chromium style, the comma goes on the previous line.

Done.

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
File
components/password_manager/content/renderer/credential_manager_client_unittest.cc
(right):

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:25:
callback_succeeded_ = false;
On 2014/08/21 17:58:15, Ilya Sherman wrote:
> nit: Perhaps initialize these in the constructor's initializer list?

Done.

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:48:
case CredentialManagerHostMsg_NotifyFailedSignIn::ID: {
On 2014/08/21 17:58:15, Ilya Sherman wrote:
> nit: The case stmts should be indented by two spaces, and their innards by
> another two.

Done.

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:93:
bool callback_errored_;
On 2014/08/21 17:58:15, Ilya Sherman wrote:
> nit: Docs, plz

Done.

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:95:
DISALLOW_COPY_AND_ASSIGN(CredentialManagerClientTest);
On 2014/08/21 17:58:15, Ilya Sherman wrote:
> nit: DISALLOW_COPY_AND_ASSIGN is often omitted for test fixture classes; but
if
> you keep it, it should have private visibility.

Done.

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:101:
TestNotificationCallbacks(CredentialManagerClientTest* test)
On 2014/08/21 17:58:15, Ilya Sherman wrote:
> nit: explicit

Done.

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:123:
TestRequestCallbacks(CredentialManagerClientTest* test) : test_(test) { }
On 2014/08/21 17:58:15, Ilya Sherman wrote:
> nit: explicit

Done.

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:139:
};
On 2014/08/21 17:58:15, Ilya Sherman wrote:
> nit: Please tuck these into an anonymous namespace at the top of the file, and
> update the style to use Chromium style rather than Blink style ;)

Done.

https://codereview.chromium.org/464883002/diff/240001/components/password_man...
components/password_manager/content/renderer/credential_manager_client_unittest.cc:150:
client_.dispatchFailedSignIn(credential, callbacks.release());
On 2014/08/21 17:58:15, Ilya Sherman wrote:
> Oof, I'm just noticing that the Blink API has the client take ownership of raw
> pointers that are passed to it.  Is there any way to pass a scoped_ptr or some
> other type of smart pointer instead?  I'm always a little nervous about APIs
> that expect some code to take ownership of memory without enforcing that
> behavior.

For better or worse, this is the WebCallbacks interface. It's how ServiceWorkers
and PushMessaging work, and given that it's the pattern that's in use, I'm loath
to change it here.

[Mike West, 2014-08-22 09:03:51]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-08-22 09:04:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkwst@chromium.org/464883002/260001

[Mike West, 2014-08-22 09:43:57]

The CQ bit was unchecked by mkwst@chromium.org

[Mike West, 2014-08-22 09:45:12]

TBRing tkent for the DEPS additions of 'third_party/WebKit/public/platform',
'third_party/WebKit/public/web'. Since he approved the Blink-side additions, I
hope that it's not too presumptuous to assume he'll approve the use of those new
interfaces. :)

[Mike West, 2014-08-22 09:45:22]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-08-22 09:45:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkwst@chromium.org/464883002/260001

[commit-bot: I haz the power, 2014-08-22 09:59:03]

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

[commit-bot: I haz the power, 2014-08-22 10:52:27]

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

[commit-bot: I haz the power, 2014-08-22 10:52:29]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Mike West, 2014-08-22 10:57:10]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-08-22 10:57:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkwst@chromium.org/464883002/260001

[commit-bot: I haz the power, 2014-08-22 12:01:32]

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

[commit-bot: I haz the power, 2014-08-22 12:56:53]

Committed patchset #7 (260001) as 291390

[tkent, 2014-08-24 23:47:51]

On 2014/08/22 09:45:12, Mike West (OOO until Sept 1) wrote:
> TBRing tkent for the DEPS additions of 'third_party/WebKit/public/platform',
> 'third_party/WebKit/public/web'. Since he approved the Blink-side additions, I
> hope that it's not too presumptuous to assume he'll approve the use of those
new
> interfaces. :)

lgtm though I don't own the DEPS file.