Do not trigger layout in PasswordAutofillAgent::DidFinishDocumentLoad

Do not trigger layout in PasswordAutofillAgent::DidFinishDocumentLoad

The CL introduces autofill::form_util::ScopedLayoutPreventer. If an object of
this type is created, the code from form_util is supposed to avoid actions
resulting in layout computation. In particular, this CL ensures that
isFocusable is not called in such case.

R=tkent@chromium.org, mathp@chromium.org

BUG=595078
Committed: https://crrev.com/5f9fce356303cad7ae82fa7f4b14a6fbc4d76ee6
Cr-Commit-Position: refs/heads/master@{#396704}

[vabr (Chromium), 2016-05-25 14:48:52]

Hi tkent@ and mathp@,

Please let me know what you think of this implementation of
http://crbug.com/595078#c13? I chose the static bool over passing the
information in arguments, because (1) it would change a ton of function
signatures, and (2) it is not clear that there is just a single place in
form_util where this is needed.

tkent@ -- do you have any idea how to test that layout computation was not done
in Blink? I'd like to add a test which calls
PasswordAutofillAgent::DidFinishDocumentLoad and checks it did not result in a
layout.

Cheers,
Vaclav

[Mathieu, 2016-05-25 17:28:23]

On 2016/05/25 at 14:48:52, vabr wrote:
> Hi tkent@ and mathp@,
> 
> Please let me know what you think of this implementation of
http://crbug.com/595078#c13? I chose the static bool over passing the
information in arguments, because (1) it would change a ton of function
signatures, and (2) it is not clear that there is just a single place in
form_util where this is needed.
> 
> tkent@ -- do you have any idea how to test that layout computation was not
done in Blink? I'd like to add a test which calls
PasswordAutofillAgent::DidFinishDocumentLoad and checks it did not result in a
layout.
> 
> Cheers,
> Vaclav

Seems like a reasonable way to do it, if that's the ultimate fix determined by
tkent@

[tkent, 2016-05-25 23:06:37]

lgtm.  I think this will fix the crash.

[Mathieu, 2016-05-26 00:32:37]

On 2016/05/25 at 23:06:37, tkent wrote:
> lgtm.  I think this will fix the crash.

Hi tkent@, vaclav has a question around testing for you do you have an idea?

[tkent, 2016-05-26 01:02:40]

I think there are no reasonable ways to test this.

[vabr (Chromium), 2016-05-30 08:13:24]

On 2016/05/26 01:02:40, tkent wrote:
> I think there are no reasonable ways to test this.

Thanks. Given the approval, I will land this then, to fix the crash.

I will think about a way to test this at least within autofill, as in: did the
agent trigger the layout preventer in DidFinishDocumentLoad? (Leaving the "Did
the preventer really prevent anything" untested, as it seems untestable.)

The above could be done now if I added some test-only tracking in the production
code of the autofill utils. But a better way would be to refactor the utils to
be mockable, i.e., go form a set of static functions to an object distributing
them, and replacing that object in tests. I will file a bug for that.

Mathieu, please do let me know if you want me to do the test by adding the
test-only tracking in the production code. My concers are mainly breaking the
prod vs. test separation and potential performance impact.

Cheers,
Vaclav

[vabr (Chromium), 2016-05-30 08:13:53]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-05-30 08:14:02]

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

[vabr (Chromium), 2016-05-30 08:22:03]

On 2016/05/30 08:13:24, vabr (Chromium) wrote:
> The above could be done now if I added some test-only tracking in the
production
> code of the autofill utils. But a better way would be to refactor the utils to
> be mockable, i.e., go form a set of static functions to an object distributing
> them, and replacing that object in tests. I will file a bug for that.

The bug is http://crbug.com/615784. Mathieu (or anybody else interested), please
feel free to object or comment there as well.

Cheers,
Vaclav

[commit-bot: I haz the power, 2016-05-30 09:41:29]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-05-30 09:42:53]

Description was changed from

==========
Do not trigger layout in PasswordAutofillAgent::DidFinishDocumentLoad

The CL introduces autofill::form_util::ScopedLayoutPreventer. If an object of
this type is created, the code from form_util is supposed to avoid actions
resulting in layout computation. In particular, this CL ensures that
isFocusable is not called in such case.

R=tkent@chromium.org, mathp@chromium.org

BUG=595078
==========

to

==========
Do not trigger layout in PasswordAutofillAgent::DidFinishDocumentLoad

The CL introduces autofill::form_util::ScopedLayoutPreventer. If an object of
this type is created, the code from form_util is supposed to avoid actions
resulting in layout computation. In particular, this CL ensures that
isFocusable is not called in such case.

R=tkent@chromium.org, mathp@chromium.org

BUG=595078

Committed: https://crrev.com/5f9fce356303cad7ae82fa7f4b14a6fbc4d76ee6
Cr-Commit-Position: refs/heads/master@{#396704}
==========

[commit-bot: I haz the power, 2016-05-30 09:42:54]

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