| Summary: | [chromium] Add possibleUserNames to WebPasswordFormData | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Garrett Casto <gcasto> | ||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, dglazkov, fishd, guohui, isherman, jamesr, tkent+wkapi, webkit.review.bot, zysxqn | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Garrett Casto
2013-03-14 12:07:41 PDT
Created attachment 193168 [details]
Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. 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.
Additional context at crbug.com/188908 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? (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. Created attachment 193342 [details]
Patch
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.
Comment on attachment 193342 [details]
Patch
API change LGTM
Can I get an r+ and cq+? (In reply to comment #10) > Can I get an r+ and cq+? Who do you have in mind to review your change? 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? Right, I'm not a reviewer, but the content of this change LGTM if we're cool with keeping this code in WebKit. Comment on attachment 193342 [details] Patch Clearing flags on attachment: 193342 Committed r146521: <http://trac.webkit.org/changeset/146521> All reviewed patches have been landed. Closing bug. |