Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2012763003:
First pass at refactoring autofill_agent to eventually incorporate Synthetic Form logic. (Closed)
|
Created:
4 years, 6 months ago by tmartino Modified:
4 years, 6 months ago Reviewers:
Mathieu CC:
bondd+autofillwatch_chromium.org, browser-components-watch_chromium.org, chromium-reviews, darin-cc_chromium.org, estade+watch_chromium.org, jam, jdonnelly+autofillwatch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, rouslan+autofill_chromium.org, vabr+watchlistautofill_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFirst pass at refactoring autofill_agent to eventually incorporate Synthetic Form logic.
BUG=613600
Committed: https://crrev.com/1c5348c95fffc38743371431c794f1db7e948242
Cr-Commit-Position: refs/heads/master@{#396245}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressing mathp comments #
Total comments: 3
Patch Set 3 : Removing braces :{ #
Messages
Total messages: 20 (8 generated)
tmartino@chromium.org changed reviewers: + mathp@chromium.org
It's a good refactor, thanks for doing it. Some comments https://codereview.chromium.org/2012763003/diff/1/components/autofill/content... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/2012763003/diff/1/components/autofill/content... components/autofill/content/renderer/autofill_agent.cc:251: FireHostSubmitEvents(form, true, false); nit: FireHostSubmitEvents(form, /* form_submitted= */ false); I find this style easier for the reader. https://codereview.chromium.org/2012763003/diff/1/components/autofill/content... components/autofill/content/renderer/autofill_agent.cc:605: } else { nit: don't need an else if you return in the above "if" block https://codereview.chromium.org/2012763003/diff/1/components/autofill/content... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/2012763003/diff/1/components/autofill/content... components/autofill/content/renderer/autofill_agent.h:134: void FireHostSubmitEvents(const blink::WebFormElement& form, it's good to have flexible functions, but in this case the second argument is always true, so let's remove it? https://codereview.chromium.org/2012763003/diff/1/components/autofill/content... components/autofill/content/renderer/autofill_agent.h:135: bool willSubmitForm, nit:use_this_name_style
Addressed comments, PTAL https://codereview.chromium.org/2012763003/diff/1/components/autofill/content... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/2012763003/diff/1/components/autofill/content... components/autofill/content/renderer/autofill_agent.cc:251: FireHostSubmitEvents(form, true, false); On 2016/05/25 at 17:48:55, Mathieu Perreault wrote: > nit: FireHostSubmitEvents(form, /* form_submitted= */ false); > > I find this style easier for the reader. Agreed, done. https://codereview.chromium.org/2012763003/diff/1/components/autofill/content... components/autofill/content/renderer/autofill_agent.cc:605: } else { On 2016/05/25 at 17:48:55, Mathieu Perreault wrote: > nit: don't need an else if you return in the above "if" block Done. https://codereview.chromium.org/2012763003/diff/1/components/autofill/content... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/2012763003/diff/1/components/autofill/content... components/autofill/content/renderer/autofill_agent.h:134: void FireHostSubmitEvents(const blink::WebFormElement& form, On 2016/05/25 at 17:48:55, Mathieu Perreault wrote: > it's good to have flexible functions, but in this case the second argument is always true, so let's remove it? Done https://codereview.chromium.org/2012763003/diff/1/components/autofill/content... components/autofill/content/renderer/autofill_agent.h:135: bool willSubmitForm, On 2016/05/25 at 17:48:55, Mathieu Perreault wrote: > nit:use_this_name_style whoops_done
lgtm with nits Thanks https://codereview.chromium.org/2012763003/diff/20001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/2012763003/diff/20001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:303: if (!form_util::ExtractFormData(form, &form_data)) { nit: unnecessary {} https://codereview.chromium.org/2012763003/diff/20001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:320: if (form_submitted) { nit: unnecessary {} https://codereview.chromium.org/2012763003/diff/20001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:603: if (form_util::AreFormContentsVisible(last_interacted_form_)) { nit: remove {}
The CQ bit was checked by tmartino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org Link to the patchset: https://codereview.chromium.org/2012763003/#ps40001 (title: "Removing braces :{")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2012763003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2012763003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tmartino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2012763003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2012763003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
The CQ bit was checked by tmartino@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2012763003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2012763003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== First pass at refactoring autofill_agent to eventually incorporate Synthetic Form logic. BUG=613600 ========== to ========== First pass at refactoring autofill_agent to eventually incorporate Synthetic Form logic. BUG=613600 Committed: https://crrev.com/1c5348c95fffc38743371431c794f1db7e948242 Cr-Commit-Position: refs/heads/master@{#396245} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1c5348c95fffc38743371431c794f1db7e948242 Cr-Commit-Position: refs/heads/master@{#396245} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
