| Summary: | chromium_android.py should implement "virtual" methods from ChromiumPort | ||||||
|---|---|---|---|---|---|---|---|
| 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 15:41:48 PST
Created attachment 126852 [details]
Patch
Comment on attachment 126852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126852&action=review > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:63 > + ['/usr/share/doc/ttf-mscorefonts-installer/', 'READ_ME!.gz'], Huh? > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:86 > + def default_child_processes(self): > + return 1 Why? Seems this may want to log? > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:89 > + def baseline_search_path(self): > + return map(self._webkit_baseline_path, self.FALLBACK_PATHS) I'm not sure if these are supposed to be absolute paths or not. > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:96 > + _log.error('You are missing %s. Try installing msttcorefonts. ' > + 'See build instructions.' % font_path) nit: No real need to wrap. > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:101 > + def check_sys_deps(self, needs_http): > + return True Wouldn't the font checks go here instead? > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:104 > + def default_worker_model(self): > + return 'inline' I wonder if this is right. Dirk would know. > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:111 > + # FIXME: This is a temporary measure to reduce the manual work when > + # updating WebKit. This method should be removed when we merge > + # test_expectations_android.txt into test_expectations.txt. We assume this hasn't been done yet? Comment on attachment 126852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126852&action=review There should probably be a chromium_unittest.py file as well; a skeleton that just subclasses port_testcase should be sufficient to get coverage. > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:71 > port_name = 'chromium-android' Are we going to be assuming that 'android' is a new "operating system" value (like linux/win/mac)? If not, 'chromium_android' might be a better name (much like we have google_chrome or chromium_gpu, to indicate that the implementation differs in ways other than just by operating system). > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:116 > + # This method is called by dump_render_tree_thread.py before testing Nit: there is no dump_render_tree_thread.py any more; presumably you mean manager.py, but you could probably omit this sentence completely and still get the point across, e.g., 'we use an http server that is always running during tests'. That said, I'm curious ... do chromium/android devs ever run these tests locally and/or interactively, and if so, how is the http server run then? > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:125 > + def start_helper(self): Nit: is this actually needed? if not, you could probably remove the FIXME and just note that it isn't. Comment on attachment 126852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126852&action=review >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:63 >> + ['/usr/share/doc/ttf-mscorefonts-installer/', 'READ_ME!.gz'], > > Huh? Presumably this is to check that you've accepted the EULA. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:86 >> + return 1 > > Why? Seems this may want to log? The tests run on the target device. Using multiple proceses in the host probably isn't helpful. We can add a comment to that effect if you like. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:89 >> + return map(self._webkit_baseline_path, self.FALLBACK_PATHS) > > I'm not sure if these are supposed to be absolute paths or not. This is the same idiom that the other ports use. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:96 >> + 'See build instructions.' % font_path) > > nit: No real need to wrap. Fixed. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:101 >> + return True > > Wouldn't the font checks go here instead? Yeah, I think we can move them here. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:104 >> + return 'inline' > > I wonder if this is right. Dirk would know. It just means that we don't use multiple threads. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:111 >> + # test_expectations_android.txt into test_expectations.txt. > > We assume this hasn't been done yet? Correct. (In reply to comment #3) > (From update of attachment 126852 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126852&action=review > > There should probably be a chromium_unittest.py file as well; a skeleton that just subclasses port_testcase should be sufficient to get coverage. Will do. > > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:71 > > port_name = 'chromium-android' > > Are we going to be assuming that 'android' is a new "operating system" value (like linux/win/mac)? If not, 'chromium_android' might be a better name (much like we have google_chrome or chromium_gpu, to indicate that the implementation differs in ways other than just by operating system). Android is a new operating system. It's PLATFORM(CHROMIUM) and OS(ANDROID). > > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:116 > > + # This method is called by dump_render_tree_thread.py before testing > > Nit: there is no dump_render_tree_thread.py any more; presumably you mean manager.py, but you could probably omit this sentence completely and still get the point across, e.g., 'we use an http server that is always running during tests'. Fixed. > That said, I'm curious ... do chromium/android devs ever run these tests locally and/or interactively, and if so, how is the http server run then? It's started in start_helper, which I haven't uploaded yet. > > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:125 > > + def start_helper(self): > > Nit: is this actually needed? if not, you could probably remove the FIXME and just note that it isn't. I'll fill it in in the next patch. (In reply to comment #4) > >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:86 > >> + return 1 > > > > Why? Seems this may want to log? > > The tests run on the target device. Using multiple proceses in the host probably isn't helpful. We can add a comment to that effect if you like. > One wonders if there could be some way to run multiple targets for a single host. > >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:89 > >> + return map(self._webkit_baseline_path, self.FALLBACK_PATHS) > > > > I'm not sure if these are supposed to be absolute paths or not. > > This is the same idiom that the other ports use. > webkit_baseline_path maps basenames to absolute paths. > >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:101 > >> + return True > > > > Wouldn't the font checks go here instead? > > Yeah, I think we can move them here. > Eric is right; this would be a slightly better place. > >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:104 > >> + return 'inline' > > > > I wonder if this is right. Dirk would know. > > It just means that we don't use multiple threads. > Not exactly. 'inline' actually uses a single python process for both manager and worker and hence means you *can't* run multiple threads; You could have 'processes' and still only use run one test at a time (this used to be useful when we had the wedge-detection code, but isn't really helpful anymore). (In reply to comment #5) > > Are we going to be assuming that 'android' is a new "operating system" value (like linux/win/mac)? If not, 'chromium_android' might be a better name (much like we have google_chrome or chromium_gpu, to indicate that the implementation differs in ways other than just by operating system). > > Android is a new operating system. It's PLATFORM(CHROMIUM) and OS(ANDROID). > I don't think I asked the right question; I was more thinking of the implications in the python code, which is more along the lines of the 'host' o/s rather than the 'target' o/s. First case where it matters: if you were to say "run-webkit-tests --chromium", normally we would figure out which version of the port to use based on the platforminfo value - which is the host value. Would we ever want/need a way for that to return 'android', or will we always expect the user/bot to use '--platform chromium-android instead'? Would it make sense to have a set-webkit-configuration setting to indicate that we should default to android instead of regular linux (or something)? The second case: what does port.operating_system() return? Here, I think the answer should clearly be 'android', just as a chromium-mac port will always return 'mac' regardless of what port it was actually created on. > First case where it matters: if you were to say "run-webkit-tests --chromium", normally we would figure out which version of the port to use based on the platforminfo value - which is the host value. Would we ever want/need a way for that to return 'android', or will we always expect the user/bot to use '--platform chromium-android instead'? Downstream has a hack where they look at some environment variables, but I think just passing --platform=chromium-android is better. > Would it make sense to have a set-webkit-configuration setting to indicate that we should default to android instead of regular linux (or something)? That sounds like an optimization that we can worry about later. > The second case: what does port.operating_system() return? Here, I think the answer should clearly be 'android', just as a chromium-mac port will always return 'mac' regardless of what port it was actually created on. Correct. > > There should probably be a chromium_unittest.py file as well; a skeleton that just subclasses port_testcase should be sufficient to get coverage.
>
> Will do.
There are some complications, which I'll handle in a follow-up patch.
Committed r107634: <http://trac.webkit.org/changeset/107634> Comment on attachment 126852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126852&action=review >>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:116 >>> + # This method is called by dump_render_tree_thread.py before testing >> >> Nit: there is no dump_render_tree_thread.py any more; presumably you mean manager.py, but you could probably omit this sentence completely and still get the point across, e.g., 'we use an http server that is always running during tests'. >> >> That said, I'm curious ... do chromium/android devs ever run these tests locally and/or interactively, and if so, how is the http server run then? > > Fixed. For chromium/android, the http server is running on the host, the DRT binary is running on the device, we actually forward the device ports (8000 and other ports for test) to the host to make it work. (In reply to comment #11) > (From update of attachment 126852 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126852&action=review > > >>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:116 > >>> + # This method is called by dump_render_tree_thread.py before testing > >> > >> Nit: there is no dump_render_tree_thread.py any more; presumably you mean manager.py, but you could probably omit this sentence completely and still get the point across, e.g., 'we use an http server that is always running during tests'. > >> > >> That said, I'm curious ... do chromium/android devs ever run these tests locally and/or interactively, and if so, how is the http server run then? > > > > Fixed. > > For chromium/android, the http server is running on the host, the DRT binary is running on the device, we actually forward the device ports (8000 and other ports for test) to the host to make it work. Normally new-run-webkit-tests starts and stops the http server on the host, so wouldn't you want to just use the default implementation, then? This code doesn't have to mean that you'd start and stop a server on the device itself ... (In reply to comment #12) > Normally new-run-webkit-tests starts and stops the http server on the host, so wouldn't you want to just use the default implementation, then? This code doesn't have to mean that you'd start and stop a server on the device itself ... For other platforms, the http server is running as required. (It's only launched for http/https test, at least last time I checked). However for Chromium/Android, due to the huge data size and slow speed to push the data to device, we actually leave all data on the host and launch the http server tp host the data during the whole test period. That means all data the DRT accesses are from the HTTP server on the host on layout tests for Android. For most tests which are assumed to access via file protocol, we have a feature called 'file over http' to convert the file request to http request and convert the http response to file response. (In reply to comment #13) > (In reply to comment #12) > > > Normally new-run-webkit-tests starts and stops the http server on the host, so wouldn't you want to just use the default implementation, then? This code doesn't have to mean that you'd start and stop a server on the device itself ... > > For other platforms, the http server is running as required. (It's only launched for http/https test, at least last time I checked). However for Chromium/Android, due to the huge data size and slow speed to push the data to device, we actually leave all data on the host and launch the http server tp host the data during the whole test period. That means all data the DRT accesses are from the HTTP server on the host on layout tests for Android. > For most tests which are assumed to access via file protocol, we have a feature called 'file over http' to convert the file request to http request and convert the http response to file response. That is why we bypass the default start/stop_http_server routines because we want to keep it during the whole test. (In reply to comment #13) > For other platforms, the http server is running as required. (It's only launched for http/https test, at least last time I checked). However for Chromium/Android, due to the huge data size and slow speed to push the data to device, we actually leave all data on the host and launch the http server tp host the data during the whole test period. That means all data the DRT accesses are from the HTTP server on the host on layout tests for Android. > For most tests which are assumed to access via file protocol, we have a feature called 'file over http' to convert the file request to http request and convert the http response to file response. Ah, gotcha. I've always imagined this would be how you would run tests on a target device :). Curious ... have you ever checked how many of the tests would fail if you just ran them over HTTP directly without the 'file over http' proxying? (In reply to comment #15) > Ah, gotcha. I've always imagined this would be how you would run tests on a target device :). Curious ... have you ever checked how many of the tests would fail if you just ran them over HTTP directly without the 'file over http' proxying? The tests which expect the file protocol (some of them explicitly check protocol scheme) are definitely failed. Besides that, less than hundred failed if I remembered correctly, forgot the detailed number now. But I think using 'file over http' on Chromium/Android can make it complied with other webkit ports |