Enable incognito_session_only preferences

Enable incognito_session_only preferences

BUG=60099
TEST=no

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

[battre, 2011-05-24 21:32:21]

And the final CL. I hope this is still reasonable... I notice that my attention
span is ceasing... (working for 14 hours now ;-))

[Bernhard Bauer, 2011-05-24 21:43:57]

http://codereview.chromium.org/7067040/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7067040/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:5416: "description": "Where to
set the preference (default: regular). One of<br><var>'regular'</var>:
preference for regular profile (which is inherited by the incognito profile if
not overridden elsewhere),<br><var>incognito_persistent</var>: preference for
incognito profile that sustains browser restarts (overrides regular
preferences),<br><var>incognito_session_only</var>: preference for incognito
profile that can only be set during an incognito session and is deleted when the
incognito session ends (overrides regular and incognito_persistent'
preferences)."
Nit: s/sustains/survives/?

[Matt Perry, 2011-05-24 22:07:11]

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

http://codereview.chromium.org/7067040/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_pref_value_map.cc:85:
NotifyPrefValueChanged(*k);
Is it necessarily true that the effective pref value changed? What if the value
being deleted was not the active value? We should only fire the onChanged event
if the effective value changed.

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

http://codereview.chromium.org/7067040/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:303: int
val_ext1_incognito_pers_;    // pref value of extension 1 incognito pers.
nit: no period at end of inline comment

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

http://codereview.chromium.org/7067040/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_preference_api.cc:53: "You cannot set
'session-only incognito' preferences when no incognito "
Reword: "You cannot set a preference with scope 'incognito_session' when no
incognito window is open."

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

http://codereview.chromium.org/7067040/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs_unittest.cc:780: class
ExtensionPrefsInstallIncognitoPers
don't abbreviate Persistent

http://codereview.chromium.org/7067040/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7067040/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:5358: "enum":
["NotControllable", "ControlledByOtherExtensions",
"ControllableByThisExtension", "ControlledByThisExtension"]
Sorry for not noticing this sooner, but we should have some consistency with our
enum values. Most released APIs use names like not_controllable, so we should go
with that naming here.

http://codereview.chromium.org/7067040/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:5389: "description": "Where to
set the preference (default: regular). One of <br><var>regular</var>: preference
for regular profile (which is inherited by the incognito profile if not
overridden elsewhere), 'incognito_persistent' = preference for incognito profile
that sustains browser restarts (overrides regular preferences)."
doesn't explain incognito_session_only

http://codereview.chromium.org/7067040/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:5416: "description": "Where to
set the preference (default: regular). One of<br><var>'regular'</var>:
preference for regular profile (which is inherited by the incognito profile if
not overridden elsewhere),<br><var>incognito_persistent</var>: preference for
incognito profile that sustains browser restarts (overrides regular
preferences),<br><var>incognito_session_only</var>: preference for incognito
profile that can only be set during an incognito session and is deleted when the
incognito session ends (overrides regular and incognito_persistent'
preferences)."
last apostrophe unnecessary.

Copy this description to the set() method.

[Bernhard Bauer, 2011-05-24 22:28:05]

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

http://codereview.chromium.org/7067040/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_pref_value_map.cc:85:
NotifyPrefValueChanged(*k);
On 2011/05/24 22:07:17, Matt Perry wrote:
> Is it necessarily true that the effective pref value changed? What if the
value
> being deleted was not the active value? We should only fire the onChanged
event
> if the effective value changed.

Hm, for an extension it might also be interesting to find out when another
extension starts controlling the value, even if it happens to be the same. I
think firing the event more often than necessary shouldn't be as bad as firing
it less often.

[Matt Perry, 2011-05-24 22:35:01]

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

http://codereview.chromium.org/7067040/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_pref_value_map.cc:85:
NotifyPrefValueChanged(*k);
On 2011/05/24 22:28:06, Bernhard Bauer wrote:
> On 2011/05/24 22:07:17, Matt Perry wrote:
> > Is it necessarily true that the effective pref value changed? What if the
> value
> > being deleted was not the active value? We should only fire the onChanged
> event
> > if the effective value changed.
> 
> Hm, for an extension it might also be interesting to find out when another
> extension starts controlling the value, even if it happens to be the same. I
> think firing the event more often than necessary shouldn't be as bad as firing
> it less often.

I agree with that (if the controlling extension changes). Maybe I'm just
confused about how this works, but I'm thinking about this scenario:
1. extensionA sets pref to X.
2. extensionB sets pref to Y, but has lower precedence.
3. extensionB clears pref.

In both step 2 and 3, the pref doesn't actually change, nor does the controlling
extension, since extensionA has precedence. onChanged should only fire in step
1.

[Bernhard Bauer, 2011-05-24 22:46:29]

On Tue, May 24, 2011 at 15:35,  <mpcomplete@chromium.org> wrote:
>
>
http://codereview.chromium.org/7067040/diff/1/chrome/browser/extensions/exten...
> File chrome/browser/extensions/extension_pref_value_map.cc (right):
>
>
http://codereview.chromium.org/7067040/diff/1/chrome/browser/extensions/exten...
> chrome/browser/extensions/extension_pref_value_map.cc:85:
> NotifyPrefValueChanged(*k);
> On 2011/05/24 22:28:06, Bernhard Bauer wrote:
>>
>> On 2011/05/24 22:07:17, Matt Perry wrote:
>> > Is it necessarily true that the effective pref value changed? What
>
> if the
>>
>> value
>> > being deleted was not the active value? We should only fire the
>
> onChanged
>>
>> event
>> > if the effective value changed.
>
>> Hm, for an extension it might also be interesting to find out when
>
> another
>>
>> extension starts controlling the value, even if it happens to be the
>
> same. I
>>
>> think firing the event more often than necessary shouldn't be as bad
>
> as firing
>>
>> it less often.
>
> I agree with that (if the controlling extension changes). Maybe I'm just
> confused about how this works, but I'm thinking about this scenario:
> 1. extensionA sets pref to X.
> 2. extensionB sets pref to Y, but has lower precedence.
> 3. extensionB clears pref.
>
> In both step 2 and 3, the pref doesn't actually change, nor does the
> controlling extension, since extensionA has precedence. onChanged should
> only fire in step 1.

I'm just not sure if it's worth the additional effort to only fire the
minimal set of events, when we could just fire an event every time.
Most extensions should be able to deal with spurious events.

>
> http://codereview.chromium.org/7067040/
>

[battre, 2011-05-25 11:51:30]

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

http://codereview.chromium.org/7067040/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_pref_value_map.cc:85:
NotifyPrefValueChanged(*k);
On 2011/05/24 22:35:02, Matt Perry wrote:
> On 2011/05/24 22:28:06, Bernhard Bauer wrote:
> > On 2011/05/24 22:07:17, Matt Perry wrote:
> > > Is it necessarily true that the effective pref value changed? What if the
> > value
> > > being deleted was not the active value? We should only fire the onChanged
> > event
> > > if the effective value changed.
> > 
> > Hm, for an extension it might also be interesting to find out when another
> > extension starts controlling the value, even if it happens to be the same. I
> > think firing the event more often than necessary shouldn't be as bad as
firing
> > it less often.
> 
> I agree with that (if the controlling extension changes). Maybe I'm just
> confused about how this works, but I'm thinking about this scenario:
> 1. extensionA sets pref to X.
> 2. extensionB sets pref to Y, but has lower precedence.
> 3. extensionB clears pref.
> 
> In both step 2 and 3, the pref doesn't actually change, nor does the
controlling
> extension, since extensionA has precedence. onChanged should only fire in step
> 1.

This is exactly what happens.

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

http://codereview.chromium.org/7067040/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_pref_value_map_unittest.cc:303: int
val_ext1_incognito_pers_;    // pref value of extension 1 incognito pers.
On 2011/05/24 22:07:17, Matt Perry wrote:
> nit: no period at end of inline comment

It is not a end of inline comment but an abbreviation (see line 300 and 301). I
suggest to keep the period.

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

http://codereview.chromium.org/7067040/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_preference_api.cc:53: "You cannot set
'session-only incognito' preferences when no incognito "
On 2011/05/24 22:07:17, Matt Perry wrote:
> Reword: "You cannot set a preference with scope 'incognito_session' when no
> incognito window is open."

Done.

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

http://codereview.chromium.org/7067040/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs_unittest.cc:780: class
ExtensionPrefsInstallIncognitoPers
On 2011/05/24 22:07:17, Matt Perry wrote:
> don't abbreviate Persistent

Done.

http://codereview.chromium.org/7067040/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7067040/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:5358: "enum":
["NotControllable", "ControlledByOtherExtensions",
"ControllableByThisExtension", "ControlledByThisExtension"]
On 2011/05/24 22:07:17, Matt Perry wrote:
> Sorry for not noticing this sooner, but we should have some consistency with
our
> enum values. Most released APIs use names like not_controllable, so we should
go
> with that naming here.

Will do in a separate CL as it is currently not sure whether we want to go
camelCase or hacker_style.

http://codereview.chromium.org/7067040/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:5389: "description": "Where to
set the preference (default: regular). One of <br><var>regular</var>: preference
for regular profile (which is inherited by the incognito profile if not
overridden elsewhere), 'incognito_persistent' = preference for incognito profile
that sustains browser restarts (overrides regular preferences)."
On 2011/05/24 22:07:17, Matt Perry wrote:
> doesn't explain incognito_session_only

Done.

http://codereview.chromium.org/7067040/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:5416: "description": "Where to
set the preference (default: regular). One of<br><var>'regular'</var>:
preference for regular profile (which is inherited by the incognito profile if
not overridden elsewhere),<br><var>incognito_persistent</var>: preference for
incognito profile that sustains browser restarts (overrides regular
preferences),<br><var>incognito_session_only</var>: preference for incognito
profile that can only be set during an incognito session and is deleted when the
incognito session ends (overrides regular and incognito_persistent'
preferences)."
On 2011/05/24 21:43:57, Bernhard Bauer wrote:
> Nit: s/sustains/survives/?

Done.

http://codereview.chromium.org/7067040/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:5416: "description": "Where to
set the preference (default: regular). One of<br><var>'regular'</var>:
preference for regular profile (which is inherited by the incognito profile if
not overridden elsewhere),<br><var>incognito_persistent</var>: preference for
incognito profile that sustains browser restarts (overrides regular
preferences),<br><var>incognito_session_only</var>: preference for incognito
profile that can only be set during an incognito session and is deleted when the
incognito session ends (overrides regular and incognito_persistent'
preferences)."
On 2011/05/24 22:07:17, Matt Perry wrote:
> last apostrophe unnecessary.
> 
> Copy this description to the set() method.

Done.

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

LGTM