[MD] Remove new incognito styling from guest mode.

[MD] Remove new incognito styling from guest mode.

Profile::IsOffTheRecord() surprisingly does not imply incognito; it also covers guest profiles.

BUG=none

Committed: https://crrev.com/1257c40097711f465538faede3fd74bfcc83ff9e
Cr-Commit-Position: refs/heads/master@{#371906}

[Evan Stade, 2016-01-26 23:01:03]

estade@chromium.org changed reviewers:
+ pkasting@chromium.org, pkotwicz@chromium.org

[Peter Kasting, 2016-01-26 23:05:29]

LGTM

https://codereview.chromium.org/1638063003/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1638063003/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:749: bool incognito =
profile->GetProfileType() == Profile::INCOGNITO_PROFILE;
Nit: You might want to explicitly note why calling IsOffTheRecord() is
incorrect.  Although there are three files where this would apply.  This makes
me wonder if we should have some kind of helper somewhere like
ShouldUseIncognitoTheme(Profile*) that will do this, to both document what's
going on and make it harder to screw up.

[Evan Stade, 2016-01-27 00:00:59]

On 2016/01/26 23:05:29, Peter Kasting wrote:
> LGTM
> 
>
https://codereview.chromium.org/1638063003/diff/1/chrome/browser/themes/theme...
> File chrome/browser/themes/theme_service.cc (right):
> 
>
https://codereview.chromium.org/1638063003/diff/1/chrome/browser/themes/theme...
> chrome/browser/themes/theme_service.cc:749: bool incognito =
> profile->GetProfileType() == Profile::INCOGNITO_PROFILE;
> Nit: You might want to explicitly note why calling IsOffTheRecord() is
> incorrect.  Although there are three files where this would apply.  This makes
> me wonder if we should have some kind of helper somewhere like
> ShouldUseIncognitoTheme(Profile*) that will do this, to both document what's
> going on and make it harder to screw up.

I sorta think this goes beyond the theming case --- it's too easy to screw up
for whatever feature you're working on. We should not have IsOffTheRecord() at
all, or at least it should be renamed.

[Evan Stade, 2016-01-27 00:19:10]

Description was changed from

==========
[MD] Remove new incognito styling from guest mode.

BUG=none
==========

to

==========
[MD] Remove new incognito styling from guest mode.

Profile::IsOffTheRecord() surprisingly does imply incognito; it also covers
guest profiles.

BUG=none
==========

[Evan Stade, 2016-01-27 00:19:50]

On 2016/01/27 00:00:59, Evan Stade wrote:
> On 2016/01/26 23:05:29, Peter Kasting wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/1638063003/diff/1/chrome/browser/themes/theme...
> > File chrome/browser/themes/theme_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/1638063003/diff/1/chrome/browser/themes/theme...
> > chrome/browser/themes/theme_service.cc:749: bool incognito =
> > profile->GetProfileType() == Profile::INCOGNITO_PROFILE;
> > Nit: You might want to explicitly note why calling IsOffTheRecord() is
> > incorrect.  Although there are three files where this would apply.  This
makes
> > me wonder if we should have some kind of helper somewhere like
> > ShouldUseIncognitoTheme(Profile*) that will do this, to both document what's
> > going on and make it harder to screw up.
> 
> I sorta think this goes beyond the theming case --- it's too easy to screw up
> for whatever feature you're working on. We should not have IsOffTheRecord() at
> all, or at least it should be renamed.

note, I've added a line to the CL description to explain this since it's not
clear how to make the code more obvious.

[Peter Kasting, 2016-01-27 00:41:16]

On 2016/01/27 00:00:59, Evan Stade wrote:
> On 2016/01/26 23:05:29, Peter Kasting wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/1638063003/diff/1/chrome/browser/themes/theme...
> > File chrome/browser/themes/theme_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/1638063003/diff/1/chrome/browser/themes/theme...
> > chrome/browser/themes/theme_service.cc:749: bool incognito =
> > profile->GetProfileType() == Profile::INCOGNITO_PROFILE;
> > Nit: You might want to explicitly note why calling IsOffTheRecord() is
> > incorrect.  Although there are three files where this would apply.  This
makes
> > me wonder if we should have some kind of helper somewhere like
> > ShouldUseIncognitoTheme(Profile*) that will do this, to both document what's
> > going on and make it harder to screw up.
> 
> I sorta think this goes beyond the theming case --- it's too easy to screw up
> for whatever feature you're working on. We should not have IsOffTheRecord() at
> all, or at least it should be renamed.

I dunno, it seems well-named and appropriate for telling whether the profile is
supposed to be keeping records or, uh, off the record.  Most of the places in
Chrome that want to know about this want to know how they should treat data, so
it's the appropriate function to use there.  The UI distinction between
incognito and guest mode is a bit unusual in that regard.

[Evan Stade, 2016-01-27 00:58:49]

On 2016/01/27 00:41:16, Peter Kasting wrote:
> On 2016/01/27 00:00:59, Evan Stade wrote:
> > On 2016/01/26 23:05:29, Peter Kasting wrote:
> > > LGTM
> > > 
> > >
> >
>
https://codereview.chromium.org/1638063003/diff/1/chrome/browser/themes/theme...
> > > File chrome/browser/themes/theme_service.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1638063003/diff/1/chrome/browser/themes/theme...
> > > chrome/browser/themes/theme_service.cc:749: bool incognito =
> > > profile->GetProfileType() == Profile::INCOGNITO_PROFILE;
> > > Nit: You might want to explicitly note why calling IsOffTheRecord() is
> > > incorrect.  Although there are three files where this would apply.  This
> makes
> > > me wonder if we should have some kind of helper somewhere like
> > > ShouldUseIncognitoTheme(Profile*) that will do this, to both document
what's
> > > going on and make it harder to screw up.
> > 
> > I sorta think this goes beyond the theming case --- it's too easy to screw
up
> > for whatever feature you're working on. We should not have IsOffTheRecord()
at
> > all, or at least it should be renamed.
> 
> I dunno, it seems well-named and appropriate for telling whether the profile
is
> supposed to be keeping records or, uh, off the record.  Most of the places in
> Chrome that want to know about this want to know how they should treat data,
so
> it's the appropriate function to use there.  The UI distinction between
> incognito and guest mode is a bit unusual in that regard.

I tried to verify if this was true. It seems a lot of the time, the code doesn't
care about the distinction simply because the code would never run in guest mode
(e.g. everything in extensions/). If so often it doesn't matter, it makes the
name all the more nefarious, because you'll see a lot of misuses that don't
actually lead to bugs. I don't like the name because in my head OTR is an idiom
and synonymous with incognito. Wouldn't it be better if it were called
ShouldRecordData or something that's not a widely used term?

Note that there is this handy utility function called
profiles::IsRegularOrGuestSession() which is basically the same as type !=
Profile::INCOGNITO_PROFILE INCOGNITO. Compare this to IsOffTheRecord(), which is
the same as type != Profile::REGULAR_PROFILE. So clearly we do have some other
places in the code (not just the places in this CL) that want to bucket things
as "guest or normal" vs "incognito". Seems like it's more foolproof to force the
person writing the client code to think in terms of the ternary rather than a
handful of binaries to suit any occasion.

[pkotwicz, 2016-01-27 21:54:51]

LGTM

[Evan Stade, 2016-01-27 21:55:09]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-01-27 21:55:19]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-01-27 21:55:24]

Description was changed from

==========
[MD] Remove new incognito styling from guest mode.

Profile::IsOffTheRecord() surprisingly does imply incognito; it also covers
guest profiles.

BUG=none
==========

to

==========
[MD] Remove new incognito styling from guest mode.

Profile::IsOffTheRecord() surprisingly does not imply incognito; it also covers
guest profiles.

BUG=none
==========

[Evan Stade, 2016-01-27 21:55:32]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-01-27 21:57:38]

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

[commit-bot: I haz the power, 2016-01-27 22:54:40]

Description was changed from

==========
[MD] Remove new incognito styling from guest mode.

Profile::IsOffTheRecord() surprisingly does not imply incognito; it also covers
guest profiles.

BUG=none
==========

to

==========
[MD] Remove new incognito styling from guest mode.

Profile::IsOffTheRecord() surprisingly does not imply incognito; it also covers
guest profiles.

BUG=none
==========

[commit-bot: I haz the power, 2016-01-27 22:54:40]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-01-27 22:56:26]

Description was changed from

==========
[MD] Remove new incognito styling from guest mode.

Profile::IsOffTheRecord() surprisingly does not imply incognito; it also covers
guest profiles.

BUG=none
==========

to

==========
[MD] Remove new incognito styling from guest mode.

Profile::IsOffTheRecord() surprisingly does not imply incognito; it also covers
guest profiles.

BUG=none

Committed: https://crrev.com/1257c40097711f465538faede3fd74bfcc83ff9e
Cr-Commit-Position: refs/heads/master@{#371906}
==========

[commit-bot: I haz the power, 2016-01-27 22:56:27]

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