Fix view-source url creation logic

Fix view-source url creation logic

BUG=336846
The logic to create a view-source URL from a non-view-source URL has
the incorrect assumption that the URL path includes both the query and the
fragment parts. This causes these parts to be repeated after the
transformation.

Committed: https://crrev.com/939a726c563033b68a307faaad16ddab06e05849
Cr-Commit-Position: refs/heads/master@{#299752}

[sgurun-gerrit only, 2014-10-14 19:17:24]

sgurun@chromium.org changed reviewers:
+ sky@chromium.org

[sgurun-gerrit only, 2014-10-14 19:17:47]

On 2014/10/14 19:17:24, sgurun_OOO_until_Oct5 wrote:
> mailto:sgurun@chromium.org changed reviewers:
> + mailto:sky@chromium.org

@sky, PTAL, thanks!

[sky, 2014-10-14 19:56:31]

https://codereview.chromium.org/655023002/diff/1/content/browser/browser_url_...
File content/browser/browser_url_handler_impl.cc (right):

https://codereview.chromium.org/655023002/diff/1/content/browser/browser_url_...
content/browser/browser_url_handler_impl.cc:66: *url = GURL(kViewSourceScheme +
std::string(":") + url->spec());
Doesn't gurl have the ability to replace parts of it? Specifically
ReplaceComponents?

[sgurun-gerrit only, 2014-10-14 21:02:52]

https://codereview.chromium.org/655023002/diff/1/content/browser/browser_url_...
File content/browser/browser_url_handler_impl.cc (right):

https://codereview.chromium.org/655023002/diff/1/content/browser/browser_url_...
content/browser/browser_url_handler_impl.cc:66: *url = GURL(kViewSourceScheme +
std::string(":") + url->spec());
On 2014/10/14 19:56:31, sky wrote:
> Doesn't gurl have the ability to replace parts of it? Specifically
> ReplaceComponents?

I could not see an easy way of doing it. Two possibilities that I can think of
are:
1. add 
repl.ClearQuery();
repl.ClearRequest();
after line 69 in previous version.
2. or add a std::string getPath() kind of a getter to GURL and then use it to
replace the path component.

[sky, 2014-10-14 22:56:28]

sky@chromium.org changed reviewers:
+ brettw@chromium.org

[sky, 2014-10-14 22:56:38]

+brettw for url replacements usage.

[brettw, 2014-10-15 03:19:19]

I think in this case the new code is the clearest. LGTM

[sgurun-gerrit only, 2014-10-15 04:56:27]

The CQ bit was checked by sgurun@chromium.org

[commit-bot: I haz the power, 2014-10-15 04:57:16]

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

[commit-bot: I haz the power, 2014-10-15 05:04:27]

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

[commit-bot: I haz the power, 2014-10-15 05:04:28]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sgurun-gerrit only, 2014-10-15 17:25:24]

On 2014/10/15 05:04:28, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

@sky, need owners LGTM

[sky, 2014-10-15 19:13:09]

LGTM

[sgurun-gerrit only, 2014-10-15 19:15:22]

The CQ bit was checked by sgurun@chromium.org

[commit-bot: I haz the power, 2014-10-15 19:16:38]

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

[commit-bot: I haz the power, 2014-10-15 20:21:25]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-10-15 20:22:04]

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

[Mark P, 2014-10-15 20:51:31]

Drive-by comment:
is it possible to add a test for this?

I tend to think all bugs found in the wild should have tests so they don't
regress again...

--mark

[sgurun-gerrit only, 2014-10-15 22:48:18]

On 2014/10/15 20:51:31, Mark P wrote:
> Drive-by comment:
> is it possible to add a test for this?
> 
> I tend to think all bugs found in the wild should have tests so they don't
> regress again...
> 
> --mark

I think we can add a unit test for that. I will look at it.

[joth, 2014-10-17 20:18:39]

joth@chromium.org changed reviewers:
+ joth@chromium.org

[joth, 2014-10-17 20:18:40]

https://codereview.chromium.org/655023002/diff/1/content/browser/browser_url_...
File content/browser/browser_url_handler_impl.cc (right):

https://codereview.chromium.org/655023002/diff/1/content/browser/browser_url_...
content/browser/browser_url_handler_impl.cc:66: *url = GURL(kViewSourceScheme +
std::string(":") + url->spec());
Thanks for fixing my bug! (well, bug my cl exposed :)

worth adding some regression tests for ReverseViewSource with a couple of the
example URLs from the bug report?

[sgurun-gerrit only, 2014-10-17 20:35:35]

https://codereview.chromium.org/655023002/diff/1/content/browser/browser_url_...
File content/browser/browser_url_handler_impl.cc (right):

https://codereview.chromium.org/655023002/diff/1/content/browser/browser_url_...
content/browser/browser_url_handler_impl.cc:66: *url = GURL(kViewSourceScheme +
std::string(":") + url->spec());
On 2014/10/17 20:18:39, joth (inactive) wrote:
> Thanks for fixing my bug! (well, bug my cl exposed :)
> 
> worth adding some regression tests for ReverseViewSource with a couple of the
> example URLs from the bug report?

Rather then regression tests, I have written a unit test, which I think was more
appropriate in this case.
https://codereview.chromium.org/659153003/

[sgurun-gerrit only, 2014-10-17 20:37:40]

https://codereview.chromium.org/655023002/diff/1/content/browser/browser_url_...
File content/browser/browser_url_handler_impl.cc (right):

https://codereview.chromium.org/655023002/diff/1/content/browser/browser_url_...
content/browser/browser_url_handler_impl.cc:66: *url = GURL(kViewSourceScheme +
std::string(":") + url->spec());
On 2014/10/17 20:35:35, sgurun wrote:
> On 2014/10/17 20:18:39, joth (inactive) wrote:
> > Thanks for fixing my bug! (well, bug my cl exposed :)
> > 
> > worth adding some regression tests for ReverseViewSource with a couple of
the
> > example URLs from the bug report?
> 
> Rather then regression tests, I have written a unit test, which I think was
more
> appropriate in this case.
> https://codereview.chromium.org/659153003/


haha I think I read regression as integration tests :) yeah wrote a unit test.