Bug 88645 – [CSS Regions] Adding feature defines for CSS Regions for Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Mihai Balan]

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.

[Mihai Balan]

Created attachment 146547 [details]
Added proper values to ENABLE_CSS_REGIONS & ENABLE_CSS_EXCLUSIONS in FeatureDefines.vsprops and FeatureDefinesCairo.vsprops

[Build Bot]

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]

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]

Created attachment 146850 [details]
Another attempt at fixing regions on Windows.

[WebKit Review Bot]

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]

Created attachment 146851 [details]
FIxed style

[Tim Horton]

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]

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]

*** Bug 87519 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

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]

All reviewed patches have been landed.  Closing bug.

[Jon Honeycutt]

This change is causing all layout tests to crash on Windows.

[WebKit Review Bot]

Re-opened since this is blocked by 89273

[Mihai Balan]

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]

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]

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 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)]

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]

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 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]

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 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]

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]

Created attachment 149718 [details]
Fixing layout test failures on cr-linux

[WebKit Review Bot]

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]

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 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]

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]

Created attachment 149744 [details]
Added chromium expectations too, since it seems it first falls through mac expectations :(

[Eric Seidel (no email)]

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]

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 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]

(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]

(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]

(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]

(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]

Created attachment 150202 [details]
Removed expectations files to play nicely with property-names.js

[Mihai Balan]

Created attachment 150205 [details]
Removed expectation files from ChangeLog, too.

[Mihai Balan]

Created attachment 150206 [details]
Fixed copy/paste error in Source/WebKit/win/WebPreferenceKeysPrivate.h

[Tony Chang]

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]

Created attachment 150213 [details]
Fixed patch. Previous was hand-edited and had some inconsistencies.

[Tony Chang]

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 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]

All reviewed patches have been landed.  Closing bug.