Bug 104600 – [Chromium] Always enable autocomplete for password fields

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

[Yue Zhang]

Ignore autocomplete=off in webkit code

[Yue Zhang]

Created attachment 178640 [details]
Patch

[Peter Beverloo]

Comment on attachment 178640 [details]
Patch

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

A few drive-by nits :-)..

> Source/WebKit/chromium/ChangeLog:1
> [phone]  Yue Zhang  <[e-mail]>

nit: [e-mail]

> Source/WebKit/chromium/ChangeLog:3
> +        Ignore autocomplete=off in webkit code

Since this only touches Chromium code (in Source/WebKit/chromium/), please prefix the subject with [Chromium]. This makes it easier to recognize for non-Chromium contributors. The "in webkit code" addition is rather redundant considering this would be committed to WebKit.

Furthermore, since this change only touches the findPasswordFormFields function, maybe "[Chromium] Always enable autocomplete for password fields" would be better?

> Source/WebKit/chromium/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Adding some rationale here would be good. What is the reason the Chromium port would like to disable autocomplete for password fields?

As a FYI, any line that contains "OOPS!", *except* for the "Reviewed by" line, will prevent you from submitting this patch.

[Yue Zhang]

Created attachment 178839 [details]
Patch

[Yue Zhang]

ping?

[Yue Zhang]

ping again?

[Peter Beverloo]

Comment on attachment 178839 [details]
Patch

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

Tony: I saw this as a drive-by, not sure if anyone on the MTV side knows more about background.
Yue: Do you perhaps have Chromium issues with more context?

> Source/WebKit/chromium/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

You should remove this line.

[Eric Seidel (no email)]

Comment on attachment 178839 [details]
Patch

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

ChangeLog needs work.

> Source/WebKit/chromium/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

The OOPS will block the commit.  YOu're suppose to delete this line and replace it with what it says to. :)

> Source/WebKit/chromium/ChangeLog:9
> +        We should always save generated passwords regardless of the autocomplete setting, since users have already given cresent to save the passwords by choosing to use generated passwords. So in this change we disable autocomplete checking in webkit and we will handle password autocomplete setting in chromium code in another chromium CL.

I think you might want to wrap this for ease of reading. :)

[Tony Chang]

+jochen who knows about privacy stuff. A Chromium bug with more discussion would also be helpful.

[Tony Chang]

Comment on attachment 178839 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:9
>> +        We should always save generated passwords regardless of the autocomplete setting, since users have already given cresent to save the passwords by choosing to use generated passwords. So in this change we disable autocomplete checking in webkit and we will handle password autocomplete setting in chromium code in another chromium CL.
> 
> I think you might want to wrap this for ease of reading. :)

cresent -> consent?

[Yue Zhang]

Created attachment 180031 [details]
Patch

[Yue Zhang]

Created attachment 180033 [details]
Patch

Isn't that a chrome/ feature (as opposed to content/)? Shouldn't we in that case make this somehow configurable?

Also, what about tests?

[Yue Zhang]

(In reply to comment #9)
> (From update of attachment 178839 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178839&action=review
> 
> >> Source/WebKit/chromium/ChangeLog:9
> >> +        We should always save generated passwords regardless of the autocomplete setting, since users have already given cresent to save the passwords by choosing to use generated passwords. So in this change we disable autocomplete checking in webkit and we will handle password autocomplete setting in chromium code in another chromium CL.
> > 
> > I think you might want to wrap this for ease of reading. :)
> 
> cresent -> consent?

Done. Also rephrase to give more background. Please have another look. Thanks!

[Yue Zhang]

(In reply to comment #12)
> Isn't that a chrome/ feature (as opposed to content/)? Shouldn't we in that case make this somehow configurable?
> 
> Also, what about tests?

Yeah it's a chrome feature. 

Regarding test, I'm not super familiar with Webiit code. I think this is a pretty trivial change. Shall we reflect it somewhere in the Webkit unittest?

(In reply to comment #14)
> (In reply to comment #12)
> > Isn't that a chrome/ feature (as opposed to content/)? Shouldn't we in that case make this somehow configurable?
> > 
> > Also, what about tests?
> 
> Yeah it's a chrome feature. 

Ok, then it should be configurable - otherwise other content embedders might end up just ignoring this field

> 
> Regarding test, I'm not super familiar with Webiit code. I think this is a pretty trivial change. Shall we reflect it somewhere in the Webkit unittest?

sounds good. there are unit tests in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests/

[Yue Zhang]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > Isn't that a chrome/ feature (as opposed to content/)? Shouldn't we in that case make this somehow configurable?
> > > 
> > > Also, what about tests?
> > 
> > Yeah it's a chrome feature. 
> 
> Ok, then it should be configurable - otherwise other content embedders might end up just ignoring this field

  I see WebPasswordFormUtils.cpp is under chromium/ directory so I thought it's only used by chromium code. No? If not, how to make it configurable? command line flags? I'm really new to WebKit so any more detailed suggestion is welcomed :)
> 
> > 
> > Regarding test, I'm not super familiar with Webiit code. I think this is a pretty trivial change. Shall we reflect it somewhere in the Webkit unittest?
> 
> sounds good. there are unit tests in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests/

Looks that there is no existing tests regarding WebPasswordFormUtils class. Is it really worth a new test file given that it is a trivial two-line change?

[Eric Seidel (no email)]

So the chrome feature is that Chrome will override the page's request to prevent autocomplete? :)  Is this because pages are explicitly trying to prevent form-fill, or because they're trying to force the user to type their password, or both?

[Yue Zhang]

(In reply to comment #17)
> So the chrome feature is that Chrome will override the page's request to prevent autocomplete? :)  Is this because pages are explicitly trying to prevent form-fill, or because they're trying to force the user to type their password, or both?

It's because we are working on a new chrome feature that will automatically generate random passwords for users during account sign up. For these passwords, it doesn't make sense if we don't save and autofill them even if autocomplete=off. So in chromium code, we will only respect autocomplete=off for normal user chosen passwords, and ignore it for chrome generated passwords. Hope this helps :)

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (In reply to comment #12)
> > > > Isn't that a chrome/ feature (as opposed to content/)? Shouldn't we in that case make this somehow configurable?
> > > > 
> > > > Also, what about tests?
> > > 
> > > Yeah it's a chrome feature. 
> > 
> > Ok, then it should be configurable - otherwise other content embedders might end up just ignoring this field
> 
>   I see WebPasswordFormUtils.cpp is under chromium/ directory so I thought it's only used by chromium code. No? If not, how to make it configurable? command line flags? I'm really new to WebKit so any more detailed suggestion is welcomed :)


The WebKit chromium/ port is used for a number of non-desktop chrome projects such as chrome on android, content_shell, etc..

You could add a bool to WebSettings.h that controls this behavior and defaults to honoring the autocomplete attribute.

In WebPasswordFormElement, you can access the settings with document()->frame()->view()->settings()



> > 
> > > 
> > > Regarding test, I'm not super familiar with Webiit code. I think this is a pretty trivial change. Shall we reflect it somewhere in the Webkit unittest?
> > 
> > sounds good. there are unit tests in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests/
> 
> Looks that there is no existing tests regarding WebPasswordFormUtils class. Is it really worth a new test file given that it is a trivial two-line change?

It worries me that ignoring the autocomplete attribute doesn't break any test (also, if it's worthwhile to change it, it should be worthwhile to test it)

Alternatively, you can try to create a layout test, whatever you prefer

[Yue Zhang]

(In reply to comment #19)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (In reply to comment #14)
> > > > (In reply to comment #12)
> > > > > Isn't that a chrome/ feature (as opposed to content/)? Shouldn't we in that case make this somehow configurable?
> > > > > 
> > > > > Also, what about tests?
> > > > 
> > > > Yeah it's a chrome feature. 
> > > 
> > > Ok, then it should be configurable - otherwise other content embedders might end up just ignoring this field
> > 
> >   I see WebPasswordFormUtils.cpp is under chromium/ directory so I thought it's only used by chromium code. No? If not, how to make it configurable? command line flags? I'm really new to WebKit so any more detailed suggestion is welcomed :)
> 
> 
> The WebKit chromium/ port is used for a number of non-desktop chrome projects such as chrome on android, content_shell, etc..
> 
> You could add a bool to WebSettings.h that controls this behavior and defaults to honoring the autocomplete attribute.
> 
> In WebPasswordFormElement, you can access the settings with document()->frame()->view()->settings()
> 

Thanks for your detailed explanation. Just one question, where could we change it to the non-default value for our chrome feature? I did a code search and it looks like we should use WebView::GetWebSettings() to change its value but it is not called anywhere currently in the code base? Thanks!

> 
> > > 
> > > > 
> > > > Regarding test, I'm not super familiar with Webiit code. I think this is a pretty trivial change. Shall we reflect it somewhere in the Webkit unittest?
> > > 
> > > sounds good. there are unit tests in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests/
> > 
> > Looks that there is no existing tests regarding WebPasswordFormUtils class. Is it really worth a new test file given that it is a trivial two-line change?
> 
> It worries me that ignoring the autocomplete attribute doesn't break any test (also, if it's worthwhile to change it, it should be worthwhile to test it)
> 
> Alternatively, you can try to create a layout test, whatever you prefer

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > (In reply to comment #14)
> > > > > (In reply to comment #12)
> > > > > > Isn't that a chrome/ feature (as opposed to content/)? Shouldn't we in that case make this somehow configurable?
> > > > > > 
> > > > > > Also, what about tests?
> > > > > 
> > > > > Yeah it's a chrome feature. 
> > > > 
> > > > Ok, then it should be configurable - otherwise other content embedders might end up just ignoring this field
> > > 
> > >   I see WebPasswordFormUtils.cpp is under chromium/ directory so I thought it's only used by chromium code. No? If not, how to make it configurable? command line flags? I'm really new to WebKit so any more detailed suggestion is welcomed :)
> > 
> > 
> > The WebKit chromium/ port is used for a number of non-desktop chrome projects such as chrome on android, content_shell, etc..
> > 
> > You could add a bool to WebSettings.h that controls this behavior and defaults to honoring the autocomplete attribute.
> > 
> > In WebPasswordFormElement, you can access the settings with document()->frame()->view()->settings()
> > 
> 
> Thanks for your detailed explanation. Just one question, where could we change it to the non-default value for our chrome feature? I did a code search and it looks like we should use WebView::GetWebSettings() to change its value but it is not called anywhere currently in the code base? Thanks!

You need to add the new setting in webkit/glue/webpreferences.*, and add the new field to content/public/common/content_param_traits_macros.h

then you can override your new setting in chrome/browser/chrome_content_browser_client.cc in the OverrideWebkitPrefs method

> 
> > 
> > > > 
> > > > > 
> > > > > Regarding test, I'm not super familiar with Webiit code. I think this is a pretty trivial change. Shall we reflect it somewhere in the Webkit unittest?
> > > > 
> > > > sounds good. there are unit tests in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests/
> > > 
> > > Looks that there is no existing tests regarding WebPasswordFormUtils class. Is it really worth a new test file given that it is a trivial two-line change?
> > 
> > It worries me that ignoring the autocomplete attribute doesn't break any test (also, if it's worthwhile to change it, it should be worthwhile to test it)
> > 
> > Alternatively, you can try to create a layout test, whatever you prefer

[Yue Zhang]

Created attachment 180437 [details]
Patch

[Yue Zhang]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > (In reply to comment #16)
> > > > (In reply to comment #15)
> > > > > (In reply to comment #14)
> > > > > > (In reply to comment #12)
> > > > > > > Isn't that a chrome/ feature (as opposed to content/)? Shouldn't we in that case make this somehow configurable?
> > > > > > > 
> > > > > > > Also, what about tests?
> > > > > > 
> > > > > > Yeah it's a chrome feature. 
> > > > > 
> > > > > Ok, then it should be configurable - otherwise other content embedders might end up just ignoring this field
> > > > 
> > > >   I see WebPasswordFormUtils.cpp is under chromium/ directory so I thought it's only used by chromium code. No? If not, how to make it configurable? command line flags? I'm really new to WebKit so any more detailed suggestion is welcomed :)
> > > 
> > > 
> > > The WebKit chromium/ port is used for a number of non-desktop chrome projects such as chrome on android, content_shell, etc..
> > > 
> > > You could add a bool to WebSettings.h that controls this behavior and defaults to honoring the autocomplete attribute.
> > > 
> > > In WebPasswordFormElement, you can access the settings with document()->frame()->view()->settings()
> > > 
> > 
> > Thanks for your detailed explanation. Just one question, where could we change it to the non-default value for our chrome feature? I did a code search and it looks like we should use WebView::GetWebSettings() to change its value but it is not called anywhere currently in the code base? Thanks!
> 
> You need to add the new setting in webkit/glue/webpreferences.*, and add the new field to content/public/common/content_param_traits_macros.h
> 
> then you can override your new setting in chrome/browser/chrome_content_browser_client.cc in the OverrideWebkitPrefs method

Thanks! I have made it configurable through WebSettings. Please have another look. The relevant chrome change (11635061) has also been uploaded and assigned to you as the reviewer. Please also have a look. Thanks!
> 
> > 
> > > 
> > > > > 
> > > > > > 
> > > > > > Regarding test, I'm not super familiar with Webiit code. I think this is a pretty trivial change. Shall we reflect it somewhere in the Webkit unittest?
> > > > > 
> > > > > sounds good. there are unit tests in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests/
> > > > 
> > > > Looks that there is no existing tests regarding WebPasswordFormUtils class. Is it really worth a new test file given that it is a trivial two-line change?
> > > 
> > > It worries me that ignoring the autocomplete attribute doesn't break any test (also, if it's worthwhile to change it, it should be worthwhile to test it)
> > > 
> > > Alternatively, you can try to create a layout test, whatever you prefer

[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.

[Darin Fisher (:fishd, Google)]

Comment on attachment 180437 [details]
Patch

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

> Source/WebCore/page/Settings.cpp:422
> +void Settings::setIgnoreWebkitAutocompleteOff(bool ignoreWebkitAutocompleteOff)

Why is the substring "Webkit" in this method name?

> Source/WebKit/chromium/src/WebPasswordFormUtils.cpp:88
> +                || inputElement->shouldAutocomplete())) {

Perhaps the implementation of shouldAutocomplete should inspect the ignoreAutocompleteOff setting?
That way, every caller of shouldAutocomplete will see the same value, and we will not have to
repeat the settings test at each callsite of shouldAutocomplete.

[Yue Zhang]

(In reply to comment #25)
> (From update of attachment 180437 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180437&action=review
> 
> > Source/WebCore/page/Settings.cpp:422
> > +void Settings::setIgnoreWebkitAutocompleteOff(bool ignoreWebkitAutocompleteOff)
> 
> Why is the substring "Webkit" in this method name?

Because we don't completely ignore autocomplete=off, just don't check/handle this in webkit (will handle this in chrome code). So we choose the name like this.
> 
> > Source/WebKit/chromium/src/WebPasswordFormUtils.cpp:88
> > +                || inputElement->shouldAutocomplete())) {
> 
> Perhaps the implementation of shouldAutocomplete should inspect the ignoreAutocompleteOff setting?
> That way, every caller of shouldAutocomplete will see the same value, and we will not have to
> repeat the settings test at each callsite of shouldAutocomplete.

The whole point of not checking autocomplete in webkit is that we don't have enough information in webkit code (we ignore autocomplete setting for chrome generated passwords, but respect it for other normal passwords). So for the same reason, I don't see how we can handle autocomplete setting in the implementation of shouldAutocomplete? maybe I miss anything?

[Darin Fisher (:fishd, Google)]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 180437 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=180437&action=review
> > 
> > > Source/WebCore/page/Settings.cpp:422
> > > +void Settings::setIgnoreWebkitAutocompleteOff(bool ignoreWebkitAutocompleteOff)
> > 
> > Why is the substring "Webkit" in this method name?
> 
> Because we don't completely ignore autocomplete=off, just don't check/handle this in webkit (will handle this in chrome code). So we choose the name like this.

But, you are checking this in WebKit.  WebPasswordFormUtils.cpp is part of WebKit.

That said, I still don't understand how "Webkit" in the name helps convey your intended meaning.


> > 
> > > Source/WebKit/chromium/src/WebPasswordFormUtils.cpp:88
> > > +                || inputElement->shouldAutocomplete())) {
> > 
> > Perhaps the implementation of shouldAutocomplete should inspect the ignoreAutocompleteOff setting?
> > That way, every caller of shouldAutocomplete will see the same value, and we will not have to
> > repeat the settings test at each callsite of shouldAutocomplete.
> 
> The whole point of not checking autocomplete in webkit is that we don't have enough information in webkit code (we ignore autocomplete setting for chrome generated passwords, but respect it for other normal passwords). So for the same reason, I don't see how we can handle autocomplete setting in the implementation of shouldAutocomplete? maybe I miss anything?

Hmm, I thought all password filling when through this code path.  That said, it seems like shouldAutocomplete is used to control whether or not other form state is saved when navigating away from a page.  It makes sense to still honor the setting in those cases.  I understand now why shouldAutocomplete would not check this preference.

Next, I wonder why you need to store this setting on WebCore::Settings if the setting is never consulted by WebCore.  It seems like we could instead just pass a boolean ignoreAutocompleteOff parameter to WebPasswordFormData's constructor, right?

[Yue Zhang]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > (From update of attachment 180437 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=180437&action=review
> > > 
> > > > Source/WebCore/page/Settings.cpp:422
> > > > +void Settings::setIgnoreWebkitAutocompleteOff(bool ignoreWebkitAutocompleteOff)
> > > 
> > > Why is the substring "Webkit" in this method name?
> > 
> > Because we don't completely ignore autocomplete=off, just don't check/handle this in webkit (will handle this in chrome code). So we choose the name like this.
> 
> But, you are checking this in WebKit.  WebPasswordFormUtils.cpp is part of WebKit.
> 
> That said, I still don't understand how "Webkit" in the name helps convey your intended meaning.

Hmm, this method/corresponding variable would also be used in chromium code so I think it's still better to make it clear that we just ignore it in webkit code (not ignore entirely). That said, maybe it is not the best name but I don't think simply using setIgnoreAutocompleteOff is the right way to do this. Thoughts?
> 
> 
> > > 
> > > > Source/WebKit/chromium/src/WebPasswordFormUtils.cpp:88
> > > > +                || inputElement->shouldAutocomplete())) {
> > > 
> > > Perhaps the implementation of shouldAutocomplete should inspect the ignoreAutocompleteOff setting?
> > > That way, every caller of shouldAutocomplete will see the same value, and we will not have to
> > > repeat the settings test at each callsite of shouldAutocomplete.
> > 
> > The whole point of not checking autocomplete in webkit is that we don't have enough information in webkit code (we ignore autocomplete setting for chrome generated passwords, but respect it for other normal passwords). So for the same reason, I don't see how we can handle autocomplete setting in the implementation of shouldAutocomplete? maybe I miss anything?
> 
> Hmm, I thought all password filling when through this code path.  That said, it seems like shouldAutocomplete is used to control whether or not other form state is saved when navigating away from a page.  It makes sense to still honor the setting in those cases.  I understand now why shouldAutocomplete would not check this preference.
> 
> Next, I wonder why you need to store this setting on WebCore::Settings if the setting is never consulted by WebCore.  It seems like we could instead just pass a boolean ignoreAutocompleteOff parameter to WebPasswordFormData's constructor, right?

Well, I'm relatively new to webkit code and using WebSettings is suggested by another reviewer jochen@. The rationale here is that WebPasswordUtil.cpp is not only used by chrome but also used by other content embedders. So if we don't make it configurable through WebSettings other content embedders will end up ignoring autocomplete=off.

So, does your suggestion do the same thing? i.e. whether we can distinguish chrome vs. other content embedders through the parameter to WebPasswordFormData's constructor? Really not familiar with this part of code...

[Darin Fisher (:fishd, Google)]

Maybe it would be helpful for me to see how you will use this code from the Chromium side, but just looking at how WebPasswordFormData gets used, it looks like content::CreatePasswordForm is the only place where WebPasswordFormData gets instantiated.

See content::CreatePasswordForm here:
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/password_form_conversion_utils.cc&exact_package=chromium&q=CreatePasswordForm&type=cs%22&l=42

It seems like you could pass a boolean parameter to that function, and then forward it to the WebPasswordFormData constructor.

In other words, it appears that you only need to ignore autocomplete=off while constructing the WebPasswordFormData, so it seems simpler to just pass a boolean flag to it rather than messing with WebSettings.

(In reply to comment #29)
> Maybe it would be helpful for me to see how you will use this code from the Chromium side, but just looking at how WebPasswordFormData gets used, it looks like content::CreatePasswordForm is the only place where WebPasswordFormData gets instantiated.
> 
> See content::CreatePasswordForm here:
> http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/password_form_conversion_utils.cc&exact_package=chromium&q=CreatePasswordForm&type=cs%22&l=42
> 
> It seems like you could pass a boolean parameter to that function, and then forward it to the WebPasswordFormData constructor.
> 
> In other words, it appears that you only need to ignore autocomplete=off while constructing the WebPasswordFormData, so it seems simpler to just pass a boolean flag to it rather than messing with WebSettings.

I guess seeing how this is going to be used would help.

Since CreatePasswordForm is invoked from RenderViewImpl, just putting it into WebSettings seemed like the easier solution.

[Yue Zhang]

(In reply to comment #29)
> Maybe it would be helpful for me to see how you will use this code from the Chromium side, but just looking at how WebPasswordFormData gets used, it looks like content::CreatePasswordForm is the only place where WebPasswordFormData gets instantiated.
> 
> See content::CreatePasswordForm here:
> http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/password_form_conversion_utils.cc&exact_package=chromium&q=CreatePasswordForm&type=cs%22&l=42
> 
> It seems like you could pass a boolean parameter to that function, and then forward it to the WebPasswordFormData constructor.
> 
> In other words, it appears that you only need to ignore autocomplete=off while constructing the WebPasswordFormData, so it seems simpler to just pass a boolean flag to it rather than messing with WebSettings.

I guess my question is: can we easily distinguish whether it's called by chrome or called by other content embedders when we call CreatePasswordForm()? I saw it is called 12 times in the code base, some under chrome/ while some under content/. Are you suggesting if it is called under chrome/ we pass "true" (means ignore) to the parameter and if it is called under content/ we pass "false"?. If so, I think using WebSettings would be easier since we don't need to change the parameter every place we call it. Thoughts?

[Garrett Casto]

(In reply to comment #31)
> (In reply to comment #29)
> > Maybe it would be helpful for me to see how you will use this code from the Chromium side, but just looking at how WebPasswordFormData gets used, it looks like content::CreatePasswordForm is the only place where WebPasswordFormData gets instantiated.
> > 
> > See content::CreatePasswordForm here:
> > http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/password_form_conversion_utils.cc&exact_package=chromium&q=CreatePasswordForm&type=cs%22&l=42
> > 
> > It seems like you could pass a boolean parameter to that function, and then forward it to the WebPasswordFormData constructor.
> > 
> > In other words, it appears that you only need to ignore autocomplete=off while constructing the WebPasswordFormData, so it seems simpler to just pass a boolean flag to it rather than messing with WebSettings.
> 
> I guess my question is: can we easily distinguish whether it's called by chrome or called by other content embedders when we call CreatePasswordForm()? I saw it is called 12 times in the code base, some under chrome/ while some under content/. Are you suggesting if it is called under chrome/ we pass "true" (means ignore) to the parameter and if it is called under content/ we pass "false"?. If so, I think using WebSettings would be easier since we don't need to change the parameter every place we call it. Thoughts?

I only see 3 files that have calls to CreatePasswordForm() (https://cs.corp.google.com/#search/&q=CreatePasswordForm%5C(%20package:%5Echrome$&type=cs). Only one is in content (content/renderer/render_view_impl.cc) but this needs to ignore autocomplete=off for Chrome as it's how the password_manager is notified. So it doesn't seem like changing the constructor helps here.

Also, are all of these other embedders located in the chrome repository? Because if so, I don't think that they are consuming this data (see https://cs.corp.google.com/#search/&q=%5C.password_form%20package:%5Echrome$&type=cs). Given that, and the fact that you already have to check to see if the <form> element has autocomplete=off set even without this change, would it make sense just to add a comment in WebPasswordFormData/PasswordForm explaining that the caller needs to deal with autocomplete?

[Adam Barth]

Comment on attachment 180437 [details]
Patch

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

> Source/WebCore/page/Settings.h:339
> +        bool m_ignoreWebkitAutocompleteOff : 1;

Please use Settings.in rather than manually adding settings.

Yue told me that Darin is ok with just ignoring autocomplete=off? If that's the case, I hand over to Darin.

Although I still think having a reasonable default implementation for autocomplete=off in WebKit wouldn't hurt. It also troubles me that there's no test for this behavior.

For the record, other embedders of the content API not in the chrome repository are for example chrome on android or CEF.

[Darin Fisher (:fishd, Google)]

I think it is reasonable to go with something like the first patch.  This doesn't need to be configurable at the WebKit level.  The guy who constructs a WebPasswordFormData object can subsequently inspect the passwordShouldAutocomplete attribute to determine if autocomplete=off was specified.  The embedder can then decide whether or not to autocomplete the form field based on their own heuristics.

[Yue Zhang]

Created attachment 181710 [details]
Patch

[Yue Zhang]

Please have another look. Thanks!

(In reply to comment #35)
> I think it is reasonable to go with something like the first patch.  This doesn't need to be configurable at the WebKit level.  The guy who constructs a WebPasswordFormData object can subsequently inspect the passwordShouldAutocomplete attribute to determine if autocomplete=off was specified.  The embedder can then decide whether or not to autocomplete the form field based on their own heuristics.

[Darin Fisher (:fishd, Google)]

Comment on attachment 181710 [details]
Patch

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

> Source/WebKit/chromium/src/WebPasswordFormUtils.cpp:83
> +        // We don't check autocomplete property here. The applications using

Since this is all part of a single function, I think it'd be OK to just
state this comment once at the top of the function, or in the header file
even.

[Yue Zhang]

Created attachment 181758 [details]
Patch

[Yue Zhang]

(In reply to comment #38)
> (From update of attachment 181710 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181710&action=review
> 
> > Source/WebKit/chromium/src/WebPasswordFormUtils.cpp:83
> > +        // We don't check autocomplete property here. The applications using
> 
> Since this is all part of a single function, I think it'd be OK to just
> state this comment once at the top of the function, or in the header file
> even.

Moved to the header. Please have another look.

[Darin Fisher (:fishd, Google)]

Comment on attachment 181758 [details]
Patch

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

> Source/WebKit/chromium/src/WebPasswordFormUtils.h:46
> +// Note that We don't check autocomplete property here. The applications using

nit: s/We/we/

[Darin Fisher (:fishd, Google)]

Comment on attachment 181758 [details]
Patch

I'm not sure if it is OK to land this patch yet.  I would imagine that some Chromium-side changes are needed to avoid regression.

[Darin Fisher (:fishd, Google)]

Comment on attachment 181758 [details]
Patch

Yue wrote:
"yeah, we already done that. I make this change the last change to be safe"

OK, CQ+

[Yue Zhang]

Created attachment 181780 [details]
Patch

[WebKit Review Bot]

Comment on attachment 181780 [details]
Patch

Rejecting attachment 181780 [details] from commit-queue.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', u'--status-host=queues.webkit.org', ..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Working directory has modifications, pass --force-clean or --no-clean to continue.
Working directory has modifications, pass --force-clean or --no-clean to continue.


Full output: http://queues.webkit.org/results/15754518

[Yue Zhang]

(In reply to comment #45)
> (From update of attachment 181780 [details])
> Rejecting attachment 181780 [details] from commit-queue.
> 
> Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', u'--status-host=queues.webkit.org', ..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue
> 
> Working directory has modifications, pass --force-clean or --no-clean to continue.
> Working directory has modifications, pass --force-clean or --no-clean to continue.
> 
> 
> Full output: http://queues.webkit.org/results/15754518

Oops, I seem to sync the working client by accident. Darin, can you pass --no-clean to continue, or do I need to do anything?

[Tony Chang]

Comment on attachment 181780 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> [phone]  Yue Zhang  <[e-mail]>
> +
> +        [Chromium] Always enable autocomplete for password fields
> +        https://bugs.webkit.org/show_bug.cgi?id=104600
> +
> +        Reviewed by NOBODY (OOPS!).

Remove this from the diff.

> Source/WebKit/chromium/ChangeLog:15
> +        * public/WebSettings.h:
> +        * src/WebPasswordFormUtils.cpp:
> +        (WebKit::findPasswordFormFields):
> +        * src/WebSettingsImpl.cpp:
> +        (WebKit::WebSettingsImpl::setIgnoreWebkitAutocompleteOff):
> +        (WebKit):

Please regenerate this.

[Yue Zhang]

Created attachment 181976 [details]
Patch

[Yue Zhang]

(In reply to comment #47)
> (From update of attachment 181780 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181780&action=review
> 
> > Source/WebCore/ChangeLog:6
> > [phone]  Yue Zhang  <[e-mail]>
> > +
> > +        [Chromium] Always enable autocomplete for password fields
> > +        https://bugs.webkit.org/show_bug.cgi?id=104600
> > +
> > +        Reviewed by NOBODY (OOPS!).
> 
> Remove this from the diff.
> 
> > Source/WebKit/chromium/ChangeLog:15
> > +        * public/WebSettings.h:
> > +        * src/WebPasswordFormUtils.cpp:
> > +        (WebKit::findPasswordFormFields):
> > +        * src/WebSettingsImpl.cpp:
> > +        (WebKit::WebSettingsImpl::setIgnoreWebkitAutocompleteOff):
> > +        (WebKit):
> 
> Please regenerate this.

I updated the ChangeLog. Please have another look.

[Tony Chang]

Comment on attachment 181976 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=104600
> +
> +        Don't check autocomplete in webkit code. Rather, we check it in chrome code in the following way: if the password field is chrome generated password, we ignore autocomplete=off and always fill the password; otherwise, we respect the autocomplete set. Since this is a chrome only feature, we make it configurable (default to false but enable this in chrome code).

Sorry, I mean to remove the WebCore/ChangeLog file.  You still need the "Reviewed by NOBODY (OOPS!)." line so the tools can fill in the actual reviewer.  You can also write "Reviewed by Darin Fisher." since he gave an r+ on an earlier patch.

[Yue Zhang]

Created attachment 182001 [details]
Patch

[WebKit Review Bot]

Comment on attachment 182001 [details]
Patch

Clearing flags on attachment: 182001

Committed r139253: <http://trac.webkit.org/changeset/139253>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.