Bug 88645 - [CSS Regions] Adding feature defines for CSS Regions for Windows
Summary: [CSS Regions] Adding feature defines for CSS Regions for Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mihai Balan
URL:
Keywords:
Depends on: 89273
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-08 04:38 PDT by Mihai Balan
Modified: 2012-06-29 15:51 PDT (History)
14 users (show)

See Also:


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-
Details | Formatted Diff | Diff
Another attempt at fixing regions on Windows. (3.61 KB, patch)
2012-06-11 06:58 PDT, Mihai Balan
no flags Details | Formatted Diff | Diff
FIxed style (3.67 KB, patch)
2012-06-11 07:07 PDT, Mihai Balan
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
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-
Details | Formatted Diff | Diff
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 Details
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 Details
Fixing layout test failures on cr-linux (72.78 KB, patch)
2012-06-27 03:37 PDT, Mihai Balan
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
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 Details
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 Details
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-
Details | Formatted Diff | Diff
Removed expectations files to play nicely with property-names.js (11.48 KB, patch)
2012-06-29 09:59 PDT, Mihai Balan
no flags Details | Formatted Diff | Diff
Removed expectation files from ChangeLog, too. (10.47 KB, patch)
2012-06-29 10:07 PDT, Mihai Balan
no flags Details | Formatted Diff | Diff
Fixed copy/paste error in Source/WebKit/win/WebPreferenceKeysPrivate.h (10.47 KB, patch)
2012-06-29 10:09 PDT, Mihai Balan
tony: review+
Details | Formatted Diff | Diff
Fixed patch. Previous was hand-edited and had some inconsistencies. (10.47 KB, patch)
2012-06-29 10:25 PDT, Mihai Balan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Balan 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.
Comment 1 Mihai Balan 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
Comment 2 Build Bot 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
Comment 3 Tim Horton 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.
Comment 4 Mihai Balan 2012-06-11 06:58:59 PDT
Created attachment 146850 [details]
Another attempt at fixing regions on Windows.
Comment 5 WebKit Review Bot 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.
Comment 6 Mihai Balan 2012-06-11 07:07:34 PDT
Created attachment 146851 [details]
FIxed style
Comment 7 Tim Horton 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).
Comment 8 Mihai Balan 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.
Comment 9 Tim Horton 2012-06-13 15:47:57 PDT
*** Bug 87519 has been marked as a duplicate of this bug. ***
Comment 10 WebKit Review Bot 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>
Comment 11 WebKit Review Bot 2012-06-13 22:23:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Jon Honeycutt 2012-06-15 15:23:50 PDT
This change is causing all layout tests to crash on Windows.
Comment 13 WebKit Review Bot 2012-06-15 21:15:57 PDT
Re-opened since this is blocked by 89273
Comment 14 Mihai Balan 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
Comment 15 WebKit Review Bot 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.
Comment 16 Mihai Balan 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.
Comment 17 Eric Seidel (no email) 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. :(
Comment 18 Eric Seidel (no email) 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.
Comment 19 Mihai Balan 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.
Comment 20 WebKit Review Bot 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
Comment 21 WebKit Review Bot 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
Comment 22 WebKit Review Bot 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
Comment 23 WebKit Review Bot 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
Comment 24 Mihai Balan 2012-06-27 03:37:24 PDT
Created attachment 149718 [details]
Fixing layout test failures on cr-linux
Comment 25 WebKit Review Bot 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
Comment 26 WebKit Review Bot 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
Comment 27 WebKit Review Bot 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
Comment 28 WebKit Review Bot 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
Comment 29 Mihai Balan 2012-06-27 07:17:45 PDT
Created attachment 149744 [details]
Added chromium expectations too, since it seems it first falls through mac expectations :(
Comment 30 Eric Seidel (no email) 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?
Comment 31 Mihai Balan 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 :(
Comment 32 Eric Seidel (no email) 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
Comment 33 Erik Arvidsson 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...
Comment 34 Ojan Vafai 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.
Comment 35 Tony Chang 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.
Comment 36 Mihai Balan 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-?
Comment 37 Mihai Balan 2012-06-29 09:59:29 PDT
Created attachment 150202 [details]
Removed expectations files to play nicely with property-names.js
Comment 38 Mihai Balan 2012-06-29 10:07:17 PDT
Created attachment 150205 [details]
Removed expectation files from ChangeLog, too.
Comment 39 Mihai Balan 2012-06-29 10:09:44 PDT
Created attachment 150206 [details]
Fixed copy/paste error in Source/WebKit/win/WebPreferenceKeysPrivate.h
Comment 40 Tony Chang 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.
Comment 41 Mihai Balan 2012-06-29 10:25:13 PDT
Created attachment 150213 [details]
Fixed patch. Previous was hand-edited and had some inconsistencies.
Comment 42 Tony Chang 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.
Comment 43 WebKit Review Bot 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>
Comment 44 WebKit Review Bot 2012-06-29 15:51:31 PDT
All reviewed patches have been landed.  Closing bug.