Fix doodle verification URL.

Fix doodle verification URL.

When verifying that the cached doodle is still valid, we load the doodle
URL and append the query param "async=es_dfp:<fingerprint>". Previously,
the ":" was being escape to "%3A", causing the server to respond with a
400 error. This mollifies the server by keeping the colon unescaped.

BUG=413845
Committed: https://crrev.com/673cc76103079eaada968c537c6605dbaf8d909c
Cr-Commit-Position: refs/heads/master@{#296512}

[newt (away), 2014-09-20 02:27:54]

newt@chromium.org changed reviewers:
+ tfarina@chromium.org

[newt (away), 2014-09-20 02:27:55]

tfarina: PTAL.

Let me know if there's a better way to do this. Thanks!

[newt (away), 2014-09-20 02:31:14]

FYI, I checked with the server-side team to see if they could accept "%3A" in
place of ":" and the answer was: no, update the client-side code to send ":"
instead of "%3A".

[tfarina, 2014-09-20 02:32:22]

tfarina@chromium.org changed reviewers:
+ justincohen@chromium.org
- tfarina@chromium.org

[tfarina, 2014-09-20 02:32:22]

I think you meant Justin rather than me.

Moving myself to CC.

Regards,

[justincohen, 2014-09-20 11:50:15]

lgtm

[newt (away), 2014-09-21 16:49:28]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2014-09-21 16:50:10]

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

[commit-bot: I haz the power, 2014-09-21 16:50:11]

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

[commit-bot: I haz the power, 2014-09-21 16:50:12]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[newt (away), 2014-09-21 16:53:00]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2014-09-21 16:54:06]

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

[commit-bot: I haz the power, 2014-09-21 16:54:08]

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

[commit-bot: I haz the power, 2014-09-21 16:54:10]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[newt (away), 2014-09-21 17:32:01]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2014-09-21 17:32:15]

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

[commit-bot: I haz the power, 2014-09-21 17:32:17]

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

[commit-bot: I haz the power, 2014-09-21 17:32:19]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[newt (away), 2014-09-21 18:09:36]

newt@chromium.org changed reviewers:
+ rsleevi@chromium.org

[newt (away), 2014-09-21 18:09:37]

rsleevi: PTAL, thanks!

[tfarina, 2014-09-21 18:57:37]

Owners should be full committers. 

http://www.chromium.org/developers/owners-files#TOC-Who-should-be-in-an-OWNER...

If Just isn't yet, maybe remove it from components/search_provider_logos/OWNERS
and add it back when his ready?

[newt (away), 2014-09-23 20:11:52]

newt@chromium.org changed reviewers:
+ mmenke@chromium.org

[newt (away), 2014-09-23 20:11:52]

+mmenke: PTAL (rsleevi has "expect delays" in his username)

This is a small change that will hopefully be merged to M38

[mmenke, 2014-09-23 21:16:17]

https://codereview.chromium.org/587943003/diff/1/components/search_provider_l...
File components/search_provider_logos/logo_tracker_unittest.cc (right):

https://codereview.chromium.org/587943003/diff/1/components/search_provider_l...
components/search_provider_logos/logo_tracker_unittest.cc:375: url_with_fp =
GURL(url_with_fp.spec() + ":" + fingerprint);
This seems really ugly.

You're basically relying on GURL's escaping behavior being different from net's
(Which itself is hideous).  net's behavior looks to be the more correct one here
- RFC 2396 (https://www.ietf.org/rfc/rfc2396.txt) pretty clearly indicates the
colon is reserved in the query component.

Of course, "fixing" GURL here may well break a bunch of sites, so it may make
more sense to "fix" escape.h instead, ideally by making it wrap the GURL code. 
This could cause unexpected regressions as well, however, so I don't think
either option is a merge candidate.

I don't suppose we could just fix the server code?

[Ryan Sleevi, 2014-09-23 21:20:22]

On 2014/09/23 21:16:17, mmenke wrote:
>
https://codereview.chromium.org/587943003/diff/1/components/search_provider_l...
> File components/search_provider_logos/logo_tracker_unittest.cc (right):
> 
>
https://codereview.chromium.org/587943003/diff/1/components/search_provider_l...
> components/search_provider_logos/logo_tracker_unittest.cc:375: url_with_fp =
> GURL(url_with_fp.spec() + ":" + fingerprint);
> This seems really ugly.
> 
> You're basically relying on GURL's escaping behavior being different from
net's
> (Which itself is hideous).  net's behavior looks to be the more correct one
here
> - RFC 2396 (https://www.ietf.org/rfc/rfc2396.txt) pretty clearly indicates the
> colon is reserved in the query component.
> 
> Of course, "fixing" GURL here may well break a bunch of sites, so it may make
> more sense to "fix" escape.h instead, ideally by making it wrap the GURL code.

> This could cause unexpected regressions as well, however, so I don't think
> either option is a merge candidate.
> 
> I don't suppose we could just fix the server code?

Commented on the bug. Both https://url.spec.whatwg.org and RFC 3986 allow the
colon to be unescaped.

[mmenke, 2014-09-23 21:28:31]

On 2014/09/23 21:20:22, Ryan Sleevi (expect_delays) wrote:
> On 2014/09/23 21:16:17, mmenke wrote:
> >
>
https://codereview.chromium.org/587943003/diff/1/components/search_provider_l...
> > File components/search_provider_logos/logo_tracker_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/587943003/diff/1/components/search_provider_l...
> > components/search_provider_logos/logo_tracker_unittest.cc:375: url_with_fp =
> > GURL(url_with_fp.spec() + ":" + fingerprint);
> > This seems really ugly.
> > 
> > You're basically relying on GURL's escaping behavior being different from
> net's
> > (Which itself is hideous).  net's behavior looks to be the more correct one
> here
> > - RFC 2396 (https://www.ietf.org/rfc/rfc2396.txt) pretty clearly indicates
the
> > colon is reserved in the query component.
> > 
> > Of course, "fixing" GURL here may well break a bunch of sites, so it may
make
> > more sense to "fix" escape.h instead, ideally by making it wrap the GURL
code.
> 
> > This could cause unexpected regressions as well, however, so I don't think
> > either option is a merge candidate.
> > 
> > I don't suppose we could just fix the server code?
> 
> Commented on the bug. Both https://url.spec.whatwg.org and RFC 3986 allow the
> colon to be unescaped.

You' right, I just blindly looked at the RFC referenced by net/escape.  I agree
that we should fix that file (Though, per my earlier comment, we really should
not have two copies of URL escaping logic), but I don't think that fix should be
merged, due to regression concerns.

[newt (away), 2014-09-23 22:16:33]

On 2014/09/23 21:28:31, mmenke wrote:
> On 2014/09/23 21:20:22, Ryan Sleevi (expect_delays) wrote:
> > On 2014/09/23 21:16:17, mmenke wrote:
> > >
> >
>
https://codereview.chromium.org/587943003/diff/1/components/search_provider_l...
> > > File components/search_provider_logos/logo_tracker_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/587943003/diff/1/components/search_provider_l...
> > > components/search_provider_logos/logo_tracker_unittest.cc:375: url_with_fp
=
> > > GURL(url_with_fp.spec() + ":" + fingerprint);
> > > This seems really ugly.
> > > 
> > > You're basically relying on GURL's escaping behavior being different from
> > net's
> > > (Which itself is hideous).  net's behavior looks to be the more correct
one
> > here
> > > - RFC 2396 (https://www.ietf.org/rfc/rfc2396.txt) pretty clearly indicates
> the
> > > colon is reserved in the query component.
> > > 
> > > Of course, "fixing" GURL here may well break a bunch of sites, so it may
> make
> > > more sense to "fix" escape.h instead, ideally by making it wrap the GURL
> code.
> > 
> > > This could cause unexpected regressions as well, however, so I don't think
> > > either option is a merge candidate.
> > > 
> > > I don't suppose we could just fix the server code?
> > 
> > Commented on the bug. Both https://url.spec.whatwg.org and RFC 3986 allow
the
> > colon to be unescaped.
> 
> You' right, I just blindly looked at the RFC referenced by net/escape.  I
agree
> that we should fix that file (Though, per my earlier comment, we really should
> not have two copies of URL escaping logic), but I don't think that fix should
be
> merged, due to regression concerns.

So, what would you recommend? Land this for M38 and file a bug to reconsider ":"
escaping in src/net?

[mmenke, 2014-09-24 15:50:08]

On 2014/09/23 22:16:33, newt wrote:
> On 2014/09/23 21:28:31, mmenke wrote:
> > On 2014/09/23 21:20:22, Ryan Sleevi (expect_delays) wrote:
> > > On 2014/09/23 21:16:17, mmenke wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/587943003/diff/1/components/search_provider_l...
> > > > File components/search_provider_logos/logo_tracker_unittest.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/587943003/diff/1/components/search_provider_l...
> > > > components/search_provider_logos/logo_tracker_unittest.cc:375:
url_with_fp
> =
> > > > GURL(url_with_fp.spec() + ":" + fingerprint);
> > > > This seems really ugly.
> > > > 
> > > > You're basically relying on GURL's escaping behavior being different
from
> > > net's
> > > > (Which itself is hideous).  net's behavior looks to be the more correct
> one
> > > here
> > > > - RFC 2396 (https://www.ietf.org/rfc/rfc2396.txt) pretty clearly
indicates
> > the
> > > > colon is reserved in the query component.
> > > > 
> > > > Of course, "fixing" GURL here may well break a bunch of sites, so it may
> > make
> > > > more sense to "fix" escape.h instead, ideally by making it wrap the GURL
> > code.
> > > 
> > > > This could cause unexpected regressions as well, however, so I don't
think
> > > > either option is a merge candidate.
> > > > 
> > > > I don't suppose we could just fix the server code?
> > > 
> > > Commented on the bug. Both https://url.spec.whatwg.org and RFC 3986 allow
> the
> > > colon to be unescaped.
> > 
> > You' right, I just blindly looked at the RFC referenced by net/escape.  I
> agree
> > that we should fix that file (Though, per my earlier comment, we really
should
> > not have two copies of URL escaping logic), but I don't think that fix
should
> be
> > merged, due to regression concerns.
> 
> So, what would you recommend? Land this for M38 and file a bug to reconsider
":"
> escaping in src/net?

Yea, if we need to get this into 38 (Or 39, for that matter), I don't see any
other option.  LogoTrackerTest::SetServerResponseWhenFingerprint in the unit
test is a bit too ugly, though...Can it just use
GoogleAppendFingerprintToLogoURL?  Or the same code as
GoogleAppendFingerprintToLogoURL?  The double-append just seems a bit too
hackish.

[newt (away), 2014-09-24 16:29:41]

On 2014/09/24 15:50:08, mmenke wrote:
> On 2014/09/23 22:16:33, newt wrote:
> > On 2014/09/23 21:28:31, mmenke wrote:
> > > On 2014/09/23 21:20:22, Ryan Sleevi (expect_delays) wrote:
> > > > On 2014/09/23 21:16:17, mmenke wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/587943003/diff/1/components/search_provider_l...
> > > > > File components/search_provider_logos/logo_tracker_unittest.cc
(right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/587943003/diff/1/components/search_provider_l...
> > > > > components/search_provider_logos/logo_tracker_unittest.cc:375:
> url_with_fp
> > =
> > > > > GURL(url_with_fp.spec() + ":" + fingerprint);
> > > > > This seems really ugly.
> > > > > 
> > > > > You're basically relying on GURL's escaping behavior being different
> from
> > > > net's
> > > > > (Which itself is hideous).  net's behavior looks to be the more
correct
> > one
> > > > here
> > > > > - RFC 2396 (https://www.ietf.org/rfc/rfc2396.txt) pretty clearly
> indicates
> > > the
> > > > > colon is reserved in the query component.
> > > > > 
> > > > > Of course, "fixing" GURL here may well break a bunch of sites, so it
may
> > > make
> > > > > more sense to "fix" escape.h instead, ideally by making it wrap the
GURL
> > > code.
> > > > 
> > > > > This could cause unexpected regressions as well, however, so I don't
> think
> > > > > either option is a merge candidate.
> > > > > 
> > > > > I don't suppose we could just fix the server code?
> > > > 
> > > > Commented on the bug. Both https://url.spec.whatwg.org and RFC 3986
allow
> > the
> > > > colon to be unescaped.
> > > 
> > > You' right, I just blindly looked at the RFC referenced by net/escape.  I
> > agree
> > > that we should fix that file (Though, per my earlier comment, we really
> should
> > > not have two copies of URL escaping logic), but I don't think that fix
> should
> > be
> > > merged, due to regression concerns.
> > 
> > So, what would you recommend? Land this for M38 and file a bug to reconsider
> ":"
> > escaping in src/net?
> 
> Yea, if we need to get this into 38 (Or 39, for that matter), I don't see any
> other option.  LogoTrackerTest::SetServerResponseWhenFingerprint in the unit
> test is a bit too ugly, though...Can it just use
> GoogleAppendFingerprintToLogoURL?

Sure. I can use GoogleAppendFingerprintToLogoURL() in the test, though then I'd
want to add a separate test that ensures the URL contains ":" not "%3A"

> Or the same code as
> GoogleAppendFingerprintToLogoURL?  The double-append just seems a bit too
> hackish.

[mmenke, 2014-09-24 16:44:46]

On 2014/09/24 16:29:41, newt wrote:
> On 2014/09/24 15:50:08, mmenke wrote:
> > On 2014/09/23 22:16:33, newt wrote:
> > > On 2014/09/23 21:28:31, mmenke wrote:
> > > > On 2014/09/23 21:20:22, Ryan Sleevi (expect_delays) wrote:
> > > > > On 2014/09/23 21:16:17, mmenke wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/587943003/diff/1/components/search_provider_l...
> > > > > > File components/search_provider_logos/logo_tracker_unittest.cc
> (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/587943003/diff/1/components/search_provider_l...
> > > > > > components/search_provider_logos/logo_tracker_unittest.cc:375:
> > url_with_fp
> > > =
> > > > > > GURL(url_with_fp.spec() + ":" + fingerprint);
> > > > > > This seems really ugly.
> > > > > > 
> > > > > > You're basically relying on GURL's escaping behavior being different
> > from
> > > > > net's
> > > > > > (Which itself is hideous).  net's behavior looks to be the more
> correct
> > > one
> > > > > here
> > > > > > - RFC 2396 (https://www.ietf.org/rfc/rfc2396.txt) pretty clearly
> > indicates
> > > > the
> > > > > > colon is reserved in the query component.
> > > > > > 
> > > > > > Of course, "fixing" GURL here may well break a bunch of sites, so it
> may
> > > > make
> > > > > > more sense to "fix" escape.h instead, ideally by making it wrap the
> GURL
> > > > code.
> > > > > 
> > > > > > This could cause unexpected regressions as well, however, so I don't
> > think
> > > > > > either option is a merge candidate.
> > > > > > 
> > > > > > I don't suppose we could just fix the server code?
> > > > > 
> > > > > Commented on the bug. Both https://url.spec.whatwg.org and RFC 3986
> allow
> > > the
> > > > > colon to be unescaped.
> > > > 
> > > > You' right, I just blindly looked at the RFC referenced by net/escape. 
I
> > > agree
> > > > that we should fix that file (Though, per my earlier comment, we really
> > should
> > > > not have two copies of URL escaping logic), but I don't think that fix
> > should
> > > be
> > > > merged, due to regression concerns.
> > > 
> > > So, what would you recommend? Land this for M38 and file a bug to
reconsider
> > ":"
> > > escaping in src/net?
> > 
> > Yea, if we need to get this into 38 (Or 39, for that matter), I don't see
any
> > other option.  LogoTrackerTest::SetServerResponseWhenFingerprint in the unit
> > test is a bit too ugly, though...Can it just use
> > GoogleAppendFingerprintToLogoURL?
> 
> Sure. I can use GoogleAppendFingerprintToLogoURL() in the test, though then
I'd
> want to add a separate test that ensures the URL contains ":" not "%3A"

I'm fine with a test that verifies that, but I'm not sure how the current
approach renders that unnecessary - seems to me like using GURL::Replacements is
just as likely to escape the colon as creating a URL with a colon in the query
string with GURL's constructor.

[newt (away), 2014-09-24 19:45:26]

On 2014/09/24 16:44:46, mmenke wrote:
> I'm fine with a test that verifies that, but I'm not sure how the current
> approach renders that unnecessary - seems to me like using GURL::Replacements
is
> just as likely to escape the colon as creating a URL with a colon in the query
> string with GURL's constructor.

Good point. I added a test that explicitly checks for the unescaped colon.

PTAL

[mmenke, 2014-09-24 19:48:58]

LGTM.

https://codereview.chromium.org/587943003/diff/20001/components/search_provid...
File components/search_provider_logos/google_logo_api.cc (right):

https://codereview.chromium.org/587943003/diff/20001/components/search_provid...
components/search_provider_logos/google_logo_api.cc:23: // See:
http://crbug.com/413845
nit:  Could you add something like:
TODO(newt):  Switch to using net::AppendQueryParameter once it's fixed.

(Or TODO(mmenke), if you prefer).

[newt (away), 2014-09-24 19:56:10]

https://codereview.chromium.org/587943003/diff/20001/components/search_provid...
File components/search_provider_logos/google_logo_api.cc (right):

https://codereview.chromium.org/587943003/diff/20001/components/search_provid...
components/search_provider_logos/google_logo_api.cc:23: // See:
http://crbug.com/413845
On 2014/09/24 19:48:58, mmenke wrote:
> nit:  Could you add something like:
> TODO(newt):  Switch to using net::AppendQueryParameter once it's fixed.
> 
> (Or TODO(mmenke), if you prefer).

Done.

[newt (away), 2014-09-24 19:56:17]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2014-09-24 19:57:32]

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

[commit-bot: I haz the power, 2014-09-24 20:55:14]

Committed patchset #3 (id:40001) as e3ae15d9a65015f3cb6722eb9d3b03ab21e2324a

[commit-bot: I haz the power, 2014-09-24 20:55:52]

Patchset 3 (id:??) landed as
https://crrev.com/673cc76103079eaada968c537c6605dbaf8d909c
Cr-Commit-Position: refs/heads/master@{#296512}

[jiayl, 2014-09-24 22:48:30]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/599243002/ by jiayl@chromium.org.

The reason for reverting is: Caused test failure:
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....