Bug 43889 – [chromium] add google-chrome layout test result directories

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

[chromium] add google-chrome layout test result directories

[Tony Chang]

Created attachment 64178 [details]
Patch

[Tony Chang]

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

[Tony Chang]

I'll give dirk and eric a chance to comment before committing.  :)

[Eric Seidel (no email)]

Comment on attachment 64178 [details]
Patch

This seems like sadness.

But OK.

[Dirk Pranke]

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.

[Eric Carlson]

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

[Tony Chang]

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

[Tony Chang]

Committed r65250: <http://trac.webkit.org/changeset/65250>

[Dirk Pranke]

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?

[Tony Chang]

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

[Dirk Pranke]

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