Fix cmd line args override in common.py

Fix cmd line args override in common.py

In certain cases, it is important to be able to specify different command line arguments to chrome at the time the test is being run rather than what is in code. For example, running against a different proxy than is in the code.
This CL turns on the handling for this functionality and fixes a few bugs with it.

BUG=669956
Committed: https://crrev.com/ec1ab21b1df2c867a746ab50261ea02aad4256bc
Cr-Commit-Position: refs/heads/master@{#435525}

[Robert Ogden, 2016-11-30 17:19:02]

robertogden@chromium.org changed reviewers:
+ tbansal@chromium.org

[tbansal1, 2016-11-30 17:26:09]

tbansal@chromium.org changed reviewers:
+ ryansturm@chromium.org

[tbansal1, 2016-11-30 17:26:10]

Removing myself, and adding ryansturm@ as the reviewer.

Just a quick comment: It might be useful to file  a tracking bug in crbug, and
use that bug in all the webdriver CLs. That would make it easier to track all
the related CLs in a single place.

[Robert Ogden, 2016-11-30 17:29:39]

Description was changed from

==========
Fix cmd line args override in common.py

BUG=
==========

to

==========
Fix cmd line args override in common.py

BUG=669956
==========

[Robert Ogden, 2016-11-30 17:29:55]

https://bugs.chromium.org/p/chromium/issues/detail?id=669956

[RyanSturm, 2016-11-30 20:46:36]

https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive...
File tools/chrome_proxy/webdriver/common.py (right):

https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/common.py:80: Overrides any given arguments in the
code with those given on the command
I'm a little confused about how this is used. Can you add a better CL
description?

The way I understand this:
Tests will be written with various flags using AddChromeArg within the code, but
if the test is run with a command line override, the value of the flag will that
is used is the one from the command line.

Let me know if that's wrong.

Assuming that is correct, it might be a little more clear to parse
self._flags.browser_args into a seperate member dict during init, and in the
Add/Remove calls only add the new arg if it's key is not in the dict.

The reason I suggest this, is I don't understand when _OverrdieChromeArgs would
be called and when AddChromeArgs would be called.

https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/common.py:89: originalArgs = {}
s/originalArgs/original_args/

AFAIK naming for python in chromium uses
https://google.github.io/styleguide/pyguide.html?showone=Naming#Naming

except for a couple cases (specifically your method names are correct for
chromium with CamelCase):

https://chromium.googlesource.com/chromium/src/+/master/styleguide/styleguide.md

https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/common.py:93: for overrideArg in
shlex.split(self._flags.browser_args):
s/overrideArg/override_arg/

https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/common.py:94: argKey = GetDictKey(overrideArg)
s/argKey/arg_key/

[Robert Ogden, 2016-12-01 01:10:22]

Description was changed from

==========
Fix cmd line args override in common.py

BUG=669956
==========

to

==========
Fix cmd line args override in common.py

In certain cases, it is important to be able to specify different command line
arguments to chrome at the time the test is being run rather than what is in
code. For example, running against a different proxy than is in the code. 
This CL turns on the handling for this functionality and fixes a few bugs with
it.

BUG=669956
==========

[Robert Ogden, 2016-12-01 01:23:04]

https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive...
File tools/chrome_proxy/webdriver/common.py (right):

https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/common.py:80: Overrides any given arguments in the
code with those given on the command
On 2016/11/30 20:46:36, Ryan Sturm wrote:
> I'm a little confused about how this is used. Can you add a better CL
> description?
> 
> The way I understand this:
> Tests will be written with various flags using AddChromeArg within the code,
but
> if the test is run with a command line override, the value of the flag will
that
> is used is the one from the command line.
> 
> Let me know if that's wrong.
> 
> Assuming that is correct, it might be a little more clear to parse
> self._flags.browser_args into a seperate member dict during init, and in the
> Add/Remove calls only add the new arg if it's key is not in the dict.
> 
> The reason I suggest this, is I don't understand when _OverrdieChromeArgs
would
> be called and when AddChromeArgs would be called.

CL description improved, but yes you were correct.

Do you mean turn _chrome_args into a dict? Filling that dict on init and then
not letting calls to AddChromeArg would prevent client code from modifying
existing flags, so flags couldn't be removed for example. Maybe that's ok, but
it gives some weird behavior that I don't think would be expected.
If not, then as it is _OverrideChromeArgs is called on _StartDriver so anything
the client code did gets wiped away at the very last second before Chrome
starts.

https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/common.py:89: originalArgs = {}
On 2016/11/30 20:46:36, Ryan Sturm wrote:
> s/originalArgs/original_args/
> 
> AFAIK naming for python in chromium uses
> https://google.github.io/styleguide/pyguide.html?showone=Naming#Naming
> 
> except for a couple cases (specifically your method names are correct for
> chromium with CamelCase):
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/styleguide.md

Done.

https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/common.py:93: for overrideArg in
shlex.split(self._flags.browser_args):
On 2016/11/30 20:46:36, Ryan Sturm wrote:
> s/overrideArg/override_arg/

Done.

https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/common.py:94: argKey = GetDictKey(overrideArg)
On 2016/11/30 20:46:36, Ryan Sturm wrote:
> s/argKey/arg_key/

Done.

[RyanSturm, 2016-12-01 01:32:11]

lgtm

https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive...
File tools/chrome_proxy/webdriver/common.py (right):

https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/common.py:80: Overrides any given arguments in the
code with those given on the command
On 2016/12/01 01:23:04, Robert Ogden wrote:
> On 2016/11/30 20:46:36, Ryan Sturm wrote:
> > I'm a little confused about how this is used. Can you add a better CL
> > description?
> > 
> > The way I understand this:
> > Tests will be written with various flags using AddChromeArg within the code,
> but
> > if the test is run with a command line override, the value of the flag will
> that
> > is used is the one from the command line.
> > 
> > Let me know if that's wrong.
> > 
> > Assuming that is correct, it might be a little more clear to parse
> > self._flags.browser_args into a seperate member dict during init, and in the
> > Add/Remove calls only add the new arg if it's key is not in the dict.
> > 
> > The reason I suggest this, is I don't understand when _OverrdieChromeArgs
> would
> > be called and when AddChromeArgs would be called.
> 
> CL description improved, but yes you were correct.
> 

SGTM.

> Do you mean turn _chrome_args into a dict? Filling that dict on init and then
> not letting calls to AddChromeArg would prevent client code from modifying
> existing flags, so flags couldn't be removed for example. Maybe that's ok, but
> it gives some weird behavior that I don't think would be expected.
> If not, then as it is _OverrideChromeArgs is called on _StartDriver so
anything
> the client code did gets wiped away at the very last second before Chrome
> starts.

That sounds fine then.

[Robert Ogden, 2016-12-01 01:44:37]

The CQ bit was checked by robertogden@chromium.org

[commit-bot: I haz the power, 2016-12-01 01:44:57]

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-12-01 01:53:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480556677683950,
"parent_rev": "e1e2e582d3fce9faa84b8debacf173b2f63085ba", "commit_rev":
"551d8781f84dc7e32c8a6a7fdf622f235dc50543"}

[commit-bot: I haz the power, 2016-12-01 01:53:49]

Description was changed from

==========
Fix cmd line args override in common.py

In certain cases, it is important to be able to specify different command line
arguments to chrome at the time the test is being run rather than what is in
code. For example, running against a different proxy than is in the code. 
This CL turns on the handling for this functionality and fixes a few bugs with
it.

BUG=669956
==========

to

==========
Fix cmd line args override in common.py

In certain cases, it is important to be able to specify different command line
arguments to chrome at the time the test is being run rather than what is in
code. For example, running against a different proxy than is in the code. 
This CL turns on the handling for this functionality and fixes a few bugs with
it.

BUG=669956
==========

[commit-bot: I haz the power, 2016-12-01 01:53:49]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-01 01:57:04]

Description was changed from

==========
Fix cmd line args override in common.py

In certain cases, it is important to be able to specify different command line
arguments to chrome at the time the test is being run rather than what is in
code. For example, running against a different proxy than is in the code. 
This CL turns on the handling for this functionality and fixes a few bugs with
it.

BUG=669956
==========

to

==========
Fix cmd line args override in common.py

In certain cases, it is important to be able to specify different command line
arguments to chrome at the time the test is being run rather than what is in
code. For example, running against a different proxy than is in the code.
This CL turns on the handling for this functionality and fixes a few bugs with
it.

BUG=669956

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

[commit-bot: I haz the power, 2016-12-01 01:57:04]

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