[Autofill] Only query for newly parsed forms

[Autofill] Only query for newly parsed forms

This changes from querying everything that has been seen so far on the page to only the new set of forms.

BUG=536165
TEST=AutofillManagerTest

Committed: https://crrev.com/1365274bd6e79d1732bda875a0a43f1aef3c472d
Cr-Commit-Position: refs/heads/master@{#351668}

[Mathieu, 2015-09-29 18:33:54]

mathp@chromium.org changed reviewers:
+ estade@chromium.org

[Mathieu, 2015-09-29 18:35:18]

Hi Evan, 

Please have a look!

https://codereview.chromium.org/1377923002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/1377923002/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.h:242: void
set_download_manager(AutofillDownloadManager* manager) {
Went with this approach for testing, since the AutofillManager holds on to an
AutofillDownloadManager pointer, but is an argument to the constructor of
AutofillDownloadManager as well. Hence you can't add AutofillDownloadManager to
the [test] constructor of AutofillManager.

[Evan Stade, 2015-09-29 22:06:45]

https://codereview.chromium.org/1377923002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1377923002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1415: FormStructure*
structure = form_structure.release();
This ownership model is super brittle and needs to be improved. You should not
use release ever. You can do form_structures_.push_back(structure.Pass())
followed by form_structures_.back() to get a reference.

Does SendAutofillTypePredictionsToRenderer take ownership? If so it now has
potential to double-free non queryable forms. If not, then the old code was
leaking.

https://codereview.chromium.org/1377923002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/1377923002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:241: }
these are not setters and getters for the same thing, so newline

[Mathieu, 2015-09-30 13:09:54]

Thanks, have a look!

https://codereview.chromium.org/1377923002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1377923002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1415: FormStructure*
structure = form_structure.release();
On 2015/09/29 22:06:45, Evan Stade wrote:
> This ownership model is super brittle and needs to be improved. You should not
> use release ever. You can do form_structures_.push_back(structure.Pass())
> followed by form_structures_.back() to get a reference.
> 
> Does SendAutofillTypePredictionsToRenderer take ownership? If so it now has
> potential to double-free non queryable forms. If not, then the old code was
> leaking.

SendAutofillTypePredictionsToRenderer does not take ownership (neither does
StartQueryRequest). I don't think the old code was leaking: the pointer was
transferred to |non_queryable_forms| but was put in |form_structures_| (a
ScopedVector) a few lines later (but you're right this was brittle). 

The new code transfers ownership clearly early on and then uses references from
|form_structures_| like you suggested.

https://codereview.chromium.org/1377923002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/1377923002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:241: }
On 2015/09/29 22:06:45, Evan Stade wrote:
> these are not setters and getters for the same thing, so newline

Done.

[Evan Stade, 2015-09-30 17:19:51]

thanks lgtm

[Mathieu, 2015-09-30 17:21:10]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2015-09-30 17:21:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377923002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377923002/40001

[commit-bot: I haz the power, 2015-09-30 17:30:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-30 17:30:16]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Mathieu, 2015-09-30 18:02:02]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2015-09-30 18:02:02]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1377923002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-09-30 18:04:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377923002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377923002/60001

[commit-bot: I haz the power, 2015-09-30 18:26:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-30 18:26:37]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mathieu, 2015-09-30 19:11:35]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2015-09-30 19:11:36]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1377923002/#ps80001
(title: "minus c++11")

[commit-bot: I haz the power, 2015-09-30 19:14:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377923002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377923002/80001

[commit-bot: I haz the power, 2015-09-30 19:59:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-30 19:59:16]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mathieu, 2015-09-30 21:20:47]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2015-09-30 21:20:47]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1377923002/#ps100001 (title: "fix text
output")

[commit-bot: I haz the power, 2015-09-30 21:22:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377923002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377923002/100001

[commit-bot: I haz the power, 2015-09-30 21:30:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-30 21:30:03]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Mathieu, 2015-09-30 21:41:33]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2015-09-30 21:41:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377923002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377923002/100001

[commit-bot: I haz the power, 2015-09-30 22:50:00]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-30 22:51:46]

Patchset 6 (id:??) landed as
https://crrev.com/1365274bd6e79d1732bda875a0a43f1aef3c472d
Cr-Commit-Position: refs/heads/master@{#351668}