[Chromium]duplicated command line options in Android LayoutTest
Created attachment 156622 [details] Patch
Attachment 156622 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1 Tools/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 156626 [details] Patch
Comment on attachment 156626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156626&action=review Seems fine, but should we add a unit test for this? > Tools/ChangeLog:12 > + If there are multiple ChromiumAndroidPort instances, these two options > + will be appended for multiple times. Do we normally have multiple ChromiumAndroidPort instances? Does having these options multiple times cause anything to break?
(In reply to comment #4) > (From update of attachment 156626 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156626&action=review > > Seems fine, but should we add a unit test for this? > ok. I will add an unit test for it. > > Tools/ChangeLog:12 > > + If there are multiple ChromiumAndroidPort instances, these two options > > + will be appended for multiple times. > > Do we normally have multiple ChromiumAndroidPort instances? Does having these options multiple times cause anything to break? when I tried chromium android layout test, there are actually two ChromiumAndroidPort instances. So the command line contains two copies of "--encode-binary" and "--enable-hardware-gpu". I have not found anything broken by it but just feel odd to have multiple same options in the file chrome-native-tests-command-line.
Comment on attachment 156626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156626&action=review >>> Tools/ChangeLog:12 >>> + will be appended for multiple times. >> >> Do we normally have multiple ChromiumAndroidPort instances? Does having these options multiple times cause anything to break? > > when I tried chromium android layout test, there are actually two ChromiumAndroidPort instances. So the command line contains two copies of "--encode-binary" and "--enable-hardware-gpu". > > I have not found anything broken by it but just feel odd to have multiple same options in the file chrome-native-tests-command-line. Just curious, do you know where the two instances are created and why they share the same self._options.additional_drt_flags?
(In reply to comment #6) > (From update of attachment 156626 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156626&action=review > > >>> Tools/ChangeLog:12 > >>> + will be appended for multiple times. > >> > >> Do we normally have multiple ChromiumAndroidPort instances? Does having these options multiple times cause anything to break? > > > > when I tried chromium android layout test, there are actually two ChromiumAndroidPort instances. So the command line contains two copies of "--encode-binary" and "--enable-hardware-gpu". > > > > I have not found anything broken by it but just feel odd to have multiple same options in the file chrome-native-tests-command-line. > > Just curious, do you know where the two instances are created and why they share the same self._options.additional_drt_flags? the call stack of the first one: 1. run_webkit_tests.py line 468, in main port = host.port_factory.get(options.platform, options) 2. factory.py line 114 return cls(self._host, port_name, options=options, **kwargs) 3. chromium_android.py __init__ the call stack of the second one: 1. run_webkit_tests.py line 110, in run unexpected_result_count = manager.run(args) 2. manager.py line 418, in run self._run_tests(self._test_names, result_summary, int(self._options.child_processes)) 3. manager.py line 471, in _run_tests return self._runner.run_tests(test_inputs, self._expectations, result_summary, num_workers, needs_http, needs_websockets, self._retrying) 4. layout_test_runner.py line 140, in run_tests pool.run(('test_list', shard.name, shard.test_inputs) for shard in all_shards) 5. message_pool.py line 97, in run self.wait() 6. message_pool.py line 127, in wait self._workers[0].run() 7. message_pool.py line 267, in run self._raise(sys.exc_info()) 8. message_pool.py line 251, in run worker.start() 9. layout_test_runner.py line 293, in start self._port = self._host.port_factory.get(self._options.platform, self._options) 10. factory.py line 114, in get return cls(self._host, port_name, options=options, **kwargs) 11. chromium_android.py __init__ they have the same options argument from main() of run_webkit_tests.py.
will it make more sense to make the port instance singleton? that is, in factory.py, get() function can store the instance and return it when others call get() for the instance for the same port-name. but I don't know whether such chagne will break anything.
Another approach would be to not mutate options.additional_drt_flag. E.g., in driver.py's cmd_line, in addition to using _port.get_option('aditional_drt_flag', ...), we could add a _port.additional_drt_flag() method. Dirk, thoughts?
(In reply to comment #9) > Another approach would be to not mutate options.additional_drt_flag. E.g., in driver.py's cmd_line, in addition to using _port.get_option('aditional_drt_flag', ...), we could add a _port.additional_drt_flag() method. > > Dirk, thoughts? That might be slightly better. It's probably a flaw to be mutating the options argument in the port classes, but the assumption that we can do that pervades the code at the moment.
Created attachment 156848 [details] Patch
tony, I have modified the patch to use your approach and added an unit test. could you help to review the new patch? thanks
Comment on attachment 156848 [details] Patch Can you rebase your change against HEAD?
Created attachment 156858 [details] Patch
Comment on attachment 156858 [details] Patch Clearing flags on attachment: 156858 Committed r124851: <http://trac.webkit.org/changeset/124851>
All reviewed patches have been landed. Closing bug.