Support persistent incognito preferences

Support persistent incognito preferences


BUG=60099
TEST=no

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

[battre, 2011-05-24 18:18:40]

And this is the next one, enabling persistent incognito preferences. I decided
to do this before the session scoped incognito preferences because it is easier
for me this way.

I can just re-enable all test-cases that we had before. I am confident to create
a good CL for the session scoped incognito preferences by tomorrow evening and
will create new test cases for this.

[Matt Perry, 2011-05-24 18:23:24]

http://codereview.chromium.org/7065033/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_pref_value_map.h (right):

http://codereview.chromium.org/7065033/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_pref_value_map.h:15: #include
"chrome/browser/extensions/extension_prefs_scope.h"
I don't see this file in the CL.

[battre, 2011-05-24 18:24:59]

http://codereview.chromium.org/7065033/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_pref_value_map.h (right):

http://codereview.chromium.org/7065033/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_pref_value_map.h:15: #include
"chrome/browser/extensions/extension_prefs_scope.h"
On 2011/05/24 18:23:24, Matt Perry wrote:
> I don't see this file in the CL.

Done.

[Matt Perry, 2011-05-24 21:04:56]

http://codereview.chromium.org/7065033/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_pref_value_map.cc (right):

http://codereview.chromium.org/7065033/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_pref_value_map.cc:124: return
&(i->second->reg_preferences);
Per the style guide, avoid abbrevs.

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:371: if (incognito &&
!include_incognito()) {
Hmm.. what is the desired behavior for the preferences API in a split-mode
extension? include_incognito will always be false in that case.

You probably want to restrict things this way:
- split-mode regular profile: error unless scope is kRegular
- split-mode incognito profile: error unless scope is kIncognito*

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs_scope.h (right):

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_scope.h:9: namespace
extension_prefs_scope {
This should not be a namespace. See this discussion on chromium-dev
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

I'd say you can just make it a member of either the ExtensionPrefs or
ExtensionPrefValueMap class.

[Bernhard Bauer, 2011-05-24 21:16:25]

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:371: if (incognito &&
!include_incognito()) {
On 2011/05/24 21:04:56, Matt Perry wrote:
> Hmm.. what is the desired behavior for the preferences API in a split-mode
> extension? include_incognito will always be false in that case.
> 
> You probably want to restrict things this way:
> - split-mode regular profile: error unless scope is kRegular
> - split-mode incognito profile: error unless scope is kIncognito*

My idea behind it was that for a split-mode incognito profile regular scope is
automatically mapped to incognito.

[Bernhard Bauer, 2011-05-25 00:43:00]

http://codereview.chromium.org/7065033/diff/1044/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/7065033/diff/1044/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:123: NOTREACHED();
In that case you should fail validation (EXTENSION_FUNCTION_VALIDATE style)
instead of DCHECKing.

[battre, 2011-05-25 11:31:45]

http://codereview.chromium.org/7065033/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_pref_value_map.cc (right):

http://codereview.chromium.org/7065033/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_pref_value_map.cc:124: return
&(i->second->reg_preferences);
On 2011/05/24 21:04:56, Matt Perry wrote:
> Per the style guide, avoid abbrevs.

Done.

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:371: if (incognito &&
!include_incognito()) {
On 2011/05/24 21:16:25, Bernhard Bauer wrote:
> On 2011/05/24 21:04:56, Matt Perry wrote:
> > Hmm.. what is the desired behavior for the preferences API in a split-mode
> > extension? include_incognito will always be false in that case.
> > 
> > You probably want to restrict things this way:
> > - split-mode regular profile: error unless scope is kRegular
> > - split-mode incognito profile: error unless scope is kIncognito*
> 
> My idea behind it was that for a split-mode incognito profile regular scope is
> automatically mapped to incognito.

I am not familiar with the split-mode preferences - in particular how I can
figure out whether a extension runs in split mode. Can you please provide the
snippet you would like to see here?
Thanks.

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs_scope.h (right):

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_scope.h:9: namespace
extension_prefs_scope {
On 2011/05/24 21:04:56, Matt Perry wrote:
> This should not be a namespace. See this discussion on chromium-dev
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
> 
> I'd say you can just make it a member of either the ExtensionPrefs or
> ExtensionPrefValueMap class.

Both would be a layering violation.
- ExtensionPrefValueMap is an implementation detail that should not be visible
to consumers of ExtensionPrefs.
- ExtensionPrefs on the other hand builds upon ExtensionPrefValueMap and
therefore should not be required for ExtensionPrefValueMap.

I have taken a note and propose to discuss this afterwards. After all it is just
an implementation detail that is not visible to the outside.

http://codereview.chromium.org/7065033/diff/1044/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/7065033/diff/1044/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:123: NOTREACHED();
On 2011/05/25 00:43:00, Bernhard Bauer wrote:
> In that case you should fail validation (EXTENSION_FUNCTION_VALIDATE style)
> instead of DCHECKing.

The EXTENSION_FUNCTION_VALIDATE style validation has taken place already. The
Extension API guarantees that only valid strings arrive here. If anything else
arrives, this is a bug and should DCHECK.

[Bernhard Bauer, 2011-05-25 17:48:34]

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs_scope.h (right):

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_scope.h:9: namespace
extension_prefs_scope {
On 2011/05/25 11:31:45, battre wrote:
> On 2011/05/24 21:04:56, Matt Perry wrote:
> > This should not be a namespace. See this discussion on chromium-dev
> >
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
> > 
> > I'd say you can just make it a member of either the ExtensionPrefs or
> > ExtensionPrefValueMap class.
> 
> Both would be a layering violation.
> - ExtensionPrefValueMap is an implementation detail that should not be visible
> to consumers of ExtensionPrefs.
> - ExtensionPrefs on the other hand builds upon ExtensionPrefValueMap and
> therefore should not be required for ExtensionPrefValueMap.
> 
> I have taken a note and propose to discuss this afterwards. After all it is
just
> an implementation detail that is not visible to the outside.

How about just calling it ExtensionPrefScope?

http://codereview.chromium.org/7065033/diff/1044/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/7065033/diff/1044/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:123: NOTREACHED();
On 2011/05/25 11:31:45, battre wrote:
> On 2011/05/25 00:43:00, Bernhard Bauer wrote:
> > In that case you should fail validation (EXTENSION_FUNCTION_VALIDATE style)
> > instead of DCHECKing.
> 
> The EXTENSION_FUNCTION_VALIDATE style validation has taken place already. The
> Extension API guarantees that only valid strings arrive here. 

As in, the Javascript API? That can be circumvented though (in which case we
should kill the renderer). I don't think this case is any different from the
other EXTENSION_FUNCTION_VALIDATE below, which also check stuff which has been
checked renderer-side by the JSON validator.

> If anything else
> arrives, this is a bug and should DCHECK.

[Matt Perry, 2011-05-25 18:40:42]

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:371: if (incognito &&
!include_incognito()) {
On 2011/05/25 11:31:45, battre wrote:
> On 2011/05/24 21:16:25, Bernhard Bauer wrote:
> > On 2011/05/24 21:04:56, Matt Perry wrote:
> > > Hmm.. what is the desired behavior for the preferences API in a split-mode
> > > extension? include_incognito will always be false in that case.
> > > 
> > > You probably want to restrict things this way:
> > > - split-mode regular profile: error unless scope is kRegular
> > > - split-mode incognito profile: error unless scope is kIncognito*
> > 
> > My idea behind it was that for a split-mode incognito profile regular scope
is
> > automatically mapped to incognito.
> 
> I am not familiar with the split-mode preferences - in particular how I can
> figure out whether a extension runs in split mode. Can you please provide the
> snippet you would like to see here?
> Thanks.

bool incognito = (scope == ...);
if (incognito) {
  // regular profiles can't access incognito unless
  // include_incognito is true.
  if (!profile()->IsOffTheRecord() && !include_incognito())
    error = kIncognitoErrorMessage;
} else {
  // incognito profiles can't access regular mode ever. 
  // (they only exist in split mode).
  if (profile()->IsOffTheRecord())
    error = "Incognito extension can't access regular settings.";
}

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs_scope.h (right):

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_scope.h:9: namespace
extension_prefs_scope {
On 2011/05/25 11:31:45, battre wrote:
> On 2011/05/24 21:04:56, Matt Perry wrote:
> > This should not be a namespace. See this discussion on chromium-dev
> >
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
> > 
> > I'd say you can just make it a member of either the ExtensionPrefs or
> > ExtensionPrefValueMap class.
> 
> Both would be a layering violation.
> - ExtensionPrefValueMap is an implementation detail that should not be visible
> to consumers of ExtensionPrefs.
> - ExtensionPrefs on the other hand builds upon ExtensionPrefValueMap and
> therefore should not be required for ExtensionPrefValueMap.
> 
> I have taken a note and propose to discuss this afterwards. After all it is
just
> an implementation detail that is not visible to the outside.

Please add a TODO(battre) in that case.

http://codereview.chromium.org/7065033/diff/1044/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/7065033/diff/1044/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:123: NOTREACHED();
On 2011/05/25 17:48:35, Bernhard Bauer wrote:
> On 2011/05/25 11:31:45, battre wrote:
> > On 2011/05/25 00:43:00, Bernhard Bauer wrote:
> > > In that case you should fail validation (EXTENSION_FUNCTION_VALIDATE
style)
> > > instead of DCHECKing.
> > 
> > The EXTENSION_FUNCTION_VALIDATE style validation has taken place already.
The
> > Extension API guarantees that only valid strings arrive here. 
> 
> As in, the Javascript API? That can be circumvented though (in which case we
> should kill the renderer). I don't think this case is any different from the
> other EXTENSION_FUNCTION_VALIDATE below, which also check stuff which has been
> checked renderer-side by the JSON validator.

Agreed. EXTENSION_FUNCTION_VALIDATE exists because a renderer could be
compromised and send bad messages to the browser. You've only validated that
it's a string. You should also validate the enum value.

[battre, 2011-05-25 19:47:43]

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:371: if (incognito &&
!include_incognito()) {
On 2011/05/25 18:40:42, Matt Perry wrote:
> On 2011/05/25 11:31:45, battre wrote:
> > On 2011/05/24 21:16:25, Bernhard Bauer wrote:
> > > On 2011/05/24 21:04:56, Matt Perry wrote:
> > > > Hmm.. what is the desired behavior for the preferences API in a
split-mode
> > > > extension? include_incognito will always be false in that case.
> > > > 
> > > > You probably want to restrict things this way:
> > > > - split-mode regular profile: error unless scope is kRegular
> > > > - split-mode incognito profile: error unless scope is kIncognito*
> > > 
> > > My idea behind it was that for a split-mode incognito profile regular
scope
> is
> > > automatically mapped to incognito.
> > 
> > I am not familiar with the split-mode preferences - in particular how I can
> > figure out whether a extension runs in split mode. Can you please provide
the
> > snippet you would like to see here?
> > Thanks.
> 
> bool incognito = (scope == ...);
> if (incognito) {
>   // regular profiles can't access incognito unless
>   // include_incognito is true.
>   if (!profile()->IsOffTheRecord() && !include_incognito())
>     error = kIncognitoErrorMessage;
> } else {
>   // incognito profiles can't access regular mode ever. 
>   // (they only exist in split mode).
>   if (profile()->IsOffTheRecord())
>     error = "Incognito extension can't access regular settings.";
> }

Done.

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs_scope.h (right):

http://codereview.chromium.org/7065033/diff/3003/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs_scope.h:9: namespace
extension_prefs_scope {
On 2011/05/25 18:40:42, Matt Perry wrote:
> On 2011/05/25 11:31:45, battre wrote:
> > On 2011/05/24 21:04:56, Matt Perry wrote:
> > > This should not be a namespace. See this discussion on chromium-dev
> > >
> >
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
> > > 
> > > I'd say you can just make it a member of either the ExtensionPrefs or
> > > ExtensionPrefValueMap class.
> > 
> > Both would be a layering violation.
> > - ExtensionPrefValueMap is an implementation detail that should not be
visible
> > to consumers of ExtensionPrefs.
> > - ExtensionPrefs on the other hand builds upon ExtensionPrefValueMap and
> > therefore should not be required for ExtensionPrefValueMap.
> > 
> > I have taken a note and propose to discuss this afterwards. After all it is
> just
> > an implementation detail that is not visible to the outside.
> 
> Please add a TODO(battre) in that case.

Done.

http://codereview.chromium.org/7065033/diff/1044/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/7065033/diff/1044/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:123: NOTREACHED();
On 2011/05/25 18:40:42, Matt Perry wrote:
> On 2011/05/25 17:48:35, Bernhard Bauer wrote:
> > On 2011/05/25 11:31:45, battre wrote:
> > > On 2011/05/25 00:43:00, Bernhard Bauer wrote:
> > > > In that case you should fail validation (EXTENSION_FUNCTION_VALIDATE
> style)
> > > > instead of DCHECKing.
> > > 
> > > The EXTENSION_FUNCTION_VALIDATE style validation has taken place already.
> The
> > > Extension API guarantees that only valid strings arrive here. 
> > 
> > As in, the Javascript API? That can be circumvented though (in which case we
> > should kill the renderer). I don't think this case is any different from the
> > other EXTENSION_FUNCTION_VALIDATE below, which also check stuff which has
been
> > checked renderer-side by the JSON validator.
> 
> Agreed. EXTENSION_FUNCTION_VALIDATE exists because a renderer could be
> compromised and send bad messages to the browser. You've only validated that
> it's a string. You should also validate the enum value.

Done.

[Matt Perry, 2011-05-25 21:13:08]

LGTM with tweaks below

http://codereview.chromium.org/7065033/diff/11003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/7065033/diff/11003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:122: NOTREACHED();
remove the NOTREACHED. VALIDATE is enough

http://codereview.chromium.org/7065033/diff/11003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:385: error_ = "Incognito
extension can't access regular settings.";
Fix the comments (full sentences), make this error message a constant, and
reword it like "Can't modify regular settings from an incognito context."

http://codereview.chromium.org/7065033/diff/11003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:440: // We don't check
incognito permissions here, as an extension should be always
line length

[battre, 2011-05-25 22:07:46]

http://codereview.chromium.org/7065033/diff/11003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/7065033/diff/11003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:122: NOTREACHED();
On 2011/05/25 21:13:08, Matt Perry wrote:
> remove the NOTREACHED. VALIDATE is enough

Done.

http://codereview.chromium.org/7065033/diff/11003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:385: error_ = "Incognito
extension can't access regular settings.";
On 2011/05/25 21:13:08, Matt Perry wrote:
> Fix the comments (full sentences), make this error message a constant, and
> reword it like "Can't modify regular settings from an incognito context."

Done.

http://codereview.chromium.org/7065033/diff/11003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:440: // We don't check
incognito permissions here, as an extension should be always
On 2011/05/25 21:13:08, Matt Perry wrote:
> line length

Done.