[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 #7 from Philippe Liard <pliard at chromium.org>  2012-08-29 08:29:00 PST ---
(From update of attachment 161190)
View in context: https://bugs.webkit.org/attachment.cgi?id=161190&action=review

>> 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.

Done.

>> 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.

Done.

>> 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?

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?

>> 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.

Done.

>> 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.

Done.

>> 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.

Done.

>> 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)?

Good point.

>> 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?

Good point.

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