Bug 35530 - Enum value FORWARD, BACKWARD, RIGHT, LEFT are causing macro conflicts.
Summary: Enum value FORWARD, BACKWARD, RIGHT, LEFT are causing macro conflicts.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-01 08:55 PST by Lyon Chen
Modified: 2010-06-01 07:19 PDT (History)
11 users (show)

See Also:


Attachments
Simple fix for bug 35530. (1.06 KB, patch)
2010-03-01 09:03 PST, Lyon Chen
darin: review-
Details | Formatted Diff | Diff
WebCore patch for bug 35530. (44.23 KB, patch)
2010-03-01 21:21 PST, Lyon Chen
no flags Details | Formatted Diff | Diff
WebKit/qt patch for bug 35530. (1.49 KB, patch)
2010-03-01 21:22 PST, Lyon Chen
no flags Details | Formatted Diff | Diff
WebKit/haiku patch for bug 35530. (4.07 KB, patch)
2010-03-01 21:22 PST, Lyon Chen
no flags Details | Formatted Diff | Diff
WebKit/chromium patch for bug 35530. (1.55 KB, patch)
2010-03-01 21:23 PST, Lyon Chen
no flags Details | Formatted Diff | Diff
Combined patch for bug 35530. (52.41 KB, patch)
2010-03-01 21:59 PST, Lyon Chen
no flags Details | Formatted Diff | Diff
Combined patch for bug 35530, with style issue fixed. (129.69 KB, patch)
2010-03-02 09:16 PST, Lyon Chen
eric: review-
eric: commit-queue-
Details | Formatted Diff | Diff
Coding style patch. (116.53 KB, patch)
2010-03-25 21:36 PDT, Lyon Chen
darin: review-
Details | Formatted Diff | Diff
Patch for bug 35530. (50.14 KB, patch)
2010-03-26 09:08 PDT, Lyon Chen
ojan: review-
Details | Formatted Diff | Diff
Cleanup patch for 35530 (122.75 KB, patch)
2010-05-30 09:55 PDT, Lyon Chen
no flags Details | Formatted Diff | Diff
Patch for bug 35530. (61.80 KB, patch)
2010-05-30 09:55 PDT, Lyon Chen
darin: review+
Details | Formatted Diff | Diff
Patch for bug 35530 with the "E" in all enumeration values removed. (61.56 KB, patch)
2010-05-30 11:21 PDT, Lyon Chen
darin: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch updated to 108b044c (61.57 KB, patch)
2010-05-31 17:35 PDT, Lyon Chen
tkent: review-
Details | Formatted Diff | Diff
Patch regenerated. (55.65 KB, patch)
2010-05-31 17:52 PDT, Lyon Chen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lyon Chen 2010-03-01 08:55:46 PST
Enum value FORWARD, BACKWARD, RIGHT, LEFT of EDirection in SelectionController will cause compile error if any of them is defined as MACRO before. Undefine all of them ahead of EDirection declaration remove potential conflict.

undef FORWARD
undef BACKWARD
undef RIGHT
undef LEFT

Of course this doesn't really remove the conflict, so maybe we should use (EForward, EBackward, ERight, ELeft) for EDirection values instead?
Comment 1 Lyon Chen 2010-03-01 09:03:32 PST
Created attachment 49727 [details]
Simple fix for bug 35530.
Comment 2 Darin Adler 2010-03-01 14:06:23 PST
Comment on attachment 49727 [details]
Simple fix for bug 35530.

A change like this needs a comment. Having these #undef at the top of the file with no comment is not good.

The extra blank line before the class definition is not wanted.

A better change would be to change these identifiers. There's no reason to use all capitals for these. While the #undef will work it's probably not a major project to simply change these to use normal mixed case as per the WebKit coding guidelines.

review- because there needs to be a comment
Comment 3 Lyon Chen 2010-03-01 14:28:01 PST
OK, will change these enumeration define instead.
Comment 4 Lyon Chen 2010-03-01 21:21:17 PST
Created attachment 49776 [details]
WebCore patch for bug 35530.
Comment 5 Lyon Chen 2010-03-01 21:22:10 PST
Created attachment 49777 [details]
WebKit/qt patch for bug 35530.
Comment 6 Lyon Chen 2010-03-01 21:22:55 PST
Created attachment 49778 [details]
WebKit/haiku patch for bug 35530.
Comment 7 Lyon Chen 2010-03-01 21:23:29 PST
Created attachment 49779 [details]
WebKit/chromium patch for bug 35530.
Comment 8 WebKit Review Bot 2010-03-01 21:27:56 PST
Attachment 49776 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/editing/SelectionController.cpp:243:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/editing/SelectionController.cpp:609:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/editing/SelectionController.cpp:645:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/editing/SelectionController.cpp:707:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/editing/SelectionController.cpp:749:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/editing/Editor.cpp:244:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 6 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 WebKit Review Bot 2010-03-01 21:30:21 PST
Attachment 49779 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/320509
Comment 10 WebKit Review Bot 2010-03-01 21:41:00 PST
Attachment 49776 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/321431
Comment 11 Lyon Chen 2010-03-01 21:59:54 PST
Created attachment 49780 [details]
Combined patch for bug 35530.
Comment 12 WebKit Review Bot 2010-03-01 22:06:23 PST
Attachment 49780 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/editing/Editor.cpp:244:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Lyon Chen 2010-03-02 09:16:23 PST
Created attachment 49807 [details]
Combined patch for bug 35530, with style issue fixed.

This patch also include many coding style fix for files related to this bug, to pass coding style check.
Comment 14 George Staikos 2010-03-02 09:34:55 PST
Sorry to be a pain but is it possible to separate the style changes from the actual patch?  They could go in separately.
Comment 15 Lyon Chen 2010-03-02 09:52:56 PST
George, that will mean 2 patches for this one bug, is that fine? If it's fine I can do it. Or should I open a new bug just for the coding style change? (Personally I prefer not open a new bug).
Comment 16 George Staikos 2010-03-02 10:40:13 PST
Comment on attachment 49807 [details]
Combined patch for bug 35530, with style issue fixed.

I guess it's fine as-is.
Comment 17 WebKit Commit Bot 2010-03-02 14:40:37 PST
Comment on attachment 49807 [details]
Combined patch for bug 35530, with style issue fixed.

Rejecting patch 49807 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--no-update', '--parent-command=commit-queue', '--build-style=both', '--quiet', '49807']" exit_code: 1
Last 500 characters of output:
ebkitpy/statusserver.py", line 87, in update_status
    return NetworkTransaction().run(lambda: self._post_to_server(queue_name, status, patch, results_file))
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/networktransaction.py", line 52, in run
    self._check_for_timeout()
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/networktransaction.py", line 58, in _check_for_timeout
    raise NetworkTimeout()
webkitpy.networktransaction.NetworkTimeout
Comment 18 Lyon Chen 2010-03-02 20:09:39 PST
File a bug against webkit-patch? Is this what supposed to be done?
Comment 19 Lyon Chen 2010-03-02 20:55:26 PST
As error info shown below, isn't this just a network timeout?

line 52, in run
    self._check_for_timeout()
  File
"/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/networktransaction.py",
line 58, in _check_for_timeout
    raise NetworkTimeout()
webkitpy.networktransaction.NetworkTimeout
Comment 20 Adam Barth 2010-03-02 21:50:30 PST
Eric: Is this related to my recent change to the network timeout code?
Comment 21 Eric Seidel (no email) 2010-03-05 12:14:38 PST
Comment on attachment 49807 [details]
Combined patch for bug 35530, with style issue fixed.

Not sure, but lets try again.
Comment 22 WebKit Commit Bot 2010-03-05 19:12:07 PST
Comment on attachment 49807 [details]
Combined patch for bug 35530, with style issue fixed.

Rejecting patch 49807 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--no-update', '--parent-command=commit-queue', '--build-style=both', '--quiet', '49807']" exit_code: 1
Last 500 characters of output:
ebkitpy/statusserver.py", line 87, in update_status
    return NetworkTransaction().run(lambda: self._post_to_server(queue_name, status, patch, results_file))
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/networktransaction.py", line 52, in run
    self._check_for_timeout()
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/networktransaction.py", line 58, in _check_for_timeout
    raise NetworkTimeout()
webkitpy.networktransaction.NetworkTimeout
Comment 23 Eric Seidel (no email) 2010-03-05 19:23:49 PST
Comment on attachment 49807 [details]
Combined patch for bug 35530, with style issue fixed.

We don't know what's causing this yet.  Adam added this code recently.  We'll figure it out. :)
Comment 24 Eric Seidel (no email) 2010-03-05 19:25:47 PST
Comment on attachment 49807 [details]
Combined patch for bug 35530, with style issue fixed.

It's hitting an error due to the failure log being too large.  This must be breaking Mac somehow.
Comment 25 Eric Seidel (no email) 2010-03-24 20:08:43 PDT
Comment on attachment 49807 [details]
Combined patch for bug 35530, with style issue fixed.

Lyon isn't a committer.  So it seems that the cq- is effectively the same as an r-.  Please revise and resubmit this patch.
Comment 26 Lyon Chen 2010-03-25 21:36:20 PDT
Created attachment 51713 [details]
Coding style patch.

This is a pre-patch, it fixes the coding style issues in files to be changed.
Comment 27 Lyon Chen 2010-03-26 09:08:10 PDT
Created attachment 51746 [details]
Patch for bug 35530.

This is the real change.
Comment 28 Darin Adler 2010-03-26 09:15:13 PDT
Comment on attachment 51713 [details]
Coding style patch.

>  #include "AXObjectCache.h"
>  #include "ApplyStyleCommand.h"
> -#include "CharacterNames.h"
> -#include "CompositionEvent.h"
> -#include "CreateLinkCommand.h"
>  #include "CSSComputedStyleDeclaration.h"
>  #include "CSSMutableStyleDeclaration.h"
>  #include "CSSProperty.h"
>  #include "CSSPropertyNames.h"
>  #include "CSSValueKeywords.h"
> +#include "CharacterNames.h"
>  #include "ClipboardEvent.h"
> +#include "CompositionEvent.h"
> +#include "CreateLinkCommand.h"
>  #include "DeleteButtonController.h"
>  #include "DeleteSelectionCommand.h"
>  #include "DocLoader.h"

This is incorrect. The coding style specifies:

    "#include statements should be in sorted order (case sensitive, as done by the command-line sort tool or the Xcode sort selection command)."

You have changed them from case sensitive sort order to a different sort order.
Comment 29 Lyon Chen 2010-03-26 09:19:52 PDT
Hmmm, so the current check-webkit-style has bug? It give me zero error for all files.
Comment 30 Lyon Chen 2010-03-26 09:29:12 PDT
Darin, checked again, could not find what's wrong. Could you please let me know which line of change in Editor.cpp is in wrong sort order?
Comment 31 Kent Tamura 2010-05-20 03:31:40 PDT
Comment on attachment 51713 [details]
Coding style patch.

(In reply to comment #30)
> Darin, checked again, could not find what's wrong. Could you please let me know which line of change in Editor.cpp is in wrong sort order?

I think your change is correct.


WebCore/ChangeLog:5
 +          Enum value FORWARD, BACKWARD, RIGHT, LEFT are causing macro conflicts.
 +          https://bugs.webkit.org/show_bug.cgi?id=35530
This 1-line description doesn't represent the change.


WebCore/ChangeLog:8
 +          No new tests. (OOPS!)
Remove this line.
Comment 32 Ojan Vafai 2010-05-27 14:19:22 PDT
Comment on attachment 51746 [details]
Patch for bug 35530.

Ugh. Sorry, my comment got lost with my r-. Anyways, the code changes look good here. r- for the changelog issues in comment 30.

Sorry for the long wait on review. I can r+ this quickly once the changelog issues are resolved.
Comment 33 Ojan Vafai 2010-05-27 14:20:11 PDT
Ugh. By comment 30, I obviously mean 31.
Comment 34 Lyon Chen 2010-05-27 14:24:01 PDT
Thanks, Ojan, will update.
Comment 35 Ojan Vafai 2010-05-27 14:40:54 PDT
Comment on attachment 51713 [details]
Coding style patch.

Sorry you had to wait so long to get a followup review here. I ran the includes in question through sort and it looks like what you have is correct. So, I can r+ this once you fix the ChangeLog descriptions and update this patch to tip of tree.

WebCore/ChangeLog:8
 +          No new tests. (OOPS!)
Please remove this line and add a line explaining that these are style cleanups in preparation for fixing bug 35530.

WebKit/qt/ChangeLog:7
 +  
Please add a line explaining that these are style cleanups in preparation for fixing bug 35530.
Comment 36 Lyon Chen 2010-05-30 09:55:14 PDT
Created attachment 57420 [details]
Cleanup patch for 35530
Comment 37 Lyon Chen 2010-05-30 09:55:59 PDT
Created attachment 57421 [details]
Patch for bug 35530.
Comment 38 Darin Adler 2010-05-30 11:06:48 PDT
Comment on attachment 57421 [details]
Patch for bug 35530.

These are fine except for the "E" prefix. They should be "DirectionForward" not "EDirectionForward". I know the enums currently have names with "E" prefixes, but it's not our standard coding style and pretty ugly!

I'm going to say r=me assuming this doesn't break the build, but I don't like all that "E".
Comment 39 Lyon Chen 2010-05-30 11:21:59 PDT
Created attachment 57424 [details]
Patch for bug 35530 with the "E" in all enumeration values removed.

Hi, Darin, I removed the "E" prefix in all these 6 changed enumeration values in the new patch.
Comment 40 Darin Adler 2010-05-30 11:24:33 PDT
Comment on attachment 57424 [details]
Patch for bug 35530 with the "E" in all enumeration values removed.

r=me
Comment 41 Kent Tamura 2010-05-30 15:26:45 PDT
Comment on attachment 57420 [details]
Cleanup patch for 35530

OK.
Comment 42 WebKit Commit Bot 2010-05-30 17:52:07 PDT
Comment on attachment 57420 [details]
Cleanup patch for 35530

Clearing flags on attachment: 57420

Committed r60421: <http://trac.webkit.org/changeset/60421>
Comment 43 Nikolas Zimmermann 2010-05-31 06:54:53 PDT
Comment on attachment 57424 [details]
Patch for bug 35530 with the "E" in all enumeration values removed.

Set cq+, on Lyons request.
Comment 44 WebKit Commit Bot 2010-05-31 07:30:50 PDT
Comment on attachment 57424 [details]
Patch for bug 35530 with the "E" in all enumeration values removed.

Rejecting patch 57424 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 57424, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
date_status
    return NetworkTransaction().run(lambda: self._post_status_to_server(queue_name, status, patch, results_file))
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 56, in run
    self._check_for_timeout()
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 63, in _check_for_timeout
    raise NetworkTimeout()
webkitpy.common.net.networktransaction.NetworkTimeout
Comment 45 Eric Seidel (no email) 2010-05-31 10:28:19 PDT
Comment on attachment 57424 [details]
Patch for bug 35530 with the "E" in all enumeration values removed.

The NetworkTimeout error often means that it's trying to upload a failure log which is too large for the server.  Which means this patch probably fails on Mac.

Looks like it also fails to apply for the stylebot.  We'll try again just to make sure.
Comment 46 WebKit Commit Bot 2010-05-31 14:18:40 PDT
Comment on attachment 57424 [details]
Patch for bug 35530 with the "E" in all enumeration values removed.

Rejecting patch 57424 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 57424, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
date_status
    return NetworkTransaction().run(lambda: self._post_status_to_server(queue_name, status, patch, results_file))
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 56, in run
    self._check_for_timeout()
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 63, in _check_for_timeout
    raise NetworkTimeout()
webkitpy.common.net.networktransaction.NetworkTimeout
Comment 47 Lyon Chen 2010-05-31 17:35:01 PDT
Created attachment 57501 [details]
Patch updated to 108b044c

Updated to commit till 108b044c.
Comment 48 Kent Tamura 2010-05-31 17:39:07 PDT
Comment on attachment 57501 [details]
Patch updated to 108b044c

The diff for WebCore/ChangeLog is not a good format.  Please make a patch with the latest revision of WebKit.
Comment 49 Lyon Chen 2010-05-31 17:52:57 PDT
Created attachment 57503 [details]
Patch regenerated.
Comment 50 Kent Tamura 2010-05-31 20:30:20 PDT
Comment on attachment 57503 [details]
Patch regenerated.

OK
Comment 51 WebKit Commit Bot 2010-05-31 21:03:25 PDT
Comment on attachment 57503 [details]
Patch regenerated.

Rejecting patch 57503 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 57503, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
date_status
    return NetworkTransaction().run(lambda: self._post_status_to_server(queue_name, status, patch, results_file))
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 56, in run
    self._check_for_timeout()
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/net/networktransaction.py", line 63, in _check_for_timeout
    raise NetworkTimeout()
webkitpy.common.net.networktransaction.NetworkTimeout
Comment 52 Lyon Chen 2010-05-31 21:12:54 PDT
Already reported a bug (https://bugs.webkit.org/show_bug.cgi?id=39954) for this patch landing error, and this is at least the 3rd time, not sure what to do anymore. Before the patch landing script issues are fixed, I don't see I have any choice but giving up.
Comment 53 Eric Seidel (no email) 2010-05-31 21:46:27 PDT
This patch breaks mac.  But the error report is too large to upload.  that's why the commit-bot is failing.  someone will have to test this on mac manually to get you the error.
Comment 54 Kent Tamura 2010-05-31 23:12:01 PDT
The error was:

WebKit/mac/WebView/WebTextCompletionController.mm: In function ‘void -[WebTextCompletionController doCompletion](WebTextCompletionController*, objc_selector*)’:
WebKit/mac/WebView/WebTextCompletionController.mm:175: error: ‘EXTEND’ is not a member of ‘WebCore::SelectionController’
WebKit/mac/WebView/WebTextCompletionController.mm:176: error: ‘BACKWARD’ is not a member of ‘WebCore::SelectionController’

ok, I'll land the patch with a fix for the above error.
Comment 55 Kent Tamura 2010-05-31 23:13:42 PDT
Committed r60463: <http://trac.webkit.org/changeset/60463>
Comment 56 Lyon Chen 2010-06-01 07:19:04 PDT
Thanks, Kent. Now I know why I failed at Mac build, because when I did a global search, I forgot to search for .m/.mm files!