Add Yandex referral ID to search URL.

Add Yandex referral ID to search URL.

The ID is generated in the downstream code based on the results
of the build configuration.

BUG=713952
Review-Url: https://codereview.chromium.org/2872453005
Cr-Commit-Position: refs/heads/master@{#471013}
Committed: https://chromium.googlesource.com/chromium/src/+/2928c63356681aa17fcaa112dc7d6bd3bc29e6d8

[Ted C, 2017-05-08 18:35:56]

tedchoc@chromium.org changed reviewers:
+ pkasting@chromium.org, yusufo@chromium.org

[Ted C, 2017-05-08 18:35:56]

PTAL

pkasting - components/search_engines/
yusufo   - chrome/

[Peter Kasting, 2017-05-09 00:14:25]

LGTM, but consider whether you want a test for this

https://codereview.chromium.org/2872453005/diff/1/components/search_engines/s...
File components/search_engines/search_terms_data.h (right):

https://codereview.chromium.org/2872453005/diff/1/components/search_engines/s...
components/search_engines/search_terms_data.h:65: // the omnibox (return the
empty string if not supported/applicable).
Nit: returns

https://codereview.chromium.org/2872453005/diff/1/components/search_engines/s...
components/search_engines/search_terms_data.h:66: virtual std::string
YandexReferralID() const;
Nit: GetYandexReferralId()

https://codereview.chromium.org/2872453005/diff/1/components/search_engines/t...
File components/search_engines/template_url.cc (right):

https://codereview.chromium.org/2872453005/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:1131: DCHECK(!i->is_post_param);
Is this important?  Why?

https://codereview.chromium.org/2872453005/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:1135: // NOTREACHED below.)
I'm confused.  Is this referring to having a case for this at all?  If so I'd
just omit this comment, as it seems more confusing than helpful.

[Ted C, 2017-05-09 04:38:33]

I'm OOO tomorrow, but I'll work on adding a test of some sort.  For the
importance of this, I'll likely write a more complete integration test since we
want broader coverage.  I attempted today briefly and was stifled, but I'll give
it a stronger go on Wednesday.

https://codereview.chromium.org/2872453005/diff/1/components/search_engines/t...
File components/search_engines/template_url.cc (right):

https://codereview.chromium.org/2872453005/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:1131: DCHECK(!i->is_post_param);
On 2017/05/09 00:14:25, Peter Kasting wrote:
> Is this important?  Why?

this and the comment below were blatantly cribbed from RLZ above.  I can remove
both entirely though.

[Yusuf, 2017-05-09 23:18:52]

android bits lgtm with a suggestion.

https://codereview.chromium.org/2872453005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java
(right):

https://codereview.chromium.org/2872453005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java:280:
protected String getYandexReferralId() {
should this be a static, so that we avoid calling into Java in two separate
methods in the native side? Then we don't have to get the instance etc.

[Ted C, 2017-05-10 15:20:29]

Updated with comments addressed (hopefully).  Working on the test still as I am
running into general test setup issues and need to sync my tree.

https://codereview.chromium.org/2872453005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java
(right):

https://codereview.chromium.org/2872453005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java:280:
protected String getYandexReferralId() {
On 2017/05/09 23:18:52, Yusuf (OOO May 10-17) wrote:
> should this be a static, so that we avoid calling into Java in two separate
> methods in the native side? Then we don't have to get the instance etc.

I thought about that as well, but then it would prevent the subclass from
overwriting it as you can do that with statics.

https://codereview.chromium.org/2872453005/diff/1/components/search_engines/s...
File components/search_engines/search_terms_data.h (right):

https://codereview.chromium.org/2872453005/diff/1/components/search_engines/s...
components/search_engines/search_terms_data.h:65: // the omnibox (return the
empty string if not supported/applicable).
On 2017/05/09 00:14:25, Peter Kasting wrote:
> Nit: returns

Done.

https://codereview.chromium.org/2872453005/diff/1/components/search_engines/s...
components/search_engines/search_terms_data.h:66: virtual std::string
YandexReferralID() const;
On 2017/05/09 00:14:25, Peter Kasting wrote:
> Nit: GetYandexReferralId()

Done.

[Ted C, 2017-05-10 15:48:13]

Test added

[Ted C, 2017-05-10 17:32:12]

tedchoc@chromium.org changed reviewers:
+ twellington@chromium.org

[Ted C, 2017-05-10 17:32:13]

+twellington to review the newly added java test: LocaleManagerReferralTest.java

[Theresa, 2017-05-10 17:37:27]

New test lgtm

[Ted C, 2017-05-10 17:38:53]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2017-05-10 17:38:54]

The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2872453005/#ps40001
(title: "Add test")

[commit-bot: I haz the power, 2017-05-10 17:40:28]

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, 2017-05-10 20:10:26]

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

[commit-bot: I haz the power, 2017-05-10 20:10:27]

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

[Ted C, 2017-05-10 21:11:52]

The CQ bit was checked by tedchoc@chromium.org

[commit-bot: I haz the power, 2017-05-10 21:13:10]

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, 2017-05-11 03:57:22]

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

[commit-bot: I haz the power, 2017-05-11 03:57:23]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ted C, 2017-05-11 16:25:52]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2017-05-11 16:25:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, yusufo@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2872453005/#ps60001
(title: "Clear app data in test setup to remove initial locale set for search
engines from previous runs.")

[commit-bot: I haz the power, 2017-05-11 16:26:27]

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, 2017-05-11 18:31:31]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494519952787730,
"parent_rev": "5a291d60a6ace6547d1653ffa531e7e12cdc9725", "commit_rev":
"2928c63356681aa17fcaa112dc7d6bd3bc29e6d8"}

[commit-bot: I haz the power, 2017-05-11 18:31:46]

Description was changed from

==========
Add Yandex referral ID to search URL.

The ID is generated in the downstream code based on the results
of the build configuration.

BUG=713952
==========

to

==========
Add Yandex referral ID to search URL.

The ID is generated in the downstream code based on the results
of the build configuration.

BUG=713952

Review-Url: https://codereview.chromium.org/2872453005
Cr-Commit-Position: refs/heads/master@{#471013}
Committed:
https://chromium.googlesource.com/chromium/src/+/2928c63356681aa17fcaa112dc7d...
==========

[commit-bot: I haz the power, 2017-05-11 18:31:48]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2928c63356681aa17fcaa112dc7d...