[Webkit-unassigned] [Bug 95346] Get rid of device/host clock synchronization.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=95346





--- Comment #2 from Peter Beverloo <peter at chromium.org>  2012-08-29 08:47:16 PST ---
(From update of attachment 161233)
View in context: https://bugs.webkit.org/attachment.cgi?id=161233&action=review

Thank you Philippe! A few more comments.

> Tools/ChangeLog:3
> +        Support LayoutTests on non-rooted devices for Chromium Android.

This should be equal to the bug's title. Maybe this would work?

Get rid of device/host clock synchronization for Chromium Android.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:44
> +    os.path.join(os.environ['CHROME_SRC'], 'build', 'android', 'pylib'))

Peter:
I'm not comfortable introducing this dependency. We're trying to get rid of dependencies on environment variable (like CHROME_SRC), and since we need adb commands in order parts of WebKit as well, I'd prefer to have a smaller but similar runner on the WebKit side. This is out of scope, but is there any way we can work around needing this right now?

Philippe:
I can avoid relying on $CHROME_SRC and use a relative path assuming that this file won't be moved. Regarding the use of android_commands, AndroidCommands.PushIfNeeded() provides a way to sync data files with the device by using a custom implementation of md5sum (written in C++ and depending on Chromium's base/). Although we could have our own Python layer for that in WebKit, we can hardly do the same for md5sum (I don't think you want to duplicate it either). So it seems that we would have to use Chromium's md5sum (being submitted under tools/android/md5sum) anyway.

Since we own that file, which is Chromium-specific, I think that preventing us from depending on Chromium might be an unnecessary constraint. I agree that we could introduce at least a thin layer in WebKit, calling Chromium's python, rather than calling it directly though.
But I'm sure that I don't see the full picture since this is my first WebKit contribution. What do you think? Is this dependency to Chromium the only one?

Peter:
The relative path would be different depending on whether layout tests are being run in a Chromium or WebKit environment (from WebKit's root: ../../build/android/pylib/ for Chromium versus Source/WebKit/chromium/build/android/pylib/ for WebKit). I don't think anything else in the layout tests runners depend on Chromium code right now, which also is the reason we have our own adb interface. Since we only need a subset of functionality here, and we need it for running other targets as well, eventually we'll probably end up with a script of our own.

Going back to this case: CHROME_SRC will not be available when running layout tests in a WebKit environment, which is my worry. If we can add the md5sum target as a dependency of DumpRenderTree, will that allow you to implement a similar PushIfNeeded method in the ChromiumAndroidDriver?

> LayoutTests/http/tests/cache/resources/current-time.cgi:5
> +print time()

nit: statements end with a semi-colon.

> LayoutTests/http/tests/cache/resources/subresource-test.js:28
> +var serverClientTimeDelta = getServerDate().getTime() - new Date().getTime();

Thanks! Very minor nit for readability: maybe define this property after the getServerDate() function?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.