PlzNavigate: send searchable form data to the browser side on form submission.

[draft] PlzNavigate: send searchable form data to the browser side on form submission.

In the non-PlzNavigate case, searchable form data is stored in the provisional
document loader by RenderFrameImpl::willSubmitForm, and later sent to the
browser in the DidCommitProvisionalLoad message.

In the PlzNavigate case, however, there isn't a provisional document loader when
RenderFrameImpl::willSubmitForm is called. Therefore, this CL sends the
information to the browser side in the BeginNavigation message, and store it in
NavigationHandleImpl.

This fixes TemplateURLScraperTest.ScrapeWithOnSubmit for PlzNavigate.

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/15ceb3909b88d7d858a47547c9fc03f1eee20776
Cr-Commit-Position: refs/heads/master@{#431915}

[yzshen1, 2016-11-08 00:40:56]

Description was changed from

==========
[draft] PlzNavigate: send searchable form data to the browser side on form
submission.

In the non-PlzNavigate case, searchable form data is stored in the provisional
document loader by RenderFrameImpl::willSubmitForm, and later sent to the
browser in the DidCommitProvisionalLoad message.

In the PlzNavigate case, however, there isn't a provisional document loader when
RenderFrameImpl::willSubmitForm is called. Therefore, this CL sends the
information to the browser side in the BeginNavigation message, and store it in
NavigationHandleImpl.

This fixes TemplateURLScraperTest.ScrapeWithOnSubmit for PlzNavigate.

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

to

==========
[draft] PlzNavigate: send searchable form data to the browser side on form
submission.

In the non-PlzNavigate case, searchable form data is stored in the provisional
document loader by RenderFrameImpl::willSubmitForm, and later sent to the
browser in the DidCommitProvisionalLoad message.

In the PlzNavigate case, however, there isn't a provisional document loader when
RenderFrameImpl::willSubmitForm is called. Therefore, this CL sends the
information to the browser side in the BeginNavigation message, and store it in
NavigationHandleImpl.

This fixes TemplateURLScraperTest.ScrapeWithOnSubmit for PlzNavigate.

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[yzshen1, 2016-11-08 00:47:58]

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

[commit-bot: I haz the power, 2016-11-08 00:48:37]

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

[yzshen1, 2016-11-08 00:50:52]

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

[commit-bot: I haz the power, 2016-11-08 00:50:58]

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

[yzshen1, 2016-11-08 00:53:15]

yzshen@chromium.org changed reviewers:
+ clamy@chromium.org

[yzshen1, 2016-11-08 00:53:16]

Hi, Camille.

Would you please take a look and see whether this approach makes sense?


Thanks!

[commit-bot: I haz the power, 2016-11-08 02:11:32]

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

[commit-bot: I haz the power, 2016-11-08 02:11:33]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-11-09 16:33:29]

Thanks! The approach looks good globally, a dew comments below.

https://codereview.chromium.org/2485783002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2485783002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:310: if
(!begin_params_.searchable_form_url.is_empty()) {
We should probably check that we have a POST navigation.

https://codereview.chromium.org/2485783002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2485783002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1246: } else {
Please be aware that I'm rewriting this code in which should land soon (and make
this code simpler).

https://codereview.chromium.org/2485783002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1259: if
(!validated_params.searchable_form_url.is_empty()) {
I'm not sure it makes sense to keep this both in the params and the
NavigationHandle. We should pick which one we expose in the public API, and just
update this one.

https://codereview.chromium.org/2485783002/diff/40001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2485783002/diff/40001/content/public/browser/...
content/public/browser/navigation_handle.h:97: virtual const GURL&
GetSearchableFormURL() = 0;
If this is not used outside of content/ this should be in NavigationhandleImpl.

[yzshen1, 2016-11-09 19:26:34]

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

[commit-bot: I haz the power, 2016-11-09 19:26:52]

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

[yzshen1, 2016-11-09 19:41:26]

yzshen@chromium.org changed reviewers:
+ sky@chromium.org, tsepez@chromium.org

[yzshen1, 2016-11-09 19:41:27]

Thanks Camille!

+more owners:
Scott: please do an OWNER review for chrome/browser/ui and WebKit.

Tom: please do a security review for frame_messages.h

https://codereview.chromium.org/2485783002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2485783002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:310: if
(!begin_params_.searchable_form_url.is_empty()) {
On 2016/11/09 16:33:29, clamy wrote:
> We should probably check that we have a POST navigation.

At least with the test ScrapeWithOnSubmit test, we are sending this request with
common_params_.method == "GET".

I think there shouldn't be any security concerns without such a check.

WDYT?

https://codereview.chromium.org/2485783002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2485783002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1246: } else {
On 2016/11/09 16:33:29, clamy wrote:
> Please be aware that I'm rewriting this code in which should land soon (and
make
> this code simpler).

ACK-ed

https://codereview.chromium.org/2485783002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1259: if
(!validated_params.searchable_form_url.is_empty()) {
On 2016/11/09 16:33:29, clamy wrote:
> I'm not sure it makes sense to keep this both in the params and the
> NavigationHandle. We should pick which one we expose in the public API, and
just
> update this one.

Done.
I chose to expose the information from NavigationHandle. In order to do that, I
changed SearchEngineTabHelper to use new WebContentsObserver methods. I also
moved these fields to FrameHostMsg_DidCommitProvisionalLoad_Params from its
parent, because they are not needed anywhere else.

https://codereview.chromium.org/2485783002/diff/40001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2485783002/diff/40001/content/public/browser/...
content/public/browser/navigation_handle.h:97: virtual const GURL&
GetSearchableFormURL() = 0;
On 2016/11/09 16:33:29, clamy wrote:
> If this is not used outside of content/ this should be in
NavigationhandleImpl.

Now it is used outside of content/

[yzshen1, 2016-11-09 19:42:06]

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

[commit-bot: I haz the power, 2016-11-09 19:42:42]

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

[Tom Sepez, 2016-11-09 21:03:45]

LGTM, this is just moving around information that is already sent.

[commit-bot: I haz the power, 2016-11-09 21:18:28]

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

[commit-bot: I haz the power, 2016-11-09 21:18:29]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-09 23:14:41]

chrome LGTM, but please find another owner for webkit.

[yzshen1, 2016-11-09 23:17:13]

Description was changed from

==========
[draft] PlzNavigate: send searchable form data to the browser side on form
submission.

In the non-PlzNavigate case, searchable form data is stored in the provisional
document loader by RenderFrameImpl::willSubmitForm, and later sent to the
browser in the DidCommitProvisionalLoad message.

In the PlzNavigate case, however, there isn't a provisional document loader when
RenderFrameImpl::willSubmitForm is called. Therefore, this CL sends the
information to the browser side in the BeginNavigation message, and store it in
NavigationHandleImpl.

This fixes TemplateURLScraperTest.ScrapeWithOnSubmit for PlzNavigate.

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[draft] PlzNavigate: send searchable form data to the browser side on form
submission.

In the non-PlzNavigate case, searchable form data is stored in the provisional
document loader by RenderFrameImpl::willSubmitForm, and later sent to the
browser in the DidCommitProvisionalLoad message.

In the PlzNavigate case, however, there isn't a provisional document loader when
RenderFrameImpl::willSubmitForm is called. Therefore, this CL sends the
information to the browser side in the BeginNavigation message, and store it in
NavigationHandleImpl.

This fixes TemplateURLScraperTest.ScrapeWithOnSubmit for PlzNavigate.

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[yzshen1, 2016-11-09 23:17:13]

yzshen@chromium.org changed reviewers:
+ yhirano@chromium.org

[yzshen1, 2016-11-09 23:17:57]

On 2016/11/09 23:14:41, sky wrote:
> chrome LGTM, but please find another owner for webkit.

+Yutaka

Hi, Would you please take a look at the WebKit side change? Thanks!

[yhirano, 2016-11-10 15:52:55]

On 2016/11/09 23:17:57, yzshen1 wrote:
> On 2016/11/09 23:14:41, sky wrote:
> > chrome LGTM, but please find another owner for webkit.
> 
> +Yutaka
> 
> Hi, Would you please take a look at the WebKit side change? Thanks!

core/loader LGTM

[clamy, 2016-11-10 17:29:33]

Thanks! content/ looks mostly good, a few comments below.

https://codereview.chromium.org/2485783002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2485783002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:768: const GURL&
NavigationHandleImpl::GetSearchableFormURL() {
Could you maintain the same function ordering in the .h and the .cc file?

https://codereview.chromium.org/2485783002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2485783002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1275:
!validated_params.searchable_form_url.is_empty()) {
Following https://codereview.chromium.org/2475693002/, this should be rebased to
apply on the NavigationHandle returned by TakeNavigationHandleForCommit.

https://codereview.chromium.org/2485783002/diff/80001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2485783002/diff/80001/content/public/browser/...
content/public/browser/navigation_handle.h:97: virtual const GURL&
GetSearchableFormURL() = 0;
These need a comment describing what they are.

[yzshen1, 2016-11-10 18:59:35]

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

[yzshen1, 2016-11-10 18:59:50]

Thanks!

https://codereview.chromium.org/2485783002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2485783002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:768: const GURL&
NavigationHandleImpl::GetSearchableFormURL() {
On 2016/11/10 17:29:33, clamy wrote:
> Could you maintain the same function ordering in the .h and the .cc file?

Done. I moved them to where it seems to make the most sense, along with the
method above.

(But there are more order mismatches. I didn't try to move more code to make the
.h/.cc files have strictly the same order.)

https://codereview.chromium.org/2485783002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2485783002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1275:
!validated_params.searchable_form_url.is_empty()) {
On 2016/11/10 17:29:33, clamy wrote:
> Following https://codereview.chromium.org/2475693002/, this should be rebased
to
> apply on the NavigationHandle returned by TakeNavigationHandleForCommit.

Done.

https://codereview.chromium.org/2485783002/diff/80001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2485783002/diff/80001/content/public/browser/...
content/public/browser/navigation_handle.h:97: virtual const GURL&
GetSearchableFormURL() = 0;
On 2016/11/10 17:29:33, clamy wrote:
> These need a comment describing what they are.

Done.

[commit-bot: I haz the power, 2016-11-10 18:59:54]

Dry run: 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-11-10 20:50:45]

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

[commit-bot: I haz the power, 2016-11-10 20:50:46]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-11-11 23:06:15]

On 2016/11/10 18:59:50, yzshen1 wrote:
> Thanks!
> 
>
https://codereview.chromium.org/2485783002/diff/80001/content/browser/frame_h...
> File content/browser/frame_host/navigation_handle_impl.cc (right):
> 
>
https://codereview.chromium.org/2485783002/diff/80001/content/browser/frame_h...
> content/browser/frame_host/navigation_handle_impl.cc:768: const GURL&
> NavigationHandleImpl::GetSearchableFormURL() {
> On 2016/11/10 17:29:33, clamy wrote:
> > Could you maintain the same function ordering in the .h and the .cc file?
> 
> Done. I moved them to where it seems to make the most sense, along with the
> method above.
> 
> (But there are more order mismatches. I didn't try to move more code to make
the
> .h/.cc files have strictly the same order.)
> 
>
https://codereview.chromium.org/2485783002/diff/80001/content/browser/frame_h...
> File content/browser/frame_host/render_frame_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2485783002/diff/80001/content/browser/frame_h...
> content/browser/frame_host/render_frame_host_impl.cc:1275:
> !validated_params.searchable_form_url.is_empty()) {
> On 2016/11/10 17:29:33, clamy wrote:
> > Following https://codereview.chromium.org/2475693002/, this should be
rebased
> to
> > apply on the NavigationHandle returned by TakeNavigationHandleForCommit.
> 
> Done.
> 
>
https://codereview.chromium.org/2485783002/diff/80001/content/public/browser/...
> File content/public/browser/navigation_handle.h (right):
> 
>
https://codereview.chromium.org/2485783002/diff/80001/content/public/browser/...
> content/public/browser/navigation_handle.h:97: virtual const GURL&
> GetSearchableFormURL() = 0;
> On 2016/11/10 17:29:33, clamy wrote:
> > These need a comment describing what they are.
> 
> Done.

Friendly ping, Camille.

[clamy, 2016-11-14 12:30:23]

Thanks! Lgtm. Sorry for the late answer, Friday was a public holiday in France.

https://codereview.chromium.org/2485783002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2485783002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:768: const GURL&
NavigationHandleImpl::GetSearchableFormURL() {
On 2016/11/10 18:59:50, yzshen1 wrote:
> On 2016/11/10 17:29:33, clamy wrote:
> > Could you maintain the same function ordering in the .h and the .cc file?
> 
> Done. I moved them to where it seems to make the most sense, along with the
> method above.
> 
> (But there are more order mismatches. I didn't try to move more code to make
the
> .h/.cc files have strictly the same order.)

No problem, I just want to avoid adding more :).

[yzshen1, 2016-11-14 16:55:32]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-11-14 16:55:33]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sky@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2485783002/#ps100001
(title: ".")

[commit-bot: I haz the power, 2016-11-14 16:56:03]

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-11-14 16:59:07]

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

[commit-bot: I haz the power, 2016-11-14 16:59:09]

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

[yzshen1, 2016-11-14 17:24:19]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-11-14 17:24:19]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
clamy@chromium.org, sky@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2485783002/#ps120001
(title: "rebase&resolve")

[commit-bot: I haz the power, 2016-11-14 17:24:47]

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-11-14 18:52:30]

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

[commit-bot: I haz the power, 2016-11-14 18:52:32]

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

[yzshen1, 2016-11-14 19:22:38]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2016-11-14 19:24:07]

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-11-14 21:05:54]

Description was changed from

==========
[draft] PlzNavigate: send searchable form data to the browser side on form
submission.

In the non-PlzNavigate case, searchable form data is stored in the provisional
document loader by RenderFrameImpl::willSubmitForm, and later sent to the
browser in the DidCommitProvisionalLoad message.

In the PlzNavigate case, however, there isn't a provisional document loader when
RenderFrameImpl::willSubmitForm is called. Therefore, this CL sends the
information to the browser side in the BeginNavigation message, and store it in
NavigationHandleImpl.

This fixes TemplateURLScraperTest.ScrapeWithOnSubmit for PlzNavigate.

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[draft] PlzNavigate: send searchable form data to the browser side on form
submission.

In the non-PlzNavigate case, searchable form data is stored in the provisional
document loader by RenderFrameImpl::willSubmitForm, and later sent to the
browser in the DidCommitProvisionalLoad message.

In the PlzNavigate case, however, there isn't a provisional document loader when
RenderFrameImpl::willSubmitForm is called. Therefore, this CL sends the
information to the browser side in the BeginNavigation message, and store it in
NavigationHandleImpl.

This fixes TemplateURLScraperTest.ScrapeWithOnSubmit for PlzNavigate.

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-11-14 21:05:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-14 21:11:55]

Description was changed from

==========
[draft] PlzNavigate: send searchable form data to the browser side on form
submission.

In the non-PlzNavigate case, searchable form data is stored in the provisional
document loader by RenderFrameImpl::willSubmitForm, and later sent to the
browser in the DidCommitProvisionalLoad message.

In the PlzNavigate case, however, there isn't a provisional document loader when
RenderFrameImpl::willSubmitForm is called. Therefore, this CL sends the
information to the browser side in the BeginNavigation message, and store it in
NavigationHandleImpl.

This fixes TemplateURLScraperTest.ScrapeWithOnSubmit for PlzNavigate.

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[draft] PlzNavigate: send searchable form data to the browser side on form
submission.

In the non-PlzNavigate case, searchable form data is stored in the provisional
document loader by RenderFrameImpl::willSubmitForm, and later sent to the
browser in the DidCommitProvisionalLoad message.

In the PlzNavigate case, however, there isn't a provisional document loader when
RenderFrameImpl::willSubmitForm is called. Therefore, this CL sends the
information to the browser side in the BeginNavigation message, and store it in
NavigationHandleImpl.

This fixes TemplateURLScraperTest.ScrapeWithOnSubmit for PlzNavigate.

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/15ceb3909b88d7d858a47547c9fc03f1eee20776
Cr-Commit-Position: refs/heads/master@{#431915}
==========

[commit-bot: I haz the power, 2016-11-14 21:11:57]

Patchset 7 (id:??) landed as
https://crrev.com/15ceb3909b88d7d858a47547c9fc03f1eee20776
Cr-Commit-Position: refs/heads/master@{#431915}