Propagate the search request params from the browser to the Instant search base page to fix the embe

Propagate the search request params from the browser to the Instant search base page to fix the embedded search request logging issues.

- Adds an attribute in EmbeddedSearch SearchBox API  to pass the embedded search request params (such as omnibox query suggestion stats, original query, etc) to the Instant search base page at the query submission time.

Server side bug: b/17555463
Server side CL:  cl/76305813

BUG=421258
TEST=none

Committed: https://crrev.com/562c49c8edf16a40ef3f76fb6a57d7ac17331216
Cr-Commit-Position: refs/heads/master@{#304544}

[kmadhusu, 2014-11-13 01:47:44]

Patchset #1 (id:1) has been deleted

[kmadhusu, 2014-11-13 01:47:56]

Patchset #1 (id:20001) has been deleted

[kmadhusu, 2014-11-13 01:48:05]

Patchset #1 (id:40001) has been deleted

[kmadhusu, 2014-11-13 01:48:15]

Patchset #1 (id:60001) has been deleted

[kmadhusu, 2014-11-13 01:48:24]

Patchset #1 (id:80001) has been deleted

[kmadhusu, 2014-11-13 01:48:34]

Patchset #1 (id:100001) has been deleted

[kmadhusu, 2014-11-13 01:48:43]

Patchset #1 (id:120001) has been deleted

[kmadhusu, 2014-11-13 01:48:51]

Patchset #1 (id:140001) has been deleted

[kmadhusu, 2014-11-13 01:49:12]

Patchset #1 (id:160001) has been deleted

[kmadhusu, 2014-11-13 01:49:21]

Patchset #1 (id:180001) has been deleted

[kmadhusu, 2014-11-13 01:49:28]

Patchset #1 (id:200001) has been deleted

[kmadhusu, 2014-11-13 01:49:35]

Patchset #1 (id:220001) has been deleted

[kmadhusu, 2014-11-13 01:49:43]

Patchset #1 (id:240001) has been deleted

[kmadhusu, 2014-11-13 02:02:56]

Patchset #1 (id:260001) has been deleted

[kmadhusu, 2014-11-13 02:08:36]

kmadhusu@chromium.org changed reviewers:
+ samarth@chromium.org

[kmadhusu, 2014-11-13 02:08:37]

Please review.

Thanks.

[samarth, 2014-11-14 08:09:31]

Didn't do a thorough review but this generally looks OK to me.

Thanks!
Samarth

https://codereview.chromium.org/609493002/diff/280001/chrome/common/instant_t...
File chrome/common/instant_types.h (right):

https://codereview.chromium.org/609493002/diff/280001/chrome/common/instant_t...
chrome/common/instant_types.h:134: // Embedded search request logging stats
params.
Do these really need to be in the header?

[kmadhusu, 2014-11-14 18:54:58]

https://codereview.chromium.org/609493002/diff/280001/chrome/common/instant_t...
File chrome/common/instant_types.h (right):

https://codereview.chromium.org/609493002/diff/280001/chrome/common/instant_t...
chrome/common/instant_types.h:134: // Embedded search request logging stats
params.
On 2014/11/14 08:09:31, samarth wrote:
> Do these really need to be in the header?

Yes. I use them in searchbox_extension.cc.

[kmadhusu, 2014-11-14 19:25:03]

kmadhusu@chromium.org changed reviewers:
+ bartn@chromium.org, jschuh@chromium.org, mpearson@chromium.org,
tedchoc@chromium.org

[kmadhusu, 2014-11-14 19:25:04]

jschuh@: Please review render_messages.h
tedchoc@: Please review tab_android.cc
mpearson@: Please review entire CL (sanity check).
bartn@: FYI

Thanks.

[Ted C, 2014-11-15 01:50:13]

On 2014/11/14 19:25:04, kmadhusu wrote:
> jschuh@: Please review render_messages.h
> tedchoc@: Please review tab_android.cc
> mpearson@: Please review entire CL (sanity check).
> bartn@: FYI
> 
> Thanks.

tab_android.cc - lgtm

[Mark P, 2014-11-17 19:15:59]

lgtm


I have sanity checked the whole changelist.  I'm not that familiar with the
prerender / search tab stuff.  Nonetheless, it looks fine to me.  I have some
minor comments.  I might as well stamp it lgtm now though.

--mark

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/android/...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:61: 
please include instant_types directly.

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/ui/brows...
chrome/browser/ui/browser_instant_controller.cc:26: 
please include instant_types directly.

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/ui/searc...
File chrome/browser/ui/search/instant_search_prerenderer_unittest.cc (right):

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/ui/searc...
chrome/browser/ui/search/instant_search_prerenderer_unittest.cc:497: #endif
Would it be hard to add a new test, perhaps in this file?  It seems appropriate.

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.h:122: void
OnNavigationEntryCommitted();
To confirm, are you sure you don't need to modify this?

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.h:136: // Tells the renderer whether
to display the Instant results.
To confirm, are you sure you don't need to modify this?

https://codereview.chromium.org/609493002/diff/280001/chrome/common/instant_t...
File chrome/common/instant_types.cc (right):

https://codereview.chromium.org/609493002/diff/280001/chrome/common/instant_t...
chrome/common/instant_types.cc:94:
EmbeddedSearchRequestParams::EmbeddedSearchRequestParams(const GURL& url) {
Can you add a simple test for this that show it extracts all parameters (with +
and space and whatever as you'd expect from a Google search URL)?

https://codereview.chromium.org/609493002/diff/280001/chrome/common/instant_t...
chrome/common/instant_types.cc:99: net::UnescapeRule::Type unescape_rules =
nit: might as well be const

https://codereview.chromium.org/609493002/diff/280001/chrome/common/instant_t...
chrome/common/instant_types.cc:117: rlz_parameter_value
=net::UnescapeAndDecodeUTF8URLComponent(
nit: "=n" -> "= n"

[jschuh, 2014-11-17 21:34:55]

lgtm for the part of ipc security that can be reviewed on the client.

[kmadhusu, 2014-11-17 22:48:01]

Patchset #2 (id:300001) has been deleted

[kmadhusu, 2014-11-17 22:52:05]

mpearson@: Addressed comments. 

Thanks.

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/android/...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/android/...
chrome/browser/android/tab_android.cc:61: 
On 2014/11/17 19:15:58, Mark P wrote:
> please include instant_types directly.

Done.

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/ui/brows...
chrome/browser/ui/browser_instant_controller.cc:26: 
On 2014/11/17 19:15:58, Mark P wrote:
> please include instant_types directly.

Done.

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/ui/searc...
File chrome/browser/ui/search/instant_search_prerenderer_unittest.cc (right):

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/ui/searc...
chrome/browser/ui/search/instant_search_prerenderer_unittest.cc:497: #endif
On 2014/11/17 19:15:58, Mark P wrote:
> Would it be hard to add a new test, perhaps in this file?  It seems
appropriate.

Done.

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ipc_router.h (right):

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.h:122: void
OnNavigationEntryCommitted();
On 2014/11/17 19:15:58, Mark P wrote:
> To confirm, are you sure you don't need to modify this?

Yes. I don't need to modify this.

https://codereview.chromium.org/609493002/diff/280001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ipc_router.h:136: // Tells the renderer whether
to display the Instant results.
On 2014/11/17 19:15:58, Mark P wrote:
> To confirm, are you sure you don't need to modify this?

Yes. I don't need to modify this.

https://codereview.chromium.org/609493002/diff/280001/chrome/common/instant_t...
File chrome/common/instant_types.cc (right):

https://codereview.chromium.org/609493002/diff/280001/chrome/common/instant_t...
chrome/common/instant_types.cc:94:
EmbeddedSearchRequestParams::EmbeddedSearchRequestParams(const GURL& url) {
On 2014/11/17 19:15:59, Mark P wrote:
> Can you add a simple test for this that show it extracts all parameters (with
+
> and space and whatever as you'd expect from a Google search URL)?

Done.

https://codereview.chromium.org/609493002/diff/280001/chrome/common/instant_t...
chrome/common/instant_types.cc:99: net::UnescapeRule::Type unescape_rules =
On 2014/11/17 19:15:59, Mark P wrote:
> nit: might as well be const

Done.

https://codereview.chromium.org/609493002/diff/280001/chrome/common/instant_t...
chrome/common/instant_types.cc:117: rlz_parameter_value
=net::UnescapeAndDecodeUTF8URLComponent(
On 2014/11/17 19:15:59, Mark P wrote:
> nit: "=n" -> "= n"

Done.

[kmadhusu, 2014-11-17 22:53:19]

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

[kmadhusu, 2014-11-17 22:53:20]

sky@: Please review chrome/common/OWNERS file.

Thanks.

[Mark P, 2014-11-17 23:08:53]

still lgtm for the record

[sky, 2014-11-18 00:12:22]

LGTM

https://codereview.chromium.org/609493002/diff/320001/chrome/common/instant_t...
File chrome/common/instant_types_unittest.cc (right):

https://codereview.chromium.org/609493002/diff/320001/chrome/common/instant_t...
chrome/common/instant_types_unittest.cc:56:
EXPECT_EQ(cases[i].expected_search_query,
Use << for these so that if something fails you know what index failed.

[kmadhusu, 2014-11-18 00:58:27]

https://codereview.chromium.org/609493002/diff/320001/chrome/common/instant_t...
File chrome/common/instant_types_unittest.cc (right):

https://codereview.chromium.org/609493002/diff/320001/chrome/common/instant_t...
chrome/common/instant_types_unittest.cc:56:
EXPECT_EQ(cases[i].expected_search_query,
On 2014/11/18 00:12:22, sky wrote:
> Use << for these so that if something fails you know what index failed.

Done.

[kmadhusu, 2014-11-18 01:09:10]

The CQ bit was checked by kmadhusu@chromium.org

[commit-bot: I haz the power, 2014-11-18 01:10:13]

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

[commit-bot: I haz the power, 2014-11-18 02:08:24]

Committed patchset #4 (id:360001)

[commit-bot: I haz the power, 2014-11-18 02:09:00]

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