[Autofill] Enable Autofill for dynamically created forms.

[Autofill] Enable Autofill for dynamically created forms.

The appropriate hooks with Blink already exist, this change just causes
the browser to be informed whenever a form is added or changed.

BUG=360904
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268107

[Garrett Casto, 2014-04-19 00:03:52]

tsepez: IPC change
isherman: Everything else.

I noticed that there was some dead code related to dynamic forms that looked
like it was left over from autocheckout. I tried to clear up as part of this
change, as that implementation seemed to take a slightly different track that
doesn't seem as general.

[Tom Sepez, 2014-04-21 18:40:01]

Messages LGTM.

[Ilya Sherman, 2014-04-23 05:51:36]

Thanks, Garrett!

https://codereview.chromium.org/242613006/diff/20001/chrome/renderer/autofill...
File chrome/renderer/autofill/form_autofill_browsertest.cc (right):

https://codereview.chromium.org/242613006/diff/20001/chrome/renderer/autofill...
chrome/renderer/autofill/form_autofill_browsertest.cc:882: "var
newInput=document.createElement('input');"
nit: Please add spaces around the equals sign.

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:174: // If the main frame
just finished loading we should process it.
nit: "finished loading we should" -> "finished loading, we should"

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:657: // Record timestamp
on document load. This is used to record overhead of
nit: "record overhead of Autofill feature" -> "measure the overhead of the
Autofill feature"

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:661: // The document has
now been fully loaded.  Scan for forms to be sent up to
This comment is somewhat misleading, as the method might be being called when
new forms were added, rather than just when the document was fully loaded.

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.h:222: // When the main
frame is first loaded a message is sent even if no forms
nit: "first loaded a message" -> "first loaded, a message"

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.h:223: // exist in the
frame. Otherwise such messages are supressed.
nit: "Otherwise such messages" -> "Otherwise, such messages"

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/form_cache.h (right):

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/form_cache.h:56: std::map<const
blink::WebFrame*, std::set<FormData> > parsed_forms_;
nit: Docs, please.

https://codereview.chromium.org/242613006/diff/20001/components/autofill/core...
File components/autofill/core/common/forms_seen_state.h (left):

https://codereview.chromium.org/242613006/diff/20001/components/autofill/core...
components/autofill/core/common/forms_seen_state.h:24: #endif  //
COMPONENTS_AUTOFILL_COMMON_FORMS_SEEN_PARAM_H_
Should there be a corresponding .gypi file update as well?

[Garrett Casto, 2014-04-23 21:51:14]

One other thing to note, given the current setup this will affect the timing
stats of forms that are not loaded dynamically but are on the same page as
dynamic forms. The load time will look later than it actually is. Not sure how
big of a deal this is.

https://codereview.chromium.org/242613006/diff/20001/chrome/renderer/autofill...
File chrome/renderer/autofill/form_autofill_browsertest.cc (right):

https://codereview.chromium.org/242613006/diff/20001/chrome/renderer/autofill...
chrome/renderer/autofill/form_autofill_browsertest.cc:882: "var
newInput=document.createElement('input');"
On 2014/04/23 05:51:36, Ilya Sherman wrote:
> nit: Please add spaces around the equals sign.

Done.

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:174: // If the main frame
just finished loading we should process it.
On 2014/04/23 05:51:36, Ilya Sherman wrote:
> nit: "finished loading we should" -> "finished loading, we should"

Done.

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:657: // Record timestamp
on document load. This is used to record overhead of
On 2014/04/23 05:51:36, Ilya Sherman wrote:
> nit: "record overhead of Autofill feature" -> "measure the overhead of the
> Autofill feature"

Done.

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:661: // The document has
now been fully loaded.  Scan for forms to be sent up to
On 2014/04/23 05:51:36, Ilya Sherman wrote:
> This comment is somewhat misleading, as the method might be being called when
> new forms were added, rather than just when the document was fully loaded.

Removed. I don't think that it adds much anymore.

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.h:222: // When the main
frame is first loaded a message is sent even if no forms
On 2014/04/23 05:51:36, Ilya Sherman wrote:
> nit: "first loaded a message" -> "first loaded, a message"

Done.

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.h:223: // exist in the
frame. Otherwise such messages are supressed.
On 2014/04/23 05:51:36, Ilya Sherman wrote:
> nit: "Otherwise such messages" -> "Otherwise, such messages"

Done.

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/form_cache.h (right):

https://codereview.chromium.org/242613006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/form_cache.h:56: std::map<const
blink::WebFrame*, std::set<FormData> > parsed_forms_;
On 2014/04/23 05:51:36, Ilya Sherman wrote:
> nit: Docs, please.

Fixed.

https://codereview.chromium.org/242613006/diff/20001/components/autofill/core...
File components/autofill/core/common/forms_seen_state.h (left):

https://codereview.chromium.org/242613006/diff/20001/components/autofill/core...
components/autofill/core/common/forms_seen_state.h:24: #endif  //
COMPONENTS_AUTOFILL_COMMON_FORMS_SEEN_PARAM_H_
On 2014/04/23 05:51:36, Ilya Sherman wrote:
> Should there be a corresponding .gypi file update as well?

You would think so, but it doesn't seem like this was ever in a .gypi file.

[Ilya Sherman, 2014-04-24 01:50:18]

On 2014/04/23 21:51:14, Garrett Casto wrote:
> One other thing to note, given the current setup this will affect the timing
> stats of forms that are not loaded dynamically but are on the same page as
> dynamic forms. The load time will look later than it actually is. Not sure how
> big of a deal this is.

Hmm, that's a good point!  We shouldn't corrupt the metrics.  We should either
include a timestamp per form (or set of forms loaded at the same time), or we
should refuse to report metrics for cases involving dynamically loaded forms, if
for some reason accurate metrics are just too difficult to report.

https://codereview.chromium.org/242613006/diff/40001/components/autofill/cont...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/242613006/diff/40001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:657: // Record timestamp
on document load. This is used to measure overhead of
nit: "overhead" -> "the overhead", "Autofill feature" -> "the Autofill feature"

[Garrett Casto, 2014-04-24 18:35:45]

Changed timestamps in the autofill_manager to be kept per form. This will change
the metrics somewhat, but in a way that is more accurate than before.

https://codereview.chromium.org/242613006/diff/40001/components/autofill/cont...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/242613006/diff/40001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:657: // Record timestamp
on document load. This is used to measure overhead of
On 2014/04/24 01:50:19, Ilya Sherman wrote:
> nit: "overhead" -> "the overhead", "Autofill feature" -> "the Autofill
feature"

Done.

[Ilya Sherman, 2014-04-25 00:43:01]

Thanks!  Could you please also add test coverage to [
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
] for metrics recorded from a page with dynamically loaded forms?  (Sorry, I
know that test code is overly gross due to all the mocked methods.  I intend to
clean it up someday, but haven't found the time to do so yet.)

https://codereview.chromium.org/242613006/diff/60001/components/autofill/core...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/242613006/diff/60001/components/autofill/core...
components/autofill/core/browser/autofill_manager.cc:332:
forms_loaded_timestamps_[forms[i]] = timestamp;
nit: I'd prefer that you add curly braces around the body of the loop, and
possibly leave a blank line below it as well.

[Garrett Casto, 2014-04-29 22:57:43]

Tests added, PTAL.

https://codereview.chromium.org/242613006/diff/60001/components/autofill/core...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/242613006/diff/60001/components/autofill/core...
components/autofill/core/browser/autofill_manager.cc:332:
forms_loaded_timestamps_[forms[i]] = timestamp;
On 2014/04/25 00:43:02, Ilya Sherman wrote:
> nit: I'd prefer that you add curly braces around the body of the loop, and
> possibly leave a blank line below it as well.

Done.

[Ilya Sherman, 2014-05-01 01:08:23]

LGTM.  Thanks, Garrett!

https://codereview.chromium.org/242613006/diff/100001/components/autofill/cor...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/242613006/diff/100001/components/autofill/cor...
components/autofill/core/browser/autofill_metrics_unittest.cc:1051: // Make sure
that loading another form doesn't effect metrics from the first
nit: "effect" -> "affect"

[Garrett Casto, 2014-05-01 20:28:29]

https://codereview.chromium.org/242613006/diff/100001/components/autofill/cor...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/242613006/diff/100001/components/autofill/cor...
components/autofill/core/browser/autofill_metrics_unittest.cc:1051: // Make sure
that loading another form doesn't effect metrics from the first
On 2014/05/01 01:08:23, Ilya Sherman wrote:
> nit: "effect" -> "affect"

Done. I never get that right.

[Garrett Casto, 2014-05-01 20:28:35]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-05-01 20:29:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gcasto@chromium.org/242613006/120001

[commit-bot: I haz the power, 2014-05-01 21:28:51]

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

[commit-bot: I haz the power, 2014-05-01 21:28:51]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium

[Garrett Casto, 2014-05-02 07:23:51]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-05-02 07:24:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gcasto@chromium.org/242613006/140001

[commit-bot: I haz the power, 2014-05-02 07:45:54]

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

[commit-bot: I haz the power, 2014-05-02 07:45:55]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[Garrett Casto, 2014-05-03 00:29:50]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-05-03 00:30:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gcasto@chromium.org/242613006/140001

[commit-bot: I haz the power, 2014-05-03 00:58:38]

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

[commit-bot: I haz the power, 2014-05-03 00:58:39]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[Garrett Casto, 2014-05-04 06:59:49]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-05-04 06:59:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gcasto@chromium.org/242613006/140001

[commit-bot: I haz the power, 2014-05-04 16:31:31]

Change committed as 268107

[DaleCurtis, 2014-05-04 18:32:57]

A revert of this CL has been created in
https://codereview.chromium.org/265203002/ by dalecurtis@chromium.org.

The reason for reverting is: Broke DataDrivenHeuristics03 on the Mac debug bots:

http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac%2010.6%20Tes...
http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac%2010.7%20Tes....