81729 2012-03-20 20:19:36 -0700 REGRESSION: On Mac, run-webkit-tests changes the display color profile even when not running pixel tests 2013-09-06 09:41:06 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Mac Unspecified RESOLVED FIXED Regression P1 Normal --- 1 mitz mjs ap dpranke mjs simon.fraser thorton zhenghao oldest_to_newest 583731 0 mitz 2012-03-20 20:19:36 -0700 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. 583733 1 thorton 2012-03-20 20:21:26 -0700 This behavior matches Chromium, I believe (since I just stole their code), but we should fix it! 612462 2 139408 mjs 2012-04-29 18:19:25 -0700 Created attachment 139408 Patch 612463 3 mjs 2012-04-29 18:21:14 -0700 Committed r115601: <http://trac.webkit.org/changeset/115601> 612464 4 thorton 2012-04-29 18:21:33 -0700 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. 612489 5 thorton 2012-04-29 20:20:12 -0700 (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! 612554 6 139408 rafael.lobo 2012-04-30 05:09:27 -0700 Comment on attachment 139408 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? 612834 7 139408 dpranke 2012-04-30 12:01:26 -0700 Comment on attachment 139408 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. 615269 8 mjs 2012-05-03 07:39:48 -0700 (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. 615312 9 dpranke 2012-05-03 08:56:47 -0700 (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. 621382 10 zhenghao 2012-05-11 01:32:52 -0700 (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. 925631 11 ap 2013-09-06 09:41:06 -0700 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. 139408 2012-04-29 18:19:25 -0700 2012-04-30 12:01:25 -0700 Patch bug-81729-20120429181925.patch text/plain 1643 mjs SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDExNTYwMCkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDEzIEBACisyMDEyLTA0LTI5ICBNYWNpZWogU3RhY2hvd2lhayAgPG1qc0BhcHBsZS5jb20+ CisKKyAgICAgICAgUkVHUkVTU0lPTjogT24gTGlvbiwgcnVuLXdlYmtpdC10ZXN0cyBjaGFuZ2Vz IHRoZSBkaXNwbGF5IGNvbG9yIHByb2ZpbGUgZXZlbiB3aGVuIG5vdCBydW5uaW5nIHBpeGVsIHRl c3RzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04MTcy OQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogU2Ny aXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvY29udHJvbGxlcnMvbWFuYWdlci5weToKKyAgICAg ICAgKE1hbmFnZXIuX3NldF91cF9ydW4pOiBPbmx5IGxhdW5jaCBMYXlvdXRUZXN0SGVscGVyIHdo ZW4gcnVubmluZyBwaXhlbCB0ZXN0cy4KKwogMjAxMi0wNC0yOSAgU3VkYXJzYW5hIE5hZ2luZW5p ICA8c3VkYXJzYW5hLm5hZ2luZW5pQGxpbnV4LmludGVsLmNvbT4KIAogICAgICAgICBbR1RLXSBE UlQgbmVlZHMgYW4gaW1wbGVtZW50YXRpb24gb2YgbGF5b3V0VGVzdENvbnRyb2xsZXIuc2V0RG9t YWluUmVsYXhhdGlvbkZvcmJpZGRlbkZvclVSTFNjaGVtZQpJbmRleDogVG9vbHMvU2NyaXB0cy93 ZWJraXRweS9sYXlvdXRfdGVzdHMvY29udHJvbGxlcnMvbWFuYWdlci5weQo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBUb29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9jb250cm9sbGVycy9tYW5hZ2Vy LnB5CShyZXZpc2lvbiAxMTU1ODEpCisrKyBUb29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90 ZXN0cy9jb250cm9sbGVycy9tYW5hZ2VyLnB5CSh3b3JraW5nIGNvcHkpCkBAIC04NDYsOCArODQ2 LDkgQEAgY2xhc3MgTWFuYWdlcihvYmplY3QpOgogICAgICAgICAiIiIKICAgICAgICAgIyBUaGlz IG11c3QgYmUgc3RhcnRlZCBiZWZvcmUgd2UgY2hlY2sgdGhlIHN5c3RlbSBkZXBlbmRlbmNpZXMs CiAgICAgICAgICMgc2luY2UgdGhlIGhlbHBlciBtYXkgZG8gdGhpbmdzIHRvIG1ha2UgdGhlIHNl dHVwIGNvcnJlY3QuCi0gICAgICAgIHNlbGYuX3ByaW50ZXIucHJpbnRfdXBkYXRlKCJTdGFydGlu ZyBoZWxwZXIgLi4uIikKLSAgICAgICAgc2VsZi5fcG9ydC5zdGFydF9oZWxwZXIoKQorICAgICAg ICBpZiBzZWxmLl9vcHRpb25zLnBpeGVsX3Rlc3RzOgorICAgICAgICAgICAgc2VsZi5fcHJpbnRl ci5wcmludF91cGRhdGUoIlN0YXJ0aW5nIHBpeGVsIHRlc3QgaGVscGVyIC4uLiIpCisgICAgICAg ICAgICBzZWxmLl9wb3J0LnN0YXJ0X2hlbHBlcigpCiAKICAgICAgICAgIyBDaGVjayB0aGF0IHRo ZSBzeXN0ZW0gZGVwZW5kZW5jaWVzICh0aGVtZXMsIGZvbnRzLCAuLi4pIGFyZSBjb3JyZWN0Lgog ICAgICAgICBpZiBub3Qgc2VsZi5fb3B0aW9ucy5ub2NoZWNrX3N5c19kZXBzOgo=