Update naver search url.

Update naver search url.

This CL adds 'sm' param to naver's search url.

BUG=

Committed: https://crrev.com/ad9f641247d9c617f7eb6e82e11e6d36a27c366a
Cr-Commit-Position: refs/heads/master@{#439407}

[j.c, 2016-12-15 09:27:44]

Description was changed from

==========
Update naver search url.

This CL adds 'sm' param to naver's search url.

BUG=
==========

to

==========
Update naver search url.

This CL adds 'sm' param to naver's search url.

BUG=
==========

[j.c, 2016-12-15 09:27:44]

j.c@navercorp.com changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2016-12-15 10:25:28]

We try to minimize query params that have no visible effect on the returned
search results (to shorten URLs and make them more readable, and make URLs
copied from the omnibox and posted elsewhere not result in misleading stats
server-side) unless there's a very good reason.  Generally, we disallow
parameters like this.  If server operators want to know, for example, that a
particular query comes from a Chrome user, we normally encourage them to glean
this information from other existing logs.

Besides this, this CL description has no motivation and no associated bug.

I'm happy to continue discussing if you have more details in response to my
general comments above.

[j.c, 2016-12-15 12:01:58]

On 2016/12/15 10:25:28, Peter Kasting wrote:
> We try to minimize query params that have no visible effect on the returned
> search results (to shorten URLs and make them more readable, and make URLs
> copied from the omnibox and posted elsewhere not result in misleading stats
> server-side) unless there's a very good reason.  Generally, we disallow
> parameters like this.  If server operators want to know, for example, that a
> particular query comes from a Chrome user, we normally encourage them to glean
> this information from other existing logs.
> 
> Besides this, this CL description has no motivation and no associated bug.
> 
> I'm happy to continue discussing if you have more details in response to my
> general comments above.

Thanks for explaining this.

At Naver, it is internal rule that every search request is accompanied with 'sm'
query param.
(For exactly the reason you state. To know that a particular query was entered 
from Chrome omnibox)
Sure search queries without the sm param is allowed now, but in the future,
we might want to block those (for abuse control) or redirect them with a default
sm query param, which will add an extra redirect for all chrome omnibox users.

Before uploading this CL, I tried to look up other search providers and found
that
Bing (
https://chromium.googlesource.com/chromium/src.git/+/master/components/search...
"FORM=CHROMN")
has a similar parameter.
Would it be too much to ask that we, naver , are allowed for this one param?

Without this, I do not know any other direct means to detect that the query came
from chrome "omnibox", as a server operator.

[Peter Kasting, 2016-12-16 06:46:23]

On 2016/12/15 12:01:58, j.c wrote:
> On 2016/12/15 10:25:28, Peter Kasting wrote:
> > We try to minimize query params that have no visible effect on the returned
> > search results (to shorten URLs and make them more readable, and make URLs
> > copied from the omnibox and posted elsewhere not result in misleading stats
> > server-side) unless there's a very good reason.  Generally, we disallow
> > parameters like this.  If server operators want to know, for example, that a
> > particular query comes from a Chrome user, we normally encourage them to
glean
> > this information from other existing logs.
> > 
> > Besides this, this CL description has no motivation and no associated bug.
> > 
> > I'm happy to continue discussing if you have more details in response to my
> > general comments above.
> 
> Thanks for explaining this.
> 
> At Naver, it is internal rule that every search request is accompanied with
'sm'
> query param.
> (For exactly the reason you state. To know that a particular query was entered

> from Chrome omnibox)

A problem with this, as I mentioned, is that if the query URL is copied and
pasted somewhere, it will result in the server having the wrong stats.  AFAIK,
there's no truly reliable way to know that a query came from the Chrome omnibox,
as opposed to just from Chrome at all (which is best accomplished by looking at
the request headers).

> Sure search queries without the sm param is allowed now, but in the future,
> we might want to block those (for abuse control) or redirect them with a
default
> sm query param, which will add an extra redirect for all chrome omnibox users.

Avoiding redirects for our users would certainly be good.

> Before uploading this CL, I tried to look up other search providers and found
> that
> Bing (
>
https://chromium.googlesource.com/chromium/src.git/+/master/components/search...
> "FORM=CHROMN")
> has a similar parameter.

I think I missed that in the review that added it, which also did a bunch of
other stuff :(

> Would it be too much to ask that we, naver , are allowed for this one param?

OK.

LGTM

[j.c, 2016-12-19 01:46:52]

On 2016/12/16 06:46:23, Peter Kasting wrote:
> On 2016/12/15 12:01:58, j.c wrote:
> > On 2016/12/15 10:25:28, Peter Kasting wrote:
> > > We try to minimize query params that have no visible effect on the
returned
> > > search results (to shorten URLs and make them more readable, and make URLs
> > > copied from the omnibox and posted elsewhere not result in misleading
stats
> > > server-side) unless there's a very good reason.  Generally, we disallow
> > > parameters like this.  If server operators want to know, for example, that
a
> > > particular query comes from a Chrome user, we normally encourage them to
> glean
> > > this information from other existing logs.
> > > 
> > > Besides this, this CL description has no motivation and no associated bug.
> > > 
> > > I'm happy to continue discussing if you have more details in response to
my
> > > general comments above.
> > 
> > Thanks for explaining this.
> > 
> > At Naver, it is internal rule that every search request is accompanied with
> 'sm'
> > query param.
> > (For exactly the reason you state. To know that a particular query was
entered
> 
> > from Chrome omnibox)
> 
> A problem with this, as I mentioned, is that if the query URL is copied and
> pasted somewhere, it will result in the server having the wrong stats.  AFAIK,
> there's no truly reliable way to know that a query came from the Chrome
omnibox,
> as opposed to just from Chrome at all (which is best accomplished by looking
at
> the request headers).
> 
> > Sure search queries without the sm param is allowed now, but in the future,
> > we might want to block those (for abuse control) or redirect them with a
> default
> > sm query param, which will add an extra redirect for all chrome omnibox
users.
> 
> Avoiding redirects for our users would certainly be good.
> 
> > Before uploading this CL, I tried to look up other search providers and
found
> > that
> > Bing (
> >
>
https://chromium.googlesource.com/chromium/src.git/+/master/components/search...
> > "FORM=CHROMN")
> > has a similar parameter.
> 
> I think I missed that in the review that added it, which also did a bunch of
> other stuff :(
> 
> > Would it be too much to ask that we, naver , are allowed for this one param?
> 
> OK.
> 
> LGTM

Thanks!

(Wondering why the approval process + automated CQ tests are not starting... ?)

[Peter Kasting, 2016-12-19 03:04:45]

> (Wondering why the approval process + automated CQ tests are not starting...
?)

Well, you're approved to land, the next step would be to check the CQ box and
wait for the bots to run tests and land for you.

I can do that if need be, but before I do, check whether you have a "Commit"
checkbox below your patchset that you're able to check.  If so, checking it
should be enough, everything should kick in several minutes later.

[j.c, 2016-12-19 03:05:54]

The CQ bit was checked by j.c@navercorp.com

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

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

[j.c, 2016-12-19 03:15:46]

On 2016/12/19 03:04:45, Peter Kasting wrote:
> > (Wondering why the approval process + automated CQ tests are not starting...
> ?)
> 
> Well, you're approved to land, the next step would be to check the CQ box and
> wait for the bots to run tests and land for you.
> 
> I can do that if need be, but before I do, check whether you have a "Commit"
> checkbox below your patchset that you're able to check.  If so, checking it
> should be enough, everything should kick in several minutes later.

Ah. didn't know I could do that.
Thanks.

[commit-bot: I haz the power, 2016-12-19 04:21:37]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1482116754690420, "parent_rev":
"02feb9d3acdfab742b4de019f7cc949abc317914", "commit_rev":
"51de1fde7e553f820e2d7b0dcf89101a7db6c29f"}

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

Description was changed from

==========
Update naver search url.

This CL adds 'sm' param to naver's search url.

BUG=
==========

to

==========
Update naver search url.

This CL adds 'sm' param to naver's search url.

BUG=

Review-Url: https://codereview.chromium.org/2578953002
==========

[commit-bot: I haz the power, 2016-12-19 04:22:20]

Committed patchset #1 (id:1)

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

Description was changed from

==========
Update naver search url.

This CL adds 'sm' param to naver's search url.

BUG=

Review-Url: https://codereview.chromium.org/2578953002
==========

to

==========
Update naver search url.

This CL adds 'sm' param to naver's search url.

BUG=

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

[commit-bot: I haz the power, 2016-12-19 04:24:25]

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