Summary: | Add ENABLE(CSS_REGIONS) guard for CSS Regions support | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mihnea Ovidenie <mihnea> | ||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, hyatt, tkent, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 57312 | ||||||||||
Attachments: |
|
Description
Mihnea Ovidenie
2011-05-27 07:05:26 PDT
Created attachment 95175 [details]
Patch
First version of the patch. The patch modifies the project files for Mac build and for Windows (Cairo too) build.
Comment on attachment 95175 [details]
Patch
The change looks ok.
I feel ENABLE_REGIONS is too simple, and like ENABLE_CSS_REGIONS, ENABLE_REGIONS_LAYOUT, or something.
(In reply to comment #2) > (From update of attachment 95175 [details]) > The change looks ok. > > I feel ENABLE_REGIONS is too simple, and like ENABLE_CSS_REGIONS, ENABLE_REGIONS_LAYOUT, or something. I used ENABLE(REGIONS) since this was the original guard in our work. ENABLE(CSS_REGIONS) may be a better name. While looking over the FeatureDefines.xcconfig files, i was not able to find out any defines using 'CSS' or 'LAYOUT' in their names. (In reply to comment #3) > I used ENABLE(REGIONS) since this was the original guard in our work. It makes no sense for WebKit community. > While looking over the FeatureDefines.xcconfig files, i was not able to find out any defines using 'CSS' or 'LAYOUT' in their names. That menas we have no other flags for CSS or layout. We have ENABLE_SVG_USE. Do you think it's ok to name it ENABLE_USE if no other SVG flags? (In reply to comment #4) > (In reply to comment #3) > > I used ENABLE(REGIONS) since this was the original guard in our work. > > It makes no sense for WebKit community. > Ok, you know better. > > While looking over the FeatureDefines.xcconfig files, i was not able to find out any defines using 'CSS' or 'LAYOUT' in their names. > > That menas we have no other flags for CSS or layout. > > We have ENABLE_SVG_USE. Do you think it's ok to name it ENABLE_USE if no other SVG flags? Good point. In this case, i vote for ENABLE_CSS_REGIONS. Should i rewrite the patch to use ENABLE_CSS_REGIONS and post a new version of the patch? Are you going to review it? (In reply to comment #5) > Good point. In this case, i vote for ENABLE_CSS_REGIONS. Should i rewrite the patch to use ENABLE_CSS_REGIONS and post a new version of the patch? Are you going to review it? Yeah, I'll review soon when you post a new patch. Created attachment 96066 [details]
Patch
I have changed ENABLE(REGIONS) to ENABLE(CSS_REGIONS).
Comment on attachment 96066 [details]
Patch
Looks ok.
Comment on attachment 96066 [details] Patch Rejecting attachment 96066 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'land-a..." exit_code: 2 Last 500 characters of output: ing: cannot set LC_CTYPE locale svnlook: warning: environment variable LANG is not set svnlook: warning: please check that your locale name is correct The following ChangeLog files contain OOPS: trunk/Source/WebCore/ChangeLog Please don't ever say "OOPS" in a ChangeLog file. at /usr/local/git/libexec/git-core/git-svn line 576 Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Updating OpenSource Current branch master is up to date. Full output: http://queues.webkit.org/results/8776046 Comment on attachment 96066 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96066&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) You need to update this line. e.g. adding a reason why there is no tests. Created attachment 96068 [details]
Second patch
Comment on attachment 96068 [details] Second patch Clearing flags on attachment: 96068 Committed r88148: <http://trac.webkit.org/changeset/88148> All reviewed patches have been landed. Closing bug. |