Implement chrome.systemPrivate.getIncognitoModeAvailability extension API function

Implement chrome.systemPrivate.getIncognitoModeAvailability extension API function.

This is a function for the bookmark manager, which wants to show the "Open in Incognito mode" option only if the incognito mode is available.

BUG=none
TEST=no


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

[battre, 2011-11-23 12:46:14]

Please review. Thanks.

[Bernhard Bauer, 2011-11-23 13:03:35]

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

http://codereview.chromium.org/8662008/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_preference_api.cc:38: // Permission requried
to access this preference.
Nit: "required"

http://codereview.chromium.org/8662008/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_preference_api.cc:169: *error =
extension_preference_api_constants::kInvalidParameter;
Wait, do we actually need this if the preference can't be changed by an
extension?

http://codereview.chromium.org/8662008/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_preference_api.cc:429: if (permission !=
ExtensionAPIPermission::kInvalid &&
Could you move this into a helper method?

http://codereview.chromium.org/8662008/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_preference_api.cc:512: for (size_t i = 0; i
< arraysize(kReadOnlyPrefs); ++i) {
Could you move this into a helper method that returns whether the preference is
read-only?

[battre, 2011-11-23 15:43:53]

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

http://codereview.chromium.org/8662008/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_preference_api.cc:38: // Permission requried
to access this preference.
On 2011/11/23 13:03:35, Bernhard Bauer wrote:
> Nit: "required"

Done.

http://codereview.chromium.org/8662008/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_preference_api.cc:169: *error =
extension_preference_api_constants::kInvalidParameter;
On 2011/11/23 13:03:35, Bernhard Bauer wrote:
> Wait, do we actually need this if the preference can't be changed by an
> extension?

I have removed the content of this function and replaced it with NOTREACHED().

Done.

http://codereview.chromium.org/8662008/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_preference_api.cc:429: if (permission !=
ExtensionAPIPermission::kInvalid &&
On 2011/11/23 13:03:35, Bernhard Bauer wrote:
> Could you move this into a helper method?

Done.

http://codereview.chromium.org/8662008/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_preference_api.cc:512: for (size_t i = 0; i
< arraysize(kReadOnlyPrefs); ++i) {
On 2011/11/23 13:03:35, Bernhard Bauer wrote:
> Could you move this into a helper method that returns whether the preference
is
> read-only?

Done.

[Bernhard Bauer, 2011-11-23 15:53:57]

LGTM.

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

http://codereview.chromium.org/8662008/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:191: for (size_t i = 0; i
< arraysize(kReadOnlyPrefs); ++i) {
Nit: Indent only two spaces.

[battre, 2011-11-23 15:56:05]

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

http://codereview.chromium.org/8662008/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_preference_api.cc:191: for (size_t i = 0; i
< arraysize(kReadOnlyPrefs); ++i) {
On 2011/11/23 15:53:57, Bernhard Bauer wrote:
> Nit: Indent only two spaces.

Done.

[battre, 2011-11-23 16:00:44]

+aa for OWNERS approval and question below.

Aaron, I am asking you for permission to "pollute" the chrome.windows namespace.
The use-case for this CL is that the bookmark manager wants to check whether the
incognito mode is disabled and in order to not offer an "open in incognito
window" option in this case. I don't know whether this is an important enough
use-case to expose this preference.

I see three options:
1) Implement chrome.windows.incognitoModeAvailability as in this CL
2) as 1) but do not generate API documentation for it
3) move the incognitoModeAvailability preference somewhere else. I think that
chrome.windows is the most appropriate namespace, though.

WDYT?

[Aaron Boodman, 2011-11-23 23:01:07]

On 2011/11/23 16:00:44, battre wrote:
> +aa for OWNERS approval and question below.
> 
> Aaron, I am asking you for permission to "pollute" the chrome.windows
namespace.
> The use-case for this CL is that the bookmark manager wants to check whether
the
> incognito mode is disabled and in order to not offer an "open in incognito
> window" option in this case. I don't know whether this is an important enough
> use-case to expose this preference.
> 
> I see three options:
> 1) Implement chrome.windows.incognitoModeAvailability as in this CL
> 2) as 1) but do not generate API documentation for it
> 3) move the incognitoModeAvailability preference somewhere else. I think that
> chrome.windows is the most appropriate namespace, though.
> 
> WDYT?

It seems like a big departure from what we've done in the past to put something
from extension_preferences.* in the windows namespace.

What other API do we have that expose browser settings? I know there is content
settings, but that is slightly different. Anything else?

Maybe we should introduce a new 'experimental.system' namespace or something.
There was also the wake notifications and volume change notifications that went
in recently that could live there.

[battre, 2011-11-24 14:29:20]

On 2011/11/23 23:01:07, Aaron Boodman wrote:
> On 2011/11/23 16:00:44, battre wrote:
> > +aa for OWNERS approval and question below.
> > 
> > Aaron, I am asking you for permission to "pollute" the chrome.windows
> namespace.
> > The use-case for this CL is that the bookmark manager wants to check whether
> the
> > incognito mode is disabled and in order to not offer an "open in incognito
> > window" option in this case. I don't know whether this is an important
enough
> > use-case to expose this preference.
> > 
> > I see three options:
> > 1) Implement chrome.windows.incognitoModeAvailability as in this CL
> > 2) as 1) but do not generate API documentation for it
> > 3) move the incognitoModeAvailability preference somewhere else. I think
that
> > chrome.windows is the most appropriate namespace, though.
> > 
> > WDYT?
> 
> It seems like a big departure from what we've done in the past to put
something
> from extension_preferences.* in the windows namespace.

Well, I did not suggest chrome.windows as a container for "misc extension
preferences" but considered the question whether the Incognito mode is available
as somewhat related to chrome.windows. Therefore this namespace.

Also note that this is an extension_preference in the sense that it has the same
API. But it is read-only. So it is more of a status indicator. At the moment we
do not plan that extensions may disable the incognito feature. 

> What other API do we have that expose browser settings? I know there is
content
> settings, but that is slightly different. Anything else?

We have the privacy API that allows configuring settings. But I think that we do
not want to bundle the harmless feature to read the incognitoAvailability with
the permission to modify all privacy settings.

chrome.tabs is also somewhat related to Incognito windows but I see a closer
connection to windows.

We could extend chrome.experimental.settings by a namespace
chrome.experimental.settings.chrome for chrome settings. I am not a big fan of
this though. chrome.experimental.settings should only be used for settings of
extensions.

My current preference would be to make it part of chrome.windows and then
discuss whether we want to expose it in the API documentation or not. Currently
the only consumer of this is a component extension, the bookmark manager.

> Maybe we should introduce a new 'experimental.system' namespace or something.

We could do that. My concern is that this would become a place where we just
dump everything.

> There was also the wake notifications and volume change notifications that
went
> in recently that could live there.

Wake notifications went into chrome.idle. The volume control went into
experimental.accessibility.

[Aaron Boodman, 2011-11-27 06:14:49]

On 2011/11/24 14:29:20, battre wrote:
> On 2011/11/23 23:01:07, Aaron Boodman wrote:
> > On 2011/11/23 16:00:44, battre wrote:
> > > +aa for OWNERS approval and question below.
> > > 
> > > Aaron, I am asking you for permission to "pollute" the chrome.windows
> > namespace.
> > > The use-case for this CL is that the bookmark manager wants to check
whether
> > the
> > > incognito mode is disabled and in order to not offer an "open in incognito
> > > window" option in this case. I don't know whether this is an important
> enough
> > > use-case to expose this preference.
> > > 
> > > I see three options:
> > > 1) Implement chrome.windows.incognitoModeAvailability as in this CL
> > > 2) as 1) but do not generate API documentation for it
> > > 3) move the incognitoModeAvailability preference somewhere else. I think
> that
> > > chrome.windows is the most appropriate namespace, though.
> > > 
> > > WDYT?
> > 
> > It seems like a big departure from what we've done in the past to put
> something
> > from extension_preferences.* in the windows namespace.
> 
> Well, I did not suggest chrome.windows as a container for "misc extension
> preferences" but considered the question whether the Incognito mode is
available
> as somewhat related to chrome.windows. Therefore this namespace.
> 
> Also note that this is an extension_preference in the sense that it has the
same
> API. But it is read-only. So it is more of a status indicator. At the moment
we
> do not plan that extensions may disable the incognito feature. 

What I mean is that we currently have the convention that chrome.foo is mostly
implemented by foo_extension_api.h and .cc. This would be a departure.

> > What other API do we have that expose browser settings? I know there is
> content
> > settings, but that is slightly different. Anything else?
> 
> We have the privacy API that allows configuring settings. But I think that we
do
> not want to bundle the harmless feature to read the incognitoAvailability with
> the permission to modify all privacy settings.
> 
> chrome.tabs is also somewhat related to Incognito windows but I see a closer
> connection to windows.

I guess, but it still seems like a pretty weak connection. In Chrome, incognito
is a feature of a profile. It's only loosely related to a window.

> We could extend chrome.experimental.settings by a namespace
> chrome.experimental.settings.chrome for chrome settings. I am not a big fan of
> this though. chrome.experimental.settings should only be used for settings of
> extensions.
> 
> My current preference would be to make it part of chrome.windows and then
> discuss whether we want to expose it in the API documentation or not.
Currently
> the only consumer of this is a component extension, the bookmark manager.

If the only consumer is a component extension and we don't have a good home for
it yet, then we should make it private for now until we figure it out.

> > Maybe we should introduce a new 'experimental.system' namespace or
something.
> 
> We could do that. My concern is that this would become a place where we just
> dump everything.
> 
> > There was also the wake notifications and volume change notifications that
> went
> > in recently that could live there.
> 
> Wake notifications went into chrome.idle. The volume control went into
> experimental.accessibility.

As of now, they are both in accessibility
(http://git.chromium.org/gitweb/?p=chromium/chromium.git;a=blob;f=chrome/commo...).
I wasn't a fan of putting the volume control into accessibility. But it's
experimental, so we could move it if that made sense.

I think that we should create a new private "system" API and put wake
notifications, volume notifications, and this thing there for now. It may become
a bit of a dumping ground, but it is a private API so that is only a problem in
the code quality sense, and we can change that if necessary.

WDYT?

[battre, 2011-11-28 14:43:29]

I have moved the preference into the chrome.system namespace and marked it as
available to component extensions only.

[Aaron Boodman, 2011-11-28 19:32:44]

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

http://codereview.chromium.org/8662008/diff/12043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
Can you file one or more bugs for yourself to fix the following issues:

1. Move extension_preference_api elsewhere. See crbug.com/101244. If there's no
home outside of chrome/browser/extensions that makes sense, you can make a
subdir inside extensions.

2. Rename to content_settings_extension_api (or content_settings_api if inside
browser/extensions/).

3. Remove extension_preference_api_constants*. No longer needed. These can be in
an anonymous namespace inside the cc file.

http://codereview.chromium.org/8662008/diff/12043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:56: const char*
kReadOnlyPrefs[] = {
Does it really make sense to overload the content settings API to serve this
different purpose?

It seems likely to cause coupling problems. And when it comes down to it the
implementation of isIncognitoAvailable() is like one line. It seems harder to
share it than it's worth.

http://codereview.chromium.org/8662008/diff/12043/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_system_api.h (right):

http://codereview.chromium.org/8662008/diff/12043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_system_api.h:5: // This is a dumping ground
for APIs that are provided for component extensions
I don't think this comment is accurate. 'system' is a reasonable enough
namespace for a method that determines whether incognito is available.

[battre, 2011-11-28 20:23:16]

Thanks for the review. I am just posting my thoughts on your points now. I will
do the code changes tomorrow.

Best regards,
Dominic

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

http://codereview.chromium.org/8662008/diff/12043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
On 2011/11/28 19:32:44, Aaron Boodman wrote:
> Can you file one or more bugs for yourself to fix the following issues:
> 
> 1. Move extension_preference_api elsewhere. See crbug.com/101244. If there's
no
> home outside of chrome/browser/extensions that makes sense, you can make a
> subdir inside extensions.

I will do that, though I am pretty sad seeing that extensions are moved out of
chrome/browser/extensions. I'd have preferred if all extensions were moved to
chrome/browser/extensions/${extension_api_name} as it used to be a very handy
reference to look up how things are done in extensions-land by just peeking at
other examples. Searching for other extensions in the code base seems more
disadvantageous to me than having the extensions in said sub directories.

Anyways, thats just for the record. It's your call and I will move my
extensions.

> 2. Rename to content_settings_extension_api (or content_settings_api if inside
> browser/extensions/).

um you mean extension_preference_extension_api? So
chrome/browser/extensions/preference/preference_extension_api.cc?

Would you prefer chrome/browser/extensions/types/types_extension_api.cc as this
is the namespace exposed to extensions? (see
http://code.google.com/chrome/extensions/trunk/types.html)

I am not sure whether "types" would make the code more discoverable. I tend to
prefer the "preference" path.

> 3. Remove extension_preference_api_constants*. No longer needed. These can be
in
> an anonymous namespace inside the cc file.

adding *_constants.* files was part of readability reviews, AFAIK. I wasn't a
big fan of that but it was considered clean style.

http://codereview.chromium.org/8662008/diff/12043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:56: const char*
kReadOnlyPrefs[] = {
On 2011/11/28 19:32:44, Aaron Boodman wrote:
> Does it really make sense to overload the content settings API to serve this
> different purpose?

We lose two things:
- subscription to changes (they can be triggered by policy changes)
- possibility to allow setting preference values in the future.

If this was a public API I'd probably stick to the current implementation. As it
is private and we can change all callers later in case we decide that we want to
allow extensions to either subscribe to or set the preference.

> It seems likely to cause coupling problems. And when it comes down to it the
> implementation of isIncognitoAvailable() is like one line. It seems harder to
> share it than it's worth.

[Bernhard Bauer, 2011-11-29 10:00:01]

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

http://codereview.chromium.org/8662008/diff/12043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
On 2011/11/28 19:32:44, Aaron Boodman wrote:
> Can you file one or more bugs for yourself to fix the following issues:
> 
> 1. Move extension_preference_api elsewhere. See crbug.com/101244. If there's
no
> home outside of chrome/browser/extensions that makes sense, you can make a
> subdir inside extensions.

If we want to move this outside of chrome/browser/extensions,
chrome/browser/prefs might make sense?

> 2. Rename to content_settings_extension_api (or content_settings_api if inside
> browser/extensions/).
> 
> 3. Remove extension_preference_api_constants*. No longer needed. These can be
in
> an anonymous namespace inside the cc file.

[Aaron Boodman, 2011-11-29 18:46:10]

On Tue, Nov 29, 2011 at 2:00 AM,  <bauerb@chromium.org> wrote:
>
>
http://codereview.chromium.org/8662008/diff/12043/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_preference_api.cc (right):
>
>
http://codereview.chromium.org/8662008/diff/12043/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_preference_api.cc:1: // Copyright
> (c) 2011 The Chromium Authors. All rights reserved.
> On 2011/11/28 19:32:44, Aaron Boodman wrote:
>>
>> Can you file one or more bugs for yourself to fix the following
>
> issues:
>
>> 1. Move extension_preference_api elsewhere. See crbug.com/101244. If
>
> there's no
>>
>> home outside of chrome/browser/extensions that makes sense, you can
>
> make a
>>
>> subdir inside extensions.
>
> If we want to move this outside of chrome/browser/extensions,
> chrome/browser/prefs might make sense?

Ah, I forgot that directoryified. Looking at the OWNERS, it seems
perfect to put it there.

- a

[Aaron Boodman, 2011-11-29 18:46:47]

On Mon, Nov 28, 2011 at 12:23 PM,  <battre@chromium.org> wrote:
> I will do that, though I am pretty sad seeing that extensions are moved
> out of chrome/browser/extensions. I'd have preferred if all extensions
> were moved to chrome/browser/extensions/${extension_api_name} as it used
> to be a very handy reference to look up how things are done in
> extensions-land by just peeking at other examples. Searching for other
> extensions in the code base seems more disadvantageous to me than having
> the extensions in said sub directories.

It is not that hard to search for subclasses of ExtensionFunction. And
I think it makes sense to have extensions APIs that are strongly
associated with a particular part of Chrome live in that area.

>> 2. Rename to content_settings_extension_api (or content_settings_api
>
> if inside
>>
>> browser/extensions/).
>
> um you mean extension_preference_extension_api? So
> chrome/browser/extensions/preference/preference_extension_api.cc?
>
> Would you prefer chrome/browser/extensions/types/types_extension_api.cc
> as this is the namespace exposed to extensions? (see
> http://code.google.com/chrome/extensions/trunk/types.html)
>
> I am not sure whether "types" would make the code more discoverable. I
> tend to prefer the "preference" path.

Nevermind, I was confused here. I didn't remember that this class
serves as a backend for multiple extension APIs.

>> 3. Remove extension_preference_api_constants*. No longer needed. These
>
> can be in
>>
>> an anonymous namespace inside the cc file.
>
> adding *_constants.* files was part of readability reviews, AFAIK. I
> wasn't a big fan of that but it was considered clean style.

It was needed in the past because of an internal project that has
since been discontinued. We can remove these _constants.* files now.

>
http://codereview.chromium.org/8662008/diff/12043/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_preference_api.cc:56: const char*
> kReadOnlyPrefs[] = {
> On 2011/11/28 19:32:44, Aaron Boodman wrote:
>>
>> Does it really make sense to overload the content settings API to
>
> serve this
>>
>> different purpose?
>
> We lose two things:
> - subscription to changes (they can be triggered by policy changes)
> - possibility to allow setting preference values in the future.
>
> If this was a public API I'd probably stick to the current
> implementation. As it is private and we can change all callers later in
> case we decide that we want to allow extensions to either subscribe to
> or set the preference.
>
>> It seems likely to cause coupling problems. And when it comes down to
>
> it the
>>
>> implementation of isIncognitoAvailable() is like one line. It seems
>
> harder to
>>
>> share it than it's worth.
>
> http://codereview.chromium.org/8662008/
>

[battre, 2011-12-05 17:34:21]

I have implemented a simple chrome.systemPrivate.getIncognitoModeAvailability
function as discussed and moved the code to chrome/browser/extensions/system/.

Please review.

Thanks,
Dominic

[Aaron Boodman, 2011-12-05 20:17:34]

lgtm thx

[commit-bot: I haz the power, 2011-12-06 10:06:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/8662008/21008

[commit-bot: I haz the power, 2011-12-06 10:06:56]

Can't apply patch for file
chrome/browser/extensions/extension_function_dispatcher.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/extension_function_dispatcher.cc
Hunk #1 succeeded at 50 with fuzz 1 (offset 1 line).
Hunk #2 FAILED at 476.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_dispatcher.cc.rej

[commit-bot: I haz the power, 2011-12-06 10:29:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/8662008/25001

[commit-bot: I haz the power, 2011-12-06 11:32:25]

Change committed as 113184