The fix for #87442 added feature defines for CSS Regions and exclusions to provide proper disabling of these features. However, the proper project defines were added only to the Mac projects, effectively disabling CSS Regions & exclusions on Windows nightlies.
Created attachment 146547 [details] Added proper values to ENABLE_CSS_REGIONS & ENABLE_CSS_EXCLUSIONS in FeatureDefines.vsprops and FeatureDefinesCairo.vsprops
Comment on attachment 146547 [details] Added proper values to ENABLE_CSS_REGIONS & ENABLE_CSS_EXCLUSIONS in FeatureDefines.vsprops and FeatureDefinesCairo.vsprops Attachment 146547 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12917502
See https://bugs.webkit.org/show_bug.cgi?id=87519, where I had the same realization and then the same issue and then never returned to test it manually and figure out what was up.
Created attachment 146850 [details] Another attempt at fixing regions on Windows.
Attachment 146850 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:10: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 146851 [details] FIxed style
Comment on attachment 146851 [details] FIxed style View in context: https://bugs.webkit.org/attachment.cgi?id=146851&action=review > Source/WebCore/css/CSSPropertyNames.in:370 > +#if defined(ENABLE_CSS_REGIONS) Won't this do the wrong thing if it's defined to 0? Is that a case we can get into? I assumed there was a reason for this construct (which is used in many places).
Created attachment 147377 [details] Pretty much the same as the initial patch, but this time I touched CSSPropertyNames.in to trigger a proper rebuild on Windows.
*** Bug 87519 has been marked as a duplicate of this bug. ***
Comment on attachment 147377 [details] Pretty much the same as the initial patch, but this time I touched CSSPropertyNames.in to trigger a proper rebuild on Windows. Clearing flags on attachment: 147377 Committed r120280: <http://trac.webkit.org/changeset/120280>
All reviewed patches have been landed. Closing bug.
This change is causing all layout tests to crash on Windows.
Re-opened since this is blocked by 89273
Created attachment 149548 [details] Enabling only regions this time. Also with additions to the settings part and a updated expectations for a couple of tests
Attachment 149548 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebKit/win/WebPreferences.h:166: Extra space after ( in function call [whitespace/parens] [4] Source/WebKit/win/WebPreferences.h:169: Extra space after ( in function call [whitespace/parens] [4] Source/WebKit/win/WebPreferences.h:170: The parameter name "enabled" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/win/WebPreferences.cpp:912: Extra space after ( in function call [whitespace/parens] [4] Source/WebKit/win/WebPreferences.cpp:919: Extra space after ( in function call [whitespace/parens] [4] Total errors found: 5 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 149553 [details] Fixed style although I'm not sure that removing the parameter name in WebPreferences.h was the right way to do it.
Comment on attachment 149553 [details] Fixed style although I'm not sure that removing the parameter name in WebPreferences.h was the right way to do it. View in context: https://bugs.webkit.org/attachment.cgi?id=149553&action=review > Source/WebCore/css/CSSPropertyNames.in:9 > +// Please file a bug against whatever build system needs this. :(
The change technically looks fine. I've CC'd Apple Windows folk in case they don't want CSS regions on for their port for some reason.
They are OK with CSS Regions on Windows, but initially they had a problem when enabling regions AND exclusions. The problem with exclusions is still there and it's pretty hairy. This patch only adds regions support. So unless I break something (again), they should be ok with it :) But it's good to have them cc'ed, too.
Comment on attachment 149553 [details] Fixed style although I'm not sure that removing the parameter name in WebPreferences.h was the right way to do it. Rejecting attachment 149553 [details] from commit-queue. New failing tests: fast/css/getComputedStyle/computed-style-without-renderer.html fast/css/getComputedStyle/computed-style.html Full output: http://queues.webkit.org/results/13098575
Created attachment 149611 [details] Archive of layout-test-results from ec2-cq-03 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: ec2-cq-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 149553 [details] Fixed style although I'm not sure that removing the parameter name in WebPreferences.h was the right way to do it. Attachment 149553 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13103576 New failing tests: fast/css/getComputedStyle/computed-style-without-renderer.html fast/css/getComputedStyle/computed-style.html
Created attachment 149648 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 149718 [details] Fixing layout test failures on cr-linux
Comment on attachment 149718 [details] Fixing layout test failures on cr-linux Attachment 149718 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13096762 New failing tests: fast/css/getComputedStyle/computed-style-without-renderer.html fast/css/getComputedStyle/computed-style.html
Created attachment 149737 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 149718 [details] Fixing layout test failures on cr-linux Attachment 149718 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13087863 New failing tests: fast/css/getComputedStyle/computed-style-without-renderer.html fast/css/getComputedStyle/computed-style.html
Created attachment 149743 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 149744 [details] Added chromium expectations too, since it seems it first falls through mac expectations :(
Comment on attachment 149744 [details] Added chromium expectations too, since it seems it first falls through mac expectations :( I'm confused why so many computed-style results are needed here? You're just turning this on for Windows, no? Alos, I thought Arv recently removed/made-consistent the computed-style tests?
Yes, but I want to make sure that it passes on all ports that have regions enabled. I haven't seen any changes to make the computed style tests aware of regions in the initial patch. So I thought this was the right way to do it. Maybe I'm missing something, I feel like I'm playing with things way above my head :(
Comment on attachment 149744 [details] Added chromium expectations too, since it seems it first falls through mac expectations :( Please run "webkit-patch optimize-baselines fast/css/getComputedStyle/computed-style.html" locally and re-upload. I expect you'll find you don't need this many copies of computed-style-expected.txt
(In reply to comment #30) > (From update of attachment 149744 [details]) > I'm confused why so many computed-style results are needed here? You're just turning this on for Windows, no? Alos, I thought Arv recently removed/made-consistent the computed-style tests? I didn't remove the computed style tests. Ojan said he was going to remove that one...
(In reply to comment #33) > (In reply to comment #30) > > (From update of attachment 149744 [details] [details]) > > I'm confused why so many computed-style results are needed here? You're just turning this on for Windows, no? Alos, I thought Arv recently removed/made-consistent the computed-style tests? > > I didn't remove the computed style tests. Ojan said he was going to remove that one... Might do it one day, but not anytime soon. If someone else wants to do this, go for it.
(In reply to comment #34) > (In reply to comment #33) > > (In reply to comment #30) > > > (From update of attachment 149744 [details] [details] [details]) > > > I'm confused why so many computed-style results are needed here? You're just turning this on for Windows, no? Alos, I thought Arv recently removed/made-consistent the computed-style tests? > > > > I didn't remove the computed style tests. Ojan said he was going to remove that one... > > Might do it one day, but not anytime soon. If someone else wants to do this, go for it. I got rid of the platform specific results in http://trac.webkit.org/changeset/120870 . They're not in ToT which is why it looks like you're adding the files in the diff.
(In reply to comment #35) > (In reply to comment #34) > > (In reply to comment #33) > > > (In reply to comment #30) > > > > (From update of attachment 149744 [details] [details] [details] [details]) > > > > I'm confused why so many computed-style results are needed here? You're just turning this on for Windows, no? Alos, I thought Arv recently removed/made-consistent the computed-style tests? > > > > > > I didn't remove the computed style tests. Ojan said he was going to remove that one... > > > > Might do it one day, but not anytime soon. If someone else wants to do this, go for it. > > I got rid of the platform specific results in http://trac.webkit.org/changeset/120870 . They're not in ToT which is why it looks like you're adding the files in the diff. Tony, could you give me some details regarding the r-?
Created attachment 150202 [details] Removed expectations files to play nicely with property-names.js
Created attachment 150205 [details] Removed expectation files from ChangeLog, too.
Created attachment 150206 [details] Fixed copy/paste error in Source/WebKit/win/WebPreferenceKeysPrivate.h
Comment on attachment 150206 [details] Fixed copy/paste error in Source/WebKit/win/WebPreferenceKeysPrivate.h It would be nice to have the win EWS bot cycle green before landing.
Created attachment 150213 [details] Fixed patch. Previous was hand-edited and had some inconsistencies.
Comment on attachment 150213 [details] Fixed patch. Previous was hand-edited and had some inconsistencies. I'll cq+ after the win EWS bot goes green.
Comment on attachment 150213 [details] Fixed patch. Previous was hand-edited and had some inconsistencies. Clearing flags on attachment: 150213 Committed r121597: <http://trac.webkit.org/changeset/121597>