47874 2010-10-18 19:41:17 -0700 chromium_gpu port of new-run-webkit-tests must search chromium-gpu directory for expectations 2010-10-19 15:43:45 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 kbr kbr abarth dpranke eric jamesr levin mihaip tony oldest_to_newest 295928 0 kbr 2010-10-18 19:41:17 -0700 Currently all flavors of the chromium-gpu port of new-run-webkit-tests search only the platform-specific chromium-gpu directory (i.e., chromium-gpu-win) for test expectations in addition to the normal expectations search path. In order to share some test expectations among platforms, the chromium-gpu directory needs to be searched as well. 295931 1 71119 kbr 2010-10-18 19:46:05 -0700 Created attachment 71119 Patch 295933 2 71119 eric 2010-10-18 19:50:19 -0700 Comment on attachment 71119 Patch Is there more code sharing to be had here? Seems we're just copy/pasting... 295937 3 kbr 2010-10-18 19:56:09 -0700 This is the simplest possible change as far as I can tell. I am not a Python expert, though. 295938 4 eric 2010-10-18 19:58:18 -0700 Wow, that file is ugly. It's all copy paste disaster. :( 295950 5 dpranke 2010-10-18 20:26:43 -0700 The code can actually be cleaned up so that there's much less copy&pasting, through some clever use of mixing-type coding, which I have done already in a branch in my tree. If I ever get the rest of my patches cleaned up and landed, I can land this one, too. Unfortunately, since we prohibit multiple inheritance, and Python doesn't have built in support for mixins or traits, this sort of thing is pretty ugly (we actually want something approaching a diamond inheritance graph). 295951 6 kbr 2010-10-18 20:28:50 -0700 (In reply to comment #5) > The code can actually be cleaned up so that there's much less copy&pasting, through some clever use of mixing-type coding, which I have done already in a branch in my tree. > > If I ever get the rest of my patches cleaned up and landed, I can land this one, too. > > Unfortunately, since we prohibit multiple inheritance, and Python doesn't have built in support for mixins or traits, this sort of thing is pretty ugly (we actually want something approaching a diamond inheritance graph). In the meantime, could I please get a review of the current change? It is blocking landing of initial test expectations for Chromium's accelerated compositor. 295955 7 dpranke 2010-10-18 20:43:42 -0700 oh, sorry :) otherwise (ignoring the duplication), the code LGTM, but it's up to Eric or someone else if they want to R+ it given the duplication. 295962 8 eric 2010-10-18 21:08:59 -0700 (In reply to comment #5) > Unfortunately, since we prohibit multiple inheritance, and Python doesn't have built in support for mixins or traits, this sort of thing is pretty ugly (we actually want something approaching a diamond inheritance graph). I'm not sure I follow. Who forbids multiple inheritance and where? Python supports multiple inheritance (and we use it a bunch in webkitpy for mixins). My (limited) understanding is they're not as plagued by the diamond problem as C++ is either: http://aartemenko.com/texts/multiple-inheritance-in-python-and/ 295963 9 eric 2010-10-18 21:10:03 -0700 I don't really think we should hold this patch hostage, just because someone previously designed this file poorly. But at the same time, this is a rather ugly patch to an ugly file. 295967 10 dpranke 2010-10-18 21:13:19 -0700 (In reply to comment #7) > oh, sorry :) > > otherwise (ignoring the duplication), the code LGTM, but it's up to Eric or someone else if they want to R+ it given the duplication. Actually, now that I think about it, we actually have GPU baselines that are the same across mac, linux, and windows? (because that's what chromium-gpu should mean). That seems a bit surprising to me (although not impossible). 295990 11 dpranke 2010-10-18 22:09:23 -0700 (In reply to comment #8) > (In reply to comment #5) > > Unfortunately, since we prohibit multiple inheritance, and Python doesn't have built in support for mixins or traits, this sort of thing is pretty ugly (we actually want something approaching a diamond inheritance graph). > > I'm not sure I follow. Who forbids multiple inheritance and where? > (agree that this is all orthogonal to whether or not this particular patch should go in ...) "Prohibit" might be a bit strong. I'm pretty sure Tony and I discussed this back when I was originally creating this file and we both thought that MI was discouraged (I think we were following the logic that it was discouraged in C++ and since it's generally considered a misfeature of languages these days, best not to use it). Tony, please correct me if I'm misremembering something. If you use it a bunch in webkitpy, that's news to me as I haven't run across it yet (certainly possible). Pointers to examples might be helpful. I have nothing against MI per se (although I do prefer mixins and traits), since it certainly would've made this particular file cleaner. > Python supports multiple inheritance (and we use it a bunch in webkitpy for mixins). My (limited) understanding is they're not as plagued by the diamond problem as C++ is either: http://aartemenko.com/texts/multiple-inheritance-in-python-and/ Disclaimer: I'm no C++ guru, so I can't swear that I fully understand the hubbub around the diamond problem in C++ (as opposed to any other language). That said, it is true that the Python MRO doesn't suffer from the immediate problem that is usually cited in C++ (classes B and C both inherit a method from class A and so the compiler would create two copies of the A method and bails out), but it does have other issues that can still lead to ambiguity. See http://www.python.org/download/releases/2.3/mro/ for the full discussion. Some would argue that requiring anyone to fully understand the subtleties of the MRO and/or the way you use the super() function to be something that is best avoided, much like we avoid the dark corners of C++ in our C++ style guideline (e.g., http://fuhm.net/super-harmful/ ). I personally don't feel strongly about it one way or another. I thought we couldn't/shouldn't use it, so I didn't, which leads to either the duplication you see here and in port/google_chrome.py, or the manual mixing you'd see in the patch I have in my tree. If we'd rather use MI, I can work up a patch for that as well and we can compare the three options. 295992 12 levin 2010-10-18 22:14:26 -0700 (In reply to comment #11) > (agree that this is all orthogonal to whether or not this particular patch should go in ...) Can we file a bug about this ugly code/copy/paste issue? Cite the bug here? Have this discussion there? (And of course r+ this after the bug is filed.) 296012 13 dpranke 2010-10-18 22:54:55 -0700 (In reply to comment #12) > (In reply to comment #11) > > (agree that this is all orthogonal to whether or not this particular patch should go in ...) > > Can we file a bug about this ugly code/copy/paste issue? > Cite the bug here? > Have this discussion there? Done. See bug 47884. > > (And of course r+ this after the bug is filed.) That's up to one of you :) 296022 14 71119 levin 2010-10-18 23:09:30 -0700 Comment on attachment 71119 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71119&action=review > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py:55 > + # test that it has the right directories in front of the search path. It would be great if the first word of the sentence was capitalized :) (I know the comment was like this before and all other comments are this way. I won't object if you don't fix this... It just would be nice you are landing it by hand.) 296274 15 kbr 2010-10-19 10:52:35 -0700 (In reply to comment #10) > (In reply to comment #7) > > oh, sorry :) > > > > otherwise (ignoring the duplication), the code LGTM, but it's up to Eric or someone else if they want to R+ it given the duplication. > > Actually, now that I think about it, we actually have GPU baselines that are the same across mac, linux, and windows? (because that's what chromium-gpu should mean). That seems a bit surprising to me (although not impossible). I am pretty sure that we will have some baselines that can be shared at least between Linux and Windows (the textual output of the layer tree). I am going to verify this with this patch and the initial baselines I'm about to check in, and may adjust this patch later to add a directory for chromium-gpu results shared only between these two platforms. 296281 16 kbr 2010-10-19 10:57:03 -0700 Committed r70068: <http://trac.webkit.org/changeset/70068> 296289 17 kbr 2010-10-19 11:09:23 -0700 (In reply to comment #14) > (From update of attachment 71119 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71119&action=review > > > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py:55 > > + # test that it has the right directories in front of the search path. > > It would be great if the first word of the sentence was capitalized :) > (I know the comment was like this before and all other comments are this way. I won't object if you don't fix this... It just would be nice you are landing it by hand.) I fixed up the comments when landing the patch. 296438 18 dpranke 2010-10-19 15:04:51 -0700 (In reply to comment #15) > (In reply to comment #10) > > (In reply to comment #7) > > > oh, sorry :) > > > > > > otherwise (ignoring the duplication), the code LGTM, but it's up to Eric or someone else if they want to R+ it given the duplication. > > > > Actually, now that I think about it, we actually have GPU baselines that are the same across mac, linux, and windows? (because that's what chromium-gpu should mean). That seems a bit surprising to me (although not impossible). > > I am pretty sure that we will have some baselines that can be shared at least between Linux and Windows (the textual output of the layer tree). I am going to verify this with this patch and the initial baselines I'm about to check in, and may adjust this patch later to add a directory for chromium-gpu results shared only between these two platforms. Hi Ken, It sounds like you and I should huddle and talk through the desired fallback paths because maybe it's a bit different than I had originally implemented. If we follow the pattern we have for platform/chromium-{^gpu}-*, we would only put stuff in platform/chromium-gpu/ if it was common across all three platforms. Normally we would have (for non-gpu tests) linux falling back and reusing win baselines automatically, but non-gpu paths trump the gpu paths as things are currently implemented (i.e., we'll fall back from linux-gpu to linux to win, instead of linux-gpu to win-gpu to ???) 296474 19 kbr 2010-10-19 15:43:45 -0700 (In reply to comment #18) > (In reply to comment #15) > > (In reply to comment #10) > > > (In reply to comment #7) > > > > oh, sorry :) > > > > > > > > otherwise (ignoring the duplication), the code LGTM, but it's up to Eric or someone else if they want to R+ it given the duplication. > > > > > > Actually, now that I think about it, we actually have GPU baselines that are the same across mac, linux, and windows? (because that's what chromium-gpu should mean). That seems a bit surprising to me (although not impossible). > > > > I am pretty sure that we will have some baselines that can be shared at least between Linux and Windows (the textual output of the layer tree). I am going to verify this with this patch and the initial baselines I'm about to check in, and may adjust this patch later to add a directory for chromium-gpu results shared only between these two platforms. > > Hi Ken, > > It sounds like you and I should huddle and talk through the desired fallback paths because maybe it's a bit different than I had originally implemented. If we follow the pattern we have for platform/chromium-{^gpu}-*, we would only put stuff in platform/chromium-gpu/ if it was common across all three platforms. Normally we would have (for non-gpu tests) linux falling back and reusing win baselines automatically, but non-gpu paths trump the gpu paths as things are currently implemented (i.e., we'll fall back from linux-gpu to linux to win, instead of linux-gpu to win-gpu to ???) Yes, let's discuss this offline. I'm in the process of committing initial Windows and Linux baselines and trying to understand which ones can be shared (pixel tests, or at least the majority thereof, must be per-platform but I believe many text results can be shared). 71119 2010-10-18 19:46:05 -0700 2010-10-18 23:09:30 -0700 Patch expectations-path.patch text/plain 3420 kbr SW5kZXg6IFdlYktpdFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJLaXRUb29scy9D aGFuZ2VMb2cJKHJldmlzaW9uIDcwMDE5KQorKysgV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCSh3b3Jr aW5nIGNvcHkpCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTAtMTAtMTggIEtlbm5ldGggUnVzc2VsbCAg PGtickBnb29nbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgIGNocm9taXVtX2dwdSBwb3J0IG9mIG5ldy1ydW4td2Via2l0LXRlc3RzIG11c3Qg c2VhcmNoIGNocm9taXVtLWdwdSBkaXJlY3RvcnkgZm9yIGV4cGVjdGF0aW9ucworICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDc4NzQKKworICAgICAgICAq IFNjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQvY2hyb21pdW1fZ3B1LnB5OgorICAg ICAgICAqIFNjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQvY2hyb21pdW1fZ3B1X3Vu aXR0ZXN0LnB5OgorCiAyMDEwLTEwLTE4ICBLZW5uZXRoIFJ1c3NlbGwgIDxrYnJAZ29vZ2xlLmNv bT4KIAogICAgICAgICBSZXZpZXdlZCBieSBFcmljIFNlaWRlbC4KSW5kZXg6IFdlYktpdFRvb2xz L1NjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQvY2hyb21pdW1fZ3B1LnB5Cj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIFdlYktpdFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQv Y2hyb21pdW1fZ3B1LnB5CShyZXZpc2lvbiA3MDAxNikKKysrIFdlYktpdFRvb2xzL1NjcmlwdHMv d2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQvY2hyb21pdW1fZ3B1LnB5CSh3b3JraW5nIGNvcHkp CkBAIC05MCw3ICs5MCw3IEBAIGNsYXNzIENocm9taXVtR3B1TGludXhQb3J0KGNocm9taXVtX2xp bnUKICAgICAgICAgY2hyb21pdW1fbGludXguQ2hyb21pdW1MaW51eFBvcnQuX19pbml0X18oc2Vs ZiwgKiprd2FyZ3MpCiAKICAgICBkZWYgYmFzZWxpbmVfc2VhcmNoX3BhdGgoc2VsZik6Ci0gICAg ICAgIHJldHVybiAoW3NlbGYuX3dlYmtpdF9iYXNlbGluZV9wYXRoKCdjaHJvbWl1bS1ncHUtbGlu dXgnKV0gKworICAgICAgICByZXR1cm4gKG1hcChzZWxmLl93ZWJraXRfYmFzZWxpbmVfcGF0aCwg WydjaHJvbWl1bS1ncHUtbGludXgnLCAnY2hyb21pdW0tZ3B1J10pICsKICAgICAgICAgICAgICAg ICBjaHJvbWl1bV9saW51eC5DaHJvbWl1bUxpbnV4UG9ydC5iYXNlbGluZV9zZWFyY2hfcGF0aChz ZWxmKSkKIAogICAgIGRlZiBwYXRoX3RvX3Rlc3RfZXhwZWN0YXRpb25zX2ZpbGUoc2VsZik6CkBA IC0xMDgsNyArMTA4LDcgQEAgY2xhc3MgQ2hyb21pdW1HcHVNYWNQb3J0KGNocm9taXVtX21hYy5D aAogICAgICAgICBjaHJvbWl1bV9tYWMuQ2hyb21pdW1NYWNQb3J0Ll9faW5pdF9fKHNlbGYsICoq a3dhcmdzKQogCiAgICAgZGVmIGJhc2VsaW5lX3NlYXJjaF9wYXRoKHNlbGYpOgotICAgICAgICBy ZXR1cm4gKFtzZWxmLl93ZWJraXRfYmFzZWxpbmVfcGF0aCgnY2hyb21pdW0tZ3B1LW1hYycpXSAr CisgICAgICAgIHJldHVybiAobWFwKHNlbGYuX3dlYmtpdF9iYXNlbGluZV9wYXRoLCBbJ2Nocm9t aXVtLWdwdS1tYWMnLCAnY2hyb21pdW0tZ3B1J10pICsKICAgICAgICAgICAgICAgICBjaHJvbWl1 bV9tYWMuQ2hyb21pdW1NYWNQb3J0LmJhc2VsaW5lX3NlYXJjaF9wYXRoKHNlbGYpKQogCiAgICAg ZGVmIHBhdGhfdG9fdGVzdF9leHBlY3RhdGlvbnNfZmlsZShzZWxmKToKQEAgLTEyNiw3ICsxMjYs NyBAQCBjbGFzcyBDaHJvbWl1bUdwdVdpblBvcnQoY2hyb21pdW1fd2luLkNoCiAgICAgICAgIGNo cm9taXVtX3dpbi5DaHJvbWl1bVdpblBvcnQuX19pbml0X18oc2VsZiwgKiprd2FyZ3MpCiAKICAg ICBkZWYgYmFzZWxpbmVfc2VhcmNoX3BhdGgoc2VsZik6Ci0gICAgICAgIHJldHVybiAoW3NlbGYu X3dlYmtpdF9iYXNlbGluZV9wYXRoKCdjaHJvbWl1bS1ncHUtd2luJyldICsKKyAgICAgICAgcmV0 dXJuIChtYXAoc2VsZi5fd2Via2l0X2Jhc2VsaW5lX3BhdGgsIFsnY2hyb21pdW0tZ3B1LXdpbics ICdjaHJvbWl1bS1ncHUnXSkgKwogICAgICAgICAgICAgICAgIGNocm9taXVtX3dpbi5DaHJvbWl1 bVdpblBvcnQuYmFzZWxpbmVfc2VhcmNoX3BhdGgoc2VsZikpCiAKICAgICBkZWYgcGF0aF90b190 ZXN0X2V4cGVjdGF0aW9uc19maWxlKHNlbGYpOgpJbmRleDogV2ViS2l0VG9vbHMvU2NyaXB0cy93 ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9jaHJvbWl1bV9ncHVfdW5pdHRlc3QucHkKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gV2ViS2l0VG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9j aHJvbWl1bV9ncHVfdW5pdHRlc3QucHkJKHJldmlzaW9uIDcwMDE2KQorKysgV2ViS2l0VG9vbHMv U2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9jaHJvbWl1bV9ncHVfdW5pdHRlc3Qu cHkJKHdvcmtpbmcgY29weSkKQEAgLTUyLDkgKzUyLDEwIEBAIGNsYXNzIENocm9taXVtR3B1VGVz dCh1bml0dGVzdC5UZXN0Q2FzZSkKICAgICAgICAgIyB3ZSB1c2Ugc3RhcnRzd2l0aCgpIGluc3Rl YWQgb2YgRXF1YWwgdG8gZ2xvc3Mgb3ZlciBwbGF0Zm9ybSB2ZXJzaW9ucy4KICAgICAgICAgc2Vs Zi5hc3NlcnRUcnVlKHBvcnQubmFtZSgpLnN0YXJ0c3dpdGgocG9ydF9uYW1lKSkKIAotICAgICAg ICAjIHRlc3QgdGhhdCBpdCBoYXMgdGhlIHJpZ2h0IGRpcmVjdG9yeSBpbiBmcm9udCBvZiB0aGUg c2VhcmNoIHBhdGguCi0gICAgICAgIHBhdGggPSBwb3J0LmJhc2VsaW5lX3NlYXJjaF9wYXRoKClb MF0KLSAgICAgICAgc2VsZi5hc3NlcnRFcXVhbChwb3J0Ll93ZWJraXRfYmFzZWxpbmVfcGF0aChw b3J0X25hbWUpLCBwYXRoKQorICAgICAgICAjIHRlc3QgdGhhdCBpdCBoYXMgdGhlIHJpZ2h0IGRp cmVjdG9yaWVzIGluIGZyb250IG9mIHRoZSBzZWFyY2ggcGF0aC4KKyAgICAgICAgcGF0aHMgPSBw b3J0LmJhc2VsaW5lX3NlYXJjaF9wYXRoKCkKKyAgICAgICAgc2VsZi5hc3NlcnRFcXVhbChwb3J0 Ll93ZWJraXRfYmFzZWxpbmVfcGF0aChwb3J0X25hbWUpLCBwYXRoc1swXSkKKyAgICAgICAgc2Vs Zi5hc3NlcnRFcXVhbChwb3J0Ll93ZWJraXRfYmFzZWxpbmVfcGF0aCgnY2hyb21pdW0tZ3B1Jyks IHBhdGhzWzFdKQogCiAgICAgICAgICMgdGVzdCB0aGF0IHdlIGhhdmUgdGhlIHJpZ2h0IGV4cGVj dGF0aW9ucyBmaWxlLgogICAgICAgICBzZWxmLmFzc2VydFRydWUoJ2Nocm9taXVtLWdwdScgaW4K