Remove URL fragment from referrer HTTP header when opening link using "Open Link in New Tab" option.

Remove URL fragment from referrer HTTP header (to comply with rfc2616) when opening link using "Open Link in New Tab" option.

Change URLRequest::set_referrer to URLRequest::SetReferrer which sanitizes referrer by removing URL fragment, user name and password.
Remove URLRequest::GetSanitizedReferrer as URLRequest::referrer is now sanitized during set.

BUG=168213
TEST=net_unittests, browser_tests

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

[mef, 2013-03-14 15:26:41]

Paul, could you take a look? 
I've reverted my changes to testserver.py.

thanks,
-m

[pauljensen, 2013-03-14 18:53:44]

Misha, this is looking right on track.  Please address my initial comments and
then post a message like "Paul, PTAL." and I'll make another pass.

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/automation...
File chrome/browser/automation/url_request_automation_job.cc (right):

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/automation...
chrome/browser/automation/url_request_automation_job.cc:485: // Ensure that we
do not send username and password fields in the referrer.
This comment doesn't make as much sense now that the code isn't calling
GetSanitizedReferrer().  I suggest changing it  to something like
"URLRequest::set_referrer() ensures we do not send..."

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu.cc:1852: // Ensure that we
do not send url fragment,
Capitalize URL.  Let's avoid "we" by saying "Ensure ... fields are not sent in
the referrer."

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu.cc:1854: GURL
referrer_url(referrer);
"referrer_url" doesn't indicate its difference from "referrer".  Maybe
"sanitized_referrer".

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:169: // Open
link in new tab shouldn't send URL fragment as referrer
Our comments are almost always complete sentances.  Please change this to
"Verify ... referrer."

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:171:
EXPECT_FALSE(browser()->profile()->IsOffTheRecord());
Why do we need a non-incognito window?

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:178: // Go
to a page with a link to echoheader url
Capitalize URL and add period.

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:183: // set
up referrer url with fragment
Capitalize "set" and URL and add period.

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:184: GURL
referrer_with_fragment_url("http://foo.com/test#fragment");
Do we need the "_url" suffix?

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:185: GURL
referrer_url("http://foo.com/test");
Can we name this more descriptively, like "correct_referrer".

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:205:
EXPECT_EQ(echoheader_url, tab->GetURL());
Should this be ASSERT instead of EXPECT?  I imagine the rest of the test won't
work if this fails.

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:206: //
Verify that the data on the page matches referrer_url (without fragment)
Add period.  When mentioning variable names in comments we prepend and suffix
"|", so "|referrer_url|" should be how "referrer_url" should appear.

[pauljensen, 2013-03-14 18:56:32]

Can you also update the description of the CL too?

[mef, 2013-03-14 20:31:38]

Paul, PTAL. 

Also, URLRequest::SetReferrer currently doesn't sanitize the URL unless it is
valid. Maybe it should set referrer_ to empty string in this case instead? 

thanks,
-m

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/automation...
File chrome/browser/automation/url_request_automation_job.cc (right):

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/automation...
chrome/browser/automation/url_request_automation_job.cc:485: // Ensure that we
do not send username and password fields in the referrer.
On 2013/03/14 18:53:44, pauljensen wrote:
> This comment doesn't make as much sense now that the code isn't calling
> GetSanitizedReferrer().  I suggest changing it  to something like
> "URLRequest::set_referrer() ensures we do not send..."

Done.

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu.cc:1852: // Ensure that we
do not send url fragment,
On 2013/03/14 18:53:44, pauljensen wrote:
> Capitalize URL.  Let's avoid "we" by saying "Ensure ... fields are not sent in
> the referrer."

Done.

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu.cc:1854: GURL
referrer_url(referrer);
On 2013/03/14 18:53:44, pauljensen wrote:
> "referrer_url" doesn't indicate its difference from "referrer".  Maybe
> "sanitized_referrer".

Done.

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:169: // Open
link in new tab shouldn't send URL fragment as referrer
On 2013/03/14 18:53:44, pauljensen wrote:
> Our comments are almost always complete sentances.  Please change this to
> "Verify ... referrer."

Done.

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:171:
EXPECT_FALSE(browser()->profile()->IsOffTheRecord());
On 2013/03/14 18:53:44, pauljensen wrote:
> Why do we need a non-incognito window?
I guess we don't.

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:178: // Go
to a page with a link to echoheader url
On 2013/03/14 18:53:44, pauljensen wrote:
> Capitalize URL and add period.

Done.

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:183: // set
up referrer url with fragment
On 2013/03/14 18:53:44, pauljensen wrote:
> Capitalize "set" and URL and add period.

Done.

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:184: GURL
referrer_with_fragment_url("http://foo.com/test#fragment");
On 2013/03/14 18:53:44, pauljensen wrote:
> Do we need the "_url" suffix?

Done.

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:185: GURL
referrer_url("http://foo.com/test");
On 2013/03/14 18:53:44, pauljensen wrote:
> Can we name this more descriptively, like "correct_referrer".

Done.

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:205:
EXPECT_EQ(echoheader_url, tab->GetURL());
On 2013/03/14 18:53:44, pauljensen wrote:
> Should this be ASSERT instead of EXPECT?  I imagine the rest of the test won't
> work if this fails.

Done.

https://codereview.chromium.org/12569007/diff/49001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:206: //
Verify that the data on the page matches referrer_url (without fragment)
On 2013/03/14 18:53:44, pauljensen wrote:
> Add period.  When mentioning variable names in comments we prepend and suffix
> "|", so "|referrer_url|" should be how "referrer_url" should appear.

Done.

[pauljensen, 2013-03-15 13:01:57]

Can you make the first line of the description a description of what your change
does, like:
Remove URL fragment from referrer HTTP header when opening link using "Open Link
in New Tab" option.

And could you add "BUG=168213" to the end of the description.  You can take a
look at some of the recent commits on http://build.chromium.org to see what
descriptions commonly look like.

[pauljensen, 2013-03-15 13:37:45]

I made some more comments.

To answer the question about an invalid URL, I'd consult the RFC about what the
referrer header is meant to hold, if it's strictly supposed to be a URL then
maybe we should ignore invalid ones.

One thing I notice when I investigate this referrer stuff is that almost every
call to set_referrer() is passing in some_gurl.spec() and almost every call to
referrer() is wrapped in GURL(referrer()).  Part of me wonders if we should be
taking and giving back GURLs in set_referrer() and referrer().  I guess this
also goes back to what the RFC says the header is supposed to contain.

https://codereview.chromium.org/12569007/diff/60001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/12569007/diff/60001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:180: +
"'>link</a>"));
I think this should be indented like so:

ui_test_utils::NavigateToURL(browser(),
                             GURL("data:text/html,<a href='" +
                                  echoheader_url.spec() +
                                  "'>link</a>"));

It might be nice to define the data URL on the prior line to avoid all the
wrapping.

https://codereview.chromium.org/12569007/diff/60001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:184: GURL
correct_referrer("http://foo.com/test");
I think correct_referrer should just be a std::string.

https://codereview.chromium.org/12569007/diff/60001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:189:
context_menu_params.link_url = GURL(echoheader_url);
Can we change "GURL(blah)" to just "blah" on these two lines?

https://codereview.chromium.org/12569007/diff/60001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:191: // The
menu_observer will select "Open in new tab", wait for the new tab to
You say "Open in new tab" here but "Open Link in New Tab" at the beginning of
the function, can you make these the same?

I think the "," should be "and".

https://codereview.chromium.org/12569007/diff/60001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:209:
"window.domAutomationController.send(window.document.referrer);",
This is sending back the referrer?  The comment says you're sending back the
"data on the page".

https://codereview.chromium.org/12569007/diff/60001/content/browser/loader/re...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/12569007/diff/60001/content/browser/loader/re...
content/browser/loader/resource_loader.cc:530: GURL
referrer(request_->referrer());
Can we just go back to the old code but use GURL(request_->referrer())?

https://codereview.chromium.org/12569007/diff/60001/net/url_request/url_reque...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/12569007/diff/60001/net/url_request/url_reque...
net/url_request/url_request.cc:432: // Ensure that we do not send url fragment,
Please capitalize URL.  Can we put "username and password" on the prior line?

https://codereview.chromium.org/12569007/diff/60001/net/url_request/url_reque...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/12569007/diff/60001/net/url_request/url_reque...
net/url_request/url_request.h:373: // Referrer is sanitized to remove URL
fragment, user name and password
Please add period.

[mef, 2013-03-15 16:36:42]

Paul, PTAL.

https://codereview.chromium.org/12569007/diff/60001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/12569007/diff/60001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:180: +
"'>link</a>"));
On 2013/03/15 13:37:45, pauljensen wrote:
> I think this should be indented like so:
> 
> ui_test_utils::NavigateToURL(browser(),
>                              GURL("data:text/html,<a href='" +
>                                   echoheader_url.spec() +
>                                   "'>link</a>"));
> 
> It might be nice to define the data URL on the prior line to avoid all the
> wrapping.

Done.

https://codereview.chromium.org/12569007/diff/60001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:184: GURL
correct_referrer("http://foo.com/test");
On 2013/03/15 13:37:45, pauljensen wrote:
> I think correct_referrer should just be a std::string.

Done.

https://codereview.chromium.org/12569007/diff/60001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:191: // The
menu_observer will select "Open in new tab", wait for the new tab to
On 2013/03/15 13:37:45, pauljensen wrote:
> You say "Open in new tab" here but "Open Link in New Tab" at the beginning of
> the function, can you make these the same?
> 
> I think the "," should be "and".

Done.

https://codereview.chromium.org/12569007/diff/60001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:209:
"window.domAutomationController.send(window.document.referrer);",
On 2013/03/15 13:37:45, pauljensen wrote:
> This is sending back the referrer?  The comment says you're sending back the
> "data on the page".

Done.

https://codereview.chromium.org/12569007/diff/60001/content/browser/loader/re...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/12569007/diff/60001/content/browser/loader/re...
content/browser/loader/resource_loader.cc:530: GURL
referrer(request_->referrer());
On 2013/03/15 13:37:45, pauljensen wrote:
> Can we just go back to the old code but use GURL(request_->referrer())?

Done.

https://codereview.chromium.org/12569007/diff/60001/content/browser/loader/re...
content/browser/loader/resource_loader.cc:530: GURL
referrer(request_->referrer());
On 2013/03/15 13:37:45, pauljensen wrote:
> Can we just go back to the old code but use GURL(request_->referrer())?

Done.

https://codereview.chromium.org/12569007/diff/60001/net/url_request/url_reque...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/12569007/diff/60001/net/url_request/url_reque...
net/url_request/url_request.cc:432: // Ensure that we do not send url fragment,
On 2013/03/15 13:37:45, pauljensen wrote:
> Please capitalize URL.  Can we put "username and password" on the prior line?

Done.

https://codereview.chromium.org/12569007/diff/60001/net/url_request/url_reque...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/12569007/diff/60001/net/url_request/url_reque...
net/url_request/url_request.h:373: // Referrer is sanitized to remove URL
fragment, user name and password
On 2013/03/15 13:37:45, pauljensen wrote:
> Please add period.

Done.

[pauljensen, 2013-03-15 21:50:00]

I think we're almost there; this is probably my last set of comments.

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/automation...
File chrome/browser/automation/url_request_automation_job.cc (right):

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/automation...
chrome/browser/automation/url_request_automation_job.cc:485: //
URLRequest::SetReferrer() ensures that
Can you move more words onto this line to bring it closer to 80 columns?

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu.cc:1852: // Ensure that URL
fragment, username and password fields
Can you move more words onto this line to bring it closer to 80 columns?

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:178: // Go
to a page with a link to echoheader URL.
Can you move the definition of |page| down here?

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:190: // The
menu_observer will select "Open Link in New Tab"
Can you move more words onto this line to bring it closer to 80 columns?

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:208:
"window.domAutomationController.send(window.document.referrer);",
Can we add another ASSERT_TRUE(ExecuteScriptAndExtractString(...)) to check that
the contents of the page match |correct_referrer|?  My concern is two-fold:
1. window.document.referrer could be sanitizing the referrer
2. why are we browsing to the echoheader URL if we aren't checking the contents
of the page

https://codereview.chromium.org/12569007/diff/66001/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/12569007/diff/66001/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:871: // Ensure that we do not send
username and password fields in the referrer.
This comment doesn't make as much sense now that the code isn't calling
GetSanitizedReferrer().  I suggest changing it  to something like
"URLRequest::set_referrer() ensures we do not send..."

[mef, 2013-03-18 16:42:07]

Paul, PTAL.

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/automation...
File chrome/browser/automation/url_request_automation_job.cc (right):

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/automation...
chrome/browser/automation/url_request_automation_job.cc:485: //
URLRequest::SetReferrer() ensures that
On 2013/03/15 21:50:00, pauljensen wrote:
> Can you move more words onto this line to bring it closer to 80 columns?

Done.

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu.cc:1852: // Ensure that URL
fragment, username and password fields
On 2013/03/15 21:50:00, pauljensen wrote:
> Can you move more words onto this line to bring it closer to 80 columns?

Done.

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:178: // Go
to a page with a link to echoheader URL.
On 2013/03/15 21:50:00, pauljensen wrote:
> Can you move the definition of |page| down here?

Done.

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:190: // The
menu_observer will select "Open Link in New Tab"
On 2013/03/15 21:50:00, pauljensen wrote:
> Can you move more words onto this line to bring it closer to 80 columns?

Done.

https://codereview.chromium.org/12569007/diff/66001/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/12569007/diff/66001/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:871: // Ensure that we do not send
username and password fields in the referrer.
On 2013/03/15 21:50:00, pauljensen wrote:
> This comment doesn't make as much sense now that the code isn't calling
> GetSanitizedReferrer().  I suggest changing it  to something like
> "URLRequest::set_referrer() ensures we do not send..."

Done.

https://codereview.chromium.org/12569007/diff/78001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/12569007/diff/78001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_unittest.cc:1290:
req.SetReferrer("123456789");
I've verified that this test passes.

[pauljensen, 2013-03-18 20:16:19]

Please capitalize URL in the CL description.

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/12569007/diff/66001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:208:
"window.domAutomationController.send(window.document.referrer);",
On 2013/03/15 21:50:00, pauljensen wrote:
> Can we add another ASSERT_TRUE(ExecuteScriptAndExtractString(...)) to check
that
> the contents of the page match |correct_referrer|?  My concern is two-fold:
> 1. window.document.referrer could be sanitizing the referrer
> 2. why are we browsing to the echoheader URL if we aren't checking the
contents
> of the page
I didn't see a response to this comment but it looks like you instead changed
the test to not check window.document.referrer.  I guess this is okay.  I was
recommending we have both tests.  The way you have it now is fine too.  In the
future please always respond individually to all comments or send a message
indicating that you've addressed all comments.  Sometimes the one message is
nicer than tons of "Done." responses.

https://codereview.chromium.org/12569007/diff/80001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/12569007/diff/80001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:210: 
You added some whitespace; please remove.

[mef, 2013-03-18 20:59:16]

Paul, PTAL. I've also removed some patch sets...

thanks,
-m

https://codereview.chromium.org/12569007/diff/80001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/12569007/diff/80001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:210: 
On 2013/03/18 20:16:19, pauljensen wrote:
> You added some whitespace; please remove.

Done.

[pauljensen, 2013-03-19 17:21:37]

LGTM

[mef, 2013-03-19 18:33:09]

Matt,

could you please review my changes for referrer fix.

thanks,
-m

[mmenke, 2013-03-19 19:01:09]

Looks pretty good.

https://codereview.chromium.org/12569007/diff/82002/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/12569007/diff/82002/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:209:
ASSERT_EQ(correct_referrer, actual_referrer);
We should check "document.referrer" just like we do
window.document.body.textContent.  (One is what Javascript sees, the other is
what the HTTP server sees).

https://codereview.chromium.org/12569007/diff/82002/net/url_request/url_reque...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/12569007/diff/82002/net/url_request/url_reque...
net/url_request/url_request.cc:437: referrer_url.is_valid());
An empty referrer URL is presumably not a valid GURL, but must be used, per
spec, when going from https to http, or when navigating to a URL from the
omnibox, to a bookmark, or from an external program, so they should either be
excluded from the histogram, or treated as valid.

https://codereview.chromium.org/12569007/diff/82002/net/url_request/url_reque...
net/url_request/url_request.cc:437: referrer_url.is_valid());
What's the motivation for gathering this information?  I'm not strongly opposed
to it, just unsure of the motivation.

https://codereview.chromium.org/12569007/diff/82002/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/12569007/diff/82002/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:3517: }
Suggest one more test:  SetReferrer with "" after a SetReferrer call with a
valid URL.  Setting a valid URL first probably doesn't happen in practice, but
best to be safe.

[mef, 2013-03-19 21:40:28]

Matt,

thanks! I've added tests that you've suggested, could you take another look?

thanks,
-m

https://codereview.chromium.org/12569007/diff/82002/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/12569007/diff/82002/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:209:
ASSERT_EQ(correct_referrer, actual_referrer);
On 2013/03/19 19:01:09, mmenke wrote:
> We should check "document.referrer" just like we do
> window.document.body.textContent.  (One is what Javascript sees, the other is
> what the HTTP server sees).

Right. We've discussed this with Paul. We are trying to verify the server's
point of view, so textContent seems more appropriate. I'll verify both of them.

https://codereview.chromium.org/12569007/diff/82002/net/url_request/url_reque...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/12569007/diff/82002/net/url_request/url_reque...
net/url_request/url_request.cc:437: referrer_url.is_valid());
On 2013/03/19 19:01:09, mmenke wrote:
> An empty referrer URL is presumably not a valid GURL, but must be used, per
> spec, when going from https to http, or when navigating to a URL from the
> omnibox, to a bookmark, or from an external program, so they should either be
> excluded from the histogram, or treated as valid.

Done.

https://codereview.chromium.org/12569007/diff/82002/net/url_request/url_reque...
net/url_request/url_request.cc:437: referrer_url.is_valid());
On 2013/03/19 19:01:09, mmenke wrote:
> What's the motivation for gathering this information?  I'm not strongly
opposed
> to it, just unsure of the motivation.
The idea is to see how often are invalid urls seen as referrers, so potentially
change referrer_ type from std::string to GURL.

https://codereview.chromium.org/12569007/diff/82002/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/12569007/diff/82002/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:3517: }
On 2013/03/19 19:01:09, mmenke wrote:
> Suggest one more test:  SetReferrer with "" after a SetReferrer call with a
> valid URL.  Setting a valid URL first probably doesn't happen in practice, but
> best to be safe.

Done.

[mmenke, 2013-03-20 16:33:50]

LGTM.

You may want to separate out the tab_contents changes to a separate CL, since
those are independent of the rest of the CL.

For other reviewers, you're going to need owners for everything except
browser/net and net/.  I'd suggest an 	 expert on chrome/browser/tab_contents
and a general owner for the other changes, which are just simple refactoring
changes.

The only tab_contents owner with knowledge of those modified files looks to be
brettw, who is also, coincidentally, an owner of chrome, content, and webkit, so
you only need his signoff.

https://codereview.chromium.org/12569007/diff/82002/net/url_request/url_reque...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/12569007/diff/82002/net/url_request/url_reque...
net/url_request/url_request.cc:437: referrer_url.is_valid());
On 2013/03/19 21:40:29, mef wrote:
> On 2013/03/19 19:01:09, mmenke wrote:
> > What's the motivation for gathering this information?  I'm not strongly
> opposed
> > to it, just unsure of the motivation.
> The idea is to see how often are invalid urls seen as referrers, so
potentially
> change referrer_ type from std::string to GURL.

Sounds reasonable to me.

https://codereview.chromium.org/12569007/diff/89001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/12569007/diff/89001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:170:
IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, OpenInNewTabReferrer) {
Optional:  Just for completeness, may want to add another test for the
IDC_CONTENT_CONTEXT_OPENLINKOFFTHERECORD case.  Can probably just copy the test
and modify one line of code, though could also double-check that the new window
is in fact an incognito window, just to broaden coverage of that case a bit.

https://codereview.chromium.org/12569007/diff/89001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:183:
std::string correct_referrer("http://foo.com/test");
nit:  Since these are both hard-coded constants, I'd suggest making them const
and using kConstantStyle naming.

https://codereview.chromium.org/12569007/diff/89001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:210: //
Verify that the referrer on the page matches |correct_referrer|.
optional nit:  May want a line break above the comment to more clearly separate
the two blocks that call into Javascript.

[mef, 2013-03-20 18:25:31]

Hi Brett,

could you please review my fix for BUG=168213 (Remove URL fragment from referrer
HTTP header when opening link using "Open Link in New Tab" option)?

thanks,
-m

https://codereview.chromium.org/12569007/diff/89001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/12569007/diff/89001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:170:
IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, OpenInNewTabReferrer) {
Done. Expectantly (I think) this case is slightly different as page open in
incognito window gets empty referrer (returned as 'None' by
testserver/echoheader). It also opens a new window as opposed to new tab, but
the tab observer picks it up correctly.


On 2013/03/20 16:33:51, mmenke wrote:
> Optional:  Just for completeness, may want to add another test for the
> IDC_CONTENT_CONTEXT_OPENLINKOFFTHERECORD case.  Can probably just copy the
test
> and modify one line of code, though could also double-check that the new
window
> is in fact an incognito window, just to broaden coverage of that case a bit.

https://codereview.chromium.org/12569007/diff/89001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:183:
std::string correct_referrer("http://foo.com/test");
On 2013/03/20 16:33:51, mmenke wrote:
> nit:  Since these are both hard-coded constants, I'd suggest making them const
> and using kConstantStyle naming.

Done.

https://codereview.chromium.org/12569007/diff/89001/chrome/browser/tab_conten...
chrome/browser/tab_contents/render_view_context_menu_browsertest.cc:210: //
Verify that the referrer on the page matches |correct_referrer|.
On 2013/03/20 16:33:51, mmenke wrote:
> optional nit:  May want a line break above the comment to more clearly
separate
> the two blocks that call into Javascript.

Done.

[mef, 2013-04-01 14:44:31]

Hi Brett,

sorry to bother you again, but is possible to review this CL some time soon? 

The net/ portion was LGTMed by Paul and Matt, so if you could look at (minor)
changes in content/browser/ and chrome/browser/, then it should be all set.


thanks,
-m

[brettw, 2013-04-03 05:23:07]

What do you need reviewed? Is everything reviewed and you just need owners
review for some files? If so, which ones?

[mef, 2013-04-03 14:43:39]

On 2013/04/03 05:23:07, brettw wrote:
> What do you need reviewed? Is everything reviewed and you just need owners
> review for some files? If so, which ones?

Brett,

thanks for getting back to me! 

Yes, everything was reviewed by Matt Menke and Paul Jensen, and at this point I
just need owner's review for everything outside of browser/net and net/.

Non-trivial changes were done only in chrome/browser/tab_contents/.

thanks,
-m

[brettw, 2013-04-08 23:23:13]

owners lgtm

[commit-bot: I haz the power, 2013-04-10 20:29:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/12569007/111001

[commit-bot: I haz the power, 2013-04-10 22:46:16]

Change committed as 193482