Summary: | Enum value FORWARD, BACKWARD, RIGHT, LEFT are causing macro conflicts. | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Lyon Chen <lyon.chen> | ||||||||||||||||||||||||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, dglazkov, eric, laszlo.gombos, liachen, ojan, staikos, tkent, webkit.review.bot, yong.li.webkit | ||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Lyon Chen
2010-03-01 08:55:46 PST
Created attachment 49727 [details] Simple fix for bug 35530. 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 OK, will change these enumeration define instead. Created attachment 49776 [details] WebCore patch for bug 35530. Created attachment 49777 [details] WebKit/qt patch for bug 35530. Created attachment 49778 [details] WebKit/haiku patch for bug 35530. Created attachment 49779 [details] WebKit/chromium patch for bug 35530. 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.
Attachment 49779 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/320509 Attachment 49776 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/321431 Created attachment 49780 [details] Combined patch for bug 35530. 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.
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. Sorry to be a pain but is it possible to separate the style changes from the actual patch? They could go in separately. 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 on attachment 49807 [details] Combined patch for bug 35530, with style issue fixed. I guess it's fine as-is. 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 File a bug against webkit-patch? Is this what supposed to be done? 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 Eric: Is this related to my recent change to the network timeout code? Comment on attachment 49807 [details] Combined patch for bug 35530, with style issue fixed. Not sure, but lets try again. 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 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 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 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. Created attachment 51713 [details]
Coding style patch.
This is a pre-patch, it fixes the coding style issues in files to be changed.
Created attachment 51746 [details] Patch for bug 35530. This is the real change. 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. Hmmm, so the current check-webkit-style has bug? It give me zero error for all files. 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 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 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. Ugh. By comment 30, I obviously mean 31. Thanks, Ojan, will update. 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. Created attachment 57420 [details]
Cleanup patch for 35530
Created attachment 57421 [details] Patch for bug 35530. 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". 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 on attachment 57424 [details] Patch for bug 35530 with the "E" in all enumeration values removed. r=me Comment on attachment 57420 [details]
Cleanup patch for 35530
OK.
Comment on attachment 57420 [details] Cleanup patch for 35530 Clearing flags on attachment: 57420 Committed r60421: <http://trac.webkit.org/changeset/60421> Comment on attachment 57424 [details] Patch for bug 35530 with the "E" in all enumeration values removed. Set cq+, on Lyons request. 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 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 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 Created attachment 57501 [details]
Patch updated to 108b044c
Updated to commit till 108b044c.
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.
Created attachment 57503 [details]
Patch regenerated.
Comment on attachment 57503 [details]
Patch regenerated.
OK
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
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. 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. 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. Committed r60463: <http://trac.webkit.org/changeset/60463> 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! |