Fix cmd line args override in common.py

tools/chrome_proxy/webdriver/common.py

 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import argparse
 import json
 import os
 import shlex
 import sys
 import time
@@ skipping 59 matching lines @@
 
   def __enter__(self):
     return self
 
   def __exit__(self, exc_type, exc_value, tb):
     if self._driver:
       self._StopDriver()
 
   def _OverrideChromeArgs(self):
     """
-    Overrides any given flags in the code with those given on the command line.
+    Overrides any given arguments in the code with those given on the command

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

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.

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

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.

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

SGTM.
That sounds fine then.

+    line. Arguments that need to be overridden may contain different values for
+    a flag given in the code. In that case, check by the flag whether to
+    override the argument.
     """
+    def GetDictKey(argument):
+      return argument.split('=', 1)[0]
     if self._flags.browser_args and len(self._flags.browser_args) > 0:
-      for arg in shlex.split(self._flags.browser_args):
-        self._chrome_args.add(arg)
+      # Build a dict of flags mapped to the whole argument.
+      originalArgs = {}

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

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

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

Done.

+      for arg in self._chrome_args:
+          originalArgs[GetDictKey(arg)] = arg
+      # Override by flag.
+      for overrideArg in shlex.split(self._flags.browser_args):

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

s/overrideArg/override_arg/

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

Done.

+        argKey = GetDictKey(overrideArg)

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

s/argKey/arg_key/

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

Done.

+        if argKey in originalArgs:
+          self._chrome_args.remove(originalArgs[argKey])
+        self._chrome_args.add(overrideArg)
 
   def _StartDriver(self):
     """
     Parses the flags to pass to Chromium, then starts the ChromeDriver.
     """
     options = Options()
     for arg in self._chrome_args:
       options.add_argument(arg)
     capabilities = {'loggingPrefs': {'performance': 'INFO'}}
     if self._flags.chrome_exec:
@@ skipping 109 matching lines @@
   def Fail(self, msg):
     sys.stderr.write("**************************************\n")
     sys.stderr.write("**************************************\n")
     sys.stderr.write("**                                  **\n")
     sys.stderr.write("**          TEST FAILURE            **\n")
     sys.stderr.write("**                                  **\n")
     sys.stderr.write("**************************************\n")
     sys.stderr.write("**************************************\n")
     sys.stderr.write(msg, '\n')
     sys.exit(1)