Functions to create and manipulate x-callback-url URLs.

Functions to create and manipulate x-callback-url URLs.

See http://x-callback-url.com/specifications/ for the specification
of x-callback-url URLs.

BUG=653086
Committed: https://crrev.com/5db5553160affb9ea2464666320c3855e5d203d2
Cr-Commit-Position: refs/heads/master@{#430571}

[sdefresne, 2016-11-07 17:49:38]

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

[commit-bot: I haz the power, 2016-11-07 17:49:59]

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

[sdefresne, 2016-11-07 17:50:10]

sdefresne@chromium.org changed reviewers:
+ mef@chromium.org, rohitrao@chromium.org

[sdefresne, 2016-11-07 17:50:10]

rohitrao: please take a look.

mef: please review new DEPS on //net from //ios/chrome/common.

[rohitrao (ping after 24h), 2016-11-07 17:58:17]

lgtm

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
File ios/chrome/common/x_callback_url.cc (right):

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
ios/chrome/common/x_callback_url.cc:40: GURL
url(base::StringPrintf("%s://%s/%s", scheme.data(), kXCallbackURLHost,
Is there any way to do this using GURL primitives, without using StringPrintf?

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
ios/chrome/common/x_callback_url.cc:44: url = net::AppendQueryParameter(url,
kSuccessURLParameterName,
Does AppendQueryParameter handle escaping?

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
File ios/chrome/common/x_callback_url_unittest.cc (right):

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
ios/chrome/common/x_callback_url_unittest.cc:64: "3Fcancelled%3D1",
Nit: could you break all of these lines before the % instead of after it? 
Seeing "3F" to start a line (instead of "%3F" looked weird.

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
ios/chrome/common/x_callback_url_unittest.cc:139: {{"x-success",
"chrome://callback/?success=1"},
Thoughts on adding another test case which has "%20" in the URL, to make sure we
don't overescape or underescape?

[mef, 2016-11-07 18:42:10]

lgtm

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
File ios/chrome/common/x_callback_url.cc (right):

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
ios/chrome/common/x_callback_url.cc:44: url = net::AppendQueryParameter(url,
kSuccessURLParameterName,
On 2016/11/07 17:58:17, rohitrao wrote:
> Does AppendQueryParameter handle escaping?

Yes,  Unsafe characters in the name and the value are escaped like %XX%XX.

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
File ios/chrome/common/x_callback_url_unittest.cc (right):

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
ios/chrome/common/x_callback_url_unittest.cc:24:
EXPECT_TRUE(IsXCallbackURL(GURL("chrome://x-callback-url")));
Maybe add another test to show that scheme: isn't limited to chrome:?

[commit-bot: I haz the power, 2016-11-07 18:57:37]

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

[commit-bot: I haz the power, 2016-11-07 18:57:38]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2016-11-08 07:15:28]

Thank you for the review.

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
File ios/chrome/common/x_callback_url.cc (right):

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
ios/chrome/common/x_callback_url.cc:40: GURL
url(base::StringPrintf("%s://%s/%s", scheme.data(), kXCallbackURLHost,
On 2016/11/07 17:58:17, rohitrao wrote:
> Is there any way to do this using GURL primitives, without using StringPrintf?

I've tried the following:

GURL url;
GURL::Replacements replacements;
replacements.SetSchemeStr(scheme);
replacements.SetHostStr(kXCallbackURLHost);
....
return url.ReplaceComponents(replacements);

But the returned GURL still return false to is_valid (as is using replacements
does not change the is_valid state of the default constructed GURL), so I don't
really know which GURL primitives can be used here.

[sdefresne, 2016-11-08 07:20:44]

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
File ios/chrome/common/x_callback_url_unittest.cc (right):

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
ios/chrome/common/x_callback_url_unittest.cc:24:
EXPECT_TRUE(IsXCallbackURL(GURL("chrome://x-callback-url")));
On 2016/11/07 18:42:10, mef wrote:
> Maybe add another test to show that scheme: isn't limited to chrome:?

Done.

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
ios/chrome/common/x_callback_url_unittest.cc:64: "3Fcancelled%3D1",
On 2016/11/07 17:58:17, rohitrao wrote:
> Nit: could you break all of these lines before the % instead of after it? 
> Seeing "3F" to start a line (instead of "%3F" looked weird.

Done.

https://codereview.chromium.org/2485573003/diff/1/ios/chrome/common/x_callbac...
ios/chrome/common/x_callback_url_unittest.cc:139: {{"x-success",
"chrome://callback/?success=1"},
On 2016/11/07 17:58:17, rohitrao wrote:
> Thoughts on adding another test case which has "%20" in the URL, to make sure
we
> don't overescape or underescape?

Done.

[sdefresne, 2016-11-08 08:04:43]

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

[commit-bot: I haz the power, 2016-11-08 08:04:55]

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

[sdefresne, 2016-11-08 08:05:44]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2016-11-08 08:05:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org,
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2485573003/#ps20001
(title: "Address comments")

[commit-bot: I haz the power, 2016-11-08 08:05:54]

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-08 10:40:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-08 10:45:20]

Description was changed from

==========
Functions to create and manipulate x-callback-url URLs.

See http://x-callback-url.com/specifications/ for the specification
of x-callback-url URLs.

BUG=653086
==========

to

==========
Functions to create and manipulate x-callback-url URLs.

See http://x-callback-url.com/specifications/ for the specification
of x-callback-url URLs.

BUG=653086

Committed: https://crrev.com/5db5553160affb9ea2464666320c3855e5d203d2
Cr-Commit-Position: refs/heads/master@{#430571}
==========

[commit-bot: I haz the power, 2016-11-08 10:45:20]

Patchset 2 (id:??) landed as
https://crrev.com/5db5553160affb9ea2464666320c3855e5d203d2
Cr-Commit-Position: refs/heads/master@{#430571}