Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2919873003:
[MD-Settings] Allow clearing local CC details (Closed)
|
Created:
3 years, 6 months ago by hcarmona Modified:
3 years, 6 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow clearing local CC details
R=dschuyler@chromium.org,dpapad@chromium.org
BUG=726062
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2919873003
Cr-Commit-Position: refs/heads/master@{#476817}
Committed: https://chromium.googlesource.com/chromium/src/+/4614abca043a0d6f46575068d605f0af3cf749cd
Patch Set 1 #
Total comments: 14
Patch Set 2 : Feedback #
Total comments: 1
Messages
Total messages: 25 (14 generated)
Description was changed from ========== Allow clearing local CC details R=dschuyler@chromium.org,dpapad@chromium.org BUG=726062 ========== to ========== Allow clearing local CC details R=dschuyler@chromium.org,dpapad@chromium.org BUG=726062 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hcarmona@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...
PTAL https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html (right): https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:185: hidden$="[[!activeCreditCard.metadata.isLocal]]" Needed this to hide button that's useless for !local
Are you planning to add tests in a follow up CL? Also let's make the CL description more descriptive by adding "MD Settings: " prefix. https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:419: * 3-dot menu shouldn't be shown if the card/address if the only option is Don't fully understand this comment, especially the part that says "if the card/address if the only option is" https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:423: */ @private https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:424: showDots_: function(metadata) { Is |item.metadata| updated always as a whole? Is there any chance that some code updates the subproperties isLocal or isCached directly, and therefore it would erroneously not trigger this binding? In other words, is it safer to change the HTML to depend on both [[showDots_(item.metadata.isLocal, item.metadata.isCached)]]? Or does this create another problem because one of them might undefined and also block the binding from executing?
https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:419: * 3-dot menu shouldn't be shown if the card/address if the only option is This comment reads oddly. I think it's trying to say, The 3-dot menu should not be shown if the card/address is entirely remote. https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:421: * @param {chrome.autofillPrivate.AutofillMetadata} metadata "{!chrome..." https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:422: * @return {boolean} @private
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:424: showDots_: function(metadata) { On 2017/06/02 19:09:59, dpapad wrote: > Is |item.metadata| updated always as a whole? Is there any chance that some code > updates the subproperties isLocal or isCached directly, and therefore it would > erroneously not trigger this binding? > > In other words, is it safer to change the HTML to depend on both > > [[showDots_(item.metadata.isLocal, item.metadata.isCached)]]? Or does this > create another problem because one of them might undefined and also block the > binding from executing? I think it comes in from https://cs.chromium.org/chromium/src/chrome/common/extensions/api/autofill_pr... which I expect is an all at once update on line 228 -- does that sound/look correct?
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
lgtm I agree with dschuyler@ and dpapad@'s comments https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:425: return metadata.isLocal || metadata.isCached; return !!(metadata.isLocal || matadata.isCached); to fix closure
The CQ bit was checked by hcarmona@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...
Addressed feedback, had to make 1 more change to the edit logic b/c we weren't linking to site https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:419: * 3-dot menu shouldn't be shown if the card/address if the only option is On 2017/06/02 19:12:07, dschuyler wrote: > This comment reads oddly. I think it's trying to say, The 3-dot menu should not > be shown if the card/address is entirely remote. Updated comment, removed reference to addresses since this is only touching CC's. https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:421: * @param {chrome.autofillPrivate.AutofillMetadata} metadata On 2017/06/02 19:12:07, dschuyler wrote: > "{!chrome..." Done. https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:422: * @return {boolean} On 2017/06/02 19:12:07, dschuyler wrote: > @private Done. https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:423: */ On 2017/06/02 19:10:00, dpapad wrote: > @private Done. https://codereview.chromium.org/2919873003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:424: showDots_: function(metadata) { On 2017/06/02 19:17:18, dschuyler wrote: > On 2017/06/02 19:09:59, dpapad wrote: > > Is |item.metadata| updated always as a whole? Is there any chance that some > code > > updates the subproperties isLocal or isCached directly, and therefore it would > > erroneously not trigger this binding? > > > > In other words, is it safer to change the HTML to depend on both > > > > [[showDots_(item.metadata.isLocal, item.metadata.isCached)]]? Or does this > > create another problem because one of them might undefined and also block the > > binding from executing? > > I think it comes in from > https://cs.chromium.org/chromium/src/chrome/common/extensions/api/autofill_pr... > which I expect is an all at once update on line 228 -- does that sound/look > correct? Correct, it's part of a list, never updated w/o refreshing the item. https://codereview.chromium.org/2919873003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2919873003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:390: if (this.activeCreditCard.metadata.isLocal) This logic was removed in http://crrev.com/2681143004 but needed to make this work.
lgtm
On 2017/06/02 at 19:10:00, dpapad wrote: > Are you planning to add tests in a follow up CL? > Also let's make the CL description more descriptive by adding "MD Settings: " prefix. Ping ^. Otherwise LGTM
Description was changed from ========== Allow clearing local CC details R=dschuyler@chromium.org,dpapad@chromium.org BUG=726062 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Allow clearing local CC details R=dschuyler@chromium.org,dpapad@chromium.org BUG=726062 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2017/06/02 21:11:44, dpapad wrote: > On 2017/06/02 at 19:10:00, dpapad wrote: > > Are you planning to add tests in a follow up CL? > > Also let's make the CL description more descriptive by adding "MD Settings: " > prefix. > > Ping ^. Otherwise LGTM Done! (Sorry i missed that, committing) Thanks everyone for the quick review!
The CQ bit was unchecked by hcarmona@chromium.org
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2919873003/#ps20001 (title: "Feedback")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496438093263030,
"parent_rev": "13f466e2e115c7554b1522317c16b7e00148fe49", "commit_rev":
"4614abca043a0d6f46575068d605f0af3cf749cd"}
Message was sent while issue was closed.
Description was changed from ========== Allow clearing local CC details R=dschuyler@chromium.org,dpapad@chromium.org BUG=726062 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Allow clearing local CC details R=dschuyler@chromium.org,dpapad@chromium.org BUG=726062 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2919873003 Cr-Commit-Position: refs/heads/master@{#476817} Committed: https://chromium.googlesource.com/chromium/src/+/4614abca043a0d6f46575068d605... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/4614abca043a0d6f46575068d605... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
