Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2253343003:
Only show the credit card assist feature in secure contexts. (Closed)
|
Created:
4 years, 4 months ago by Roger McFarlane (Chromium) Modified:
4 years, 4 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, estade+watch_chromium.org, vabr+watchlistautofill_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnly show the credit card assist feature in secure contexts.
BUG=630656
TEST=TEST=AutofillAssistantTest.CanShowCreditCardAssist_FeatureOn_NotSecure
Committed: https://crrev.com/bc8c9e6315aee20ab680eb4223a824d397aac02c
Cr-Commit-Position: refs/heads/master@{#413887}
Patch Set 1 : Initial CL for review #
Total comments: 2
Patch Set 2 : Move invariant check out of loop #
Total comments: 2
Patch Set 3 : remove redudant comments #
Messages
Total messages: 34 (21 generated)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rogerm@chromium.org changed reviewers: + estade@chromium.org
Please take a look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
can you seek a review from someone who's active on autofill? If you need my OWNER stamp I can provide it after mathp or someone provides an initial review.
rogerm@chromium.org changed reviewers: + sebsg@chromium.org
+sebsg
rogerm@chromium.org changed reviewers: + rouslan@chromium.org
+rouslan
lgtm with comment. Thanks! https://codereview.chromium.org/2253343003/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_assistant.cc (right): https://codereview.chromium.org/2253343003/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_assistant.cc:35: const FormData& cur_form_data = cur_form->ToFormData(); Since forms sometimes have a lot of fields, I think it would be more efficient to check for the secure context first? Looks like checking the context is O(1) and checking if it's a complete credit card form is O(n) where n is the number of fields in the form. It should be negligible in practice though so it's up to you.
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks. Another look? https://codereview.chromium.org/2253343003/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_assistant.cc (right): https://codereview.chromium.org/2253343003/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_assistant.cc:35: const FormData& cur_form_data = cur_form->ToFormData(); On 2016/08/19 14:23:01, sebsg wrote: > Since forms sometimes have a lot of fields, I think it would be more efficient > to check for the secure context first? > > Looks like checking the context is O(1) and checking if it's a complete credit > card form is O(n) where n is the number of fields in the form. > > It should be negligible in practice though so it's up to you. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/18 23:10:08, Evan Stade wrote: > can you seek a review from someone who's active on autofill? If you need my > OWNER stamp I can provide it after mathp or someone provides an initial review. done. estade@: Another look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nit https://codereview.chromium.org/2253343003/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_assistant.cc (right): https://codereview.chromium.org/2253343003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_assistant.cc:33: !autofill_manager_->client()->IsContextSecure( // Insecure? nit: I don't think these comments are that helpful, the code is pretty self documenting (e.g. IsAutofillCreditCardAssistEnabled doesn't require further explanation)
Thanks. https://codereview.chromium.org/2253343003/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_assistant.cc (right): https://codereview.chromium.org/2253343003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_assistant.cc:33: !autofill_manager_->client()->IsContextSecure( // Insecure? On 2016/08/22 15:07:08, Evan Stade (ooo wed-thurs) wrote: > nit: I don't think these comments are that helpful, the code is pretty self > documenting (e.g. IsAutofillCreditCardAssistEnabled doesn't require further > explanation) Done.
The CQ bit was checked by rogerm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org, rouslan@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2253343003/#ps60001 (title: "remove redudant comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Only show the credit card assist feature in secure contexts. BUG=630656 TEST=TEST=AutofillAssistantTest.CanShowCreditCardAssist_FeatureOn_NotSecure ========== to ========== Only show the credit card assist feature in secure contexts. BUG=630656 TEST=TEST=AutofillAssistantTest.CanShowCreditCardAssist_FeatureOn_NotSecure Committed: https://crrev.com/bc8c9e6315aee20ab680eb4223a824d397aac02c Cr-Commit-Position: refs/heads/master@{#413887} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bc8c9e6315aee20ab680eb4223a824d397aac02c Cr-Commit-Position: refs/heads/master@{#413887} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
