Blink: prevent didAssociateFormControls notification while parsing the document.

Blink: prevent didAssociateFormControls notification while parsing the document.

Also remove Document::m_associatedFormControls unused by the consumer.

BUG=631958
Committed: https://crrev.com/c7e3c1f85c4691489409104d9ab53d30b52cd8d9
Cr-Commit-Position: refs/heads/master@{#408388}

[vasilii, 2016-07-27 13:43:47]

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

[commit-bot: I haz the power, 2016-07-27 13:43:59]

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

[vasilii, 2016-07-27 13:46:03]

vasilii@chromium.org changed reviewers:
+ tkent@chromium.org, vabr@chromium.org

[vasilii, 2016-07-27 13:46:04]

Hi guys,

please review the CL.

[vabr (Chromium), 2016-07-27 14:06:50]

LGTM, thanks!
Vaclav

https://codereview.chromium.org/2189513004/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/2189513004/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_browsertest.cc:209:
NoPromptIfPasswordFormManagerDestroyed) {
Just a note: One thing I'm wary of here is that this test would be flaky (not
failing 100%) without the rest of this patch, because the core problem is a race
between how long rendering takes and the artificial 0.3s delay.
Having said that, this is a property of the issue you are fixing, not a of a
particular way to write the test, so this is fine with me.

https://codereview.chromium.org/2189513004/diff/1/components/autofill/content...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/2189513004/diff/1/components/autofill/content...
components/autofill/content/renderer/autofill_agent.cc:821: // Only monitors
dynamic forms created in the top frame. Dynamic forms
Have we now understood why the comment speaks about main frame only? Which part
of the code makes sure that this AutofillAgent instance is associated with the
main frame?

[vasilii, 2016-07-27 14:25:35]

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

[commit-bot: I haz the power, 2016-07-27 14:25:51]

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

[vasilii, 2016-07-27 14:26:09]

Description was changed from

==========
Blink: prevent didAssociateFormControls notification while parsing the document.

Also remove Document::m_associatedFormControls unused by the consumer.

BUG=631958
==========

to

==========
Blink: prevent didAssociateFormControls notification while parsing the document.

Also remove Document::m_associatedFormControls unused by the consumer.

BUG=631958
==========

[vasilii, 2016-07-27 14:26:09]

vasilii@chromium.org changed reviewers:
+ sky@chromium.org

[vasilii, 2016-07-27 14:26:59]

Scott, please review chrome/test/base/chrome_render_view_test.cc

https://codereview.chromium.org/2189513004/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/2189513004/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_manager_browsertest.cc:209:
NoPromptIfPasswordFormManagerDestroyed) {
On 2016/07/27 14:06:49, vabr (Chromium) wrote:
> Just a note: One thing I'm wary of here is that this test would be flaky (not
> failing 100%) without the rest of this patch, because the core problem is a
race
> between how long rendering takes and the artificial 0.3s delay.
> Having said that, this is a property of the issue you are fixing, not a of a
> particular way to write the test, so this is fine with me.

It's actually failing > 50% of the times on my machine. Thus, if the production
code breaks I expect the flakiness to be noticeable.

https://codereview.chromium.org/2189513004/diff/1/components/autofill/content...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/2189513004/diff/1/components/autofill/content...
components/autofill/content/renderer/autofill_agent.cc:821: // Only monitors
dynamic forms created in the top frame. Dynamic forms
On 2016/07/27 14:06:49, vabr (Chromium) wrote:
> Have we now understood why the comment speaks about main frame only? Which
part
> of the code makes sure that this AutofillAgent instance is associated with the
> main frame?

Looks false to me.

[vabr (Chromium), 2016-07-27 14:28:44]

Thanks for answers, still LGTM!

[sky, 2016-07-27 15:30:47]

LGTM

[commit-bot: I haz the power, 2016-07-27 16:52:35]

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

[commit-bot: I haz the power, 2016-07-27 16:52:36]

Dry run: This issue passed the CQ dry run.

[tkent, 2016-07-28 00:21:24]

Let's rename ChromeClient::didAssociateFormControls and
WebAutofillClient::didAssociateFormControls to
didAssociateFormControlsAfterLoad, didAssociateFormControlsAfterParsing,
didAssociateFormControlsDynamically, or something.

https://codereview.chromium.org/2189513004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2189513004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:5740: if (!frame() ||
!frame()->page() || !hasFinishedParsing())
We should use !loadEventFinished() if this should be consistent with
frame->isLoading() in AutofillAgent::didAssociateFormControls.

[vasilii, 2016-07-28 12:38:30]

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

[commit-bot: I haz the power, 2016-07-28 12:38:44]

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

[vasilii, 2016-07-28 12:38:50]

Renamed.

https://codereview.chromium.org/2189513004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2189513004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:5740: if (!frame() ||
!frame()->page() || !hasFinishedParsing())
On 2016/07/28 00:21:24, tkent wrote:
> We should use !loadEventFinished() if this should be consistent with
> frame->isLoading() in AutofillAgent::didAssociateFormControls.

Done.

[commit-bot: I haz the power, 2016-07-28 13:51:05]

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

[commit-bot: I haz the power, 2016-07-28 13:51:05]

Dry run: This issue passed the CQ dry run.

[tkent, 2016-07-28 14:17:47]

lgtm

[vasilii, 2016-07-28 14:25:47]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2016-07-28 14:25:47]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2189513004/#ps40001
(title: "rename")

[commit-bot: I haz the power, 2016-07-28 14:26:09]

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-28 14:29:56]

Description was changed from

==========
Blink: prevent didAssociateFormControls notification while parsing the document.

Also remove Document::m_associatedFormControls unused by the consumer.

BUG=631958
==========

to

==========
Blink: prevent didAssociateFormControls notification while parsing the document.

Also remove Document::m_associatedFormControls unused by the consumer.

BUG=631958
==========

[commit-bot: I haz the power, 2016-07-28 14:29:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-28 14:31:29]

Description was changed from

==========
Blink: prevent didAssociateFormControls notification while parsing the document.

Also remove Document::m_associatedFormControls unused by the consumer.

BUG=631958
==========

to

==========
Blink: prevent didAssociateFormControls notification while parsing the document.

Also remove Document::m_associatedFormControls unused by the consumer.

BUG=631958

Committed: https://crrev.com/c7e3c1f85c4691489409104d9ab53d30b52cd8d9
Cr-Commit-Position: refs/heads/master@{#408388}
==========

[commit-bot: I haz the power, 2016-07-28 14:31:30]

Patchset 3 (id:??) landed as
https://crrev.com/c7e3c1f85c4691489409104d9ab53d30b52cd8d9
Cr-Commit-Position: refs/heads/master@{#408388}