| Summary: | Finish implementing start_helper for ChromiumAndroidPort | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||
| Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | dpranke, eric, jnd, ojan, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 78524 | ||||||||
| Attachments: |
|
||||||||
|
Description
Adam Barth
2012-02-13 23:44:46 PST
Created attachment 126917 [details]
Patch
Comment on attachment 126917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126917&action=review > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:315 > + except: > + # Reset to 1970-01-01 00:00:00 UTC. > + host_datetime = 0 I wonder if anyone has ever used this except: path... I doubt it works. I'm not sure having an except here is helpful. > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:326 > + def _run_adb_command(self, cmd, ignore_error=False): IT seems ignore_error=true is never used. Comment on attachment 126917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126917&action=review >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:315 >> + host_datetime = 0 > > I wonder if anyone has ever used this except: path... I doubt it works. I'm not sure having an except here is helpful. Another question is whether we should be using os.popen at all rather than using Executive. I'll probably switch it to executive before landing. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:326 >> + def _run_adb_command(self, cmd, ignore_error=False): > > IT seems ignore_error=true is never used. It's used on the very next line in the if condition. Comment on attachment 126917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126917&action=review Patch looks fine to me, just some editing of the comments ... > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:51 > +# FIXME: find a solution foe multi-core devices. Nit: "for", not "foe". > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:63 > +FORWARD_PORTS = '8000 8080 8443 8880 9323' These comments could use some grammatical editing as well. As an aside, I didn't realize 9323 was being used for anything ... > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:208 > + # Start the HTTP server for device to access test cases. Nit: s/for device to access/so the device can access the/. > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:309 > + # We need to make them synchronized, otherwise test may fail. Nit: s/test/tests/ (or 'a test'). Created attachment 127006 [details]
Patch for landing
Committed r107725: <http://trac.webkit.org/changeset/107725> |