[Webkit-unassigned] [Bug 91872] [Chromium-Android] Run layout tests on unrooted devices

[bugzilla-daemon at webkit.org]

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





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

Thanks, Philippe! Some comments in-line..

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

Since this also touches a non-Chromium for Android specific test, I wouldn't use the [Chromium-Android] prefix here and append "for Chromium Android" to the title.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:410
> +                                 'Please pass --%s.' % (configuration, most_recent_binary, most_recent_binary.lower()))

nit: The warning() method accepts multiple arguments for formatting, we should probably prefer that over string formatting.

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

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?

> LayoutTests/ChangeLog:5
> +        http://crbug.com/143114

nit: it's uncommon to for Chromium to mention our own bug tracker, but it's harmless too. In general, you could use the URL field in the bug for this.

> LayoutTests/http/tests/cache/resources/cache-simulator.cgi:-6
> -print "Content-type: text/javascript\n"; 

I'd avoid touching this file if you're just removing a space.

> LayoutTests/http/tests/cache/resources/subresource-test.js:31
> +    var url = document.URL.replace(/subresource-expiration-..html/, "resources/current-time.cgi");

This seems rather fragile, can we rely on relative paths to work? Requesting "resources/current-time.cgi" should work fine.

> LayoutTests/http/tests/cache/resources/subresource-test.js:40
> +    return new Date((parseInt(req.responseText) * 1000) + elapsedTime);

nit: elapsedTime is the time the round trip took, while I guess we only care about half of that (server -> client)?

> LayoutTests/http/tests/cache/resources/subresource-test.js:85
> +    now = getServerDate();

This introduces 24 additional synchronous requests for the subresource-expiration-{1,2}.html test cases. While this probably isn't a huge deal considering it's (for most platforms) local, could we maybe determine the difference between the server and client times once during initialization, and then correct the "new Date()" statement here with the offset?

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