RESOLVED FIXED 88645
[CSS Regions] Adding feature defines for CSS Regions for Windows
https://bugs.webkit.org/show_bug.cgi?id=88645
Summary [CSS Regions] Adding feature defines for CSS Regions for Windows
Mihai Balan
Reported 2012-06-08 04:38:57 PDT
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.
Attachments
Added proper values to ENABLE_CSS_REGIONS & ENABLE_CSS_EXCLUSIONS in FeatureDefines.vsprops and FeatureDefinesCairo.vsprops (2.27 KB, patch)
2012-06-08 06:02 PDT, Mihai Balan
buildbot: commit-queue-
Another attempt at fixing regions on Windows. (3.61 KB, patch)
2012-06-11 06:58 PDT, Mihai Balan
no flags
FIxed style (3.67 KB, patch)
2012-06-11 07:07 PDT, Mihai Balan
no flags
Pretty much the same as the initial patch, but this time I touched CSSPropertyNames.in to trigger a proper rebuild on Windows. (3.80 KB, patch)
2012-06-13 11:48 PDT, Mihai Balan
no flags
Enabling only regions this time. Also with additions to the settings part and a updated expectations for a couple of tests (12.00 KB, patch)
2012-06-26 10:00 PDT, Mihai Balan
no flags
Fixed style although I'm not sure that removing the parameter name in WebPreferences.h was the right way to do it. (11.99 KB, patch)
2012-06-26 10:17 PDT, Mihai Balan
eric: review+
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cq-03 (573.60 KB, application/zip)
2012-06-26 15:04 PDT, WebKit Review Bot
no flags
Archive of layout-test-results from ec2-cr-linux-03 (596.16 KB, application/zip)
2012-06-26 17:53 PDT, WebKit Review Bot
no flags
Fixing layout test failures on cr-linux (72.78 KB, patch)
2012-06-27 03:37 PDT, Mihai Balan
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02 (584.79 KB, application/zip)
2012-06-27 06:09 PDT, WebKit Review Bot
no flags
Archive of layout-test-results from ec2-cr-linux-04 (614.14 KB, application/zip)
2012-06-27 07:11 PDT, WebKit Review Bot
no flags
Added chromium expectations too, since it seems it first falls through mac expectations :( (88.15 KB, patch)
2012-06-27 07:17 PDT, Mihai Balan
tony: review-
tony: commit-queue-
Removed expectations files to play nicely with property-names.js (11.48 KB, patch)
2012-06-29 09:59 PDT, Mihai Balan
no flags
Removed expectation files from ChangeLog, too. (10.47 KB, patch)
2012-06-29 10:07 PDT, Mihai Balan
no flags
Fixed copy/paste error in Source/WebKit/win/WebPreferenceKeysPrivate.h (10.47 KB, patch)
2012-06-29 10:09 PDT, Mihai Balan
tony: review+
Fixed patch. Previous was hand-edited and had some inconsistencies. (10.47 KB, patch)
2012-06-29 10:25 PDT, Mihai Balan
no flags
Mihai Balan
Comment 1 2012-06-08 06:02:15 PDT
Created attachment 146547 [details] Added proper values to ENABLE_CSS_REGIONS & ENABLE_CSS_EXCLUSIONS in FeatureDefines.vsprops and FeatureDefinesCairo.vsprops
Build Bot
Comment 2 2012-06-08 07:23:30 PDT
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
Tim Horton
Comment 3 2012-06-08 07:41:49 PDT
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.
Mihai Balan
Comment 4 2012-06-11 06:58:59 PDT
Created attachment 146850 [details] Another attempt at fixing regions on Windows.
WebKit Review Bot
Comment 5 2012-06-11 07:03:09 PDT
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.
Mihai Balan
Comment 6 2012-06-11 07:07:34 PDT
Created attachment 146851 [details] FIxed style
Tim Horton
Comment 7 2012-06-11 09:38:20 PDT
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).
Mihai Balan
Comment 8 2012-06-13 11:48:05 PDT
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.
Tim Horton
Comment 9 2012-06-13 15:47:57 PDT
*** Bug 87519 has been marked as a duplicate of this bug. ***
WebKit Review Bot
Comment 10 2012-06-13 22:23:01 PDT
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>
WebKit Review Bot
Comment 11 2012-06-13 22:23:06 PDT
All reviewed patches have been landed. Closing bug.
Jon Honeycutt
Comment 12 2012-06-15 15:23:50 PDT
This change is causing all layout tests to crash on Windows.
WebKit Review Bot
Comment 13 2012-06-15 21:15:57 PDT
Re-opened since this is blocked by 89273
Mihai Balan
Comment 14 2012-06-26 10:00:38 PDT
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
WebKit Review Bot
Comment 15 2012-06-26 10:09:29 PDT
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.
Mihai Balan
Comment 16 2012-06-26 10:17:43 PDT
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.
Eric Seidel (no email)
Comment 17 2012-06-26 11:02:10 PDT
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. :(
Eric Seidel (no email)
Comment 18 2012-06-26 11:03:01 PDT
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.
Mihai Balan
Comment 19 2012-06-26 11:09:30 PDT
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.
WebKit Review Bot
Comment 20 2012-06-26 15:04:35 PDT
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
WebKit Review Bot
Comment 21 2012-06-26 15:04:45 PDT
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
WebKit Review Bot
Comment 22 2012-06-26 17:53:25 PDT
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
WebKit Review Bot
Comment 23 2012-06-26 17:53:30 PDT
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
Mihai Balan
Comment 24 2012-06-27 03:37:24 PDT
Created attachment 149718 [details] Fixing layout test failures on cr-linux
WebKit Review Bot
Comment 25 2012-06-27 06:09:07 PDT
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
WebKit Review Bot
Comment 26 2012-06-27 06:09:12 PDT
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
WebKit Review Bot
Comment 27 2012-06-27 07:11:44 PDT
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
WebKit Review Bot
Comment 28 2012-06-27 07:11:49 PDT
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
Mihai Balan
Comment 29 2012-06-27 07:17:45 PDT
Created attachment 149744 [details] Added chromium expectations too, since it seems it first falls through mac expectations :(
Eric Seidel (no email)
Comment 30 2012-06-27 07:48:04 PDT
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?
Mihai Balan
Comment 31 2012-06-27 08:01:26 PDT
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 :(
Eric Seidel (no email)
Comment 32 2012-06-27 08:10:35 PDT
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
Erik Arvidsson
Comment 33 2012-06-27 13:37:55 PDT
(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...
Ojan Vafai
Comment 34 2012-06-27 13:48:44 PDT
(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.
Tony Chang
Comment 35 2012-06-27 14:18:27 PDT
(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.
Mihai Balan
Comment 36 2012-06-29 08:30:38 PDT
(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-?
Mihai Balan
Comment 37 2012-06-29 09:59:29 PDT
Created attachment 150202 [details] Removed expectations files to play nicely with property-names.js
Mihai Balan
Comment 38 2012-06-29 10:07:17 PDT
Created attachment 150205 [details] Removed expectation files from ChangeLog, too.
Mihai Balan
Comment 39 2012-06-29 10:09:44 PDT
Created attachment 150206 [details] Fixed copy/paste error in Source/WebKit/win/WebPreferenceKeysPrivate.h
Tony Chang
Comment 40 2012-06-29 10:14:03 PDT
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.
Mihai Balan
Comment 41 2012-06-29 10:25:13 PDT
Created attachment 150213 [details] Fixed patch. Previous was hand-edited and had some inconsistencies.
Tony Chang
Comment 42 2012-06-29 10:36:59 PDT
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.
WebKit Review Bot
Comment 43 2012-06-29 15:51:23 PDT
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>
WebKit Review Bot
Comment 44 2012-06-29 15:51:31 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.