Bug 81729 - REGRESSION: On Mac, run-webkit-tests changes the display color profile even when not running pixel tests
Summary: REGRESSION: On Mac, run-webkit-tests changes the display color profile even w...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P1 Normal
Assignee: Maciej Stachowiak
URL:
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2012-03-20 20:19 PDT by mitz
Modified: 2013-09-06 09:41 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.60 KB, patch)
2012-04-29 18:19 PDT, Maciej Stachowiak
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2012-03-20 20:19:36 PDT
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.
Comment 1 Tim Horton 2012-03-20 20:21:26 PDT
This behavior matches Chromium, I believe (since I just stole their code), but we should fix it!
Comment 2 Maciej Stachowiak 2012-04-29 18:19:25 PDT
Created attachment 139408 [details]
Patch
Comment 3 Maciej Stachowiak 2012-04-29 18:21:14 PDT
Committed r115601: <http://trac.webkit.org/changeset/115601>
Comment 4 Tim Horton 2012-04-29 18:21:33 PDT
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.
Comment 5 Tim Horton 2012-04-29 20:20:12 PDT
(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 6 Rafael Brandao 2012-04-30 05:09:27 PDT
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 7 Dirk Pranke 2012-04-30 12:01:26 PDT
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.
Comment 8 Maciej Stachowiak 2012-05-03 07:39:48 PDT
(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.
Comment 9 Dirk Pranke 2012-05-03 08:56:47 PDT
(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.
Comment 10 Hao Zheng 2012-05-11 01:32:52 PDT
(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.
Comment 11 Alexey Proskuryakov 2013-09-06 09:41:06 PDT
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.