Add delay to didAssociateFormControlsTimer so it doesn't fire too frequently

Add delay to didAssociateFormControlsTimer so it doesn't fire too frequently

When JS iteratively constructs a DOM tree containing form controls, didAssociateFormControlsTimer was fireing for every iteration, causing a massive slow down.

BUG=614856
Committed: https://crrev.com/6e1c00f9b9656319829c290a280b5ffb07bc4d1a
Cr-Commit-Position: refs/heads/master@{#404083}

[keishi, 2016-06-09 05:27:59]

keishi@chromium.org changed reviewers:
+ tkent@chromium.org

[keishi, 2016-06-09 05:28:00]

PTAL

[tkent, 2016-06-09 05:33:08]

lgtm.
We should have a comment explaining a reason of the delay.

[keishi, 2016-06-09 06:29:22]

On 2016/06/09 05:33:08, tkent wrote:
> lgtm.
> We should have a comment explaining a reason of the delay.

Thanks. Done.

[keishi, 2016-06-09 06:29:32]

The CQ bit was checked by keishi@chromium.org

[keishi, 2016-06-09 06:29:32]

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

[commit-bot: I haz the power, 2016-06-09 06:29:47]

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

[commit-bot: I haz the power, 2016-06-09 07:03:11]

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

[commit-bot: I haz the power, 2016-06-09 07:03:12]

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_...)

[keishi, 2016-06-09 09:03:54]

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

[keishi, 2016-06-09 09:03:55]

+vabr Looks like the tests need to wait for the timer to fire. How should I do
this? I can't find any examples.

[vabr (Chromium), 2016-06-09 14:59:46]

On 2016/06/09 09:03:55, keishi wrote:
> +vabr Looks like the tests need to wait for the timer to fire. How should I do
> this? I can't find any examples.

You probably have to wait until didAssociateFormControls gets called. Perhaps
you could add a helper class implementing blink::WebAutofillClient which would
listen for didAssociateFormControls and signal a base::WaitableEvent, which
would block the failing check in the tests?

Cheers,
Vaclav

[keishi, 2016-06-21 07:53:53]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-21 07:54:04]

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

[keishi, 2016-06-21 08:41:48]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-21 08:42:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2055693002/60001

[commit-bot: I haz the power, 2016-06-21 09:20:22]

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

[commit-bot: I haz the power, 2016-06-21 09:20:23]

Dry run: 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_...)

[keishi, 2016-06-21 12:48:38]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-21 12:48:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2055693002/80001

[keishi, 2016-06-21 12:51:14]

vabr@ could you take a look at the autofill tests. The blink code and test code
all run on the main thread so I ended up using a base::RunLoop to run the timer
instead of using base::WaitableEvent.

[commit-bot: I haz the power, 2016-06-21 14:04:13]

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

[commit-bot: I haz the power, 2016-06-21 14:04:14]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-06-22 05:50:35]

Thank you for the solution!
LGTM with a comment.

Cheers,
Vaclav

https://codereview.chromium.org/2055693002/diff/80001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/2055693002/diff/80001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.h:67: friend class
::MockAutofillAgent;
Instead of the friend declaration, could you make the didAssociateFormControls a
protected method in AutofillAgent?

Once the friend declaration is there, the code is prone to mess up testing with
implementation details, which leads to fragile tests.

[keishi, 2016-06-22 08:27:34]

keishi@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[keishi, 2016-06-22 08:27:35]

+phajdan.jr could you review the changes to
chrome/test/base/chrome_render_view_test.{h,cc} ? Thanks.

https://codereview.chromium.org/2055693002/diff/80001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.h (right):

https://codereview.chromium.org/2055693002/diff/80001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.h:67: friend class
::MockAutofillAgent;
On 2016/06/22 05:50:35, vabr (Chromium) wrote:
> Instead of the friend declaration, could you make the didAssociateFormControls
a
> protected method in AutofillAgent?
> 
> Once the friend declaration is there, the code is prone to mess up testing
with
> implementation details, which leads to fragile tests.

Done.

[Paweł Hajdan Jr., 2016-06-24 09:13:55]

https://codereview.chromium.org/2055693002/diff/100001/chrome/renderer/autofi...
File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right):

https://codereview.chromium.org/2055693002/diff/100001/chrome/renderer/autofi...
chrome/renderer/autofill/autofill_renderer_browsertest.cc:139:
WaitForAutofillDidAssociateFormControl();
Isn't this very similar to RunUntilIdle?

I'm wondering - can we have something more windowed, i.e. start listening for
didAssociateFormControls *before* it may be triggered, trigger it, and run the
loop not until idle, but until the listener quits it.

Note that I'm not familiar with blink threads etc. so feel free to just educate
me a bit on that.

[keishi, 2016-06-24 11:35:46]

https://codereview.chromium.org/2055693002/diff/100001/chrome/renderer/autofi...
File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right):

https://codereview.chromium.org/2055693002/diff/100001/chrome/renderer/autofi...
chrome/renderer/autofill/autofill_renderer_browsertest.cc:139:
WaitForAutofillDidAssociateFormControl();
On 2016/06/24 09:13:55, Paweł Hajdan Jr. wrote:
> Isn't this very similar to RunUntilIdle?
> 
> I'm wondering - can we have something more windowed, i.e. start listening for
> didAssociateFormControls *before* it may be triggered, trigger it, and run the
> loop not until idle, but until the listener quits it.
> 
> Note that I'm not familiar with blink threads etc. so feel free to just
educate
> me a bit on that.

RunUntilIdle isn't working because it quits before the scheduled timer fires.

I'm not sure how this CL is different from what you're describing.
We calls WaitForAutofillDidAssociateFormControl() before
didAssociateFormControls is expected.
didAssociateFormControls quits the base::RunLoop (in
MockAutofillAgent::didAssociateFormControls)

[Paweł Hajdan Jr., 2016-06-27 18:01:34]

https://codereview.chromium.org/2055693002/diff/100001/chrome/renderer/autofi...
File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right):

https://codereview.chromium.org/2055693002/diff/100001/chrome/renderer/autofi...
chrome/renderer/autofill/autofill_renderer_browsertest.cc:139:
WaitForAutofillDidAssociateFormControl();
On 2016/06/24 at 11:35:46, keishi wrote:
> On 2016/06/24 09:13:55, Paweł Hajdan Jr. wrote:
> > Isn't this very similar to RunUntilIdle?
> > 
> > I'm wondering - can we have something more windowed, i.e. start listening
for
> > didAssociateFormControls *before* it may be triggered, trigger it, and run
the
> > loop not until idle, but until the listener quits it.
> > 
> > Note that I'm not familiar with blink threads etc. so feel free to just
educate
> > me a bit on that.
> 
> RunUntilIdle isn't working because it quits before the scheduled timer fires.

Is it correct to keep RunUntilIdle calls then? I'm not entirely sure what the
_right_ thing is, but explaining things more should get us all on the right
track.

> I'm not sure how this CL is different from what you're describing.
> We calls WaitForAutofillDidAssociateFormControl() before
didAssociateFormControls is expected.

Is the way we do that non-racy? What guarantees whatever triggers
didAssociateFormControls runs after the test calls
WaitForAutofillDidAssociateFormControl? I'm not familiar with this code, but
it's not obviously to me what'd guarantee such call order.

> didAssociateFormControls quits the base::RunLoop (in
MockAutofillAgent::didAssociateFormControls)

Yup, that part matches my understanding and looks good.

[keishi, 2016-06-28 02:09:18]

https://codereview.chromium.org/2055693002/diff/100001/chrome/renderer/autofi...
File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right):

https://codereview.chromium.org/2055693002/diff/100001/chrome/renderer/autofi...
chrome/renderer/autofill/autofill_renderer_browsertest.cc:139:
WaitForAutofillDidAssociateFormControl();
On 2016/06/27 18:01:34, Paweł Hajdan Jr. wrote:
> On 2016/06/24 at 11:35:46, keishi wrote:
> > On 2016/06/24 09:13:55, Paweł Hajdan Jr. wrote:
> > > Isn't this very similar to RunUntilIdle?
> > > 
> > > I'm wondering - can we have something more windowed, i.e. start listening
> for
> > > didAssociateFormControls *before* it may be triggered, trigger it, and run
> the
> > > loop not until idle, but until the listener quits it.
> > > 
> > > Note that I'm not familiar with blink threads etc. so feel free to just
> educate
> > > me a bit on that.
> > 
> > RunUntilIdle isn't working because it quits before the scheduled timer
fires.
> 
> Is it correct to keep RunUntilIdle calls then? I'm not entirely sure what the
> _right_ thing is, but explaining things more should get us all on the right
> track.
Removed RunUntilIdle.

> > I'm not sure how this CL is different from what you're describing.
> > We calls WaitForAutofillDidAssociateFormControl() before
> didAssociateFormControls is expected.
> 
> Is the way we do that non-racy? What guarantees whatever triggers
> didAssociateFormControls runs after the test calls
> WaitForAutofillDidAssociateFormControl? I'm not familiar with this code, but
> it's not obviously to me what'd guarantee such call order.
The RenderThread is a mock so Blink main thread seems to be the same as this
test code, so we shouldn't have racing due to threads. I'm not sure if it
happens but I guess if the computer is very slow, the msg_loop_.RunUntilIdle
above could process the didAssociateFormControltimer timer task. Removing
msg_loop_.RunUntilIdle should eliminate that risk.

> > didAssociateFormControls quits the base::RunLoop (in
> MockAutofillAgent::didAssociateFormControls)
> 
> Yup, that part matches my understanding and looks good.

[Paweł Hajdan Jr., 2016-06-28 13:53:15]

LGTM

[keishi, 2016-06-29 07:04:48]

The CQ bit was checked by keishi@chromium.org

[keishi, 2016-06-29 07:04:49]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2055693002/#ps140001
(title: "fix")

[commit-bot: I haz the power, 2016-06-29 07:05:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 07:08:08]

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

[commit-bot: I haz the power, 2016-06-29 07:08:09]

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

[keishi, 2016-07-07 04:05:42]

The CQ bit was checked by keishi@chromium.org

[keishi, 2016-07-07 04:05:43]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
vabr@chromium.org, phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/2055693002/#ps160001
(title: "rebased")

[commit-bot: I haz the power, 2016-07-07 04:05:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 05:21:03]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-07 05:21:12]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 05:23:09]

Description was changed from

==========
Add delay to didAssociateFormControlsTimer so it doesn't fire too frequently

When JS iteratively constructs a DOM tree containing form controls,
didAssociateFormControlsTimer was fireing for every iteration, causing a massive
slow down.

BUG=614856
==========

to

==========
Add delay to didAssociateFormControlsTimer so it doesn't fire too frequently

When JS iteratively constructs a DOM tree containing form controls,
didAssociateFormControlsTimer was fireing for every iteration, causing a massive
slow down.

BUG=614856

Committed: https://crrev.com/6e1c00f9b9656319829c290a280b5ffb07bc4d1a
Cr-Commit-Position: refs/heads/master@{#404083}
==========

[commit-bot: I haz the power, 2016-07-07 05:23:10]

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