[Webkit-unassigned] [Bug 104600] [Chromium] Always enable autocomplete for password fields

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=104600





--- Comment #28 from Yue Zhang <zysxqn at chromium.org>  2012-12-20 17:18:53 PST ---
(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 at . 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...

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.