Bug 112375 – [chromium] Add possibleUserNames to WebPasswordFormData

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Garrett Casto]

Add possibleUserNames to WebPasswordFormData

[Garrett Casto]

Created attachment 193168 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 193168 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/public/WebPasswordFormData.h', u'Source/WebKit/chromium/src/WebPasswordFormData.cpp', u'Source/WebKit/chromium/src/WebPasswordFormUtils.cpp', u'Source/WebKit/chromium/src/WebPasswordFormUtils.h']" exit_code: 1
Source/WebKit/chromium/src/WebPasswordFormData.cpp:178:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Garrett Casto]

Additional context at crbug.com/188908

[Ilya Sherman]

Comment on attachment 193168 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193168&action=review

Is there any way to implement this within the Chrome layer, rather than in the WebKit layer?  It seems strange to have feature implementation code in the Chromium WebKit glue layer.

> Source/WebKit/chromium/src/WebPasswordFormUtils.cpp:-104
> -            // Various input types such as text, url, email can be a username field.

nit: Did you intentionally remove this comment?

[Garrett Casto]

(In reply to comment #5)
> (From update of attachment 193168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193168&action=review
> 
> Is there any way to implement this within the Chrome layer, rather than in the WebKit layer?  It seems strange to have feature implementation code in the Chromium WebKit glue layer.
> 

It looks like this could be implemented in content if we wanted to do that. I made this change because at the moment all parsing for content::PasswordForm happens here and I thought that it made sense to keep it together. Though now that you mention it, I'm not sure if there is a good reason for this code to live in WebKit or if it's just leftover from a time when the Chromium WebKit API wasn't expansive enough.

WebKit reviewers, do you have any thoughts on either if this change should go here or in Chromium, or if this code in general (WebPasswordFormData and WebPasswordFormUtils) should stay in WebKit long term.

> > Source/WebKit/chromium/src/WebPasswordFormUtils.cpp:-104
> > -            // Various input types such as text, url, email can be a username field.
> 
> nit: Did you intentionally remove this comment?

No, good catch.

[Garrett Casto]

Created attachment 193342 [details]
Patch

[WebKit Review Bot]

Attachment 193342 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/public/WebPasswordFormData.h', u'Source/WebKit/chromium/src/WebPasswordFormData.cpp', u'Source/WebKit/chromium/src/WebPasswordFormUtils.cpp', u'Source/WebKit/chromium/src/WebPasswordFormUtils.h']" exit_code: 1
Source/WebKit/chromium/src/WebPasswordFormData.cpp:178:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 193342 [details]
Patch

API change LGTM

[Garrett Casto]

Can I get an r+ and cq+?

[Adam Barth]

(In reply to comment #10)
> Can I get an r+ and cq+?

Who do you have in mind to review your change?

[Garrett Casto]

Ah, apologies. isherman@ was going to review the content of this change, but the LGTM that he gave me was out of band. Ilya, do you mind?

[Ilya Sherman]

Right, I'm not a reviewer, but the content of this change LGTM if we're cool with keeping this code in WebKit.

[WebKit Review Bot]

Comment on attachment 193342 [details]
Patch

Clearing flags on attachment: 193342

Committed r146521: <http://trac.webkit.org/changeset/146521>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.