ProxyServer commandline flag for android webview

Move ProxyServer commandline switch to content layer and add support for it in android_webview.

BUG=639632
Committed: https://crrev.com/f544bdca1ead7127418455c5c0bb7e91a59cd153
Cr-Commit-Position: refs/heads/master@{#418424}

[sgurun-gerrit only, 2016-09-09 21:38:36]

sgurun@chromium.org changed reviewers:
+ sgurun@chromium.org

[sgurun-gerrit only, 2016-09-09 21:38:37]

is this ready for review?

when ready please add reviewers (see the OWNERS file) and also send a few try
jobs

thanks!

[Nate Fischer, 2016-09-09 22:01:41]

ntfschr@chromium.org changed reviewers:
+ hush@chromium.org

[hush (inactive), 2016-09-09 22:24:16]

https://codereview.chromium.org/2325763004/diff/20001/chrome/browser/net/prox...
File chrome/browser/net/proxy_browsertest.cc (right):

https://codereview.chromium.org/2325763004/diff/20001/chrome/browser/net/prox...
chrome/browser/net/proxy_browsertest.cc:24: #include
"content/public/common/content_switches.h"
Except for this line, are the changes in the file generated by "git cl format"?

https://codereview.chromium.org/2325763004/diff/20001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2325763004/diff/20001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:686: const char kNoProxyServer[]               
 = "no-proxy-server";
I think this is another commandline flag you need to move to content/ layer. 
Looks like proxy-server and no-proxy-server are a pair. But we probably need to
hear what the owner has to say.

[Nate Fischer, 2016-09-09 22:55:32]

https://codereview.chromium.org/2325763004/diff/20001/chrome/browser/net/prox...
File chrome/browser/net/proxy_browsertest.cc (right):

https://codereview.chromium.org/2325763004/diff/20001/chrome/browser/net/prox...
chrome/browser/net/proxy_browsertest.cc:24: #include
"content/public/common/content_switches.h"
On 2016/09/09 22:24:16, hush wrote:
> Except for this line, are the changes in the file generated by "git cl
format"?

Whoops! I think that was generated by "clang-format." I can revert the changes
though.

[sgurun-gerrit only, 2016-09-12 17:41:37]

On 2016/09/09 22:55:36, ntfschr1 wrote:

can you please update the description to be more detailed, i.e. move
proxy-server flag to content layer and add support for it to android_webview.
Also can you please add owners for files and get their reviews.

aw/ lgtm

[Nate Fischer, 2016-09-12 21:44:24]

Description was changed from

==========
ProxyServer commandline flag for android webview

BUG=639632
==========

to

==========
ProxyServer commandline flag for android webview

BUG=639632
==========

[Nate Fischer, 2016-09-12 21:44:24]

ntfschr@chromium.org changed reviewers:
+ bauerb@chromium.org, eroman@chromium.org, sievers@chromium.org

[Nate Fischer, 2016-09-12 21:47:47]

On 2016/09/12 17:41:37, sgurun wrote:
> On 2016/09/09 22:55:36, ntfschr1 wrote:
> 
> can you please update the description to be more detailed, i.e. move
> proxy-server flag to content layer and add support for it to android_webview.
> Also can you please add owners for files and get their reviews.
> 
> aw/ lgtm

bauerb@ I need a code review for chrome/browser/prefs/.

eroman@ I need a code review for chrome/browser/net/.

sievers@ I need a code review for content/public/common/.

Thanks.

[Nate Fischer, 2016-09-12 22:43:21]

Description was changed from

==========
ProxyServer commandline flag for android webview

BUG=639632
==========

to

==========
Move ProxyServer commandline switch to content layer and add support for it in
WebView.

BUG=639632
==========

[Nate Fischer, 2016-09-12 22:43:47]

Description was changed from

==========
Move ProxyServer commandline switch to content layer and add support for it in
WebView.

BUG=639632
==========

to

==========
Move ProxyServer commandline switch to content layer and add support for it in
android_webview.

BUG=639632
==========

[Bernhard Bauer, 2016-09-13 08:21:28]

prefs/ LGTM.

[no sievers, 2016-09-13 17:31:52]

content lgtm

[eroman, 2016-09-13 18:34:27]

I am not convinced that content/ is the right place for this flag.

It isn't in any way a part of the content implementation, or enforced at that
layer. It just happens to be a common dependency for chrome and android webview.

Moreover the other proxy related chrome flags like --no-proxy-server,
--proxy-auto-detect, --proxy-pac-url are not being enforced here.

My recommendation would be to simply duplicate the single --proxy-server for
Android Web View, rather than extract Chrome's switch to content.

But if //content owners are OK with having this in //content, then LGTM for the
functional changes so I don't block this CL.

[no sievers, 2016-09-13 18:38:57]

On 2016/09/13 18:34:27, eroman wrote:
> I am not convinced that content/ is the right place for this flag.
> 
> It isn't in any way a part of the content implementation, or enforced at that
> layer. It just happens to be a common dependency for chrome and android
webview.
> 
> Moreover the other proxy related chrome flags like --no-proxy-server,
> --proxy-auto-detect, --proxy-pac-url are not being enforced here.
> 
> My recommendation would be to simply duplicate the single --proxy-server for
> Android Web View, rather than extract Chrome's switch to content.
> 
> But if //content owners are OK with having this in //content, then LGTM for
the
> functional changes so I don't block this CL.

That's a valid point. Actually it looks like it's only used in android webview
and chrome tests?
That smells like something is weird here.

[Nate Fischer, 2016-09-13 18:44:42]

On 2016/09/13 18:38:57, sievers (slow) wrote:
> On 2016/09/13 18:34:27, eroman wrote:
> > I am not convinced that content/ is the right place for this flag.
> > 
> > It isn't in any way a part of the content implementation, or enforced at
that
> > layer. It just happens to be a common dependency for chrome and android
> webview.
> > 
> > Moreover the other proxy related chrome flags like --no-proxy-server,
> > --proxy-auto-detect, --proxy-pac-url are not being enforced here.
> > 
> > My recommendation would be to simply duplicate the single --proxy-server for
> > Android Web View, rather than extract Chrome's switch to content.
> > 
> > But if //content owners are OK with having this in //content, then LGTM for
> the
> > functional changes so I don't block this CL.
> 
> That's a valid point. Actually it looks like it's only used in android webview
> and chrome tests?
> That smells like something is weird here.

I don't mind just repeating the flag declaration and reverting the refactor. The
only issue I think is if both switches.h files get included, since that would
declare the flag twice. I could choose a different variable name, but that might
confuse people down the road.

Would it be better to refactor the other proxy-related switches as I did with
kProxyServer or to re-declare the flag? Thanks.

[eroman, 2016-09-13 18:57:01]

> That's a valid point. Actually it looks like it's only used in android webview
> and chrome tests?
> That smells like something is weird here.

Both the content and chrome flags dump into "switches" namespace, so that means
the other consumers of the flag, i.e.
https://cs.chromium.org/chromium/src/chrome/browser/prefs/command_line_pref_s...)
would keep compiling (just moved the storage not the name).

> Would it be better to refactor the other proxy-related switches as I did with
> kProxyServer or to re-declare the flag? Thanks.

The other flags can't really be made applicable to webview, since PAC is kind of
special on android. I would probably only support --proxy-server for WebView,
and maybe optionally --no-proxy-server too (although the latter isn't strictly
necessary since you can always do --proxy-server=direct://

> I don't mind just repeating the flag declaration and reverting the refactor.
The
> only issue I think is if both switches.h files get included, since that would
> declare the flag twice. I could choose a different variable name, but that
might
> confuse people down the road.

I was thinking you just make the flag definition local to
aw_url_request_context_getter.cc, maybe some variable called kProxyServerSwitch.
Not sure if this is idiomatic for android web view (perhaps there a central file
where it keeps any android-webview-specific switches, and where it would be
better to put it).

Conceptually I am viewing this as a web-view specific flag, that just happens to
use the same switch value (--proxy-server) as one of the existing chrome flags.

[sgurun-gerrit only, 2016-09-13 19:01:00]

On 2016/09/13 18:57:01, eroman wrote:
> > That's a valid point. Actually it looks like it's only used in android
webview
> > and chrome tests?
> > That smells like something is weird here.
> 
> Both the content and chrome flags dump into "switches" namespace, so that
means
> the other consumers of the flag, i.e.
>
https://cs.chromium.org/chromium/src/chrome/browser/prefs/command_line_pref_s...)
> would keep compiling (just moved the storage not the name).
> 
> > Would it be better to refactor the other proxy-related switches as I did
with
> > kProxyServer or to re-declare the flag? Thanks.
> 
> The other flags can't really be made applicable to webview, since PAC is kind
of
> special on android. I would probably only support --proxy-server for WebView,
> and maybe optionally --no-proxy-server too (although the latter isn't strictly
> necessary since you can always do --proxy-server=direct://

+1 let's limit this only to proxy-server

> 
> > I don't mind just repeating the flag declaration and reverting the refactor.
> The
> > only issue I think is if both switches.h files get included, since that
would
> > declare the flag twice. I could choose a different variable name, but that
> might
> > confuse people down the road.
> 
> I was thinking you just make the flag definition local to
> aw_url_request_context_getter.cc, maybe some variable called
kProxyServerSwitch.
> Not sure if this is idiomatic for android web view (perhaps there a central
file
> where it keeps any android-webview-specific switches, and where it would be
> better to put it).
> 
> Conceptually I am viewing this as a web-view specific flag, that just happens
to
> use the same switch value (--proxy-server) as one of the existing chrome
flags.

I think you can put the flag definition local to aw_url_request_context_getter.

[chromium-reviews, 2016-09-13 19:01:46]

Ok, I'll update accordingly.


Nate Fischer | Software Engineer | ntfschr@google.com |


On Tue, Sep 13, 2016 at 12:01 PM, <sgurun@chromium.org> wrote:

> On 2016/09/13 18:57:01, eroman wrote:
> > > That's a valid point. Actually it looks like it's only used in android
> webview
> > > and chrome tests?
> > > That smells like something is weird here.
> >
> > Both the content and chrome flags dump into "switches" namespace, so that
> means
> > the other consumers of the flag, i.e.
> >
> https://cs.chromium.org/chromium/src/chrome/browser/
> prefs/command_line_pref_store.cc?q=kProxyServer&sq=package:
> chromium&dr=C&l=103)
> > would keep compiling (just moved the storage not the name).
> >
> > > Would it be better to refactor the other proxy-related switches as I
> did
> with
> > > kProxyServer or to re-declare the flag? Thanks.
> >
> > The other flags can't really be made applicable to webview, since PAC is
> kind
> of
> > special on android. I would probably only support --proxy-server for
> WebView,
> > and maybe optionally --no-proxy-server too (although the latter isn't
> strictly
> > necessary since you can always do --proxy-server=direct://
>
> +1 let's limit this only to proxy-server
>
> >
> > > I don't mind just repeating the flag declaration and reverting the
> refactor.
> > The
> > > only issue I think is if both switches.h files get included, since that
> would
> > > declare the flag twice. I could choose a different variable name, but
> that
> > might
> > > confuse people down the road.
> >
> > I was thinking you just make the flag definition local to
> > aw_url_request_context_getter.cc, maybe some variable called
> kProxyServerSwitch.
> > Not sure if this is idiomatic for android web view (perhaps there a
> central
> file
> > where it keeps any android-webview-specific switches, and where it would
> be
> > better to put it).
> >
> > Conceptually I am viewing this as a web-view specific flag, that just
> happens
> to
> > use the same switch value (--proxy-server) as one of the existing chrome
> flags.
>
> I think you can put the flag definition local to
> aw_url_request_context_getter.
>
>
>
> https://codereview.chromium.org/2325763004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nate Fischer, 2016-09-13 20:18:50]

I've reverted the refactor, and just duplicated the flag definition in a local
constant.

sgurun@ let me know if there are any issues.

[eroman, 2016-09-13 22:34:42]

LGTM after removing the static initializer.

https://codereview.chromium.org/2325763004/diff/100001/android_webview/browse...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/2325763004/diff/100001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.cc:62: const
std::string kProxyServerSwitch = "proxy-server";
Write this instead as:

const char kProxyServerSwitch[] = "proxy-server";

(We aren't allowed to define global std::string like this since it adds static
initializers to the binary)

[sgurun-gerrit only, 2016-09-13 22:46:20]

On 2016/09/13 22:34:42, eroman wrote:
> LGTM after removing the static initializer.
> 
>
https://codereview.chromium.org/2325763004/diff/100001/android_webview/browse...
> File android_webview/browser/net/aw_url_request_context_getter.cc (right):
> 
>
https://codereview.chromium.org/2325763004/diff/100001/android_webview/browse...
> android_webview/browser/net/aw_url_request_context_getter.cc:62: const
> std::string kProxyServerSwitch = "proxy-server";
> Write this instead as:
> 
> const char kProxyServerSwitch[] = "proxy-server";
> 
> (We aren't allowed to define global std::string like this since it adds static
> initializers to the binary)

same as eroman. see code conventions here: 


https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

[Nate Fischer, 2016-09-13 22:53:54]

https://codereview.chromium.org/2325763004/diff/100001/android_webview/browse...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/2325763004/diff/100001/android_webview/browse...
android_webview/browser/net/aw_url_request_context_getter.cc:62: const
std::string kProxyServerSwitch = "proxy-server";
On 2016/09/13 22:34:42, eroman wrote:
> Write this instead as:
> 
> const char kProxyServerSwitch[] = "proxy-server";
> 
> (We aren't allowed to define global std::string like this since it adds static
> initializers to the binary)

Done. Good to know

[eroman, 2016-09-13 22:57:04]

patchset#7 LGTM.

Is there an easy unit-test that could be added ?

(I am fine without one for this particular change, or if it is done as a
follow-up that would be great too).

[Nate Fischer, 2016-09-13 23:05:44]

On 2016/09/13 22:57:04, eroman wrote:
> patchset#7 LGTM.
> 
> Is there an easy unit-test that could be added ?
> 
> (I am fine without one for this particular change, or if it is done as a
> follow-up that would be great too).

I couldn't find any other unit tests for this function, so adding unit tests
just for this might be more trouble than it's worth. Unless someone can point me
toward the unit tests, in which case I'm more than happy to give it a try.

[Nate Fischer, 2016-09-13 23:05:49]

On 2016/09/13 22:57:04, eroman wrote:
> patchset#7 LGTM.
> 
> Is there an easy unit-test that could be added ?
> 
> (I am fine without one for this particular change, or if it is done as a
> follow-up that would be great too).

I couldn't find any other unit tests for this function, so adding unit tests
just for this might be more trouble than it's worth. Unless someone can point me
toward the unit tests, in which case I'm more than happy to give it a try.

[sgurun-gerrit only, 2016-09-13 23:20:21]

On 2016/09/13 23:05:49, Nate Fischer wrote:
> On 2016/09/13 22:57:04, eroman wrote:
> > patchset#7 LGTM.
> > 
> > Is there an easy unit-test that could be added ?
> > 
> > (I am fine without one for this particular change, or if it is done as a
> > follow-up that would be great too).
> 
> I couldn't find any other unit tests for this function, so adding unit tests
> just for this might be more trouble than it's worth. Unless someone can point
me
> toward the unit tests, in which case I'm more than happy to give it a try.

this change is particularly for telemetry tests so we can assume it will be
(albeit indirectly) tested :)

[Nate Fischer, 2016-09-13 23:22:29]

The CQ bit was checked by ntfschr@chromium.org

[Nate Fischer, 2016-09-13 23:22:30]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
sgurun@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/2325763004/#ps120001
(title: "ProxyServer commandline flag for android webview")

[Nate Fischer, 2016-09-13 23:22:30]

The CQ bit was unchecked by ntfschr@chromium.org

[Nate Fischer, 2016-09-13 23:22:32]

The CQ bit was checked by ntfschr@chromium.org

[commit-bot: I haz the power, 2016-09-13 23:23:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 00:06:21]

Description was changed from

==========
Move ProxyServer commandline switch to content layer and add support for it in
android_webview.

BUG=639632
==========

to

==========
Move ProxyServer commandline switch to content layer and add support for it in
android_webview.

BUG=639632
==========

[commit-bot: I haz the power, 2016-09-14 00:06:22]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-14 00:09:09]

Description was changed from

==========
Move ProxyServer commandline switch to content layer and add support for it in
android_webview.

BUG=639632
==========

to

==========
Move ProxyServer commandline switch to content layer and add support for it in
android_webview.

BUG=639632

Committed: https://crrev.com/f544bdca1ead7127418455c5c0bb7e91a59cd153
Cr-Commit-Position: refs/heads/master@{#418424}
==========

[commit-bot: I haz the power, 2016-09-14 00:09:11]

Patchset 7 (id:??) landed as
https://crrev.com/f544bdca1ead7127418455c5c0bb7e91a59cd153
Cr-Commit-Position: refs/heads/master@{#418424}