When the user navigates to the home page, make sure to set the RLZ string

When the user navigates to the home page, make sure to set the RLZ string
if needed.


John: can you plase look at the content\... changes?
Scott: can you plase look at the chrome\... changes?

BUG=0
TEST=Install an official chrome build (not a chromium build) with a non
organic brand code.  During the install, make sure to choose google as the
home page.  When google is running, pressing the home page button should
cause the request to include the RLZ http header.  Pressed Alt-Home should
do the samer thing.

When starting chrome, the RLZ header should also be present since the startup
page is the home page, and the home page is set to google.

Navigating to the google.com manually by typing the URL or by clicking on
a link should not cause the hrrp header to be added.

Not choosing google as the home during install, or changing the home page
to be something other than google should stop the http header from being
added.  This is true is even if the user subsequently set his home page to
google again.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99283
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99309

[sky, 2011-08-30 15:56:53]

http://codereview.chromium.org/7791029/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7791029/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:1888: std::string homepage =
pref_service->GetString(prefs::kHomePage);
I don't know enough about the install flow. If you choose google as the home
page do we always put GoogleURLTracker::kDefaultGoogleHomepage in your prefs
regardless of where in the world you might be?

http://codereview.chromium.org/7791029/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:1899:
RLZTracker::GetAccessPointRlz(rlz_lib::CHROME_HOME_PAGE, NULL);
This sounds like you're trying to work around a race condition, and that it'll
impact startup. What exactly does this do?

http://codereview.chromium.org/7791029/diff/1/chrome/browser/ui/browser_navig...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/7791029/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:415: std::string extra_headers;
nit: move this closer to the two places that use it.

[Roger Tawa OOO till Jul 10th, 2011-08-30 16:19:31]

Thanks Scott.  See comments below.

http://codereview.chromium.org/7791029/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7791029/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:1888: std::string homepage =
pref_service->GetString(prefs::kHomePage);
On 2011/08/30 15:56:53, sky wrote:
> I don't know enough about the install flow. If you choose google as the home
> page do we always put GoogleURLTracker::kDefaultGoogleHomepage in your prefs
> regardless of where in the world you might be?

Yes, I already checked this wit the PSO folks.

http://codereview.chromium.org/7791029/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:1899:
RLZTracker::GetAccessPointRlz(rlz_lib::CHROME_HOME_PAGE, NULL);
On 2011/08/30 15:56:53, sky wrote:
> This sounds like you're trying to work around a race condition, and that it'll
> impact startup. What exactly does this do?

This posts a tasks to the file thread to fetch the rlz string from the registry.
 You can see the code in rlz.cc in this CL.  I don't think this will impact
startup.

http://codereview.chromium.org/7791029/diff/1/chrome/browser/ui/browser_navig...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/7791029/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:415: std::string extra_headers;
On 2011/08/30 15:56:53, sky wrote:
> nit: move this closer to the two places that use it.

This is the closest given the nesting of the if blocks and such.  However, I
could move it down to line 419 if you like.

[sky, 2011-08-30 16:44:28]

LGTM

http://codereview.chromium.org/7791029/diff/1/chrome/browser/ui/browser_navig...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/7791029/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:415: std::string extra_headers;
On 2011/08/30 16:19:31, Roger Tawa wrote:
> On 2011/08/30 15:56:53, sky wrote:
> > nit: move this closer to the two places that use it.
> 
> This is the closest given the nesting of the if blocks and such.  However, I
> could move it down to line 419 if you like.

I was misreading the code. It's fine, leave it where it is.

[jam, 2011-08-30 16:56:29]

content/common lgtm. sky is an owner for content/browser

[sky, 2011-08-30 17:05:25]

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/na...
File content/browser/tab_contents/navigation_controller.cc (right):

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_controller.cc:278:
Details<NavigationEntry>(entry));
Update comments for NAV_ENTRY_PENDING indicating what the details is.

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/na...
File content/browser/tab_contents/navigation_controller.h (right):

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_controller.h:167: void LoadURL(const
GURL& url, const GURL& referrer,
Nuke this one in favor of new. I'm fine with doing that in a separate pass
(assuming you'll do it), just add a TODO here.

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_controller.h:172: void
LoadURLWithHeaders(const GURL& url, const GURL& referrer,
each param on its own line.

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/na...
File content/browser/tab_contents/navigation_entry.h (right):

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_entry.h:408: // WARNING WARNING WARNING
WARNING WARNING WARNING WARNING WARNING WARNING
I think you need to update session restore to deal with the extra headers.

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/ta...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents.cc:161: params->extra_headers +=
delegate->GetNavigationHeaders(params->url);
Is concatanating the strings going to do the right thing? Maybe
GetNavigationHeaders should take the proposed headers and return the resulting
headers.

[Roger Tawa OOO till Jul 10th, 2011-08-30 18:58:32]

Thanks Scott, please take another look.

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/na...
File content/browser/tab_contents/navigation_controller.cc (right):

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_controller.cc:278:
Details<NavigationEntry>(entry));
On 2011/08/30 17:05:25, sky wrote:
> Update comments for NAV_ENTRY_PENDING indicating what the details is.

Done.

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/na...
File content/browser/tab_contents/navigation_controller.h (right):

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_controller.h:167: void LoadURL(const
GURL& url, const GURL& referrer,
On 2011/08/30 17:05:25, sky wrote:
> Nuke this one in favor of new. I'm fine with doing that in a separate pass
> (assuming you'll do it), just add a TODO here.

I'll do it as part of this CL.  It will create a lot of one line changes in lots
of places though.  Will upload the change shortly.

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_controller.h:172: void
LoadURLWithHeaders(const GURL& url, const GURL& referrer,
On 2011/08/30 17:05:25, sky wrote:
> each param on its own line.

Done.

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/na...
File content/browser/tab_contents/navigation_entry.h (right):

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/na...
content/browser/tab_contents/navigation_entry.h:408: // WARNING WARNING WARNING
WARNING WARNING WARNING WARNING WARNING WARNING
On 2011/08/30 17:05:25, sky wrote:
> I think you need to update session restore to deal with the extra headers.

Done.  Please take a close look though.

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/ta...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/7791029/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents.cc:161: params->extra_headers +=
delegate->GetNavigationHeaders(params->url);
On 2011/08/30 17:05:25, sky wrote:
> Is concatanating the strings going to do the right thing? Maybe
> GetNavigationHeaders should take the proposed headers and return the resulting
> headers.

Good catch.  Done.

[sky, 2011-08-30 20:16:33]

http://codereview.chromium.org/7791029/diff/4004/chrome/browser/sessions/base...
File chrome/browser/sessions/base_session_service.cc (right):

http://codereview.chromium.org/7791029/diff/4004/chrome/browser/sessions/base...
chrome/browser/sessions/base_session_service.cc:180: WriteStringToPickle(pickle,
&bytes_written, max_state_size,
Add test coverage of this.

http://codereview.chromium.org/7791029/diff/4004/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents_delegate_unittest.cc (right):

http://codereview.chromium.org/7791029/diff/4004/content/browser/tab_contents...
content/browser/tab_contents/tab_contents_delegate_unittest.cc:38: virtual void
AddNavigationHeaders(const GURL& url, std::string* headers) {
While you're here add OVERRIDE.

[Roger Tawa OOO till Jul 10th, 2011-08-31 20:14:01]

Hi Scott,

This change is getting rather large.  I think I'll open myself a bug to merge
LoadURLWithHeaders() with  LoadURL() as you had suggested earlier and do it as a
follow up CL.  I will have the sync folks review the changes to that part of the
code.  Thanks.

http://codereview.chromium.org/7791029/diff/4004/chrome/browser/sessions/base...
File chrome/browser/sessions/base_session_service.cc (right):

http://codereview.chromium.org/7791029/diff/4004/chrome/browser/sessions/base...
chrome/browser/sessions/base_session_service.cc:180: WriteStringToPickle(pickle,
&bytes_written, max_state_size,
On 2011/08/30 20:16:33, sky wrote:
> Add test coverage of this.

Done.

http://codereview.chromium.org/7791029/diff/4004/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents_delegate_unittest.cc (right):

http://codereview.chromium.org/7791029/diff/4004/content/browser/tab_contents...
content/browser/tab_contents/tab_contents_delegate_unittest.cc:38: virtual void
AddNavigationHeaders(const GURL& url, std::string* headers) {
On 2011/08/30 20:16:33, sky wrote:
> While you're here add OVERRIDE.

Done.

[sky, 2011-08-31 20:29:58]

LGTM

[Roger Tawa OOO till Jul 10th, 2011-08-31 21:03:48]

Hi Scott,

After discussing with Tim and Nicolas, I will remove all the session restore
code I have written, since its not needed for the home page change that is the
reason for this CL, and its not clear if session restore would be needed in the
general case either for extra headers.  If session restore is required, it can
be done as a follow up CL.

I will remove the changes and upload another patch that includes only the
original changes.  Although this code is in theory already lgtm'ed, I'll let you
take another look before committing.  I'll upload shortly.  Thanks.

[sky, 2011-09-01 03:46:43]

SLGTM

http://codereview.chromium.org/7791029/diff/19013/content/browser/tab_content...
File content/browser/tab_contents/navigation_entry.h (right):

http://codereview.chromium.org/7791029/diff/19013/content/browser/tab_content...
content/browser/tab_contents/navigation_entry.h:427: std::string extra_headers_;
Put a newline between 426 and 427 and document this is NOT persisted with
session restore.

[Roger Tawa OOO till Jul 10th, 2011-09-01 13:52:57]

Thanks Scott.  Change uploaded.  Will commit after trybots.

http://codereview.chromium.org/7791029/diff/19013/content/browser/tab_content...
File content/browser/tab_contents/navigation_entry.h (right):

http://codereview.chromium.org/7791029/diff/19013/content/browser/tab_content...
content/browser/tab_contents/navigation_entry.h:427: std::string extra_headers_;
On 2011/09/01 03:46:44, sky wrote:
> Put a newline between 426 and 427 and document this is NOT persisted with
> session restore.

Done.