Bug 61631

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.