Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
512933004:
mac: Don't show "Access Address Book" prompt if Autofill is disabled. (Closed)
|
Created:
6 years, 3 months ago by erikchen Modified:
6 years, 3 months ago CC:
chromium-reviews, benquan, browser-components-watch_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptionmac: Fix 2 bugs where "Access Address Book" prompt is incorrectly shown.
Don't show "Access Address Book" prompt if Autofill is disabled, the form has
the attribute autocomplete="off", or if Chrome doesn't think the form is
Autofillable.
BUG=398476
Committed: https://crrev.com/d4dd2df0e956f0ba64e988d48acc3d81c02aad35
Cr-Commit-Position: refs/heads/master@{#292538}
Patch Set 1 #Patch Set 2 : Require forms be autofillable. #
Total comments: 7
Patch Set 3 : Fix logic in autofill manager. #
Total comments: 2
Patch Set 4 : Respond to estade's style suggestion. #
Total comments: 4
Patch Set 5 : Add virtual/OVERRIDE. #Patch Set 6 : Respond to comments from estade (remove mock function). #
Total comments: 2
Patch Set 7 : Check for form autofillability. #
Messages
Total messages: 42 (0 generated)
erikchen@chromium.org changed reviewers: + isherman@chromium.org
isherman: Please review. Are there any obvious logic mistakes that we should be looking out for? (e.g. do we need to check that the form is Autofill enabled?)
LGTM
On 2014/08/27 23:09:27, erikchen wrote: > Are there any obvious logic mistakes that we should be looking out for? (e.g. do > we need to check that the form is Autofill enabled?) I'm not sure what you mean. Do we not already check to make sure that the form could have Autofill suggestions? If we do not, we should.
No we do not, I'm glad I asked. Updated the CL. PTAL.
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/512933004/diff/20001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/512933004/diff/20001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:295: if (!form_structure->IsAutofillable()) why are we even getting to this point if the form isn't autofillable? https://codereview.chromium.org/512933004/diff/20001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:518: // Add the results from AutoComplete. They come back asynchronously, so we it doesn't seem like we should be doing this either for autocomplete="off", but I only see an form_structure->IsAutofillable() above, not here.
https://codereview.chromium.org/512933004/diff/20001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/512933004/diff/20001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:296: return false; Could you add test coverage for this change as well?
Come to think of it, I don't think we should show ever show the form disabled warning either (that could clash with a js dropdown just as easily).
On 2014/08/28 00:35:40, Evan Stade wrote: > Come to think of it, I don't think we should show ever show the form disabled > warning either (that could clash with a js dropdown just as easily). We show the form disabled warning if the form specifies autocomplete="off", and the form field does not. This allows the author to suppress the warning by adding autocomplete="off" to individual fields. I'm not sure how useful the warning is in this state, though.
https://codereview.chromium.org/512933004/diff/20001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/512933004/diff/20001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:295: if (!form_structure->IsAutofillable()) On 2014/08/28 00:33:16, Evan Stade wrote: > why are we even getting to this point if the form isn't autofillable? I don't know. See the first exchange between isherman and myself. It's quite possible that the Autofill logic needs a refactor.
I don't think IsAutofillable() is the right method here... // Runs a quick heuristic to rule out forms that are obviously not // auto-fillable, like google/yahoo/msn search, etc. bool IsAutofillable() const; The check for <form autocomplete="off"> does come higher up in the call stack[1]. For individual fields, the correct thing to check is FormFieldData::should_autocomplete. [1] https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
On 2014/08/28 00:51:42, Evan Stade wrote: > I don't think IsAutofillable() is the right method here... > > // Runs a quick heuristic to rule out forms that are obviously not > // auto-fillable, like google/yahoo/msn search, etc. > bool IsAutofillable() const; > > The check for <form autocomplete="off"> does come higher up in the call > stack[1]. > > For individual fields, the correct thing to check is > FormFieldData::should_autocomplete. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... I see. Thanks for the info! (I thought that Autocomplete was distinct from Autofill. I guess Autofill is the name of a Chrome feature that triggers off of web forms which use the key word Autocomplete, not to be confused with the Chrome feature called Autocomplete...)
On 2014/08/28 01:47:12, erikchen wrote: > On 2014/08/28 00:51:42, Evan Stade wrote: > > I don't think IsAutofillable() is the right method here... > > > > // Runs a quick heuristic to rule out forms that are obviously not > > // auto-fillable, like google/yahoo/msn search, etc. > > bool IsAutofillable() const; > > > > The check for <form autocomplete="off"> does come higher up in the call > > stack[1]. > > > > For individual fields, the correct thing to check is > > FormFieldData::should_autocomplete. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... > > I see. Thanks for the info! > > (I thought that Autocomplete was distinct from Autofill. I guess Autofill is the > name of a Chrome feature that triggers off of web forms which use the key word > Autocomplete, not to be confused with the Chrome feature called Autocomplete...) Autofill is the feature that parses field types and fills in an entire form when you start typing in a single field. Autocomplete is an html5 attribute, as well as the Chrome feature that completes /just the field you're typing in/, based on what you've typed in that exact field in the past.
Patchset #3 (id:40001) has been deleted
isherman: PTAL estade: Thanks for the explanation! https://codereview.chromium.org/512933004/diff/20001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/512933004/diff/20001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:296: return false; On 2014/08/28 00:34:19, Ilya Sherman wrote: > Could you add test coverage for this change as well? Done. https://codereview.chromium.org/512933004/diff/20001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:518: // Add the results from AutoComplete. They come back asynchronously, so we On 2014/08/28 00:33:16, Evan Stade wrote: > it doesn't seem like we should be doing this either for autocomplete="off", but > I only see an form_structure->IsAutofillable() above, not here. I'm probably not the best person to answer this. isherman?
your CL description confuses me. "Disabling" Autofill implies that a user went to chrome://settings and unchecked the use autofill box. I assume this is already respected? https://codereview.chromium.org/512933004/diff/20001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/512933004/diff/20001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:518: // Add the results from AutoComplete. They come back asynchronously, so we On 2014/08/28 17:01:17, erikchen wrote: > On 2014/08/28 00:33:16, Evan Stade wrote: > > it doesn't seem like we should be doing this either for autocomplete="off", > but > > I only see an form_structure->IsAutofillable() above, not here. > > I'm probably not the best person to answer this. isherman? ignore this question; <form autocomplete="off"> is indeed handled further up (i.e. we wouldn't get here) https://codereview.chromium.org/512933004/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/512933004/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:295: if (!field.should_autocomplete) nit: combine this with the check on L288
On 2014/08/28 17:18:58, Evan Stade wrote: > your CL description confuses me. "Disabling" Autofill implies that a user went > to chrome://settings and unchecked the use autofill box. I assume this is > already respected? Ah nevermind, I get it now. Could you change the first line of the description to mac: Don't show "Access Address Book" prompt if Autofill is disabled or the input has autocomplete="off". Also, I don't think the second paragraph is necessary. > > https://codereview.chromium.org/512933004/diff/20001/components/autofill/core... > File components/autofill/core/browser/autofill_manager.cc (right): > > https://codereview.chromium.org/512933004/diff/20001/components/autofill/core... > components/autofill/core/browser/autofill_manager.cc:518: // Add the results > from AutoComplete. They come back asynchronously, so we > On 2014/08/28 17:01:17, erikchen wrote: > > On 2014/08/28 00:33:16, Evan Stade wrote: > > > it doesn't seem like we should be doing this either for autocomplete="off", > > but > > > I only see an form_structure->IsAutofillable() above, not here. > > > > I'm probably not the best person to answer this. isherman? > > ignore this question; <form autocomplete="off"> is indeed handled further up > (i.e. we wouldn't get here) > > https://codereview.chromium.org/512933004/diff/60001/components/autofill/core... > File components/autofill/core/browser/autofill_manager.cc (right): > > https://codereview.chromium.org/512933004/diff/60001/components/autofill/core... > components/autofill/core/browser/autofill_manager.cc:295: if > (!field.should_autocomplete) > nit: combine this with the check on L288
estade: PTAL I modified the CL description's title to be slightly more generic, and then used your recommendation as the second line, in order to keep the CL's title under 72 characters. https://codereview.chromium.org/512933004/diff/60001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/512933004/diff/60001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:295: if (!field.should_autocomplete) On 2014/08/28 17:18:58, Evan Stade wrote: > nit: combine this with the check on L288 Done.
https://codereview.chromium.org/512933004/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/512933004/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_manager_unittest.cc:137: bool ShouldShowAccessAddressBookSuggestion(AutofillType type) { return true; } where is this used? is it supposed to be virtual/OVERRIDE?
https://codereview.chromium.org/512933004/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/512933004/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_manager_unittest.cc:137: bool ShouldShowAccessAddressBookSuggestion(AutofillType type) { return true; } On 2014/08/28 17:31:55, Evan Stade wrote: > where is this used? is it supposed to be virtual/OVERRIDE? Yup. Good catch, the method it was overriding wasn't virtual. :(
https://codereview.chromium.org/512933004/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/512933004/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_manager_unittest.cc:137: bool ShouldShowAccessAddressBookSuggestion(AutofillType type) { return true; } On 2014/08/28 17:37:52, erikchen wrote: > On 2014/08/28 17:31:55, Evan Stade wrote: > > where is this used? is it supposed to be virtual/OVERRIDE? > > Yup. Good catch, the method it was overriding wasn't virtual. :( can you just remove it instead?
I would prefer to not do that, since it would make a unit test for the autofill_manager dependent on an implementation detail of the personal_data_manager.
On 2014/08/28 17:44:55, erikchen wrote: > I would prefer to not do that, since it would make a unit test for the > autofill_manager dependent on an implementation detail of the > personal_data_manager. We only mock things that we need to. If we mocked everything we could, regardless of necessity, the list of mocked out functions would grow unmanageably large.
estade: I have done as you suggested. https://codereview.chromium.org/512933004/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/512933004/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_manager_unittest.cc:137: bool ShouldShowAccessAddressBookSuggestion(AutofillType type) { return true; } On 2014/08/28 17:42:32, Evan Stade wrote: > On 2014/08/28 17:37:52, erikchen wrote: > > On 2014/08/28 17:31:55, Evan Stade wrote: > > > where is this used? is it supposed to be virtual/OVERRIDE? > > > > Yup. Good catch, the method it was overriding wasn't virtual. :( > > can you just remove it instead? Done.
lgtm
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/512933004/120001
https://codereview.chromium.org/512933004/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/512933004/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:290: We still shouldn't be showing these suggestions for forms that are not autofillable. Are you planning to implement that change in a follow-up CL?
my understanding of comment #12 from estade is that the check already happens earlier.
On 2014/08/28 18:42:54, erikchen wrote: > my understanding of comment #12 from estade is that the check already happens > earlier. Ok. I'm assuming you've double-checked that that's accurate?
The CQ bit was unchecked by erikchen@chromium.org
No, I'm checking now.
There are 4 reasons why we shouldn't show the prompt: 1. The form has attribute autocomplete = NO 2. The form field has attribute autocomplete = NO 3. The Chrome setting for "Autofill" is turned off. 4. Chrome doesn't think that the form is auto-fillable. estade's comment #12 was correct in fixing (1) and (2), but his explanation was wrong. When a form is given the attribute autocomplete = NO, the code in question is still called, but field.should_autocomplete returns NO. So that conditional checks for (1) and (2). We still need manual checks for (3) and (4).
https://codereview.chromium.org/512933004/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/512933004/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:290: On 2014/08/28 18:40:49, Ilya Sherman wrote: > We still shouldn't be showing these suggestions for forms that are not > autofillable. Are you planning to implement that change in a follow-up CL? Added the check.
On 2014/08/28 20:52:55, erikchen wrote: > There are 4 reasons why we shouldn't show the prompt: > > 1. The form has attribute autocomplete = NO > 2. The form field has attribute autocomplete = NO > 3. The Chrome setting for "Autofill" is turned off. > 4. Chrome doesn't think that the form is auto-fillable. > > estade's comment #12 was correct in fixing (1) and (2), but his explanation was > wrong. When a form is given the attribute autocomplete = NO, the code in > question is still called, but field.should_autocomplete returns NO. So that > conditional checks for (1) and (2). > > We still need manual checks for (3) and (4). so we go back to the question of 'why does this code even get called when form autocomplete="off"'? Fixing (or at least understanding) that is less error prone than sprinkling checks throughout the codebase.
Isn't this the same code path that is responsible for showing the "form disabled warning" when autocomplete="off"? If so, it still needs to be called even when autocomplete="off".
ok, lgtm
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/512933004/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as 29e9af1e7eb0edb5db92ab35bee20793ba23ed9c
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d4dd2df0e956f0ba64e988d48acc3d81c02aad35 Cr-Commit-Position: refs/heads/master@{#292538} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
