PlzNavigate: inform the WebFrameClient that a form will be submitted

PlzNavigate: inform the WebFrameClient that a form will be submitted

This CL makes it so that the WebFrameClient is informed that a form will be
submitted when the embedder handles the navigation.

BUG=504347
Committed: https://crrev.com/bca103622d505bb537dbc2032de5fcd051c39e0d
Cr-Commit-Position: refs/heads/master@{#379001}

[clamy, 2015-11-16 16:04:49]

Description was changed from

==========
PlzNavigate: inform the WebFrameClient that a form will be submitted

This CL makes it so that the WebFrameClient is informed that a form will be
submitted when the embedder handls the navigation. In order to properly
identify this case, the CL also switches the navigation policy returned by
RenderFrameImpl::decidePolicyForNavigation to
WebNavigationPolicyHandledByClient when the browser handles the navigation.
The CL results in all Autofill browser tests passing with PlzNavigate enabled.

BUG=504347
==========

to

==========
PlzNavigate: inform the WebFrameClient that a form will be submitted

This CL makes it so that the WebFrameClient is informed that a form will be
submitted when the embedder handls the navigation. In order to properly
identify this case, the CL also switches the navigation policy returned by
RenderFrameImpl::decidePolicyForNavigation to
WebNavigationPolicyHandledByClient when the browser handles the navigation.
The CL results in all Autofill browser tests passing with PlzNavigate enabled.

BUG=504347
==========

[clamy, 2015-11-16 16:04:50]

clamy@chromium.org changed reviewers:
+ japhet@google.com, nasko@chromium.org

[clamy, 2015-11-16 16:05:03]

@nasko, japhet: PTAL

[clamy, 2015-11-16 16:05:43]

clamy@chromium.org changed reviewers:
+ japhet@chromium.org
- japhet@google.com

[Nate Chapin, 2015-11-16 20:14:45]

On 2015/11/16 16:05:03, clamy wrote:
> @nasko, japhet: PTAL

dispatchWillSubmitForm() already does this. Can we move that earlier?

[clamy, 2015-11-17 14:38:02]

This would mean moving it above shouldContinueForNavigationPolicy, which may
make the navigation stop (if you get a NavitionpolicyIgnore). I'm worried of
possible side effects there.

[clamy, 2015-11-25 14:22:14]

@japhet: friendly ping :).

[Nate Chapin, 2015-12-08 23:18:38]

Sorry for delay, I'm not catching up very quickly after vacation.

https://codereview.chromium.org/1446253002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1446253002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2573:
navigation_state->set_transition_type(ui::PAGE_TRANSITION_FORM_SUBMIT);
I think you can check and modify
pending_navigation_params_.common_params_.transition and get the same behavior?
That removes one of the bits of data that is collected from
provisionalDataSource().

https://codereview.chromium.org/1446253002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2579:
internal_data->set_searchable_form_encoding(
Can we stash these on one of the navigation params structs instead? If so, then
maybe we can get away without special-casing for kEnableBrowserSideNavigation,
because we won't need the provisionalDataSource() at all.

https://codereview.chromium.org/1446253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1446253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1340:
m_progressTracker->progressStarted(false);
I don't particularly like the special case boolean here. Is there anything we
can do to make it unnecessary? Why is components/html_viewer/ ok receiving this
message but not PlzNavigate?

[clamy, 2015-12-11 14:50:06]

Thanks!

https://codereview.chromium.org/1446253002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1446253002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2573:
navigation_state->set_transition_type(ui::PAGE_TRANSITION_FORM_SUBMIT);
On 2015/12/08 23:18:38, Nate Chapin wrote:
> I think you can check and modify
> pending_navigation_params_.common_params_.transition and get the same
behavior?
> That removes one of the bits of data that is collected from
> provisionalDataSource().

At that point pending_navigation_params_ should be null and the
CommonNavigationParams have been transfered to the NavigationState. (This
happens after didCreateDataSource). Also using the pending_navigation_params_
would be a problem in renderer-initiated navigations, since we don't have them
there.

https://codereview.chromium.org/1446253002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2579:
internal_data->set_searchable_form_encoding(
On 2015/12/08 23:18:38, Nate Chapin wrote:
> Can we stash these on one of the navigation params structs instead? If so,
then
> maybe we can get away without special-casing for kEnableBrowserSideNavigation,
> because we won't need the provisionalDataSource() at all.

As explained above, we can't use the pending_navigation_params_ here.

https://codereview.chromium.org/1446253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1446253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1340:
m_progressTracker->progressStarted(false);
On 2015/12/08 23:18:38, Nate Chapin wrote:
> I don't particularly like the special case boolean here. Is there anything we
> can do to make it unnecessary? Why is components/html_viewer/ ok receiving
this
> message but not PlzNavigate?

I think this is due to how the loading state is implemented in
components/html_viewer/ vs in content/. Receiving that message throws off the
load state tracking in the browser. I have tried to make it work while receiving
it, but this means ignoring any state message sent by the renderer. This in turn
stops some tests from working, since they rely on very precise behavior from the
browser that I cannot match . Honestly, the way this work on the browser side is
not good (it used to be worse though), and we can have a discussion to see if it
can be simplified. Otherwise I don't have a choice but not to send this message
to the browser.

[dcheng, 2015-12-11 17:14:05]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2015-12-11 17:14:06]

https://codereview.chromium.org/1446253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1446253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:125:
m_frame->loader().client()->didStartLoading(NavigationToDifferentDocument);
We absolutely can not special-case mandoline here. We already have trouble with
varying the order of loading-related callbacks. Allowing callbacks to optionally
occur just makes it even worse.

creis@ added some history navigation code that takes advantage of
NavigationPolicyHandledByClient recently, and as far as I know, it Just Worked.
How does //content handle this so that it doesn't need a special case here?

[clamy, 2015-12-14 15:47:19]

https://codereview.chromium.org/1446253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ProgressTracker.cpp (right):

https://codereview.chromium.org/1446253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ProgressTracker.cpp:125:
m_frame->loader().client()->didStartLoading(NavigationToDifferentDocument);
On 2015/12/11 17:14:06, dcheng wrote:
> We absolutely can not special-case mandoline here. We already have trouble
with
> varying the order of loading-related callbacks. Allowing callbacks to
optionally
> occur just makes it even worse.
> 
> creis@ added some history navigation code that takes advantage of
> NavigationPolicyHandledByClient recently, and as far as I know, it Just
Worked.
> How does //content handle this so that it doesn't need a special case here?

The issue here is the combination of what content clients expect + PlzNavigate
loading decisions + the fact that we allow concurrent navigations in a FTN
(something that Mandoline people seemed not to bother about when they landed
this). Roughly, the browser can decide (for valid reasons) not to proceed with
the renderer-initiated navigations. So in the browser, we should not consider
that our frame is loading during that time (or the page may end up in a
perpetually loading state). But if we inform the client here, then the renderer
sends a DidStartLoading message to the browser. The simple fix would seem to be
to ignore that message, which means ignoring all DidStartLoading IPCs from the
renderer since we can't properly distinguish that case.

But, doing that prevents us from reproducing the exact behavior with which we
call WCO::DidStartLoading when PlzNavigate is enabled. This is because the
firing of WCO::DidStartLoading is heavily dependent on the notion of RFH, and it
also depends on the presence of a pending RFh in the FTN (something a bit hard
to match in PlzNavigate before we actually tell the frame to commit the
navigation). We got away until then by reproducing part of this logic when the
frame is about to commit the navigation/the error page. However, us ignoring the
signals from the renderer effectively prevent us from firing
WCO::DidStartLoading based on what the renderer about to commit is telling us.

To sum it up, ideally:
1) with PlzNavigate enabled we should ignore all DidStartLoading IPCs from the
renderer except for in-page navigations (since for all other navigations, the
browser decides whether it should proceed or not).
2) we should have a simpler model for deciding when to fire WCO::DidStartLoading
so that it can work with PlzNavigate as well.

I will be looking at 2, but this requires a rewrite of several tests that rely
on the current behavior for WCO::DidStartLoading. If that goes through, I can
then have 1 work, and we won't need to special case here.

[clamy, 2016-02-24 15:54:20]

Description was changed from

==========
PlzNavigate: inform the WebFrameClient that a form will be submitted

This CL makes it so that the WebFrameClient is informed that a form will be
submitted when the embedder handls the navigation. In order to properly
identify this case, the CL also switches the navigation policy returned by
RenderFrameImpl::decidePolicyForNavigation to
WebNavigationPolicyHandledByClient when the browser handles the navigation.
The CL results in all Autofill browser tests passing with PlzNavigate enabled.

BUG=504347
==========

to

==========
PlzNavigate: inform the WebFrameClient that a form will be submitted

This CL makes it so that the WebFrameClient is informed that a form will be
submitted when the embedder handles the navigation.

BUG=504347
==========

[clamy, 2016-02-24 16:20:24]

PTAL. Following the refactoring of DidStart/StopLoading and the switch to
WebNavigationPolicy::HandledByClient in PlzNavigate, this patch has been rebased
and is now much simpler.

[dcheng, 2016-02-24 21:24:55]

https://codereview.chromium.org/1446253002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1446253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1359:
client()->dispatchWillSubmitForm(form);
Just curious: shouldn't the embedder "know" it is dealing with a form? It seems
a little unusual that Blink has to tell the embedder about something it should
already know.

[clamy, 2016-02-25 14:34:07]

https://codereview.chromium.org/1446253002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1446253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1359:
client()->dispatchWillSubmitForm(form);
On 2016/02/24 21:24:55, dcheng wrote:
> Just curious: shouldn't the embedder "know" it is dealing with a form? It
seems
> a little unusual that Blink has to tell the embedder about something it should
> already know.

I'm not sure how exactly the embedder know the navigation is a form: for
example, a form could have been filled, but the user clicked on a link, so we're
not actually submitting the form.

[dcheng, 2016-02-25 21:56:17]

lgtm

https://codereview.chromium.org/1446253002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1446253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1359:
client()->dispatchWillSubmitForm(form);
On 2016/02/25 at 14:34:07, clamy wrote:
> On 2016/02/24 21:24:55, dcheng wrote:
> > Just curious: shouldn't the embedder "know" it is dealing with a form? It
seems
> > a little unusual that Blink has to tell the embedder about something it
should
> > already know.
> 
> I'm not sure how exactly the embedder know the navigation is a form: for
example, a form could have been filled, but the user clicked on a link, so we're
not actually submitting the form.

Makes sense, thanks.

[nasko, 2016-02-29 18:24:25]

Rubberstamp LGTM

[clamy, 2016-03-01 12:29:07]

@japhet: ping :)

[Nate Chapin, 2016-03-01 23:15:50]

Sorry, thought you had all the LGTMs you needed. LGTM :)

[clamy, 2016-03-02 12:42:07]

Thanks!

[clamy, 2016-03-02 12:42:15]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-03-02 12:42:15]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
dcheng@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1446253002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-02 12:42:28]

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

[commit-bot: I haz the power, 2016-03-02 15:31:48]

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

[commit-bot: I haz the power, 2016-03-02 15:31:49]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[clamy, 2016-03-03 10:26:53]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2016-03-03 10:27:08]

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

[commit-bot: I haz the power, 2016-03-03 12:02:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-03 12:04:10]

Description was changed from

==========
PlzNavigate: inform the WebFrameClient that a form will be submitted

This CL makes it so that the WebFrameClient is informed that a form will be
submitted when the embedder handles the navigation.

BUG=504347
==========

to

==========
PlzNavigate: inform the WebFrameClient that a form will be submitted

This CL makes it so that the WebFrameClient is informed that a form will be
submitted when the embedder handles the navigation.

BUG=504347

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

[commit-bot: I haz the power, 2016-03-03 12:04:12]

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

[nasko, 2016-03-04 19:03:20]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1767693002/ by nasko@chromium.org.

The reason for reverting is: This is causing a bunch of crashes on Canary.
Details are in https://crbug.com/591910..