Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7514003:
Show display warning if the form is autocomplete off. (Closed)
|
Created:
9 years, 5 months ago by honten.org Modified:
9 years, 4 months ago CC:
chromium-reviews, GeorgeY, Ilya Sherman, dhollowa, darin-cc_chromium.org, brettw-cc_chromium.org Visibility:
Public. |
DescriptionShow display warning if the form is autocomplete off.
Show display warning if the form is autocomplete off.
Need WebKit side patch. https://bugs.webkit.org/show_bug.cgi?id=65304
BUG=63553
TEST=1,Make form with autocomplete="off", make sure the warning message is shown.
2, Make form with field with autocomple="off", make sure the warning message is not shown.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95058
Patch Set 1 #
Total comments: 4
Patch Set 2 : Change autocomplete off check. #
Total comments: 9
Patch Set 3 : Add comment and change autofill_query_node_ type. #
Total comments: 2
Patch Set 4 : Change autofill_query_node_ to autofill_query_element_ #Messages
Total messages: 12 (0 generated)
This is not complete patch yet. Once it is Ok, I'll upload WebKit patch too.
http://codereview.chromium.org/7514003/diff/1/chrome/renderer/autofill/autofi... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/7514003/diff/1/chrome/renderer/autofill/autofi... chrome/renderer/autofill/autofill_agent.cc:237: if (display_warning_if_disabled_) { If display_warning_if_disabled_ is true, that does not mean that Autofill is disabled. I think the check you want here is something along the lines of if (!autofill_query_node_.isNull() && !autofill_query_node_.autoComplete()) http://codereview.chromium.org/7514003/diff/1/chrome/renderer/autofill/autofi... chrome/renderer/autofill/autofill_agent.cc:355: display_warning_if_disabled && !form.autoComplete()); Does this not prevent other warnings from ever displaying, e.g. the warning that we show when trying to fill a credit card form on an HTTP (not HTTPS) website?
http://codereview.chromium.org/7514003/diff/1/chrome/renderer/autofill/autofi... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/7514003/diff/1/chrome/renderer/autofill/autofi... chrome/renderer/autofill/autofill_agent.cc:237: if (display_warning_if_disabled_) { I needed a couple of cast, so I made a function. On 2011/07/26 09:01:55, Ilya Sherman wrote: > If display_warning_if_disabled_ is true, that does not mean that Autofill is > disabled. I think the check you want here is something along the lines of > > if (!autofill_query_node_.isNull() && !autofill_query_node_.autoComplete()) http://codereview.chromium.org/7514003/diff/1/chrome/renderer/autofill/autofi... chrome/renderer/autofill/autofill_agent.cc:355: display_warning_if_disabled && !form.autoComplete()); I don't need this check anymore. On 2011/07/26 09:01:55, Ilya Sherman wrote: > Does this not prevent other warnings from ever displaying, e.g. the warning that > we show when trying to fill a credit card form on an HTTP (not HTTPS) website? Done.
Please review again. On 2011/07/27 03:29:37, honten.org wrote: > http://codereview.chromium.org/7514003/diff/1/chrome/renderer/autofill/autofi... > File chrome/renderer/autofill/autofill_agent.cc (right): > > http://codereview.chromium.org/7514003/diff/1/chrome/renderer/autofill/autofi... > chrome/renderer/autofill/autofill_agent.cc:237: if > (display_warning_if_disabled_) { > I needed a couple of cast, so I made a function. > > On 2011/07/26 09:01:55, Ilya Sherman wrote: > > If display_warning_if_disabled_ is true, that does not mean that Autofill is > > disabled. I think the check you want here is something along the lines of > > > > if (!autofill_query_node_.isNull() && !autofill_query_node_.autoComplete()) > > http://codereview.chromium.org/7514003/diff/1/chrome/renderer/autofill/autofi... > chrome/renderer/autofill/autofill_agent.cc:355: display_warning_if_disabled && > !form.autoComplete()); > I don't need this check anymore. > > On 2011/07/26 09:01:55, Ilya Sherman wrote: > > Does this not prevent other warnings from ever displaying, e.g. the warning > that > > we show when trying to fill a credit card form on an HTTP (not HTTPS) website? > > Done.
http://codereview.chromium.org/7514003/diff/1002/chrome/renderer/autofill/aut... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/7514003/diff/1002/chrome/renderer/autofill/aut... chrome/renderer/autofill/autofill_agent.cc:44: bool autoCompleteOff(const WebNode& node) { nit: Chromium function names start with capital letters, and boolean function names typically start with "Is". Hence, I would name this function something along the lines of "IsAutocompleteDisabled()" Alternately, you could just update the type for autofill_query_node_ to be a WebInputElement, and skip these type checks and casts. http://codereview.chromium.org/7514003/diff/1002/chrome/renderer/autofill/aut... chrome/renderer/autofill/autofill_agent.cc:49: node.nodeType() != WebNode::TextNode) I don't think you want TextNode here... http://codereview.chromium.org/7514003/diff/1002/chrome/renderer/autofill/aut... chrome/renderer/autofill/autofill_agent.cc:254: l10n_util::GetStringUTF16(IDS_AUTOFILL_WARNING_FORM_DISABLED)); nit: This looks like it could fit on a single line. http://codereview.chromium.org/7514003/diff/1002/chrome/renderer/autofill/aut... chrome/renderer/autofill/autofill_agent.cc:346: (!element.autoComplete() && form.autoComplete()) || nit: This line is rather confusing at a glance, and could use an explanatory comment.
http://codereview.chromium.org/7514003/diff/1002/chrome/renderer/autofill/aut... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/7514003/diff/1002/chrome/renderer/autofill/aut... chrome/renderer/autofill/autofill_agent.cc:346: (!element.autoComplete() && form.autoComplete()) || Should I use extra bool flag like this? // We should not show warning message for disabled autocomple when // field autocomplete is off and form autocomplete is on. bool disabled_autocomplete_off_warning = !element.autoComplete() && form.autocomplete(); Or just adding comment is fine? On 2011/07/27 23:41:30, Ilya Sherman wrote: > nit: This line is rather confusing at a glance, and could use an explanatory > comment.
http://codereview.chromium.org/7514003/diff/1002/chrome/renderer/autofill/aut... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/7514003/diff/1002/chrome/renderer/autofill/aut... chrome/renderer/autofill/autofill_agent.cc:346: (!element.autoComplete() && form.autoComplete()) || On 2011/07/28 00:04:50, honten.org wrote: > Should I use extra bool flag like this? > > // We should not show warning message for disabled autocomple when > // field autocomplete is off and form autocomplete is on. This comment only describes part of the intent. The full intent is better captured by something along the lines of "If autocomplete is disabled at the form level, then we might want to show a warning in place of suggestions. However, if autocomplete is disabled specifically for this field, we never want to show a warning. Otherwise, we might interfere with custom popups (e.g. search suggestions) used by the website." > bool disabled_autocomplete_off_warning = !element.autoComplete() > && form.autocomplete(); > > Or just adding comment is fine? Just adding a comment is fine, though you're welcome to break this logic out into a boolean variable if you think that is more readable. If you prefer to add a variable, please try to find a clearer name for it. "disabled_autocomplete_off_warning" reads like a noun (with modifiers), whereas boolean variables -- like boolean functions -- should typically read like full sentences in the form of a question.
Please review again. http://codereview.chromium.org/7514003/diff/1002/chrome/renderer/autofill/aut... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/7514003/diff/1002/chrome/renderer/autofill/aut... chrome/renderer/autofill/autofill_agent.cc:44: bool autoCompleteOff(const WebNode& node) { Change the type. On 2011/07/27 23:41:30, Ilya Sherman wrote: > nit: Chromium function names start with capital letters, and boolean function > names typically start with "Is". Hence, I would name this function something > along the lines of "IsAutocompleteDisabled()" > > Alternately, you could just update the type for autofill_query_node_ to be a > WebInputElement, and skip these type checks and casts. http://codereview.chromium.org/7514003/diff/1002/chrome/renderer/autofill/aut... chrome/renderer/autofill/autofill_agent.cc:254: l10n_util::GetStringUTF16(IDS_AUTOFILL_WARNING_FORM_DISABLED)); On 2011/07/27 23:41:30, Ilya Sherman wrote: > nit: This looks like it could fit on a single line. Done. http://codereview.chromium.org/7514003/diff/1002/chrome/renderer/autofill/aut... chrome/renderer/autofill/autofill_agent.cc:346: (!element.autoComplete() && form.autoComplete()) || Just added comment. On 2011/07/28 00:29:33, Ilya Sherman wrote: > On 2011/07/28 00:04:50, http://honten.org wrote: > > Should I use extra bool flag like this? > > > > // We should not show warning message for disabled autocomple when > > // field autocomplete is off and form autocomplete is on. > > This comment only describes part of the intent. The full intent is better > captured by something along the lines of "If autocomplete is disabled at the > form level, then we might want to show a warning in place of suggestions. > However, if autocomplete is disabled specifically for this field, we never want > to show a warning. Otherwise, we might interfere with custom popups (e.g. > search suggestions) used by the website." > > > bool disabled_autocomplete_off_warning = !element.autoComplete() > > && form.autocomplete(); > > > > Or just adding comment is fine? > > Just adding a comment is fine, though you're welcome to break this logic out > into a boolean variable if you think that is more readable. If you prefer to > add a variable, please try to find a clearer name for it. > "disabled_autocomplete_off_warning" reads like a noun (with modifiers), whereas > boolean variables -- like boolean functions -- should typically read like full > sentences in the form of a question.
Looks good, with one nit (and pending the WebKit change). Thanks :) http://codereview.chromium.org/7514003/diff/8002/chrome/renderer/autofill/aut... File chrome/renderer/autofill/autofill_agent.h (right): http://codereview.chromium.org/7514003/diff/8002/chrome/renderer/autofill/aut... chrome/renderer/autofill/autofill_agent.h:149: WebKit::WebInputElement autofill_query_node_; nit: We should probably also rename |autofill_query_node_| to |autofill_query_element_| for clarity when it's used in the implementation file.
Also I uploaded WebKit patch too. http://codereview.chromium.org/7514003/diff/8002/chrome/renderer/autofill/aut... File chrome/renderer/autofill/autofill_agent.h (right): http://codereview.chromium.org/7514003/diff/8002/chrome/renderer/autofill/aut... chrome/renderer/autofill/autofill_agent.h:149: WebKit::WebInputElement autofill_query_node_; On 2011/07/28 06:15:00, Ilya Sherman wrote: > nit: We should probably also rename |autofill_query_node_| to > |autofill_query_element_| for clarity when it's used in the implementation file. Done.
LGTM.
Change committed as 95058 |
