Apparently Chromium port doesn't inherit from Base port :(
(In reply to comment #0) > Apparently Chromium port doesn't inherit from Base port :( I mean WebKit* port.
Yes. Base should die. Most of its logic should split off into other objects, and what remains should be folded up into WebKit. Chromium needs to become a real boy some day.
(In reply to comment #2) > Yes. Base should die. Most of its logic should split off into other objects, and what remains should be folded up into WebKit. Chromium needs to become a real boy some day. That sounds like a good idea. But, I think I'm going to take the easy way out and move stuff from WebKit to Base for now :( Making Chromium a real WebKit port seems to require too much effort just to fix this bug.
I agree that BasePort and WebKitPort should probably merge, but merging webkit and chromium is a fair pile of work (that I'm gradually working on) and we need to do that carefully. Not something to tackle here :).
Created attachment 123505 [details] fixes the bug
Comment on attachment 123505 [details] fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=123505&action=review > Tools/ChangeLog:10 > + layout tests but performacne tests don't use test_expectations.txt so Chromium port performacne: typo
I understand that you're just moving code around, but this stuff is poorly factored. The base port shouldn't be a god object that understands details of how to parse skipped lists.
(In reply to comment #7) > I understand that you're just moving code around, but this stuff is poorly factored. The base port shouldn't be a god object that understands details of how to parse skipped lists. It's not obvious what the correct refactoring is. It seems that the first refactoring we need to do is to turn Chromium into a proper webkit port.
> It's not obvious what the correct refactoring is. It seems that the first refactoring we need to do is to turn Chromium into a proper webkit port. I more incremental approach is to create a class for Skipped lists that knows how to parser them and translate them into expectations rather than have the port know all those details.
(In reply to comment #9) > > It's not obvious what the correct refactoring is. It seems that the first refactoring we need to do is to turn Chromium into a proper webkit port. > > I more incremental approach is to create a class for Skipped lists that knows how to parser them and translate them into expectations rather than have the port know all those details. Do you strongly feel that it's a blocker for this bug? If not, I'm not motivated to do that the refactoring.
Comment on attachment 123505 [details] fixes the bug I think this change is fine. While I agree with the general "god" sentiment about the Port class, it's not at all clear what the right way to factor out logic is here, and creating a class just for parsing Skipped files itself would be silly. I think perhaps a separate change that moved all of the expectations-related logic out might be good, but I don't really want to make that a blocker of this and I'd want to think about that more.
Comment on attachment 123505 [details] fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=123505&action=review > Tools/Scripts/webkitpy/layout_tests/port/base.py:599 > + @memoized Note that I wouldn't bother with the @memoized here (I realize that the WebKitPort had it); this routine can't be called all that often, and memoizing it but not other expectations-related routines might lead to weird consistency issues.
(In reply to comment #11) > I think perhaps a separate change that moved all of the expectations-related logic out might be good, but I don't really want to make that a blocker of this and I'd want to think about that more. That sounds like a good idea. Maybe we can create a class for the "list of tests" that can parse test_expectations.txt and Skipped.
(In reply to comment #12) > (From update of attachment 123505 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123505&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/base.py:599 > > + @memoized > > Note that I wouldn't bother with the @memoized here (I realize that the WebKitPort had it); this routine can't be called all that often, and memoizing it but not other expectations-related routines might lead to weird consistency issues. This function is called by skips_perf_test, which is called per test. So it should be memoized.
(In reply to comment #14) > (In reply to comment #12) > > (From update of attachment 123505 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=123505&action=review > > > > > Tools/Scripts/webkitpy/layout_tests/port/base.py:599 > > > + @memoized > > > > Note that I wouldn't bother with the @memoized here (I realize that the WebKitPort had it); this routine can't be called all that often, and memoizing it but not other expectations-related routines might lead to weird consistency issues. > > This function is called by skips_perf_test, which is called per test. So it should be memoized. Okay, for now :). Ultimately I would probably put skips_perf_test() in perftestrunner and just merged it into _collect_tests(). Neither skips_layout_test or skips_perf_test() really belongs on the Port object (see bug 47528). The port object should have no real knowledge of the actual expectations, just a way to provide the contents of the files. Memoization often (not always) suggests to me that you have the wrong algorithmic design and you should fix the algorithm rather than relying on memoizing to get the performance you need.
(In reply to comment #15) > Ultimately I would probably put skips_perf_test() in perftestrunner and just merged it into _collect_tests(). Neither skips_layout_test or skips_perf_test() really belongs on the Port object (see bug 47528). The port object should have no real knowledge of the actual expectations, just a way to provide the contents of the files. Memoization often (not always) suggests to me that you have the wrong algorithmic design and you should fix the algorithm rather than relying on memoizing to get the performance you need. That makes a lot of sense. I added perf_test version but was wondering why those methods are hanging off of the port object. We should probably look at the bigger picture of isolating test expectation related stuff into a separate class though.
anyway, thanks for the review.
Committed r105644: <http://trac.webkit.org/changeset/105644>
(In reply to comment #16) > We should probably look at the bigger picture of isolating test expectation related stuff into a separate class though. There are (at least) two different operations going on ... the first is getting the content of things like test expectations files and skipped files, and that is (IMO) a port-specific operation, at the very least to figure out where the paths to the files are, and I try to go a step further so that you don't actually need a filesystem at all (you just ask for the a block of text data). The second is actually parsing those files to turn them into real objects, and that is already done in generic code (in models/test_expectations.py). Possibly it makes sense to add skipped file parsing logic to the latter.