Add new method xhrSucceeded() to WebAutofillClient.

Add new method xhrSucceeded() to WebAutofillClient.

Expose when an XHR request has successfully completed to the embedder.
This can be a useful signal for autofill or password management to determine
if a form submit occurred without navigation.

BUG=464891
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192050

[Garrett Casto, 2015-03-13 06:40:20]

gcasto@chromium.org changed reviewers:
+ kouhei@chromium.org, mkwst@chromium.org

[Garrett Casto, 2015-03-13 06:40:21]

kouhei@: XMLHTTPRequest changes. One thing that I believe is true that I want to
verify is that this new function will not be called until after the JS
onreadystatechange handler has finished.
mkwst@: The rest

Testing for this code will be added in Chromium.

[kouhei (in TOK), 2015-03-13 07:22:32]

kouhei@chromium.org changed reviewers:
+ tyoshino@chromium.org

[kouhei (in TOK), 2015-03-13 07:22:33]

This is a "big hammer" solution to the problem, so I'd prefer other way if you
have alternative ideas. In particular, I'm worried that this patch would only
cover XHR case and there are other methods for asynchronous data submission
(window.fetch, WebSocket, etc.).

+tyoshino-san: Do you have any other suggestions on how to do this?

https://codereview.chromium.org/1006793002/diff/20001/Source/core/xmlhttprequ...
File Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1006793002/diff/20001/Source/core/xmlhttprequ...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:674: if (status() == 200)
May be allow all 2xx?

https://codereview.chromium.org/1006793002/diff/20001/Source/core/xmlhttprequ...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:675:
document()->frame()->page()->chrome().client().xhrSucceeded(document()->frame());
I think the better place for this would be in endLoading. The added logic here
doesn't "dispatchReadyStateChangeEvent".

[Garrett Casto, 2015-03-13 18:37:07]

There are two other options that I'm exploring, but they seem less likely to pan
out. The most promising is adding a notification when HTMLFormElements are no
longer visible, but I'm still not sure if that is going to work out. Certainly
having a solution that was more general would be nice, but right now the
majority of sites are using XHR, and if we need to add similar hooks to other
API calls, I'm assuming that wouldn't be particularly difficult.

Any other suggestions are certainly welcome.

https://codereview.chromium.org/1006793002/diff/20001/Source/core/xmlhttprequ...
File Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1006793002/diff/20001/Source/core/xmlhttprequ...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:674: if (status() == 200)
On 2015/03/13 07:22:32, kouhei wrote:
> May be allow all 2xx?

Done

https://codereview.chromium.org/1006793002/diff/20001/Source/core/xmlhttprequ...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:675:
document()->frame()->page()->chrome().client().xhrSucceeded(document()->frame());
On 2015/03/13 07:22:33, kouhei wrote:
> I think the better place for this would be in endLoading. The added logic here
> doesn't "dispatchReadyStateChangeEvent".

Done.

[Garrett Casto, 2015-03-16 07:42:50]

After further consideration, I'm pretty sure that this is the best approach at
the moment. Adding hooks to form disappearance is both more complicated and more
prone to false positives. For example, if a user tries to login but fails (e.g.
bad password) and then cancels the login process, just watching for form
disappearance would trigger saving, while looking for form disappearance + XHR
wouldn't. It's certainly still possible to have false positives (user then
clicks on a link which navigates via XHR), but it's less likely. I think that
this is probably true of any change that we make, but this one has the advantage
of both being technically trivial and being effective on most of the sites that
we currently know of that perform login this way.

In the future, I'm assuming that adding similar hooks to e.g. window.fetch
should hopefully be easy, though I haven't yet looked into that yet. We will
still have problems with sites that do server communication outside of
standardized channels, but luckily those sites seem to be rarer at the moment.

[kouhei (in TOK), 2015-03-16 07:48:14]

On 2015/03/16 07:42:50, Garrett Casto wrote:
> After further consideration, I'm pretty sure that this is the best approach at
> the moment. Adding hooks to form disappearance is both more complicated and
more
> prone to false positives. For example, if a user tries to login but fails
(e.g.
> bad password) and then cancels the login process, just watching for form
> disappearance would trigger saving, while looking for form disappearance + XHR
> wouldn't. It's certainly still possible to have false positives (user then
> clicks on a link which navigates via XHR), but it's less likely. I think that
> this is probably true of any change that we make, but this one has the
advantage
> of both being technically trivial and being effective on most of the sites
that
> we currently know of that perform login this way.
> 
> In the future, I'm assuming that adding similar hooks to e.g. window.fetch
> should hopefully be easy, though I haven't yet looked into that yet. We will
> still have problems with sites that do server communication outside of
> standardized channels, but luckily those sites seem to be rarer at the moment.

Got it. I'm ok with the change if tyoshino-san does't have any other suggestion
for this.
Would you upload the revised patch?

[Garrett Casto, 2015-03-16 07:57:18]

Apologies, uploaded.

[Mike West, 2015-03-16 08:07:33]

On 2015/03/16 at 07:57:18, gcasto wrote:
> Apologies, uploaded.

Sorry, I missed this last week.

I think the change is fine % comments. Will we be able to get rid of some of
Vadym's pushState() code by triggering off of XHR submissions? Or do we need
both?

[tyoshino (SeeGerritForStatus), 2015-03-16 08:59:19]

> Got it. I'm ok with the change if tyoshino-san does't have any other
suggestion for this.

Hmm, no, I don't.

> kouhei@: XMLHTTPRequest changes. One thing that I believe is true that I want
to verify is that this new function will not be called until after the JS
onreadystatechange handler has finished.

Verified.

Could you point me to the Chromium side patch?
https://codereview.chromium.org/1002653002/ This one?

[tyoshino (SeeGerritForStatus), 2015-03-16 08:59:28]

https://codereview.chromium.org/1006793002/diff/40001/Source/core/xmlhttprequ...
File Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1006793002/diff/40001/Source/core/xmlhttprequ...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:672: return;
move also this to L1581?

[Mike West, 2015-03-16 09:02:44]

Er. I said % comments, and then didn't submit them. Sorry! Same thing that
tyoshino-san noted. :)

https://codereview.chromium.org/1006793002/diff/40001/Source/core/xmlhttprequ...
File Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1006793002/diff/40001/Source/core/xmlhttprequ...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:672: return;
This doesn't make much sense. You're returning out of the function anyway, and
none of these accessors have side-effects...

https://codereview.chromium.org/1006793002/diff/40001/Source/core/xmlhttprequ...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:1583:
document()->frame()->page()->chrome().client().xhrSucceeded(document()->frame());
Did you mean to move them here?

[kouhei (in TOK), 2015-03-16 10:08:43]

lgtm % comments by tyoshino/mkwst

[Garrett Casto, 2015-03-16 17:31:55]

Sorry for the bad upload. Probably shouldn't be working at 1 in the morning.

mkwst@: This will basically supersede the history.pushState() change, thought
it's mostly just changing the render side implementation and leaving the browser
changes that Vadym made.

tyoshino@: That was one version of the Chrome changes that I was making, but
it's incredibly unpolished and contains some other changes mixed in. I had just
uploaded it to patch to a different computer. The real change will be at
https://codereview.chromium.org/1012853002/ (currently lacking tests).

https://codereview.chromium.org/1006793002/diff/40001/Source/core/xmlhttprequ...
File Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1006793002/diff/40001/Source/core/xmlhttprequ...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:672: return;
On 2015/03/16 09:02:43, Mike West wrote:
> This doesn't make much sense. You're returning out of the function anyway, and
> none of these accessors have side-effects...

Should have been moved to 1581.

https://codereview.chromium.org/1006793002/diff/40001/Source/core/xmlhttprequ...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:672: return;
On 2015/03/16 08:59:28, tyoshino wrote:
> move also this to L1581?

Done.

[Mike West, 2015-03-16 18:56:14]

public/ LGTM.

[Garrett Casto, 2015-03-17 18:08:22]

The CQ bit was checked by gcasto@chromium.org

[Garrett Casto, 2015-03-17 18:08:23]

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

[commit-bot: I haz the power, 2015-03-17 18:08:41]

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

[Garrett Casto, 2015-03-17 21:35:36]

The CQ bit was checked by gcasto@chromium.org

[Garrett Casto, 2015-03-17 21:35:37]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/1006793002/#ps80001
(title: "Test")

[commit-bot: I haz the power, 2015-03-17 21:36:01]

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

[commit-bot: I haz the power, 2015-03-18 00:56:40]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192050

[tyoshino (SeeGerritForStatus), 2015-03-18 02:27:27]

lgtm

Thanks > Chromium side patch