[Password Generation] Enable generation for dynamically created forms.

[Password Generation] Enable generation for dynamically created forms.

BUG=172188
Committed: https://crrev.com/93a96501c4f5924972b09e0dff0f879134927db5
Cr-Commit-Position: refs/heads/master@{#300418}

[Garrett Casto, 2014-10-15 22:11:45]

gcasto@chromium.org changed reviewers:
+ vabr@chromium.org

[Garrett Casto, 2014-10-15 22:11:46]

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

https://codereview.chromium.org/662493002/diff/20001/chrome/renderer/autofill...
chrome/renderer/autofill/password_generation_agent_browsertest.cc:368:
ProcessPendingMessages();
I'm slightly worried that this might flake because the timing of when
didAssociateFormControls() is run is not something that I understand perfectly.
I could explicitly call OnDynamicFormsSeen(GetMainFrame()), but that makes the
test weaker. I can go either way on this.

[vabr (Chromium), 2014-10-16 07:46:15]

Hi Garrett,

LGTM with comments.

Thanks,
Vaclav

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

https://codereview.chromium.org/662493002/diff/20001/chrome/renderer/autofill...
chrome/renderer/autofill/password_generation_agent_browsertest.cc:368:
ProcessPendingMessages();
On 2014/10/15 22:11:45, Garrett Casto wrote:
> I'm slightly worried that this might flake because the timing of when
> didAssociateFormControls() is run is not something that I understand
perfectly.
> I could explicitly call OnDynamicFormsSeen(GetMainFrame()), but that makes the
> test weaker. I can go either way on this.

If it does not flake during tryjobs and in the tree right after commit, I would
try the current version. But it may be helpful to add a code comment roughly
saying the same as you review comment here, to help fix the situation quickly if
the flakes occur.

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

https://codereview.chromium.org/662493002/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.cc:126: if
(password_elements_.empty()) {
If I understand correctly, this logs the event from the previous load, to avoid
logging both NO_SIGN_UP_DETECTED and SIGN_UP_DETECTED on pages with only
dynamically generated sign-up forms.
If that's the case, a code comment would be helpful to understand this. Also,
are we concerned about situations where the user does not navigate further
(e.g., closes the browser), and thus nothing is logged?

[Garrett Casto, 2014-10-16 19:14:06]

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

https://codereview.chromium.org/662493002/diff/20001/chrome/renderer/autofill...
chrome/renderer/autofill/password_generation_agent_browsertest.cc:368:
ProcessPendingMessages();
On 2014/10/16 07:46:15, vabr (Chromium) wrote:
> On 2014/10/15 22:11:45, Garrett Casto wrote:
> > I'm slightly worried that this might flake because the timing of when
> > didAssociateFormControls() is run is not something that I understand
> perfectly.
> > I could explicitly call OnDynamicFormsSeen(GetMainFrame()), but that makes
the
> > test weaker. I can go either way on this.
> 
> If it does not flake during tryjobs and in the tree right after commit, I
would
> try the current version. But it may be helpful to add a code comment roughly
> saying the same as you review comment here, to help fix the situation quickly
if
> the flakes occur.

Added a comment. I've run this over 200 times locally and it seems fine, but
there are some flakes that I've never been able to reproduce locally so still
might be a problem.

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

https://codereview.chromium.org/662493002/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_generation_agent.cc:126: if
(password_elements_.empty()) {
On 2014/10/16 07:46:15, vabr (Chromium) wrote:
> If I understand correctly, this logs the event from the previous load, to
avoid
> logging both NO_SIGN_UP_DETECTED and SIGN_UP_DETECTED on pages with only
> dynamically generated sign-up forms.
> If that's the case, a code comment would be helpful to understand this. Also,
> are we concerned about situations where the user does not navigate further
> (e.g., closes the browser), and thus nothing is logged?

I think that happens rarely enough not to worry about. Plus we already have this
issue for the other stats, so at least in this case they shouldn't be skewed
with respect to each other.

[Garrett Casto, 2014-10-16 19:14:11]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-10-16 19:15:32]

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

[commit-bot: I haz the power, 2014-10-16 19:25:38]

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

[commit-bot: I haz the power, 2014-10-16 19:25:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Garrett Casto, 2014-10-17 01:17:27]

gcasto@chromium.org changed reviewers:
+ isherman@chromium.org

[Garrett Casto, 2014-10-17 01:17:28]

And apparently CQ still doesn't think that I'm an OWNER/committer. Le sigh.
Ilya, can I get an LGTM?

[Ilya Sherman, 2014-10-17 22:12:08]

Rubber stamp LGTM.

[Ilya Sherman, 2014-10-17 22:12:11]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-10-17 22:14:04]

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

[commit-bot: I haz the power, 2014-10-17 23:20:17]

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

[commit-bot: I haz the power, 2014-10-17 23:20:18]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Garrett Casto, 2014-10-20 17:41:07]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-10-20 17:41:29]

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

[commit-bot: I haz the power, 2014-10-20 18:52:42]

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

[commit-bot: I haz the power, 2014-10-20 18:52:43]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Garrett Casto, 2014-10-20 20:12:20]

I'm not sure why this is failing. The test seems pretty unrelated, but it's
consistent. I'm going to send this through one more time while working on
reproducing this failure locally.

[Garrett Casto, 2014-10-20 20:12:27]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-10-20 20:13:55]

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

[commit-bot: I haz the power, 2014-10-20 21:17:58]

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

[commit-bot: I haz the power, 2014-10-20 21:17:59]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Garrett Casto, 2014-10-20 23:05:56]

Patchset #4 (id:60001) has been deleted

[Garrett Casto, 2014-10-20 23:07:15]

gcasto@chromium.org changed reviewers:
+ boliu@chromium.org

[Garrett Casto, 2014-10-20 23:07:16]

Bo, could you take a look at the android_webview changes?

[Garrett Casto, 2014-10-20 23:17:39]

gcasto@chromium.org changed reviewers:
+ rouslan@chromium.org

[Garrett Casto, 2014-10-20 23:17:39]

For background, this feature is not currently enabled on Android which is why
the agent was NULL to begin with. The feature is behind a flag, so this change
won't enable it. Given that we are going to have UI on Android enabled shortly
this seems more reasonable than adding if statements in AutofillAgent just for
testing.

[Garrett Casto, 2014-10-20 23:20:13]

gcasto@chromium.org changed reviewers:
- rouslan@chromium.org

[boliu, 2014-10-20 23:24:30]

boliu@chromium.org changed reviewers:
+ rouslan@chromium.org

[boliu, 2014-10-20 23:24:31]

You mention the switch, where is it disabled? If it's in chrome/ code, it does
not affect android webview.

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

https://codereview.chromium.org/662493002/diff/80001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:126:
password_autofill_agent_(password_autofill_agent),
This seems unsafe as to have two RVObservers have raw pointers to each other. I
don't think destruction order for ObserverList is explicitly defined, although
it's implemented as a vector, might be ok by coincidence.

Much better to have AutofillAgent own PasswordAutofillAgent and
PasswordGenerationAgent, since they all appear to have the same life time
anyway?

I guess it's not an issue of this CL, but should be fixed.

[Garrett Casto, 2014-10-20 23:40:59]

Switch is in components/,
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil....
Though thinking about this some more, I think that I may have misunderstood what
this code is for. The webview is for native Android outside of Chrome, right? In
that situation I guess we don't ever want to allow generation (It's specifically
a Chrome feature). Assuming that's true, I'll just add null checks in
autofill_agent.

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

https://codereview.chromium.org/662493002/diff/80001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:126:
password_autofill_agent_(password_autofill_agent),
On 2014/10/20 23:24:31, boliu wrote:
> This seems unsafe as to have two RVObservers have raw pointers to each other.
I
> don't think destruction order for ObserverList is explicitly defined, although
> it's implemented as a vector, might be ok by coincidence.
> 
> Much better to have AutofillAgent own PasswordAutofillAgent and
> PasswordGenerationAgent, since they all appear to have the same life time
> anyway?
> 
> I guess it's not an issue of this CL, but should be fixed.

Well, access and deletion of these objects always happens on the UI thread, so I
think that this is safe regardless of destruction order. The setup of having
PasswordAutofillAgent owned by AutofillAgent predates me, so I'm not entirely
sure what the reasoning for this is, but it does make testing easier.

[boliu, 2014-10-20 23:45:10]

You can add kEnablePasswordGeneration in aw_main_delegate.cc

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

https://codereview.chromium.org/662493002/diff/80001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:126:
password_autofill_agent_(password_autofill_agent),
On 2014/10/20 23:40:59, Garrett Casto wrote:
> On 2014/10/20 23:24:31, boliu wrote:
> > This seems unsafe as to have two RVObservers have raw pointers to each
other.
> I
> > don't think destruction order for ObserverList is explicitly defined,
although
> > it's implemented as a vector, might be ok by coincidence.
> > 
> > Much better to have AutofillAgent own PasswordAutofillAgent and
> > PasswordGenerationAgent, since they all appear to have the same life time
> > anyway?
> > 
> > I guess it's not an issue of this CL, but should be fixed.
> 
> Well, access and deletion of these objects always happens on the UI thread, so
I
> think that this is safe regardless of destruction order. The setup of having
> PasswordAutofillAgent owned by AutofillAgent predates me, so I'm not entirely
> sure what the reasoning for this is, but it does make testing easier.

The issue is accessing the raw pointer in AutofillAgent destructor. There is
assurance that the object it points to is still there, since ObserverList
destruction order isn't defined.

Not trying to block this CL on this though..

[boliu, 2014-10-20 23:45:28]

On 2014/10/20 23:45:10, boliu wrote:
> You can add kEnablePasswordGeneration in aw_main_delegate.cc

Ehh, Disable

[Garrett Casto, 2014-10-21 00:02:08]

gcasto@chromium.org changed reviewers:
- rouslan@chromium.org

[Garrett Casto, 2014-10-21 00:02:22]

gcasto@chromium.org changed reviewers:
- boliu@chromium.org

[Garrett Casto, 2014-10-21 00:02:30]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-10-21 00:04:38]

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

[commit-bot: I haz the power, 2014-10-21 02:08:40]

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

[commit-bot: I haz the power, 2014-10-21 02:08:42]

Exceeded time limit waiting for builds to trigger.

[Garrett Casto, 2014-10-21 03:18:14]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-10-21 03:19:07]

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

[commit-bot: I haz the power, 2014-10-21 04:15:40]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2014-10-21 04:16:32]

Patchset 5 (id:??) landed as
https://crrev.com/93a96501c4f5924972b09e0dff0f879134927db5
Cr-Commit-Position: refs/heads/master@{#300418}