run-webkit-tests changes the main display’s color profile when it’s run even if the --pixel option is not specified, so pixel tests aren’t run.
This behavior matches Chromium, I believe (since I just stole their code), but we should fix it!
Created attachment 139408 [details] Patch
Committed r115601: <http://trac.webkit.org/changeset/115601>
I wonder if we should leave a note in the LayoutTestHelper code, in case anyone goes to add anything not-pixel-test-specific there. Or should rename LayoutTestHelper to PixelTestHelper or something.
(In reply to comment #4) > I wonder if we should leave a note in the LayoutTestHelper code, in case anyone goes to add anything not-pixel-test-specific there. Or should rename LayoutTestHelper to PixelTestHelper or something. Woah, you said the same thing on webkit-dev. Nevermind!
Comment on attachment 139408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139408&action=review > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:851 > + self._port.start_helper() Wouldn't be a good idea to have this call named as "start_pixel_test_helper" instead?
Comment on attachment 139408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139408&action=review >> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:851 >> + self._port.start_helper() > > Wouldn't be a good idea to have this call named as "start_pixel_test_helper" instead? No. The intent of this method is to provide a hook for a port to do *whatever* customization is needed prior to a test run, not just pixel-test specific things. In fact, this change would break the android port if it tried to run w/o -p, for example. The correct thing to do would've been to change mac.py's implementation to be a no-op if options.pixel_test was false.
(In reply to comment #7) > (From update of attachment 139408 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139408&action=review > > >> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:851 > >> + self._port.start_helper() > > > > Wouldn't be a good idea to have this call named as "start_pixel_test_helper" instead? > > No. The intent of this method is to provide a hook for a port to do *whatever* customization is needed prior to a test run, not just pixel-test specific things. > > In fact, this change would break the android port if it tried to run w/o -p, for example. > > The correct thing to do would've been to change mac.py's implementation to be a no-op if options.pixel_test was false. Maybe there should be separate hooks. The Chromium Android port that needs non-pixel-test setup isn't in webkit svn (so how could anyone know it exists, let alone when it needs to run?) while the three ports using this in open source (mac, chromium-mac, chromium-win) all need it only for pixel tests. Seems error prone to put the pixel test conditionality in each separate port.
(In reply to comment #8) > (In reply to comment #7) > > (From update of attachment 139408 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=139408&action=review > > > > >> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:851 > > >> + self._port.start_helper() > > > > > > Wouldn't be a good idea to have this call named as "start_pixel_test_helper" instead? > > > > No. The intent of this method is to provide a hook for a port to do *whatever* customization is needed prior to a test run, not just pixel-test specific things. > > > > In fact, this change would break the android port if it tried to run w/o -p, for example. > > > > The correct thing to do would've been to change mac.py's implementation to be a no-op if options.pixel_test was false. > > Maybe there should be separate hooks. The Chromium Android port that needs non-pixel-test setup isn't in webkit svn (so how could anyone know it exists, let alone when it needs to run?) The python code for it is: http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py > Seems error prone to put the pixel test conditionality in each separate port. Perhaps. However, as stated above, the contract for the function was not pixel-test specific, so one had no choice given the current API.
(In reply to comment #7) > (From update of attachment 139408 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139408&action=review > > >> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:851 > >> + self._port.start_helper() > > > > Wouldn't be a good idea to have this call named as "start_pixel_test_helper" instead? > > No. The intent of this method is to provide a hook for a port to do *whatever* customization is needed prior to a test run, not just pixel-test specific things. > > In fact, this change would break the android port if it tried to run w/o -p, for example. > > The correct thing to do would've been to change mac.py's implementation to be a no-op if options.pixel_test was false. +1 Even if you want to make this change, you'd better change the name of the method to start_pixel_helper and change start_helper to setup_test_run in chromium_android. That would be a more sound change. This change actually breaks chromium android, although we don't run layout test publicly now as much stuff has not been upstreamed yet.
In addition to the above comments, this patch breaks reftests, as they are also dependent on color profile (they shouldn't be, but they are). I'm proposing to undo this change in bug 120755.