Move Proxy Settings API out of experimental

Move Proxy Settings API out of experimental

Please not that a couple of clean-ups will follow this. Consider this really stable once bug 60099 is labeled as fixed.

BUG=60099
TEST=no

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

[battre, 2011-05-24 13:56:44]

This is the next CL that moves the proxy settings API out of experimental.
Again, I am punting the documentation work to another CL to speedup the
pipeline.

@bauerb, mpcomplete: please review the CL
@mkwst: please review the changes to the sample extension

Thanks.

[Mike West, 2011-05-24 14:10:00]

Two nits, otherwise the sample extension LGTM.

http://codereview.chromium.org/6992022/diff/1/chrome/common/extensions/docs/e...
File
chrome/common/extensions/docs/examples/extensions/proxy_configuration/proxy_form_controller.js
(right):

http://codereview.chromium.org/6992022/diff/1/chrome/common/extensions/docs/e...
chrome/common/extensions/docs/examples/extensions/proxy_configuration/proxy_form_controller.js:106:
}
Nit: No braces for a one-line `if`.

http://codereview.chromium.org/6992022/diff/1/chrome/common/extensions/docs/e...
chrome/common/extensions/docs/examples/extensions/proxy_configuration/proxy_form_controller.js:531:
{value: this.config_.incognito, scope: 'TODO(battre)'},
Won't this throw an error since the value's not defined in `extension_api.json`?

[battre, 2011-05-24 14:42:30]

http://codereview.chromium.org/6992022/diff/1/chrome/common/extensions/docs/e...
File
chrome/common/extensions/docs/examples/extensions/proxy_configuration/proxy_form_controller.js
(right):

http://codereview.chromium.org/6992022/diff/1/chrome/common/extensions/docs/e...
chrome/common/extensions/docs/examples/extensions/proxy_configuration/proxy_form_controller.js:106:
}
On 2011/05/24 14:10:00, Mike West wrote:
> Nit: No braces for a one-line `if`.

Done.

http://codereview.chromium.org/6992022/diff/1/chrome/common/extensions/docs/e...
chrome/common/extensions/docs/examples/extensions/proxy_configuration/proxy_form_controller.js:531:
{value: this.config_.incognito, scope: 'TODO(battre)'},
On 2011/05/24 14:10:00, Mike West wrote:
> Won't this throw an error since the value's not defined in
`extension_api.json`?

It would, but this.config_.incognito is currently never true, so it is never
called. This will be fixed in one of the next CLs.

[Bernhard Bauer, 2011-05-24 15:13:38]

http://codereview.chromium.org/6992022/diff/1/chrome/common/extensions/docs/e...
File
chrome/common/extensions/docs/examples/extensions/proxy_configuration/proxy_form_controller.js
(right):

http://codereview.chromium.org/6992022/diff/1/chrome/common/extensions/docs/e...
chrome/common/extensions/docs/examples/extensions/proxy_configuration/proxy_form_controller.js:350:
chrome.proxy.settings.getEffective({incognito: false},
I think this change belongs into http://codereview.chromium.org/6990054/ (or a
separate CL), not here (same goes for other occurences of |getEffective|).

http://codereview.chromium.org/6992022/diff/1/chrome/common/extensions/docs/e...
chrome/common/extensions/docs/examples/extensions/proxy_configuration/proxy_form_controller.js:531:
{value: this.config_.incognito, scope: 'TODO(battre)'},
On 2011/05/24 14:42:31, battre wrote:
> On 2011/05/24 14:10:00, Mike West wrote:
> > Won't this throw an error since the value's not defined in
> `extension_api.json`?
> 
> It would, but this.config_.incognito is currently never true, so it is never
> called. This will be fixed in one of the next CLs.

Can you comment it out then?

[battre, 2011-05-24 17:19:03]

http://codereview.chromium.org/6992022/diff/1/chrome/common/extensions/docs/e...
File
chrome/common/extensions/docs/examples/extensions/proxy_configuration/proxy_form_controller.js
(right):

http://codereview.chromium.org/6992022/diff/1/chrome/common/extensions/docs/e...
chrome/common/extensions/docs/examples/extensions/proxy_configuration/proxy_form_controller.js:350:
chrome.proxy.settings.getEffective({incognito: false},
On 2011/05/24 15:13:38, Bernhard Bauer wrote:
> I think this change belongs into http://codereview.chromium.org/6990054/ (or a
> separate CL), not here (same goes for other occurences of |getEffective|).

Done.

[Bernhard Bauer, 2011-05-24 17:28:37]

LGTM with a nit:

http://codereview.chromium.org/6992022/diff/2003/chrome/common/extensions/doc...
File
chrome/common/extensions/docs/examples/extensions/proxy_configuration/proxy_form_controller.js
(right):

http://codereview.chromium.org/6992022/diff/2003/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/proxy_configuration/proxy_form_controller.js:530:
//     {value: this.config_.incognito, scope: 'TODO(battre)'},
Nit: I think you should change the scope value to "incognito" now.

[Matt Perry, 2011-05-24 18:15:29]

http://codereview.chromium.org/6992022/diff/2003/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6992022/diff/2003/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:5335: "incognito": {
This should now be an enum based on the decision from yesterday's meeting.
Either "no", "sessionOnly", or "persistent" (or some similar wording). And
"persistent" support can wait till next milestone.

http://codereview.chromium.org/6992022/diff/2003/chrome/common/extensions/doc...
File chrome/common/extensions/docs/static/proxy.html (right):

http://codereview.chromium.org/6992022/diff/2003/chrome/common/extensions/doc...
chrome/common/extensions/docs/static/proxy.html:251: {'incognito': false},
this sample code is out of date, right?

[battre, 2011-05-24 18:22:52]

http://codereview.chromium.org/6992022/diff/2003/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6992022/diff/2003/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:5335: "incognito": {
On 2011/05/24 18:15:30, Matt Perry wrote:
> This should now be an enum based on the decision from yesterday's meeting.
> Either "no", "sessionOnly", or "persistent" (or some similar wording). And
> "persistent" support can wait till next milestone.

I thought a lot about the parameters of the get method. I think this should not
be "no", "sessionOnly" or "persistent" if we are interested in the *effective*
value. There is exactly one value effective for the incognito profile.
Regardless whether this is "sessionOnly" or "persistent".

Note that this function is does not query the value that was set by the
extension (which I recommend to implement as a get() function, see other
discussion). This function retrieves the effective value.

http://codereview.chromium.org/6992022/diff/2003/chrome/common/extensions/doc...
File chrome/common/extensions/docs/static/proxy.html (right):

http://codereview.chromium.org/6992022/diff/2003/chrome/common/extensions/doc...
chrome/common/extensions/docs/static/proxy.html:251: {'incognito': false},
On 2011/05/24 18:15:30, Matt Perry wrote:
> this sample code is out of date, right?

in my opinion, getEffective should continue to have a true/false parameter (see
my other comment).

[Matt Perry, 2011-05-24 18:33:36]

LGTM

http://codereview.chromium.org/6992022/diff/2003/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6992022/diff/2003/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:5335: "incognito": {
On 2011/05/24 18:22:53, battre wrote:
> On 2011/05/24 18:15:30, Matt Perry wrote:
> > This should now be an enum based on the decision from yesterday's meeting.
> > Either "no", "sessionOnly", or "persistent" (or some similar wording). And
> > "persistent" support can wait till next milestone.
> 
> I thought a lot about the parameters of the get method. I think this should
not
> be "no", "sessionOnly" or "persistent" if we are interested in the *effective*
> value. There is exactly one value effective for the incognito profile.
> Regardless whether this is "sessionOnly" or "persistent".
> 
> Note that this function is does not query the value that was set by the
> extension (which I recommend to implement as a get() function, see other
> discussion). This function retrieves the effective value.

I see. That does make sense! Thanks for the explanation.