Prevent non-Incognito windows in the Guest session.

Prevent non-Incognito windows in the Guest session.

BUG=chromium-os:10273
TEST=manual

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=74263

[Evan Stade, 2011-01-24 21:59:42]

I don't know. Why are you allowed to open settings at all in guest mode? it
seems like most if not all of the settings would be irrelevant. I'd like to see
Glen or Ben or some other UI lead's take on the expected behavior.

[Glen Murphy, 2011-01-24 22:12:03]

It's a little weird, I agree, but I don't think we should prevent
users from accessing it - some people may use long-running guest
sessions, or have i18n things that they would want to change.

There's a slight conflict in that we don't allow settings to open in
incognito in regular sessions (opening settings from an incognito
window should open settings in a regular window).



On Mon, Jan 24, 2011 at 1:59 PM,  <estade@chromium.org> wrote:
> I don't know. Why are you allowed to open settings at all in guest mode? it
> seems like most if not all of the settings would be irrelevant. I'd like to
> see
> Glen or Ben or some other UI lead's take on the expected behavior.
>
> http://codereview.chromium.org/6272016/
>

[Evan Stade, 2011-01-24 22:49:07]

certain parts of the UI would be unintuitive. The fact that the startup pages
still exists would probably confuse the user, or make them think that it would
be respected. I would say that at the very least, we should go section by
section in the UI and disable (grey out) those that don't make sense in guest
mode.

Also, I bet that there are things which would actually break if we commit this
patch without testing, i.e. places where we use profile() but need to use
profile()->GetOriginalProfile() instead.

[altimofeev, 2011-01-25 12:10:41]

On 2011/01/24 22:49:07, Evan Stade wrote:
> certain parts of the UI would be unintuitive. The fact that the startup pages
> still exists would probably confuse the user, or make them think that it would
> be respected. I would say that at the very least, we should go section by
> section in the UI and disable (grey out) those that don't make sense in guest
> mode.

What "startup pages" are you saying about? 
IMO, not having settings page will be more confusing, even for guests. BTW,
there is http://code.google.com/p/chromium-os/issues/detail?id=7316 which
discusses the things you are speaking about.

> Also, I bet that there are things which would actually break if we commit this
> patch without testing, i.e. places where we use profile() but need to use
> profile()->GetOriginalProfile() instead.

And you win :) I forgot to pass non-NULL PersonalDataManager.

So, I have also added OffTheRecordProfileImpl subclass for cases when user is in
the Guest session. Please take another look.

[altimofeev, 2011-01-25 12:15:43]

@kan: I need your opinion on the question. Should we bring settings page back to
the incognito tabs or we should first decide what options will be shown to the
guests?

[Ben Goodger (Google), 2011-01-25 20:04:51]

I need to think through the implications of this. I am at an event all day
today and tomorrow morning. Hope to get back to you tomorrow afternoon on
the best way to proceed here.

-Ben

On Mon, Jan 24, 2011 at 8:26 AM, <altimofeev@chromium.org> wrote:

> Reviewers: Evan Stade, Ben Goodger,
>
> Description:
> Prevent non-Incognito windows in the Guest session.
>
> BUG=chromium-os:10273
> TEST=manual
>
> Please review this at http://codereview.chromium.org/6272016/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/chrome/browser
>
> Affected files:
>  M chrome/browser/dom_ui/options/options_ui.cc
>  M chrome/browser/profiles/profile.h
>  M chrome/browser/profiles/profile.cc
>  M chrome/browser/ui/browser_navigator.cc
>
>
> Index: chrome/browser/dom_ui/options/options_ui.cc
> diff --git a/chrome/browser/dom_ui/options/options_ui.cc
> b/chrome/browser/dom_ui/options/options_ui.cc
> index
>
fcc3b8062e69dbaa1f57eacbbf13eb0e9e86300d..2507648de770162e24865f5535a7fcc3824af15f
> 100644
> --- a/chrome/browser/dom_ui/options/options_ui.cc
> +++ b/chrome/browser/dom_ui/options/options_ui.cc
> @@ -256,7 +256,7 @@ RefCountedMemory* OptionsUI::GetFaviconResourceBytes()
> {
>  }
>
>  void OptionsUI::InitializeHandlers() {
> -  DCHECK(!GetProfile()->IsOffTheRecord());
> +  DCHECK(!GetProfile()->IsOffTheRecord() || Profile::IsGuestSession());
>
>   std::vector<DOMMessageHandler*>::iterator iter;
>   for (iter = handlers_.begin(); iter != handlers_.end(); ++iter) {
> Index: chrome/browser/profiles/profile.cc
> diff --git a/chrome/browser/profiles/profile.cc
> b/chrome/browser/profiles/profile.cc
> index
>
488ca6e72485b4bdd41b538143e1373ba3ac72f8..f06bf18c239fdb898973f6c3af6cac04946c5ef9
> 100644
> --- a/chrome/browser/profiles/profile.cc
> +++ b/chrome/browser/profiles/profile.cc
> @@ -30,6 +30,7 @@
>  #include "chrome/browser/ui/find_bar/find_bar_state.h"
>  #include "chrome/common/chrome_constants.h"
>  #include "chrome/common/chrome_paths.h"
> +#include "chrome/common/chrome_switches.h"
>  #include "chrome/common/json_pref_store.h"
>  #include "chrome/common/notification_service.h"
>  #include "chrome/common/pref_names.h"
> @@ -122,6 +123,16 @@ URLRequestContextGetter*
> Profile::GetDefaultRequestContext() {
>   return default_request_context_;
>  }
>
> +bool Profile::IsGuestSession() {
> +#if defined(OS_CHROMEOS)
> +  static bool is_guest_session =
> +
>  CommandLine::ForCurrentProcess()->HasSwitch(switches::kGuestSession);
> +  return is_guest_session;
> +#else
> +  return false;
> +#endif
> +}
> +
>  bool Profile::IsSyncAccessible() {
>   ProfileSyncService* syncService = GetProfileSyncService();
>   return syncService && !syncService->IsManaged();
> Index: chrome/browser/profiles/profile.h
> diff --git a/chrome/browser/profiles/profile.h
> b/chrome/browser/profiles/profile.h
> index
>
e13c0b367d98a73ffd0ee89692461cbc98e87440..fa1176c3734ab8ef1c0d238e35e9011451c7b49b
> 100644
> --- a/chrome/browser/profiles/profile.h
> +++ b/chrome/browser/profiles/profile.h
> @@ -507,6 +507,9 @@ class Profile {
>   // profile.
>   virtual PrerenderManager* GetPrerenderManager() = 0;
>
> +  // Returns whether it is a guest session.
> +  static bool IsGuestSession();
> +
>  #ifdef UNIT_TEST
>   // Use with caution.  GetDefaultRequestContext may be called on any
> thread!
>   static void set_default_request_context(URLRequestContextGetter* c) {
> Index: chrome/browser/ui/browser_navigator.cc
> diff --git a/chrome/browser/ui/browser_navigator.cc
> b/chrome/browser/ui/browser_navigator.cc
> index
>
f9dca07f548dc0ff869fc0007310c8409e06d338..bcd40cf505c3ece2701b3780f4700163af16d227
> 100644
> --- a/chrome/browser/ui/browser_navigator.cc
> +++ b/chrome/browser/ui/browser_navigator.cc
> @@ -105,7 +105,8 @@ int GetIndexOfSingletonTab(browser::NavigateParams*
> params) {
>
>  // Change some of the navigation parameters based on the particular URL.
>  // Currently this applies to chrome://settings and the bookmark manager,
> -// which we always want to open in a normal (not incognito) window.
> +// which we always want to open in a normal (not incognito) window. Guest
> +// session is an exception.
>  void AdjustNavigateParamsForURL(browser::NavigateParams* params) {
>   if (!params->target_contents &&
>       params->url.scheme() == chrome::kChromeUIScheme &&
> @@ -114,7 +115,7 @@ void
> AdjustNavigateParamsForURL(browser::NavigateParams* params) {
>     Profile* profile =
>         params->browser ? params->browser->profile() : params->profile;
>
> -    if (profile->IsOffTheRecord()) {
> +    if (profile->IsOffTheRecord() && !Profile::IsGuestSession()) {
>       profile = profile->GetOriginalProfile();
>
>       params->disposition = SINGLETON_TAB;
>
>
>

[altimofeev, 2011-01-28 08:51:37]

Ping?

On 2011/01/25 20:04:51, Ben Goodger wrote:
> I need to think through the implications of this. I am at an event all day
> today and tomorrow morning. Hope to get back to you tomorrow afternoon on
> the best way to proceed here.
> 
> -Ben
> 
> On Mon, Jan 24, 2011 at 8:26 AM, <mailto:altimofeev@chromium.org> wrote:
> 
> > Reviewers: Evan Stade, Ben Goodger,
> >
> > Description:
> > Prevent non-Incognito windows in the Guest session.
> >
> > BUG=chromium-os:10273
> > TEST=manual
> >
> > Please review this at http://codereview.chromium.org/6272016/
> >
> > SVN Base: svn://svn.chromium.org/chrome/trunk/src/chrome/browser
> >
> > Affected files:
> >  M chrome/browser/dom_ui/options/options_ui.cc
> >  M chrome/browser/profiles/profile.h
> >  M chrome/browser/profiles/profile.cc
> >  M chrome/browser/ui/browser_navigator.cc
> >
> >
> > Index: chrome/browser/dom_ui/options/options_ui.cc
> > diff --git a/chrome/browser/dom_ui/options/options_ui.cc
> > b/chrome/browser/dom_ui/options/options_ui.cc
> > index
> >
>
fcc3b8062e69dbaa1f57eacbbf13eb0e9e86300d..2507648de770162e24865f5535a7fcc3824af15f
> > 100644
> > --- a/chrome/browser/dom_ui/options/options_ui.cc
> > +++ b/chrome/browser/dom_ui/options/options_ui.cc
> > @@ -256,7 +256,7 @@ RefCountedMemory* OptionsUI::GetFaviconResourceBytes()
> > {
> >  }
> >
> >  void OptionsUI::InitializeHandlers() {
> > -  DCHECK(!GetProfile()->IsOffTheRecord());
> > +  DCHECK(!GetProfile()->IsOffTheRecord() || Profile::IsGuestSession());
> >
> >   std::vector<DOMMessageHandler*>::iterator iter;
> >   for (iter = handlers_.begin(); iter != handlers_.end(); ++iter) {
> > Index: chrome/browser/profiles/profile.cc
> > diff --git a/chrome/browser/profiles/profile.cc
> > b/chrome/browser/profiles/profile.cc
> > index
> >
>
488ca6e72485b4bdd41b538143e1373ba3ac72f8..f06bf18c239fdb898973f6c3af6cac04946c5ef9
> > 100644
> > --- a/chrome/browser/profiles/profile.cc
> > +++ b/chrome/browser/profiles/profile.cc
> > @@ -30,6 +30,7 @@
> >  #include "chrome/browser/ui/find_bar/find_bar_state.h"
> >  #include "chrome/common/chrome_constants.h"
> >  #include "chrome/common/chrome_paths.h"
> > +#include "chrome/common/chrome_switches.h"
> >  #include "chrome/common/json_pref_store.h"
> >  #include "chrome/common/notification_service.h"
> >  #include "chrome/common/pref_names.h"
> > @@ -122,6 +123,16 @@ URLRequestContextGetter*
> > Profile::GetDefaultRequestContext() {
> >   return default_request_context_;
> >  }
> >
> > +bool Profile::IsGuestSession() {
> > +#if defined(OS_CHROMEOS)
> > +  static bool is_guest_session =
> > +
> >  CommandLine::ForCurrentProcess()->HasSwitch(switches::kGuestSession);
> > +  return is_guest_session;
> > +#else
> > +  return false;
> > +#endif
> > +}
> > +
> >  bool Profile::IsSyncAccessible() {
> >   ProfileSyncService* syncService = GetProfileSyncService();
> >   return syncService && !syncService->IsManaged();
> > Index: chrome/browser/profiles/profile.h
> > diff --git a/chrome/browser/profiles/profile.h
> > b/chrome/browser/profiles/profile.h
> > index
> >
>
e13c0b367d98a73ffd0ee89692461cbc98e87440..fa1176c3734ab8ef1c0d238e35e9011451c7b49b
> > 100644
> > --- a/chrome/browser/profiles/profile.h
> > +++ b/chrome/browser/profiles/profile.h
> > @@ -507,6 +507,9 @@ class Profile {
> >   // profile.
> >   virtual PrerenderManager* GetPrerenderManager() = 0;
> >
> > +  // Returns whether it is a guest session.
> > +  static bool IsGuestSession();
> > +
> >  #ifdef UNIT_TEST
> >   // Use with caution.  GetDefaultRequestContext may be called on any
> > thread!
> >   static void set_default_request_context(URLRequestContextGetter* c) {
> > Index: chrome/browser/ui/browser_navigator.cc
> > diff --git a/chrome/browser/ui/browser_navigator.cc
> > b/chrome/browser/ui/browser_navigator.cc
> > index
> >
>
f9dca07f548dc0ff869fc0007310c8409e06d338..bcd40cf505c3ece2701b3780f4700163af16d227
> > 100644
> > --- a/chrome/browser/ui/browser_navigator.cc
> > +++ b/chrome/browser/ui/browser_navigator.cc
> > @@ -105,7 +105,8 @@ int GetIndexOfSingletonTab(browser::NavigateParams*
> > params) {
> >
> >  // Change some of the navigation parameters based on the particular URL.
> >  // Currently this applies to chrome://settings and the bookmark manager,
> > -// which we always want to open in a normal (not incognito) window.
> > +// which we always want to open in a normal (not incognito) window. Guest
> > +// session is an exception.
> >  void AdjustNavigateParamsForURL(browser::NavigateParams* params) {
> >   if (!params->target_contents &&
> >       params->url.scheme() == chrome::kChromeUIScheme &&
> > @@ -114,7 +115,7 @@ void
> > AdjustNavigateParamsForURL(browser::NavigateParams* params) {
> >     Profile* profile =
> >         params->browser ? params->browser->profile() : params->profile;
> >
> > -    if (profile->IsOffTheRecord()) {
> > +    if (profile->IsOffTheRecord() && !Profile::IsGuestSession()) {
> >       profile = profile->GetOriginalProfile();
> >
> >       params->disposition = SINGLETON_TAB;
> >
> >
> >

[Ben Goodger (Google), 2011-01-28 19:12:08]

http://codereview.chromium.org/6272016/diff/6001/chrome/browser/ui/browser_na...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/6272016/diff/6001/chrome/browser/ui/browser_na...
chrome/browser/ui/browser_navigator.cc:118: if (profile->IsOffTheRecord() &&
!Profile::IsGuestSession()) {
Can you add a unit test?

[altimofeev, 2011-01-31 16:26:39]

http://codereview.chromium.org/6272016/diff/6001/chrome/browser/ui/browser_na...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/6272016/diff/6001/chrome/browser/ui/browser_na...
chrome/browser/ui/browser_navigator.cc:118: if (profile->IsOffTheRecord() &&
!Profile::IsGuestSession()) {
On 2011/01/28 19:12:08, Ben Goodger wrote:
> Can you add a unit test?

Added browsertest. Can you take another look?

[Evan Stade, 2011-01-31 21:06:21]

thanks for the test

http://codereview.chromium.org/6272016/diff/16001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.h (right):

http://codereview.chromium.org/6272016/diff/16001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.h:11: #include "base/command_line.h"
forward declare CommandLine

http://codereview.chromium.org/6272016/diff/16001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.h:19: class
BrowserGuestSessionNavigatorTest;
no indent necessary inside namespaces.

http://codereview.chromium.org/6272016/diff/16001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/mock_authenticator.h (right):

http://codereview.chromium.org/6272016/diff/16001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/mock_authenticator.h:164: CommandLine*
command_line) { return std::string(); }
function body on new line

http://codereview.chromium.org/6272016/diff/16001/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator_browsertest.cc (right):

http://codereview.chromium.org/6272016/diff/16001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.cc:713: // This test verifies
that the bookmarks page isn't open in the incognito
s/open/opened (also in the other comments)

[altimofeev, 2011-02-01 12:42:14]

http://codereview.chromium.org/6272016/diff/16001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.h (right):

http://codereview.chromium.org/6272016/diff/16001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.h:11: #include "base/command_line.h"
On 2011/01/31 21:06:22, Evan Stade wrote:
> forward declare CommandLine

Done.

http://codereview.chromium.org/6272016/diff/16001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.h:19: class
BrowserGuestSessionNavigatorTest;
On 2011/01/31 21:06:22, Evan Stade wrote:
> no indent necessary inside namespaces.

Done.

http://codereview.chromium.org/6272016/diff/16001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/mock_authenticator.h (right):

http://codereview.chromium.org/6272016/diff/16001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/mock_authenticator.h:164: CommandLine*
command_line) { return std::string(); }
On 2011/01/31 21:06:22, Evan Stade wrote:
> function body on new line

Done.

http://codereview.chromium.org/6272016/diff/16001/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator_browsertest.cc (right):

http://codereview.chromium.org/6272016/diff/16001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.cc:713: // This test verifies
that the bookmarks page isn't open in the incognito
On 2011/01/31 21:06:22, Evan Stade wrote:
> s/open/opened (also in the other comments)

Done.

[Ben Goodger (Google), 2011-02-01 17:59:09]

http://codereview.chromium.org/6272016/diff/20001/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator_browsertest.cc (right):

http://codereview.chromium.org/6272016/diff/20001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.cc:740: #if defined(OS_CHROMEOS)
I would rather this live in browser_navigator_browsertest_chromeos.cc

[Ben Goodger (Google), 2011-02-01 17:59:54]

Or is it browser_navigator_chromeos_browsertest.cc? I am not sure of the
preferred style.

[Evan Stade, 2011-02-01 21:12:35]

http://codereview.chromium.org/6272016/diff/20001/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator_browsertest.cc (right):

http://codereview.chromium.org/6272016/diff/20001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.cc:740: #if defined(OS_CHROMEOS)
On 2011/02/01 17:59:09, Ben Goodger wrote:
> I would rather this live in browser_navigator_browsertest_chromeos.cc

I actually like this here, because it is logically related to the two tests
directly above. Also, it inherits from BrowserNavigatorTest, which is private to
this file.

[altimofeev, 2011-02-02 13:06:34]

On 2011/02/01 21:12:35, Evan Stade wrote:
>
http://codereview.chromium.org/6272016/diff/20001/chrome/browser/ui/browser_n...
> File chrome/browser/ui/browser_navigator_browsertest.cc (right):
> 
>
http://codereview.chromium.org/6272016/diff/20001/chrome/browser/ui/browser_n...
> chrome/browser/ui/browser_navigator_browsertest.cc:740: #if
defined(OS_CHROMEOS)
> On 2011/02/01 17:59:09, Ben Goodger wrote:
> > I would rather this live in browser_navigator_browsertest_chromeos.cc
> 
> I actually like this here, because it is logically related to the two tests
> directly above. Also, it inherits from BrowserNavigatorTest, which is private
to
> this file.

I agree with Evan. Furthermore, currently there is nor
'.*chromeos_browsertest.*" neither ".*browsertest_chromeos.*" files. At the same
time #define(OS_CHROMEOS) is widely used in the browsertests.

[altimofeev, 2011-02-04 15:20:33]

Ping?

On 2011/02/02 13:06:34, altimofeev wrote:
> On 2011/02/01 21:12:35, Evan Stade wrote:
> >
>
http://codereview.chromium.org/6272016/diff/20001/chrome/browser/ui/browser_n...
> > File chrome/browser/ui/browser_navigator_browsertest.cc (right):
> > 
> >
>
http://codereview.chromium.org/6272016/diff/20001/chrome/browser/ui/browser_n...
> > chrome/browser/ui/browser_navigator_browsertest.cc:740: #if
> defined(OS_CHROMEOS)
> > On 2011/02/01 17:59:09, Ben Goodger wrote:
> > > I would rather this live in browser_navigator_browsertest_chromeos.cc
> > 
> > I actually like this here, because it is logically related to the two tests
> > directly above. Also, it inherits from BrowserNavigatorTest, which is
private
> to
> > this file.
> 
> I agree with Evan. Furthermore, currently there is nor
> '.*chromeos_browsertest.*" neither ".*browsertest_chromeos.*" files. At the
same
> time #define(OS_CHROMEOS) is widely used in the browsertests.

[Ben Goodger (Google), 2011-02-04 15:29:03]

I am still not sold on this test living with this file. It is the dependency on
ChromeOS' login utils that bothers me. Navigator is in my mind at a lower level
of the abstraction onion. I would rather this live in a ChromeOS specific file
to start with. If there are shared base classes those can be moved into a
separate header/cc file and used by both.

[altimofeev, 2011-02-07 14:53:13]

Done. Please take a look.

On 2011/02/04 15:29:03, Ben Goodger wrote:
> I am still not sold on this test living with this file. It is the dependency
on
> ChromeOS' login utils that bothers me. Navigator is in my mind at a lower
level
> of the abstraction onion. I would rather this live in a ChromeOS specific file
> to start with. If there are shared base classes those can be moved into a
> separate header/cc file and used by both.

[Ben Goodger (Google), 2011-02-07 15:22:56]

LGTM, thanks.

[altimofeev, 2011-02-07 17:01:27]

@Evan: ping

[Evan Stade, 2011-02-07 19:02:30]

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator_browsertest.h (right):

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
year

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.h:9: #ifndef
CHROME_BROWSER_UI_BROWSER_NAVIGATOR_BROWSERTEST_H_
put header guards outside of includes

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.h:19: class NavigateParams;
nit: no indent in namespaces

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.h:22: class BrowserNavigatorTest
: public InProcessBrowserTest,
class comment (who uses this?)

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator_browsertest_chromeos.cc (right):

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest_chromeos.cc:1: // Copyright (c)
2010 The Chromium Authors. All rights reserved.
year again (every file in this CL should be updated to 2011)

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest_chromeos.cc:4: 
extra line

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest_chromeos.cc:17: #if
defined(OS_CHROMEOS)
not necessary. This file will only be compiled on chromeos due to the name of
the file (gyp is smart like that)

[altimofeev, 2011-02-08 14:20:59]

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator_browsertest.h (right):

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2011/02/07 19:02:30, Evan Stade wrote:
> year

Done.

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.h:9: #ifndef
CHROME_BROWSER_UI_BROWSER_NAVIGATOR_BROWSERTEST_H_
On 2011/02/07 19:02:30, Evan Stade wrote:
> put header guards outside of includes

Done.

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.h:19: class NavigateParams;
On 2011/02/07 19:02:30, Evan Stade wrote:
> nit: no indent in namespaces

Done.

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.h:22: class BrowserNavigatorTest
: public InProcessBrowserTest,
On 2011/02/07 19:02:30, Evan Stade wrote:
> class comment (who uses this?)

Done.

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator_browsertest_chromeos.cc (right):

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest_chromeos.cc:1: // Copyright (c)
2010 The Chromium Authors. All rights reserved.
On 2011/02/07 19:02:30, Evan Stade wrote:
> year again (every file in this CL should be updated to 2011)

Done.

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest_chromeos.cc:4: 
On 2011/02/07 19:02:30, Evan Stade wrote:
> extra line

Done.

http://codereview.chromium.org/6272016/diff/18002/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest_chromeos.cc:17: #if
defined(OS_CHROMEOS)
On 2011/02/07 19:02:30, Evan Stade wrote:
> not necessary. This file will only be compiled on chromeos due to the name of
> the file (gyp is smart like that)

Done.

[Evan Stade, 2011-02-08 23:11:43]

lgtm

http://codereview.chromium.org/6272016/diff/28001/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator_browsertest.h (right):

http://codereview.chromium.org/6272016/diff/28001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.h:18: namespace browser { class
NavigateParams; }
what I meant when I said no indent was:

namespace browser {
class NavigateParams;
}

[altimofeev, 2011-02-09 08:12:40]

http://codereview.chromium.org/6272016/diff/28001/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator_browsertest.h (right):

http://codereview.chromium.org/6272016/diff/28001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.h:18: namespace browser { class
NavigateParams; }
On 2011/02/08 23:11:43, Evan Stade wrote:
> what I meant when I said no indent was:
> 
> namespace browser {
> class NavigateParams;
> }

Done.