[chromium] add google-chrome layout test result directories
Created attachment 64178 [details] Patch
The background is that the Google Chrome builds often have slightly different layout test results. For example, the media tests might have differences depending on which codecs are installed or the Linux bots may have different results due to rounding and what compiler flags we happen to use for Google Chrome Builds (see http://code.google.com/p/chromium/issues/detail?id=8475 ). Everytime we make a release branch, we have to go through and re-verify that they're expected. It's a pain, so we should just check in these results (on linux, it's only like 150 tests, on windows and mac, it's even less).
I'll give dirk and eric a chance to comment before committing. :)
Comment on attachment 64178 [details] Patch This seems like sadness. But OK.
Comment on attachment 64178 [details] Patch Works for me. Good to see this get fixed; it was on my to-do list for a while but I never got around to it.
(In reply to comment #2) > For example, the media tests might have differences depending on which codecs are installed This *shouldn't* be a consideration as we strive to make the results not include anything caused by which codecs/containers a port supports. If there are media test results that differ because of codec or container, we should fix them (bugs please!). Having said that, I agree with the other Eric that it seems sad but OK.
(In reply to comment #6) > (In reply to comment #2) > > For example, the media tests might have differences depending on which codecs are installed > > This *shouldn't* be a consideration as we strive to make the results not include anything caused by which codecs/containers a port supports. If there are media test results that differ because of codec or container, we should fix them (bugs please!). Ok, I will file bugs for any tests that are sensitive to which codecs are installed. I'm actually not sure which tests they are, but I think it's specific to the chromium-win port.
Committed r65250: <http://trac.webkit.org/changeset/65250>
Okay, you've already committed this, and it'll work, but I wonder if this might be slightly cleaner if we implemented this using delegation instead of inheritance. The patch would look something like: % cat google_chrome.py class GoogleChromePort(object): def __init__(self, port_name, options): self.name = port_name if port_name == 'google-chrome-mac': self.__delegate = factory.get('chromium-mac', options) elif port_name == 'google-chrome-win': self.__delegate = factory.get('chomium-win', options) elif ... def __getattr__(self, name): return getattr(self.__delegate, name): def baseline_search_path(self): return [ self.__delegate._chromium_baseline_path(self.name) ] + self._delegate.baseline_search_path(self)] % cat factory.py def get(): ... if port_to_use.startswith('google-chrome'): import google_chrome return GoogleChromePort(port_to_use, options) elif ... % I think this'll work, it's a lot fewer lines of code, and perhaps a little clearer. You might need to override one or two other methods as well. Thoughts?
(In reply to comment #9) > Okay, you've already committed this, and it'll work, but I wonder if this might be slightly cleaner if we implemented this using delegation instead of inheritance. I didn't merge baseline_search_path() into a single function because I wasn't sure if it was correct to always use self._name. For example, if we add a google-chrome-win-vista, we need to add more fallback logic to baseline_search_path(). If you want to write a follow up patch to reduce the number of lines of code, I'd be happy to review . . .
(In reply to comment #10) > (In reply to comment #9) > > Okay, you've already committed this, and it'll work, but I wonder if this might be slightly cleaner if we implemented this using delegation instead of inheritance. > > I didn't merge baseline_search_path() into a single function because I wasn't sure if it was correct to always use self._name. For example, if we add a google-chrome-win-vista, we need to add more fallback logic to baseline_search_path(). > > If you want to write a follow up patch to reduce the number of lines of code, I'd be happy to review . . . Fair enough. I'm not actually sure it's safe, either :) We'll consider this a low-priority TODO :)