Implementation of getCurrentProxySettings for proxy settings API.

Implementation of getCurrentProxySettings for proxy settings API.


BUG=71666
TEST=none

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

[battre, 2011-02-09 18:20:48]

Please review. Thanks!

[Bernhard Bauer, 2011-02-10 12:38:01]

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

http://codereview.chromium.org/6468002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_proxy_api.cc:261: if
(profile_->IsOffTheRecord() && !incognito) {
Does this really happen? I thought |profile_| is always the regular one.

http://codereview.chromium.org/6468002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_proxy_api.cc:374: // dictionary.
You could just refer to the ProxyServer type in |experimental.proxy|, which has
autogenerated documentation. That way, it's always current.

[battre, 2011-02-10 13:42:36]

Thanks. I have addressed your comments.

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

http://codereview.chromium.org/6468002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_proxy_api.cc:261: if
(profile_->IsOffTheRecord() && !incognito) {
On 2011/02/10 12:38:01, Bernhard Bauer wrote:
> Does this really happen? I thought |profile_| is always the regular one.

I have added all access to profile_ as follows

  Profile* profile = profile();
  if (profile->IsOffTheRecord())
    profile = profile->GetOriginalProfile();

and added a TODO to ask mpcomplete to review this code w.r.t. the
include_incognito flag.

http://codereview.chromium.org/6468002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_proxy_api.cc:374: // dictionary.
On 2011/02/10 12:38:01, Bernhard Bauer wrote:
> You could just refer to the ProxyServer type in |experimental.proxy|, which
has
> autogenerated documentation. That way, it's always current.

Deleted the comment and added the reference in the header file.

Done.

[battre, 2011-02-10 13:57:07]

Hi Matt,

could you please have a look at this file
http://codereview.chromium.org/6450006/diff/10001/chrome/browser/extensions/e...
(the entire file, not just the green parts)?

We are not quite sure how to handle the include_incognito flag correctly. - In
particular what happens in case of the split incognito mode.

I think the desired behavior is that an extension can modify the proxy settings
for regular and incognito windows - regardless whether the include_incognito
flag is activated(?).

Best regards,
Dominic

[Bernhard Bauer, 2011-02-10 16:00:51]

LGTM.

[Matt Perry, 2011-02-10 22:12:18]

On 2011/02/10 13:57:07, battre wrote:
> Hi Matt,
> 
> could you please have a look at this file
>
http://codereview.chromium.org/6450006/diff/10001/chrome/browser/extensions/e...
> (the entire file, not just the green parts)?
> 
> We are not quite sure how to handle the include_incognito flag correctly. - In
> particular what happens in case of the split incognito mode.

The general idea is that if include_incognito is true, then we should allow
extensions to cross profiles (ie, regular profile can receive events about
updates to the incognito profile). If it is false, then we should ensure that
the profile of the extension process matches the profile of whatever thing is
happening (ie, if profileA calls tabs.update in profileB, don't allow it).
Perhaps allow_cross_profile would have been a better name.

Also note that the only time include_incognito will be true is if the user
checked "Allow in incognito" and the extension uses spanning (not split)
incognito mode.

> I think the desired behavior is that an extension can modify the proxy
settings
> for regular and incognito windows - regardless whether the include_incognito
> flag is activated(?).

I don't feel strongly about it, but I think that the extension should only be
able to modify the incognito proxy if it was allowed in incognito. If you agree,
then you should use the strategy I outlined above. (check for matching
profiles).

[battre, 2011-02-10 23:11:22]

> > We are not quite sure how to handle the include_incognito flag correctly. -
In
> > particular what happens in case of the split incognito mode.
> 
> The general idea is that if include_incognito is true, then we should allow
> extensions to cross profiles (ie, regular profile can receive events about
> updates to the incognito profile). If it is false, then we should ensure that
> the profile of the extension process matches the profile of whatever thing is
> happening (ie, if profileA calls tabs.update in profileB, don't allow it).
> Perhaps allow_cross_profile would have been a better name.

What determines the profile an extension is running in? I can imagine that a
proxy extension has either only a options link on chrome://extensions/ or maybe
a browser action button in the toolbar. It will also have a background page.

> Also note that the only time include_incognito will be true is if the user
> checked "Allow in incognito" and the extension uses spanning (not split)
> incognito mode.

Hm, I am not familiar with the concepts behind this. Can you point me to an
example that shows how to protect access correctly? I am looking at the
extension_context_menu_api.cc for example right now. I don't see any place that
differentiates between regular and incognito windows. The same holds for
extension_bookmark_manager_api.cc

> > I think the desired behavior is that an extension can modify the proxy
> settings
> > for regular and incognito windows - regardless whether the include_incognito
> > flag is activated(?).
> 
> I don't feel strongly about it, but I think that the extension should only be
> able to modify the incognito proxy if it was allowed in incognito. If you
agree,
> then you should use the strategy I outlined above. (check for matching
> profiles).

It would be great if you could point me to an example where this has been done
already.

We currently have the following semantics for setting proxy settings

clean start()
setCustomProxySettings(A, incognito=false);
setCustomProxySettings(B, incognito=true);
--> A will be used for original profile, B for incognito profile

clean start()
setCustomProxySettings(A, incognito=false);
--> A will be used for original profile and for incognito profile

clean start()
setCustomProxySettings(B, incognito=true);
--> B will be used for incognito profile, A remains at the default (e.g. system
settings provided by the OS)

Would you disallow a call of setCustomProxySettings(*, incognito=true) if the
"Allow in incognito" check box is not checked?

Or would you also disable the inheritance shown in the second example?

My opinion was that calling setCustomProxySettings(*, incognito=false) should be
equivalent to changing the system settings --> by default is applies to
everything.

Thanks,
Dominic

[Matt Perry, 2011-02-11 00:06:52]

On 2011/02/10 23:11:22, battre wrote:
> > > We are not quite sure how to handle the include_incognito flag correctly.
-
> In
> > > particular what happens in case of the split incognito mode.
> > 
> > The general idea is that if include_incognito is true, then we should allow
> > extensions to cross profiles (ie, regular profile can receive events about
> > updates to the incognito profile). If it is false, then we should ensure
that
> > the profile of the extension process matches the profile of whatever thing
is
> > happening (ie, if profileA calls tabs.update in profileB, don't allow it).
> > Perhaps allow_cross_profile would have been a better name.
> 
> What determines the profile an extension is running in? I can imagine that a
> proxy extension has either only a options link on chrome://extensions/ or
maybe
> a browser action button in the toolbar. It will also have a background page.

Generally, an extension will be running in the normal profile. If an extension
uses "split" incognito mode, then a separate process will load for incognito
windows. Then there will be 2 copies of the extension running: one for the
normal profile, one for incognito.

> > Also note that the only time include_incognito will be true is if the user
> > checked "Allow in incognito" and the extension uses spanning (not split)
> > incognito mode.
> 
> Hm, I am not familiar with the concepts behind this. Can you point me to an
> example that shows how to protect access correctly? I am looking at the
> extension_context_menu_api.cc for example right now. I don't see any place
that
> differentiates between regular and incognito windows. The same holds for
> extension_bookmark_manager_api.cc

Take a look at extension_tabs_module.cc and search for include_incognito. This
might also help explain the concepts behind the 2 incognito modes:
http://code.google.com/chrome/extensions/manifest.html#incognito

> 
> > > I think the desired behavior is that an extension can modify the proxy
> > settings
> > > for regular and incognito windows - regardless whether the
include_incognito
> > > flag is activated(?).
> > 
> > I don't feel strongly about it, but I think that the extension should only
be
> > able to modify the incognito proxy if it was allowed in incognito. If you
> agree,
> > then you should use the strategy I outlined above. (check for matching
> > profiles).
> 
> It would be great if you could point me to an example where this has been done
> already.
> 
> We currently have the following semantics for setting proxy settings
> 
> clean start()
> setCustomProxySettings(A, incognito=false);
> setCustomProxySettings(B, incognito=true);
> --> A will be used for original profile, B for incognito profile
> 
> clean start()
> setCustomProxySettings(A, incognito=false);
> --> A will be used for original profile and for incognito profile
> 
> clean start()
> setCustomProxySettings(B, incognito=true);
> --> B will be used for incognito profile, A remains at the default (e.g.
system
> settings provided by the OS)
> 
> Would you disallow a call of setCustomProxySettings(*, incognito=true) if the
> "Allow in incognito" check box is not checked?

That would be my preference. "split" mode introduces a more subtle distinction,
though. A split-mode extension would have 2 running background page. If the
incognito bg page calls setCustomProxySettings(settings), it should probably
change the proxy for incognito mode.

Also, looking at the API in this way, it seems a bit confusing. Maybe
setCustomProxySettings should have a second parameter that says where it
applies, which defaults to everywhere. Like

setCustomProxySettings(A, {incognito: true});  // incognito only
setCustomProxySettings(A, {normal: true});  // normal only
setCustomProxySettings(A);  // both (or maybe, "just this window's profile")

What do you think?

[battre, 2011-02-11 12:00:27]

> > > > I think the desired behavior is that an extension can modify the proxy
> > > settings
> > > > for regular and incognito windows - regardless whether the
> include_incognito
> > > > flag is activated(?).
> > > 
> > > I don't feel strongly about it, but I think that the extension should only
> be
> > > able to modify the incognito proxy if it was allowed in incognito. If you
> > agree,
> > > then you should use the strategy I outlined above. (check for matching
> > > profiles).
> > 
> > It would be great if you could point me to an example where this has been
done
> > already.
> > 
> > We currently have the following semantics for setting proxy settings
> > 
> > clean start()
> > setCustomProxySettings(A, incognito=false);
> > setCustomProxySettings(B, incognito=true);
> > --> A will be used for original profile, B for incognito profile
> > 
> > clean start()
> > setCustomProxySettings(A, incognito=false);
> > --> A will be used for original profile and for incognito profile
> > 
> > clean start()
> > setCustomProxySettings(B, incognito=true);
> > --> B will be used for incognito profile, A remains at the default (e.g.
> system
> > settings provided by the OS)
> > 
> > Would you disallow a call of setCustomProxySettings(*, incognito=true) if
the
> > "Allow in incognito" check box is not checked?
> 
> That would be my preference. "split" mode introduces a more subtle
distinction,
> though. A split-mode extension would have 2 running background page. If the
> incognito bg page calls setCustomProxySettings(settings), it should probably
> change the proxy for incognito mode.

I think if the incognito bg page in split mode calls setCustomProxySettings, we
can expect it to pass incognito=true and report a violation otherwise.

> Also, looking at the API in this way, it seems a bit confusing. Maybe
> setCustomProxySettings should have a second parameter that says where it
> applies, which defaults to everywhere. Like
> 
> setCustomProxySettings(A, {incognito: true});  // incognito only
> setCustomProxySettings(A, {normal: true});  // normal only
> setCustomProxySettings(A);  // both (or maybe, "just this window's profile")
> 
> What do you think?

I discussed this with Bernhard. We generally like the copy-on-write character of
the incognito mode. You also inherit cookies like you would inherit proxy
settings.

In this case we would stick to the semantic described above for regular bg pages
in split mode:

> clean start()
> setCustomProxySettings(A, incognito=false);
> setCustomProxySettings(B, incognito=true);
> --> A will be used for original profile, B for incognito profile
> 
> clean start()
> setCustomProxySettings(A, incognito=false);
> --> A will be used for original profile and for incognito profile
> 
> clean start()
> setCustomProxySettings(B, incognito=true);
> --> B will be used for incognito profile, A remains at the default 
> (e.g. system settings provided by the OS)

And use the following for incognito bg pages:

clean start()
setCustomProxySettings(A, incognito=false); --> not allowed
setCustomProxySettings(B, incognito=true);
--> B will be used for incognito profile

clean start()
setCustomProxySettings(A, incognito=false); --> not allowed

clean start()
setCustomProxySettings(B, incognito=true);
--> B will be used for incognito profile

Anyway... I think we should defer this from this CL and discuss it in a larger
scope. Bernhard is working on an API for Content Settings and I want to stick to
his API as closely as possible to have consistent APIs.

If you don't object, I would skip this topic for this CL.

[Bernhard Bauer, 2011-02-11 12:06:39]

On Fri, Feb 11, 2011 at 13:00,  <battre@chromium.org> wrote:
>> > > > I think the desired behavior is that an extension can modify the
>> > > > proxy
>> > > settings
>> > > > for regular and incognito windows - regardless whether the
>> include_incognito
>> > > > flag is activated(?).
>> > >
>> > > I don't feel strongly about it, but I think that the extension should
>> > > only
>> be
>> > > able to modify the incognito proxy if it was allowed in incognito. If
>> > > you
>> > agree,
>> > > then you should use the strategy I outlined above. (check for matching
>> > > profiles).
>> >
>> > It would be great if you could point me to an example where this has
>> > been
>
> done
>>
>> > already.
>> >
>> > We currently have the following semantics for setting proxy settings
>> >
>> > clean start()
>> > setCustomProxySettings(A, incognito=false);
>> > setCustomProxySettings(B, incognito=true);
>> > --> A will be used for original profile, B for incognito profile
>> >
>> > clean start()
>> > setCustomProxySettings(A, incognito=false);
>> > --> A will be used for original profile and for incognito profile
>> >
>> > clean start()
>> > setCustomProxySettings(B, incognito=true);
>> > --> B will be used for incognito profile, A remains at the default (e.g.
>> system
>> > settings provided by the OS)
>> >
>> > Would you disallow a call of setCustomProxySettings(*, incognito=true)
>> > if
>
> the
>>
>> > "Allow in incognito" check box is not checked?
>
>> That would be my preference. "split" mode introduces a more subtle
>
> distinction,
>>
>> though. A split-mode extension would have 2 running background page. If
>> the
>> incognito bg page calls setCustomProxySettings(settings), it should
>> probably
>> change the proxy for incognito mode.
>
> I think if the incognito bg page in split mode calls setCustomProxySettings,
> we
> can expect it to pass incognito=true and report a violation otherwise.
>
>> Also, looking at the API in this way, it seems a bit confusing. Maybe
>> setCustomProxySettings should have a second parameter that says where it
>> applies, which defaults to everywhere. Like
>
>> setCustomProxySettings(A, {incognito: true});  // incognito only
>> setCustomProxySettings(A, {normal: true});  // normal only
>> setCustomProxySettings(A);  // both (or maybe, "just this window's
>> profile")
>
>> What do you think?
>
> I discussed this with Bernhard. We generally like the copy-on-write
> character of
> the incognito mode. You also inherit cookies like you would inherit proxy
> settings.
>
> In this case we would stick to the semantic described above for regular bg
> pages
> in split mode:
>
>> clean start()
>> setCustomProxySettings(A, incognito=false);
>> setCustomProxySettings(B, incognito=true);
>> --> A will be used for original profile, B for incognito profile
>
>> clean start()
>> setCustomProxySettings(A, incognito=false);
>> --> A will be used for original profile and for incognito profile
>
>> clean start()
>> setCustomProxySettings(B, incognito=true);
>> --> B will be used for incognito profile, A remains at the default
>> (e.g. system settings provided by the OS)
>
> And use the following for incognito bg pages:
>
> clean start()
> setCustomProxySettings(A, incognito=false); --> not allowed
> setCustomProxySettings(B, incognito=true);
> --> B will be used for incognito profile
>
> clean start()
> setCustomProxySettings(A, incognito=false); --> not allowed
>
> clean start()
> setCustomProxySettings(B, incognito=true);
> --> B will be used for incognito profile
>
> Anyway... I think we should defer this from this CL and discuss it in a
> larger
> scope. Bernhard is working on an API for Content Settings and I want to
> stick to
> his API as closely as possible to have consistent APIs.

To be exact, I'm working on a more general mechanism for extension
APIs to have consistent access to preferences, so we don't have to
solve the same problems every time we implement an API that allows
preference access.

Bernhard.

> If you don't object, I would skip this topic for this CL.
>
> http://codereview.chromium.org/6468002/
>

[Matt Perry, 2011-02-11 19:36:14]

On 2011/02/11 12:00:27, battre wrote:
> > > Would you disallow a call of setCustomProxySettings(*, incognito=true) if
> the
> > > "Allow in incognito" check box is not checked?
> > 
> > That would be my preference. "split" mode introduces a more subtle
> distinction,
> > though. A split-mode extension would have 2 running background page. If the
> > incognito bg page calls setCustomProxySettings(settings), it should probably
> > change the proxy for incognito mode.
> 
> I think if the incognito bg page in split mode calls setCustomProxySettings,
we
> can expect it to pass incognito=true and report a violation otherwise.

One of the goals of "split" mode was that the extension would not have to worry
about which profile its process was running in. Things would just work for that
profile.

> Anyway... I think we should defer this from this CL and discuss it in a larger
> scope. Bernhard is working on an API for Content Settings and I want to stick
to
> his API as closely as possible to have consistent APIs.
> 
> If you don't object, I would skip this topic for this CL.

Sure, that's fine with me.