Bug 47874 – chromium_gpu port of new-run-webkit-tests must search chromium-gpu directory for expectations

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kenneth Russell]

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.

[Kenneth Russell]

Created attachment 71119 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 71119 [details]
Patch

Is there more code sharing to be had here?  Seems we're just copy/pasting...

[Kenneth Russell]

This is the simplest possible change as far as I can tell. I am not a Python expert, though.

[Eric Seidel (no email)]

Wow, that file is ugly.  It's all copy paste disaster.  :(

[Dirk Pranke]

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

[Kenneth Russell]

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

[Dirk Pranke]

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.

[Eric Seidel (no email)]

(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/

[Eric Seidel (no email)]

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.

[Dirk Pranke]

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

[Dirk Pranke]

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

[David Levin]

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

[Dirk Pranke]

(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 :)

[David Levin]

Comment on attachment 71119 [details]
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.)

[Kenneth Russell]

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

[Kenneth Russell]

Committed r70068: <http://trac.webkit.org/changeset/70068>

[Kenneth Russell]

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

[Dirk Pranke]

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

[Kenneth Russell]

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