Move JS-related password manager code upstream

Move JS-related password manager code upstream

Moves
credential_manager.h
credential_manager.mm
js_credential_manager.h
js_credential_manager.mm
js_password_manager.h
js_password_manager.mm
resources/credential_manager.js
resources/password_controller.js
from the downstream to ios/chrome/browser/passwords.

This is the upstream part of the internal CL 310987013.

BUG=514241
Committed: https://crrev.com/b0a8964d59e03ab82709c85dd4f201cd7d44d3ba
Cr-Commit-Position: refs/heads/master@{#360617}

[vabr (Chromium), 2015-11-18 15:54:41]

vabr@chromium.org changed reviewers:
+ eugenebut@chromium.org

[vabr (Chromium), 2015-11-18 15:54:41]

Hi Eugene!

I'd like your expertise in answering the question below.

Thanks!

Vaclav

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/DEPS
File ios/chrome/browser/DEPS (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/DEPS#new...
ios/chrome/browser/DEPS:73: "+ios/web/web_state/js/credential_util.h",
I expect this is not the right way to do this.
What would be? Could credential_util.h be moved to ios/web/public? Or something
else?

[vabr (Chromium), 2015-11-18 15:55:40]

vabr@chromium.org changed reviewers:
+ droger@chromium.org

[vabr (Chromium), 2015-11-18 15:55:40]

David,

Could you ask you for the complete review and OWNERS approval? Feel free to wait
until Eugene has a chance to answer the DEPS question.

Thanks!
Vaclav

[droger, 2015-11-18 16:24:17]

LGTM

This is mostly a rubberstamp, left a few style nits.
Please wait for eugene's answer to your question before committing.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/credential_manager.h (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.h:25: namespace ios {
We don't use the ios:: namespace in //ios.

We also may not need the passwords namespace either, although this could be
argued either way.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.h:90: CredentialManager&
operator=(const CredentialManager&) = delete;
Could this be replaced by the
DISALLOW_COPY_AND_ASSIGN macro?

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/credential_manager.mm (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:41: default:
Remove the default case, this way there will be a compile error if somebody adds
a new value.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:69: default:
Same here.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:100: // Ensure the JavaScript
is loaded when the page finishes loading.
Move this comment to the .h or inside the function.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:117: // SendCredential.
Move this comment to the .h or inside the function.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:174: // page to resolve the
Promise associated with |request_id|.
Move this comment to the .h or inside the function.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:215: // disabled for the
current page origin.
Move this comment to the .h or inside the function.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:255: // When the WebState is
destroyed, clean up everything that depends on it.
Move this comment to the .h or inside the function.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:261: // when the user isn't
in incognito mode.
Move this comment to the .h or inside the function.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:276: // resolving the
JavaScript Promise associated with |request_id|.
Move this comment to the .h or inside the function.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:306: // in the password
manager.
Move this comment to the .h or inside the function.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:322: // that should have
zero-click sign-in disabled.
Move this comment to the .h or inside the function.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:348: default:
Remove the default.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_credential_manager.h (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_credential_manager.h:15: namespace ios {
Remove the ios:: namespace

[vabr (Chromium), 2015-11-18 16:52:28]

Thanks for the review!

(Just to explain -- I try on purpose not to mix moving files with other changes,
to keep reviewing simple. But of course happy to address the issues if you don't
mind having them mixed as a reviewer.)

I confirm that I will wait for Eugene's advice on the DEPS change.

Cheers,
Vaclav

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/credential_manager.h (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.h:25: namespace ios {
On 2015/11/18 16:24:16, droger wrote:
> We don't use the ios:: namespace in //ios.
> 
> We also may not need the passwords namespace either, although this could be
> argued either way.

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.h:90: CredentialManager&
operator=(const CredentialManager&) = delete;
On 2015/11/18 16:24:16, droger wrote:
> Could this be replaced by the
> DISALLOW_COPY_AND_ASSIGN macro?

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/credential_manager.mm (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:41: default:
On 2015/11/18 16:24:17, droger wrote:
> Remove the default case, this way there will be a compile error if somebody
adds
> a new value.

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:69: default:
On 2015/11/18 16:24:17, droger wrote:
> Same here.

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:100: // Ensure the JavaScript
is loaded when the page finishes loading.
On 2015/11/18 16:24:16, droger wrote:
> Move this comment to the .h or inside the function.

Done. Moved all inside the functions, because the methods are overloaded, and
the "public" description is already at the original declaration.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:117: // SendCredential.
On 2015/11/18 16:24:16, droger wrote:
> Move this comment to the .h or inside the function.

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:174: // page to resolve the
Promise associated with |request_id|.
On 2015/11/18 16:24:16, droger wrote:
> Move this comment to the .h or inside the function.

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:215: // disabled for the
current page origin.
On 2015/11/18 16:24:16, droger wrote:
> Move this comment to the .h or inside the function.

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:255: // When the WebState is
destroyed, clean up everything that depends on it.
On 2015/11/18 16:24:16, droger wrote:
> Move this comment to the .h or inside the function.

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:261: // when the user isn't
in incognito mode.
On 2015/11/18 16:24:16, droger wrote:
> Move this comment to the .h or inside the function.

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:276: // resolving the
JavaScript Promise associated with |request_id|.
On 2015/11/18 16:24:16, droger wrote:
> Move this comment to the .h or inside the function.

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:306: // in the password
manager.
On 2015/11/18 16:24:16, droger wrote:
> Move this comment to the .h or inside the function.

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:322: // that should have
zero-click sign-in disabled.
On 2015/11/18 16:24:16, droger wrote:
> Move this comment to the .h or inside the function.

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:348: default:
On 2015/11/18 16:24:16, droger wrote:
> Remove the default.

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_credential_manager.h (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_credential_manager.h:15: namespace ios {
On 2015/11/18 16:24:17, droger wrote:
> Remove the ios:: namespace

Done.

[droger, 2015-11-18 16:54:48]

Thanks for the changes.

[vabr (Chromium), 2015-11-18 16:55:13]

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_credential_manager.h (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_credential_manager.h:16: namespace passwords {
Hm, I see you only advised against the 'ios' here, and I believe I should
actually keep the constants in a namespace, because their names are fairly
generic. Will fix that in a moment.

[Eugene But (OOO till 7-30), 2015-11-18 17:06:48]

lgtm, I commented only deviations from our Style Guides. The last 3 comments are
optional, so please feel free to leave the code as it is.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/DEPS
File ios/chrome/browser/DEPS (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/DEPS#new...
ios/chrome/browser/DEPS:73: "+ios/web/web_state/js/credential_util.h",
On 2015/11/18 15:54:41, vabr (Chromium) wrote:
> I expect this is not the right way to do this.
> What would be? Could credential_util.h be moved to ios/web/public? Or
something
> else?
You are right. Correct thing would be moving this file to
ios/web/public/web_state/js

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/credential_manager.h (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.h:11: #include
"base/mac/scoped_nsobject.h"
s/include/import because scoped_nsobject.h is an Objective-C header and should
be imported, not included.

https://google.github.io/styleguide/objcguide.xml#_import_and__include

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.h:105: bool
GetURLWithAbsoluteTrust(GURL* page_url);
According to C++ Style guide this should be GetUrlWithAbsoluteTrust:
https://google.github.io/styleguide/cppguide.html#Function_Names

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/credential_manager.mm (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:8: #include
"base/ios/weak_nsobject.h"
s/include/import

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:17: #include
"ios/web/public/url_scheme_util.h"
s/include/import

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:100: // Ensure the JavaScript
is loaded when the page finishes loading.
NIT: If you choose to move this comment to .h, then please s/Ensure/Ensures,
since method comments should be descriptive rather than imperative.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_credential_manager.mm (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_credential_manager.mm:12: #import
"ios/web/web_state/js/credential_util.h"
s/import/include

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_password_manager.h (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.h:38: completionHandler:(void
(^)(NSString*))completionHandler;
Indent 4 spaces:

https://google.github.io/styleguide/objcguide.xml?showone=Method_Declarations...

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.h:40: // Fills in the password
form specified by |jsonString| with the given
s/jsonString/JSONString

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.h:46: -
(void)fillPasswordForm:(NSString*)jsonString
s/jsonString/JSONString

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_password_manager.mm (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.mm:14: NSString*
JSONEscape(NSString* str) {
NIT: s/str/JSON here and in the comment

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.mm:40: completionHandler:(void
(^)(NSString*))completionHandler {
Indent 4 spaces

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.mm:100: NSString* js = [[self
injectionContent] stringByAppendingString:script];
s/js/JS

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/resources/password_controller.js (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/resources/password_controller.js:66: for (var i =
0; i < forms.length; i++) {
Optional NIT: the body of this for-loop is large, so I would replace i with
FormIndex name

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/resources/password_controller.js:79: for (var j =
0; j < fields.length; j++) {
Optional NIT: s/j/fieldIndex

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/resources/password_controller.js:87: for (var k =
0; k < inputs.length; k++) {
Optional NIT: s/k/inputElementIndex

[droger, 2015-11-18 17:07:41]

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_credential_manager.h (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_credential_manager.h:16: namespace passwords {
On 2015/11/18 16:55:13, vabr (Chromium) wrote:
> Hm, I see you only advised against the 'ios' here, and I believe I should
> actually keep the constants in a namespace, because their names are fairly
> generic. Will fix that in a moment.

Some people argue that we should not use any namespace in //chrome or
//ios/chrome.
See for example discussions like:
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/namesp...

I don't have a really strong opinion.

[droger, 2015-11-18 17:11:27]

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_credential_manager.h (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_credential_manager.h:16: namespace passwords {
On 2015/11/18 17:07:40, droger wrote:
> On 2015/11/18 16:55:13, vabr (Chromium) wrote:
> > Hm, I see you only advised against the 'ios' here, and I believe I should
> > actually keep the constants in a namespace, because their names are fairly
> > generic. Will fix that in a moment.
> 
> Some people argue that we should not use any namespace in //chrome or
> //ios/chrome.
> See for example discussions like:
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/namesp...
> 
> I don't have a really strong opinion.

This discussion is also related:
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/namesp...

[vabr (Chromium), 2015-11-18 17:14:07]

Quick response to David below.
Will address the comments from Eugene soon.

Thanks both for your quick and helpful review!

Vaclav

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_credential_manager.h (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_credential_manager.h:16: namespace passwords {
On 2015/11/18 17:07:40, droger wrote:
> On 2015/11/18 16:55:13, vabr (Chromium) wrote:
> > Hm, I see you only advised against the 'ios' here, and I believe I should
> > actually keep the constants in a namespace, because their names are fairly
> > generic. Will fix that in a moment.
> 
> Some people argue that we should not use any namespace in //chrome or
> //ios/chrome.
> See for example discussions like:
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/namesp...
> 
> I don't have a really strong opinion.

As an alternative I can just prefix all the names with Credentials, as in
kCredentialsPendingRequestErrorType, that should be specific enough. Will go
with that.

[vabr (Chromium), 2015-11-18 17:21:53]

On 2015/11/18 17:14:07, vabr (Chromium) wrote:
> Quick response to David below.
> Will address the comments from Eugene soon.

All right, soon = tomorrow :).

Next time I upstream something, I will fix it downstream first to avoid this.
The code we have in ios_internal/chrome/browser/passwords seems to have been
written in a hurry.

Cheers,
Vaclav

[vabr (Chromium), 2015-11-19 10:02:28]

Thanks for your reviews so far!

Eugene, PTAL, in particular at:
  * my push-back against variable naming (search for jsonString below)
  * the issue with formatting in js_password_manager.mm
  * the move of ios/web/web_state/js/credential_util.h to public

David, if you want, check whether you like the prefix-instead-namespace for
constants defined in js_credential_manager.h.

Thanks both for your time!
Vacalv

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/DEPS
File ios/chrome/browser/DEPS (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/DEPS#new...
ios/chrome/browser/DEPS:73: "+ios/web/web_state/js/credential_util.h",
On 2015/11/18 17:06:48, eugenebut wrote:
> On 2015/11/18 15:54:41, vabr (Chromium) wrote:
> > I expect this is not the right way to do this.
> > What would be? Could credential_util.h be moved to ios/web/public? Or
> something
> > else?
> You are right. Correct thing would be moving this file to
> ios/web/public/web_state/js

Thanks for confirmation, done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/credential_manager.h (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.h:11: #include
"base/mac/scoped_nsobject.h"
On 2015/11/18 17:06:48, eugenebut wrote:
> s/include/import because scoped_nsobject.h is an Objective-C header and should
> be imported, not included.
> 
> https://google.github.io/styleguide/objcguide.xml#_import_and__include

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.h:105: bool
GetURLWithAbsoluteTrust(GURL* page_url);
On 2015/11/18 17:06:48, eugenebut wrote:
> According to C++ Style guide this should be GetUrlWithAbsoluteTrust:
> https://google.github.io/styleguide/cppguide.html#Function_Names

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/credential_manager.mm (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:8: #include
"base/ios/weak_nsobject.h"
On 2015/11/18 17:06:48, eugenebut wrote:
> s/include/import

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:17: #include
"ios/web/public/url_scheme_util.h"
On 2015/11/18 17:06:48, eugenebut wrote:
> s/include/import

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/credential_manager.mm:100: // Ensure the JavaScript
is loaded when the page finishes loading.
On 2015/11/18 17:06:48, eugenebut wrote:
> NIT: If you choose to move this comment to .h, then please s/Ensure/Ensures,
> since method comments should be descriptive rather than imperative.

Acknowledged.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_credential_manager.mm (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_credential_manager.mm:12: #import
"ios/web/web_state/js/credential_util.h"
On 2015/11/18 17:06:48, eugenebut wrote:
> s/import/include

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_password_manager.h (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.h:38: completionHandler:(void
(^)(NSString*))completionHandler;
On 2015/11/18 17:06:48, eugenebut wrote:
> Indent 4 spaces:
> 
>
https://google.github.io/styleguide/objcguide.xml?showone=Method_Declarations...

Done + filed internal bug 25783407 for clang-format (which caused the current
indenting).

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.h:40: // Fills in the password
form specified by |jsonString| with the given
On 2015/11/18 17:06:48, eugenebut wrote:
> s/jsonString/JSONString

Pushing back:

How am I supposed to deduce this from
https://google.github.io/styleguide/objcguide.xml?showone=Method_Declarations...
or http://google.github.io/styleguide/cppguide.html#Variable_Names ?
ios/web/test/web_test.mm also uses jsonString.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.h:46: -
(void)fillPasswordForm:(NSString*)jsonString
On 2015/11/18 17:06:48, eugenebut wrote:
> s/jsonString/JSONString

ditto

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_password_manager.mm (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.mm:14: NSString*
JSONEscape(NSString* str) {
On 2015/11/18 17:06:48, eugenebut wrote:
> NIT: s/str/JSON here and in the comment

Also here, I do not understand why the capitalisation.
My suggestion would be jsonString as in
ios/chrome/browser/passwords/js_password_manager.h.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.mm:40: completionHandler:(void
(^)(NSString*))completionHandler {
On 2015/11/18 17:06:48, eugenebut wrote:
> Indent 4 spaces

Cannot do, it conflicts with git cl format, and the presubmit says:
"The chrome directory requires source formatting. Please run git cl format
ios/chrome"

The internal bug for that is 25783407.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.mm:100: NSString* js = [[self
injectionContent] stringByAppendingString:script];
On 2015/11/18 17:06:48, eugenebut wrote:
> s/js/JS

Same push-back as for jsonString in
ios/chrome/browser/passwords/js_password_manager.h.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/resources/password_controller.js (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/resources/password_controller.js:66: for (var i =
0; i < forms.length; i++) {
On 2015/11/18 17:06:48, eugenebut wrote:
> Optional NIT: the body of this for-loop is large, so I would replace i with
> FormIndex name

Sorry, I don't understand. Do you mean to rename i -> formIndex, or do you mean
something else?
Also, while the loop is large, |i| is only used on its first line, where it is
"transformed" into |form|.
What exactly is the concern here? Maybe I don't understand that.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/resources/password_controller.js:79: for (var j =
0; j < fields.length; j++) {
On 2015/11/18 17:06:48, eugenebut wrote:
> Optional NIT: s/j/fieldIndex

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/resources/password_controller.js:87: for (var k =
0; k < inputs.length; k++) {
On 2015/11/18 17:06:48, eugenebut wrote:
> Optional NIT: s/k/inputElementIndex

I considered this, but because |input| instead of |k| is used in the loop body,
I went with a shorter statement on line 87.

[droger, 2015-11-19 12:32:19]

The naming looks good. Still LGTM.

[Eugene But (OOO till 7-30), 2015-11-19 15:13:21]

lgtm modulo fixing acronyms and formatting. Hope my explanations make sense :)

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_password_manager.h (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.h:40: // Fills in the password
form specified by |jsonString| with the given
On 2015/11/19 10:02:27, vabr (Chromium) wrote:
> On 2015/11/18 17:06:48, eugenebut wrote:
> > s/jsonString/JSONString
> 
> Pushing back:
> 
> How am I supposed to deduce this from
>
https://google.github.io/styleguide/objcguide.xml?showone=Method_Declarations...
> or http://google.github.io/styleguide/cppguide.html#Variable_Names ?
> ios/web/test/web_test.mm also uses jsonString.
This is an Objective-C method and you suppose to follow Objective-C style guide
here:

Any class, category, method, or variable name may use all capitals for
initialisms within the name. This follows Apple's standard of using all capitals
within a name for initialisms such as URL, TIFF, and EXIF.

See Naming section below this link:
https://engdoc.corp.google.com/eng/doc/objcguide.xml?showone=Exceptions&cl=he...

Unfortunately our codebase has multiple examples where acronyms are not
capitalized, but those examples are incorrect. Please fix, as current code is
deviation from Objective-C style guide.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_password_manager.mm (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.mm:40: completionHandler:(void
(^)(NSString*))completionHandler {
On 2015/11/19 10:02:27, vabr (Chromium) wrote:
> On 2015/11/18 17:06:48, eugenebut wrote:
> > Indent 4 spaces
> 
> Cannot do, it conflicts with git cl format, and the presubmit says:
> "The chrome directory requires source formatting. Please run git cl format
> ios/chrome"
> 
> The internal bug for that is 25783407.
This is a bug in clang format and presubmit message is just a warning not an
error. Following Style guide is required. And changing our code to workaround
clang format issues is not (especially where clang format gives incorrect
results).

[vabr (Chromium), 2015-11-19 16:45:02]

Thanks, I appreciated the deeper explanation!

This time no push-back, all addressed.

Will be sending to the CQ soon.

Cheers,
Vaclav

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_password_manager.h (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.h:40: // Fills in the password
form specified by |jsonString| with the given
On 2015/11/19 15:13:21, eugenebut wrote:
> On 2015/11/19 10:02:27, vabr (Chromium) wrote:
> > On 2015/11/18 17:06:48, eugenebut wrote:
> > > s/jsonString/JSONString
> > 
> > Pushing back:
> > 
> > How am I supposed to deduce this from
> >
>
https://google.github.io/styleguide/objcguide.xml?showone=Method_Declarations...
> > or http://google.github.io/styleguide/cppguide.html#Variable_Names ?
> > ios/web/test/web_test.mm also uses jsonString.
> This is an Objective-C method and you suppose to follow Objective-C style
guide
> here:
> 
> Any class, category, method, or variable name may use all capitals for
> initialisms within the name. This follows Apple's standard of using all
capitals
> within a name for initialisms such as URL, TIFF, and EXIF.
> 
> See Naming section below this link:
>
https://engdoc.corp.google.com/eng/doc/objcguide.xml?showone=Exceptions&cl=he...
> 
> Unfortunately our codebase has multiple examples where acronyms are not
> capitalized, but those examples are incorrect. Please fix, as current code is
> deviation from Objective-C style guide.
> 
> 

Thanks, I missed that paragraph. Makes sense now, fixed.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.h:46: -
(void)fillPasswordForm:(NSString*)jsonString
On 2015/11/19 10:02:27, vabr (Chromium) wrote:
> On 2015/11/18 17:06:48, eugenebut wrote:
> > s/jsonString/JSONString
> 
> ditto

Done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/js_password_manager.mm (right):

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.mm:14: NSString*
JSONEscape(NSString* str) {
On 2015/11/19 10:02:27, vabr (Chromium) wrote:
> On 2015/11/18 17:06:48, eugenebut wrote:
> > NIT: s/str/JSON here and in the comment
> 
> Also here, I do not understand why the capitalisation.
> My suggestion would be jsonString as in
> ios/chrome/browser/passwords/js_password_manager.h.

Change to JSONString to match the others.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.mm:40: completionHandler:(void
(^)(NSString*))completionHandler {
On 2015/11/19 15:13:21, eugenebut wrote:
> On 2015/11/19 10:02:27, vabr (Chromium) wrote:
> > On 2015/11/18 17:06:48, eugenebut wrote:
> > > Indent 4 spaces
> > 
> > Cannot do, it conflicts with git cl format, and the presubmit says:
> > "The chrome directory requires source formatting. Please run git cl format
> > ios/chrome"
> > 
> > The internal bug for that is 25783407.
> This is a bug in clang format and presubmit message is just a warning not an
> error. Following Style guide is required. And changing our code to workaround
> clang format issues is not (especially where clang format gives incorrect
> results).

Fair enough, done.

https://codereview.chromium.org/1456983002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/js_password_manager.mm:100: NSString* js = [[self
injectionContent] stringByAppendingString:script];
On 2015/11/19 10:02:27, vabr (Chromium) wrote:
> On 2015/11/18 17:06:48, eugenebut wrote:
> > s/js/JS
> 
> Same push-back as for jsonString in
> ios/chrome/browser/passwords/js_password_manager.h.

Done.

[vabr (Chromium), 2015-11-19 16:45:46]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 16:46:37]

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

[commit-bot: I haz the power, 2015-11-19 16:55:41]

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

[commit-bot: I haz the power, 2015-11-19 16:55:41]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2015-11-19 17:35:59]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2015-11-19 17:35:59]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/1456983002/#ps60001
(title: "Indent fixed, capitalisation fixed, typo fixed")

[commit-bot: I haz the power, 2015-11-19 17:36:42]

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

[commit-bot: I haz the power, 2015-11-19 17:42:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-19 17:43:41]

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