Changed new tab content for Guest Session.

Changed new tab content for Guest Session.

Use special text for Guest Session instead of using standard text for
Incognito mode. Also changes icon and link which are shown.

BUG=chromium-os:6907
TEST=Enter Guest mode, open new tab. Check opened page.

Commited: http://src.chromium.org/viewvc/chrome?view=rev&revision=66423

[altimofeev, 2010-10-29 10:37:02]

Another try. 

Added TODO to fix 'if def' related issue. This CL is not the first one that uses
'if def' to customize incognito session, so it look logical to fix the approach
in separate CL.

[Nikita (slow), 2010-10-29 14:44:45]

http://codereview.chromium.org/4186009/diff/1/4
File chrome/browser/dom_ui/ntp_resource_cache.cc (right):

http://codereview.chromium.org/4186009/diff/1/4#newcode207
chrome/browser/dom_ui/ntp_resource_cache.cc:207: //TODO(altimofeev): consider
implementation without 'if def' usage.
nit: spacing // TODO
Please file an issue for that.

http://codereview.chromium.org/4186009/diff/1/5
File chrome/browser/resources/guest_session_tab.html (right):

http://codereview.chromium.org/4186009/diff/1/5#newcode52
chrome/browser/resources/guest_session_tab.html:52: <span
i18n-values=".innerHTML:content"></span>
innerHTML is needed because content is also name for top level div?
Link placeholder should be renamed then.

http://codereview.chromium.org/4186009/diff/1/5#newcode62
chrome/browser/resources/guest_session_tab.html:62:
document.documentElement.setAttribute("bookmarkbarattached", "true");
Is this needed? Bookmarks are disabled in Guest Mode and they are not used on
this page.

http://codereview.chromium.org/4186009/diff/1/5#newcode65
chrome/browser/resources/guest_session_tab.html:65: function
bookmarkBarDetached() {
Same here.

[Ben Goodger (Google), 2010-10-29 16:53:12]

http://codereview.chromium.org/4186009/diff/13001/14006
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/4186009/diff/13001/14006#newcode608
chrome/browser/views/frame/browser_view.cc:608:
!CommandLine::ForCurrentProcess()->HasSwitch(switches::kGuestSession);
I don't get this logic... you're saying that the incognito man should be shown
if the browser was NOT run with --guest-session.

Yet you detailed earlier that the "Guest" session had similar semantics as
incognito... i.e. suggest does not work, Ctrl+Shift+T to restore tabs does not
work, etc.

If the behavior is functionally identical to Incognito it should render as
Incognito.

Is there a design doc that explains how this feature works somewhere?

[altimofeev, 2010-10-30 14:57:38]

http://codereview.chromium.org/4186009/diff/1/4
File chrome/browser/dom_ui/ntp_resource_cache.cc (right):

http://codereview.chromium.org/4186009/diff/1/4#newcode207
chrome/browser/dom_ui/ntp_resource_cache.cc:207: //TODO(altimofeev): consider
implementation without 'if def' usage.
On 2010/10/29 14:44:46, Nikita Kostylev wrote:
> nit: spacing // TODO
> Please file an issue for that.

Done.

http://codereview.chromium.org/4186009/diff/1/5
File chrome/browser/resources/guest_session_tab.html (right):

http://codereview.chromium.org/4186009/diff/1/5#newcode52
chrome/browser/resources/guest_session_tab.html:52: <span
i18n-values=".innerHTML:content"></span>
On 2010/10/29 14:44:46, Nikita Kostylev wrote:
> innerHTML is needed because content is also name for top level div?
> Link placeholder should be renamed then.

This line says that 'innerHTML' member of the <span> should be assigned value
that corresponds to 'content' in our templates dictionary.

http://codereview.chromium.org/4186009/diff/1/5#newcode62
chrome/browser/resources/guest_session_tab.html:62:
document.documentElement.setAttribute("bookmarkbarattached", "true");
On 2010/10/29 14:44:46, Nikita Kostylev wrote:
> Is this needed? Bookmarks are disabled in Guest Mode and they are not used on
> this page.

Deleted.

http://codereview.chromium.org/4186009/diff/1/5#newcode65
chrome/browser/resources/guest_session_tab.html:65: function
bookmarkBarDetached() {
On 2010/10/29 14:44:46, Nikita Kostylev wrote:
> Same here.

Deleted.

http://codereview.chromium.org/4186009/diff/13001/14006
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/4186009/diff/13001/14006#newcode608
chrome/browser/views/frame/browser_view.cc:608:
!CommandLine::ForCurrentProcess()->HasSwitch(switches::kGuestSession);
On 2010/10/29 16:53:12, Ben Goodger wrote:
> I don't get this logic... you're saying that the incognito man should be shown
> if the browser was NOT run with --guest-session.

Probably you have missed '!' sign in the code. The logic is exactly as you have
said.

> Yet you detailed earlier that the "Guest" session had similar semantics as
> incognito... i.e. suggest does not work, Ctrl+Shift+T to restore tabs does not
> work, etc.
> 
> If the behavior is functionally identical to Incognito it should render as
> Incognito.

Guest session is something more than Incognito mode. It is session in terms of
OS, not only the browser mode. For example, it uses tmpfs, it doesn't have
bookmarks, etc. Anyway, since it wasn't only my decision to name it guest and
render with different icon, better place to discussions will be
http://code.google.com/p/chromium-os/issues/detail?id=6907.

> Is there a design doc that explains how this feature works somewhere?

http://www.chromium.org/chromium-os/chromiumos-design-docs/user-accounts-and-...

but it is slightly outdated

[Ben Goodger (Google), 2010-11-01 14:40:49]

http://codereview.chromium.org/4186009/diff/13001/14006
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/4186009/diff/13001/14006#newcode608
chrome/browser/views/frame/browser_view.cc:608:
!CommandLine::ForCurrentProcess()->HasSwitch(switches::kGuestSession);
OK so it sounds like when you are in "Guest" mode you use the incognito profile
but do not show any incognito UI. Correct?

My question is is there any other UI code in BrowserView or related classes that
adjusts the presentation of the UI based on incognito-ness that needs to benefit
from this check. You have just adjusted the function that shows/hides the
incognito man. There are other affordances to Incognito though that you may want
to disable. You should verify that these are also disabled by looking for places
in src/chrome/browser/views/frame for calls to IsOffTheRecord().

Rather than replicating this check at each of those call sites, I think it'd be
a better idea to create a new function:

bool BrowserView::ShouldShowIncognitoUI() const;

... that implements this check and reimplement ShouldShowIncognitoAvatar in
terms of this function (since it only cares about IsBrowserTypeNormal():

bool BrowserView::ShouldShowIncognitoAvatar() const {
  return IsBrowserTypeNormal() && ShouldShowIncognitoUI();
}

Then any other frame/UI code can call BrowserView::ShouldShowIncognitoUI instead
of checking IsOffTheRecord directly.

[arv (Not doing code reviews), 2010-11-01 20:21:58]

http://codereview.chromium.org/4186009/diff/13001/14004
File chrome/browser/resources/guest_session_tab.html (right):

http://codereview.chromium.org/4186009/diff/13001/14004#newcode7
chrome/browser/resources/guest_session_tab.html:7: margin:10px 8px 10px 8px;
Use shorthand

margin: 10px 8px;

http://codereview.chromium.org/4186009/diff/13001/14004#newcode7
chrome/browser/resources/guest_session_tab.html:7: margin:10px 8px 10px 8px;
Whitespace after :

[altimofeev, 2010-11-02 13:36:56]

http://codereview.chromium.org/4186009/diff/13001/14004
File chrome/browser/resources/guest_session_tab.html (right):

http://codereview.chromium.org/4186009/diff/13001/14004#newcode7
chrome/browser/resources/guest_session_tab.html:7: margin:10px 8px 10px 8px;
On 2010/11/01 20:21:58, arv wrote:
> Whitespace after :

Done.

http://codereview.chromium.org/4186009/diff/13001/14006
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/4186009/diff/13001/14006#newcode608
chrome/browser/views/frame/browser_view.cc:608:
!CommandLine::ForCurrentProcess()->HasSwitch(switches::kGuestSession);
Please, find my comments inside.

On 2010/11/01 14:40:49, Ben Goodger wrote:
> OK so it sounds like when you are in "Guest" mode you use the incognito
profile
> but do not show any incognito UI. Correct?

Not exactly. Current version intentionally uses Incognito UI but without avatar.

> My question is is there any other UI code in BrowserView or related classes
that
> adjusts the presentation of the UI based on incognito-ness that needs to
benefit
> from this check. You have just adjusted the function that shows/hides the
> incognito man. There are other affordances to Incognito though that you may
want
> to disable. You should verify that these are also disabled by looking for
places
> in src/chrome/browser/views/frame for calls to IsOffTheRecord().
> Rather than replicating this check at each of those call sites, I think it'd
be
> a better idea to create a new function:
> 
> bool BrowserView::ShouldShowIncognitoUI() const;
> 
> ... that implements this check and reimplement ShouldShowIncognitoAvatar in
> terms of this function (since it only cares about IsBrowserTypeNormal():
> 
> bool BrowserView::ShouldShowIncognitoAvatar() const {
>   return IsBrowserTypeNormal() && ShouldShowIncognitoUI();
> }
> 
> Then any other frame/UI code can call BrowserView::ShouldShowIncognitoUI
instead
> of checking IsOffTheRecord directly.

I like your idea, but as I have mentioned, currently the only difference is the
avatar absence (also Guest Session uses different content of the new tab). This
means that implementing another method like 'ShouldShowIncognitoUI' looks
premature. Furthermore 'ShouldShowOffTheRecordAvatar' fits ideally to the
current scheme.

IMO, the thing that should be done is getting rid of the switch checking and
using Profile instead (this is meant in my TODO) Profile could store whether
Guest Session is active.

[Ben Goodger (Google), 2010-11-02 14:54:35]

Let me discuss with other UI people when they come in to work over the next
few hours. I am not sure of the advantages of complicating the code and
adding a third type of UI display.

-Ben

On Tue, Nov 2, 2010 at 6:36 AM, <altimofeev@chromium.org> wrote:

>
> http://codereview.chromium.org/4186009/diff/13001/14004
> File chrome/browser/resources/guest_session_tab.html (right):
>
> http://codereview.chromium.org/4186009/diff/13001/14004#newcode7
> chrome/browser/resources/guest_session_tab.html:7: margin:10px 8px 10px
> 8px;
> On 2010/11/01 20:21:58, arv wrote:
>
>> Whitespace after :
>>
>
> Done.
>
>
> http://codereview.chromium.org/4186009/diff/13001/14006
> File chrome/browser/views/frame/browser_view.cc (right):
>
> http://codereview.chromium.org/4186009/diff/13001/14006#newcode608
> chrome/browser/views/frame/browser_view.cc:608:
> !CommandLine::ForCurrentProcess()->HasSwitch(switches::kGuestSession);
> Please, find my comments inside.
>
>
> On 2010/11/01 14:40:49, Ben Goodger wrote:
>
>> OK so it sounds like when you are in "Guest" mode you use the
>>
> incognito profile
>
>> but do not show any incognito UI. Correct?
>>
>
> Not exactly. Current version intentionally uses Incognito UI but without
> avatar.
>
>
>  My question is is there any other UI code in BrowserView or related
>>
> classes that
>
>> adjusts the presentation of the UI based on incognito-ness that needs
>>
> to benefit
>
>> from this check. You have just adjusted the function that shows/hides
>>
> the
>
>> incognito man. There are other affordances to Incognito though that
>>
> you may want
>
>> to disable. You should verify that these are also disabled by looking
>>
> for places
>
>> in src/chrome/browser/views/frame for calls to IsOffTheRecord().
>> Rather than replicating this check at each of those call sites, I
>>
> think it'd be
>
>> a better idea to create a new function:
>>
>
>  bool BrowserView::ShouldShowIncognitoUI() const;
>>
>
>  ... that implements this check and reimplement
>>
> ShouldShowIncognitoAvatar in
>
>> terms of this function (since it only cares about
>>
> IsBrowserTypeNormal():
>
>  bool BrowserView::ShouldShowIncognitoAvatar() const {
>>   return IsBrowserTypeNormal() && ShouldShowIncognitoUI();
>> }
>>
>
>  Then any other frame/UI code can call
>>
> BrowserView::ShouldShowIncognitoUI instead
>
>> of checking IsOffTheRecord directly.
>>
>
> I like your idea, but as I have mentioned, currently the only difference
> is the avatar absence (also Guest Session uses different content of the
> new tab). This means that implementing another method like
> 'ShouldShowIncognitoUI' looks premature. Furthermore
> 'ShouldShowOffTheRecordAvatar' fits ideally to the current scheme.
>
> IMO, the thing that should be done is getting rid of the switch checking
> and using Profile instead (this is meant in my TODO) Profile could store
> whether Guest Session is active.
>
>
> http://codereview.chromium.org/4186009/show
>

[altimofeev, 2010-11-03 07:12:02]

On 2010/11/02 14:54:35, Ben Goodger wrote:
> Let me discuss with other UI people when they come in to work over the next
> few hours. I am not sure of the advantages of complicating the code and
> adding a third type of UI display.

Since we decided to make refactoring later, can you take another look on this
CL?

[Nikita (slow), 2010-11-03 08:17:10]

LGTM for my comments

http://codereview.chromium.org/4186009/diff/1/4
File chrome/browser/dom_ui/ntp_resource_cache.cc (right):

http://codereview.chromium.org/4186009/diff/1/4#newcode207
chrome/browser/dom_ui/ntp_resource_cache.cc:207: //TODO(altimofeev): consider
implementation without 'if def' usage.
On 2010/10/30 14:57:38, altimofeev wrote:
> On 2010/10/29 14:44:46, Nikita Kostylev wrote:
> > nit: spacing // TODO
> > Please file an issue for that.
> 
> Done.

What's the issue #?

[altimofeev, 2010-11-03 08:51:20]

> What's the issue #?
http://code.google.com/p/chromium-os/issues/detail?id=8626

[arv (Not doing code reviews), 2010-11-03 17:38:35]

http://codereview.chromium.org/4186009/diff/22001/23004
File chrome/browser/resources/guest_session_tab.html (right):

http://codereview.chromium.org/4186009/diff/22001/23004#newcode40
chrome/browser/resources/guest_session_tab.html:40: }
I should have seen this before...

Can you move this CSS to a css file since it is the same as in
incognito_tab.css?

[altimofeev, 2010-11-08 22:37:17]

http://codereview.chromium.org/4186009/diff/22001/23004
File chrome/browser/resources/guest_session_tab.html (right):

http://codereview.chromium.org/4186009/diff/22001/23004#newcode40
chrome/browser/resources/guest_session_tab.html:40: }
On 2010/11/03 17:38:35, arv wrote:
> I should have seen this before...
> 
> Can you move this CSS to a css file since it is the same as in
> incognito_tab.css?

Done.

[arv (Not doing code reviews), 2010-11-08 22:41:13]

HTML/CSS LGTM

[Nikita (slow), 2010-11-09 10:46:44]

trybots?

[altimofeev, 2010-11-09 13:36:38]

On 2010/11/09 10:46:44, Nikita Kostylev wrote:
> trybots?

There are new files in this CL, so trybots will a priory fail.

[Nikita (slow), 2010-11-09 13:38:16]

Since HTML changes are ok, you might submit them as a separate CL.

[Ben Goodger (Google), 2010-11-15 17:23:26]

OK

[altimofeev, 2010-11-16 17:38:08]

On 2010/11/15 17:23:26, Ben Goodger wrote:
> OK

Is it equivalent to LGTM?

[Ben Goodger (Google), 2010-11-16 18:03:54]

Indeed.

-Ben

On Tue, Nov 16, 2010 at 9:38 AM, <altimofeev@chromium.org> wrote:

> On 2010/11/15 17:23:26, Ben Goodger wrote:
>
>> OK
>>
>
> Is it equivalent to LGTM?
>
>
> http://codereview.chromium.org/4186009/
>