Preparing to support fragment resolution against non-hierarchical schemes

Preparing to support fragment resolution against non-hierarchical schemes

Adds a new GURL::GetContent() to retrieve the text after scheme: specifically
intended for use in non-hierarchical schemes (data: etc).
Doing the first part of https://codereview.chromium.org/23835019/
in a preparation CL.

BUG=291747
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230781

[Kristian Monsen, 2013-09-19 05:53:11]

I think this is what we agreed to?

[joth, 2013-09-19 06:15:30]

Yes this looks like it. Some small nits will send in morning but this will
make the followup much simpler.

On Wednesday, 18 September 2013,  <kristianm@chromium.org> wrote:
> Reviewers: joth,
>
> Message:
> I think this is what we agreed to?
>
> Description:
> Preparing to support fragment resolution against non-hierarchical schemes
>
> Doing the first part of https://codereview.chromium.org/23526048/
> in a preparation CL that should not make any tests fail.
>
> BUG=291747
>
> Please review this at https://codereview.chromium.org/23549039/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+39, -18 lines):
>   M chrome/browser/chromeos/drive/file_system_util.cc
>   M chrome/browser/extensions/api/identity/gaia_web_auth_flow.cc
>   M chrome/browser/extensions/api/tabs/tabs_api.cc
>   M chrome_frame/navigation_constraints.cc
>   M chrome_frame/utils.cc
>   M content/browser/browser_url_handler_impl.cc
>   M content/browser/child_process_security_policy_impl.cc
>   M net/base/net_util_unittest.cc
>   M url/gurl.h
>   M url/third_party/mozilla/url_parse.h
>   M url/third_party/mozilla/url_parse.cc
>
>
> Index: chrome/browser/chromeos/drive/file_system_util.cc
> diff --git a/chrome/browser/chromeos/drive/file_system_util.cc
b/chrome/browser/chromeos/drive/file_system_util.cc
> index
db13b712cb4139e828fd7661cf43ce27b8ad0460..8568b39eb01017e21c1368b9b39e1c4f1f3be8b2
100644
> --- a/chrome/browser/chromeos/drive/file_system_util.cc
> +++ b/chrome/browser/chromeos/drive/file_system_util.cc
> @@ -212,7 +212,7 @@ base::FilePath DriveURLToFilePath(const GURL& url) {
>    if (!url.is_valid() || url.scheme() != chrome::kDriveScheme)
>      return base::FilePath();
>    std::string path_string = net::UnescapeURLComponent(
> -      url.path(), net::UnescapeRule::NORMAL);
> +      url.GetContent(), net::UnescapeRule::NORMAL);
>    return base::FilePath::FromUTF8Unsafe(path_string);
>  }
>
> Index: chrome/browser/extensions/api/identity/gaia_web_auth_flow.cc
> diff --git a/chrome/browser/extensions/api/identity/gaia_web_auth_flow.cc
b/chrome/browser/extensions/api/identity/gaia_web_auth_flow.cc
> index
2ee70f3b9602efcf99a9dda8b3c0a8683bcf30bc..ad08cae189b5cce45e123242621c9e9d0b0a2fee
100644
> --- a/chrome/browser/extensions/api/identity/gaia_web_auth_flow.cc
> +++ b/chrome/browser/extensions/api/identity/gaia_web_auth_flow.cc
> @@ -113,11 +113,11 @@ void GaiaWebAuthFlow::OnAuthFlowURLChange(const
GURL& url) {
>    // interpreted as a path, including the fragment.
>
>    if (url.scheme() == redirect_scheme_ && !url.has_host() &&
!url.has_port() &&
> -      StartsWithASCII(url.path(), redirect_path_prefix_, true)) {
> +      StartsWithASCII(url.GetContent(), redirect_path_prefix_, true)) {
>      web_flow_.release()->DetachDelegateAndDelete();
>
> -    std::string fragment =
> -        url.path().substr(redirect_path_prefix_.length(),
std::string::npos);
> +    std::string fragment = url.GetContent().substr(
> +        redirect_path_prefix_.length(), std::string::npos);
>      std::vector<std::pair<std::string, std::string> > pairs;
>      base::SplitStringIntoKeyValuePairs(fragment, '=', '&', &pairs);
>      std::string access_token;
> @@ -172,4 +172,4 @@ scoped_ptr<WebAuthFlow>
GaiaWebAuthFlow::CreateWebAuthFlow(GURL url) {
>
WebAuthFlow::INTERACTIVE));
>  }
>
> -}  // extensions
> +}  // namespace extensions
> Index: chrome/browser/extensions/api/tabs/tabs_api.cc
> diff --git a/chrome/browser/extensions/api/tabs/tabs_api.cc
b/chrome/browser/extensions/api/tabs/tabs_api.cc
> index
d0f23482891b179d8b8324de4170ba74cb5798f8..3379d8b2841543c9cc828288207ed4411c59c542
100644
> --- a/chrome/browser/extensions/api/tabs/tabs_api.cc
> +++ b/chrome/browser/extensions/api/tabs/tabs_api.cc
> @@ -1342,7 +1342,7 @@ bool TabsUpdateFunction::UpdateURL(const
std::string &url_string,
>          script_executor()->ExecuteScript(
>              extension_id(),
>              ScriptExecutor::JAVASCRIPT,
> -            url.path(),
> +            url.GetContent(),
>              ScriptExecutor::TOP_FRAME,
>              UserScript::DOCUMENT_IDLE,
>              ScriptExecutor::MAIN_WORLD,
> Index: chrome_frame/navigation_constraints.cc
> diff --git a/chrome_frame/navigation_constraints.cc
b/chrome_frame/navigation_constraints.cc
> index
59efb0e87035cbaec2ca0ea0f55429047d4c248c..11a22a9355a1ad4c53ee0b624eb53ef1387d4af1
100644
> --- a/chrome_frame/navigation_constraints.cc
> +++ b/chrome_frame/navigation_constraints.cc
> @@ -32,7 +32,7 @@ bool NavigationConstraintsImpl::IsSchemeAllowed(const
GURL& url) {
>    // Additional checking for view-source. Allow only http and https
>    // URLs in view source.
>    if (url.SchemeIs(content::kViewSourceScheme)) {
> -    GURL sub_url(url.path());
> +    GURL sub_url(url.Content());
>      if (sub_url.SchemeIs(content::kHttpScheme) ||
>          sub_url.SchemeIs(content::kHttpsScheme))
>        return true;
> Index: chrome_frame/utils.cc
> diff --git a/chrome_frame/utils.cc b/chrome_frame/utils.cc
> index
7c8b713ee9bf2e4591d80a5880b8a8216183fb2a..d768e4ff6c4682e0b62b3a5fcf3419e2c572467f
100644
> --- a/chrome_frame/utils.cc
> +++ b/chrome_frame/utils.cc
> @@ -1009,7 +1009,7 @@ bool IsValidUrlScheme(const GURL& url, bool
is_privileged) {
>    // Additional checking for view-source. Allow only http and https
>    // URLs in view source.
>    if (url.SchemeIs(content::kViewSourceScheme)) {
> -    GURL sub_url(url.path());
> +    GURL sub_url(url.Content());
>      if (sub_url.SchemeIs(content::kHttpScheme) ||
>          sub_url.SchemeIs(content::kHttpsScheme))
>        return true;
> Index: content/browser/browser_url_handler_impl.cc
> diff --git a/content/browser/browser_url_handler_impl.cc
b/content/browser/browser_url_handler_impl.cc
> index
5ff1309f1a8dfef9dae037c229a81f676511ccf2..37eeddad20bfd99e5940fe58bb275e5ef63c7d25
100644
> --- a/content/browser/browser_url_handler_impl.cc
> +++ b/content/browser/browser_url_handler_impl.cc
> @@ -16,7 +16,7 @@ namespace content {
>  static bool HandleViewSource(GURL* url, BrowserContext* browser_context)
{
>    if (url->SchemeIs(kViewSourceScheme)) {
>      // Load the inner URL instead.
> -    *url = GURL(url->path());
> +    *url = GURL(url->GetContent());
>
>      // Bug 26129: limit view-source to view the content and not any
>      // other kind of 'active' url scheme like 'javascript' or 'data'.
> Index: content/browser/child_process_security_policy_impl.cc
> diff --git a/content/browser/child_process_security_policy_impl.cc
b/content/browser/child_process_security_policy_impl.cc
> index
6933f9e0b1de14ec97c9d33b3abd649a85e4acc2..53b3dfc01bbcf87fccc39c9981ad3c806af0247f
100644
> --- a/content/browser/child_process_security_policy_impl.cc
> +++ b/content/browser/child_process_security_policy_impl.cc
> @@ -396,7 +396,7 @@ void ChildProcessSecurityPolicyImpl::GrantRequestURL(
>        //   view-source:http://www.google.com/a
>        // In order to request these URLs, the child_id needs to be able to
>        // request the embedded URL.
> -      GrantRequestURL(child_id, GURL(url.path()));
> +      GrantRequestURL(child_id, GURL(url.GetContent()));
>      }
>
>      return;  // Can't grant the capability to request pseudo schemes.
> @@ -586,7 +586,7 @@ bool ChildProcessSecurityPolicyImpl::CanRequestURL(
>      if (url.SchemeIs(kViewSourceScheme)) {
>        // A view-source URL is allowed if the child process is permitted
to
>        // request the embedded URL. Careful to avoid pointless recursion.
> -      GURL child_url(url.path());
> +      GURL child_url(url.GetContent());
>        if (child_url.SchemeIs(kViewSourceScheme) &&
>            url.SchemeIs(kViewSourceScheme))
>            return false;
> Index: net/base/net_util_unittest.cc
> diff --git a/net/base/net_util_unittest.cc b/net/base/net_util_unittest.cc
> index
207e05a1cc077909f78c1a703d434d3b1f23bff9..4f08eb7a7929a85f40cb1005ec25fefce9cf501c
100644
> --- a/net/base/net_util_unittest.cc
> +++ b/net/base/net_util_unittest.cc
> @@ -3026,9 +3026,9 @@ TEST(NetUtilTest, SimplifyUrlForRequest) {
>        "ftp://user:pass@google.com:80/sup?yo#X#X",
>        "ftp://google.com:80/sup?yo",
>      },
> -    { // Try an nonstandard URL
> -      "foobar://user:pass@google.com:80/sup?yo#X#X",
> +    { // Try a nonstandard URL
>        "foobar://user:pass@google.com:80/sup?yo#X#X",
> +      "foobar://user:pass@google.com:80/sup?yo",
>      },
>    };
>    for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
> Index: url/gurl.h
> diff --git a/url/gurl.h b/url/gurl.h
> index
4b986a6a4deb4a22dbe204385c7c92f892ace6a3..922f2c71a8a292d51758cdba69875254852732ec
100644
> --- a/url/gurl.h
> +++ b/url/gurl.h
> @@ -223,6 +223,12 @@ class URL_EXPORT GURL {
>          (SchemeIsFileSystem() && inner_url() &&
inner_url()->SchemeIsSecure());
>    }
>
> +  // The "content" or the URL is everything after the scheme (skipping
the
> +  // scheme delimiting colon), or the entire URL if it has no scheme.
> +  std::string GetContent() const {
> +    return ComponentString(parsed_.GetContent());
> +  }
> +
>    // Returns true if the hostname is an IP address. Note: this function
isn't
>    // as cheap as a simple getter because it re-parses the hostname to
verify.
>    // This currently identifies only IPv4 addresses (bug 822685).
> Index: url/third_party/mozilla/url_parse.cc
> diff --git a/url/third_party/mozilla/url_parse.cc
b/url/third_party/mozilla/url_parse.cc
> index
52c619645d984aaeb98e2bbf85bb62c7316b7371..2a9491f045ce1667736daa190144166782004909
100644
> --- a/url/third_party/mozilla/url_parse.cc
> +++ b/url/third_party/mozilla/url_parse.cc
> @@ -481,7 +481,6 @@ void DoParsePathURL(const CHAR* spec, int spec_len,
Parsed* parsed) {
>    if (ExtractScheme(&spec[begin], spec_len - begin, &parsed->scheme)) {
>      // Offset the results since we gave ExtractScheme a substring.
>      parsed->scheme.begin += begin;
> -
>      // For compatability with the standard URL parser, we treat no path
as
>      // -1, rather than having a length of 0 (we normally wouldn't care so
>      // much for these non-standard URLs).
> @@ -792,6 +791,15 @@ int Parsed::CountCharactersBefore(ComponentType type,
>    return cur;
>  }
>
> +Component Parsed::GetContent() const {
> +  const int begin = CountCharactersBefore(USERNAME, false);
> +  const int len = Length() - begin;
> +  // For compatability with the standard URL parser, we treat no content
as
> +  // -1, rather than having a length of 0 (we normally wouldn't care so
> +  // much for these non-standard URLs).
> +  return len ? Component(begin, len) : Component();
> +}
> +
>  bool ExtractScheme(const char* url, int url_len, Component* scheme) {
>    return DoExtractScheme(url, url_len, scheme);
>  }
> Index: url/third_party/mozilla/url_parse.h
> diff --git a/url/third_party/mozilla/url_parse.h
b/url/third_party/mozilla/url_parse.h
> index
fd974f8863b079dd0d8abad325aca72a5ceab500..5fa9322512632f88f96b8e8061391c2ba01b0561
100644
> --- a/url/third_party/mozilla/url_parse.h
> +++ b/url/third_party/mozilla/url_parse.h
> @@ -159,10 +159,11 @@ struct URL_EXPORT Parsed {
>    // Port number.
>    Component port;
>
> -  // Path, this is everything following the host name. Length will be -1
if
> -  // unspecified. This includes the preceeding slash, so the path on
> -  // http://www.google.com/asdf" is "/asdf". As a result, it is
impossible to
> -  // have a 0 length path, it will be -1 in cases like "http://host?foo".
> +  // Path, this is everything following the host name, stopping at the
query of
> +  // ref delimiter (if any). Length will be -1 if unspecified. This
includes
> +  // the preceeding slash, so the path on http://www.google.com/asdf" is
> +  // "/asdf". As a result, it is impossible to have a 0 length path, it
will
> +  // be -1 in cases like "http://host?foo".
>    // Note that we treat backslashes the same as slashes.
>    Component path;
>
> @@ -177,6 +178,12 @@ struct URL_EXPORT Parsed {
>    // nothing follows it.
>    Component ref;
>
> +  // The URL spec from the character after the scheme: until the end of
the
> +  // URL, regardless of the scheme. This is mostly useful for 'opaque'
non-
> +  // hierarchical schemes like data: and javascript: as a convient way
to get
> +  // the string with the scheme stripped off.
> +  Component GetContent() const;
> +
>    // This is used for nested URL types, currently only filesystem.  If
you
>    // parse a filesystem URL, the resulting Parsed will have a nested
>    // inner_parsed_ to hold the parsed inner URL's component information.
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[joth, 2013-09-19 22:50:02]

lgtm

https://codereview.chromium.org/23549039/diff/12001/chrome_frame/utils.cc
File chrome_frame/utils.cc (right):

https://codereview.chromium.org/23549039/diff/12001/chrome_frame/utils.cc#new...
chrome_frame/utils.cc:1012: GURL sub_url(url.Content());
GetContent

https://codereview.chromium.org/23549039/diff/12001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/23549039/diff/12001/url/gurl.h#newcode227
url/gurl.h:227: // scheme delimiting colon), or the entire URL if it has no
scheme.
Probably safer as:

// The "content" or the URL is everything after the scheme (skipping the scheme
delimiting colon). It is an error to get the origin of an invalid URL. The
result will be an empty string.

...
return is_valid_ ? ComponentString(parsed_.GetContent()) : "";

we should probably move the function body to the .cc too

https://codereview.chromium.org/23549039/diff/12001/url/third_party/mozilla/u...
File url/third_party/mozilla/url_parse.cc (left):

https://codereview.chromium.org/23549039/diff/12001/url/third_party/mozilla/u...
url/third_party/mozilla/url_parse.cc:484: 
unneeded edit

[Kristian Monsen, 2013-09-20 05:57:30]

Addressed comments, looks to work on the trybots now. Will start to send reviews
tomorrow if there is a clean run tonight.

https://codereview.chromium.org/23549039/diff/12001/chrome_frame/utils.cc
File chrome_frame/utils.cc (right):

https://codereview.chromium.org/23549039/diff/12001/chrome_frame/utils.cc#new...
chrome_frame/utils.cc:1012: GURL sub_url(url.Content());
On 2013/09/19 22:50:02, joth wrote:
> GetContent

Done.

https://codereview.chromium.org/23549039/diff/12001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/23549039/diff/12001/url/gurl.h#newcode227
url/gurl.h:227: // scheme delimiting colon), or the entire URL if it has no
scheme.
On 2013/09/19 22:50:02, joth wrote:
> Probably safer as:
> 
> // The "content" or the URL is everything after the scheme (skipping the
scheme
> delimiting colon). It is an error to get the origin of an invalid URL. The
> result will be an empty string.
> 
> ...
> return is_valid_ ? ComponentString(parsed_.GetContent()) : "";
> 
> we should probably move the function body to the .cc too

Done.

https://codereview.chromium.org/23549039/diff/12001/url/third_party/mozilla/u...
File url/third_party/mozilla/url_parse.cc (left):

https://codereview.chromium.org/23549039/diff/12001/url/third_party/mozilla/u...
url/third_party/mozilla/url_parse.cc:484: 
On 2013/09/19 22:50:02, joth wrote:
> unneeded edit

Done.

[joth, 2013-09-20 18:08:15]

https://codereview.chromium.org/23549039/diff/29001/url/gurl.cc
File url/gurl.cc (right):

https://codereview.chromium.org/23549039/diff/29001/url/gurl.cc#newcode423
url/gurl.cc:423: return ComponentString(parsed_.GetContent());
return is_valid_ ? ComponentString(parsed_.GetContent()) : "";

[Kristian Monsen, 2013-09-20 21:59:04]

https://codereview.chromium.org/23549039/diff/29001/url/gurl.cc
File url/gurl.cc (right):

https://codereview.chromium.org/23549039/diff/29001/url/gurl.cc#newcode423
url/gurl.cc:423: return ComponentString(parsed_.GetContent());
On 2013/09/20 18:08:16, joth wrote:
> return is_valid_ ? ComponentString(parsed_.GetContent()) : "";

Done.

[Kristian Monsen, 2013-09-20 22:46:56]

Brett, can you do a high level pass of this?

Landing this would make https://codereview.chromium.org/23526048/ smaller and
easier to land.

[joth, 2013-10-10 06:40:35]

On 2013/09/20 22:46:56, Kristian Monsen wrote:
> Brett, can you do a high level pass of this?
> 
> Landing this would make https://codereview.chromium.org/23526048/ smaller and
> easier to land.

ping! brett could you look over this? would be good to get this part landed
before it goes stale.

[brettw, 2013-10-10 21:21:14]

Sorry! LGTM

https://codereview.chromium.org/23549039/diff/42001/url/gurl.cc
File url/gurl.cc (right):

https://codereview.chromium.org/23549039/diff/42001/url/gurl.cc#newcode423
url/gurl.cc:423: return is_valid_ ? ComponentString(parsed_.GetContent()) : "";
Can you return "std::string()" instead (faster & less code that relying on the
implicit char* -> std::string constructor).

[Kristian Monsen, 2013-10-18 05:58:22]

https://codereview.chromium.org/23549039/diff/42001/url/gurl.cc
File url/gurl.cc (right):

https://codereview.chromium.org/23549039/diff/42001/url/gurl.cc#newcode423
url/gurl.cc:423: return is_valid_ ? ComponentString(parsed_.GetContent()) : "";
On 2013/10/10 21:21:14, brettw wrote:
> Can you return "std::string()" instead (faster & less code that relying on the
> implicit char* -> std::string constructor).

Done.

[commit-bot: I haz the power, 2013-10-18 06:32:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kristianm@chromium.org/23549039/55001

[Kristian Monsen, 2013-10-18 18:23:36]

Need lgtm for various subfolders were the function is used.

[not at google - send to devlin, 2013-10-18 18:31:28]

lgtm

[commit-bot: I haz the power, 2013-10-18 18:44:38]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Michael Courage, 2013-10-18 18:52:09]

chrome/browser/extensions/api/identity lgtm

[hashimoto, 2013-10-21 03:49:05]

chrome/browser/chromeos/drive/file_system_util.cc lgtm

https://codereview.chromium.org/23549039/diff/55001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/23549039/diff/55001/url/gurl.h#newcode229
url/gurl.h:229: // The "content" or the URL is everything after the scheme
(skipping the
nit: s/or/of/?

[Kristian Monsen, 2013-10-21 21:05:42]

https://codereview.chromium.org/23549039/diff/55001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/23549039/diff/55001/url/gurl.h#newcode229
url/gurl.h:229: // The "content" or the URL is everything after the scheme
(skipping the
On 2013/10/21 03:49:06, hashimoto wrote:
> nit: s/or/of/?

Done.

[Kristian Monsen, 2013-10-24 00:02:37]

erikwright for chrome_frame owners.

[grt (UTC plus 2), 2013-10-24 12:49:04]

chrome_frame/* LGTM

[commit-bot: I haz the power, 2013-10-24 15:56:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kristianm@chromium.org/23549039/245001

[commit-bot: I haz the power, 2013-10-24 19:05:23]

Change committed as 230781