Bug 35530 – Enum value FORWARD, BACKWARD, RIGHT, LEFT are causing macro conflicts.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Lyon Chen]

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?

[Lyon Chen]

Created attachment 49727 [details]
Simple fix for bug 35530.

[Darin Adler]

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

[Lyon Chen]

OK, will change these enumeration define instead.

[Lyon Chen]

Created attachment 49776 [details]
WebCore patch for bug 35530.

[Lyon Chen]

Created attachment 49777 [details]
WebKit/qt patch for bug 35530.

[Lyon Chen]

Created attachment 49778 [details]
WebKit/haiku patch for bug 35530.

[Lyon Chen]

Created attachment 49779 [details]
WebKit/chromium patch for bug 35530.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 49779 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/320509

[WebKit Review Bot]

Attachment 49776 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/321431

[Lyon Chen]

Created attachment 49780 [details]
Combined patch for bug 35530.

[WebKit Review Bot]

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.

[Lyon Chen]

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.

[George Staikos]

Sorry to be a pain but is it possible to separate the style changes from the actual patch?  They could go in separately.

[Lyon Chen]

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).

[George Staikos]

Comment on attachment 49807 [details]
Combined patch for bug 35530, with style issue fixed.

I guess it's fine as-is.

[WebKit Commit Bot]

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

[Lyon Chen]

File a bug against webkit-patch? Is this what supposed to be done?

[Lyon Chen]

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

[Adam Barth]

Eric: Is this related to my recent change to the network timeout code?

[Eric Seidel (no email)]

Comment on attachment 49807 [details]
Combined patch for bug 35530, with style issue fixed.

Not sure, but lets try again.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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. :)

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Lyon Chen]

Created attachment 51713 [details]
Coding style patch.

This is a pre-patch, it fixes the coding style issues in files to be changed.

[Lyon Chen]

Created attachment 51746 [details]
Patch for bug 35530.

This is the real change.

[Darin Adler]

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.

[Lyon Chen]

Hmmm, so the current check-webkit-style has bug? It give me zero error for all files.

[Lyon Chen]

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?

[Kent Tamura]

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.

[Ojan Vafai]

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.

[Ojan Vafai]

Ugh. By comment 30, I obviously mean 31.

[Lyon Chen]

Thanks, Ojan, will update.

[Ojan Vafai]

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.

[Lyon Chen]

Created attachment 57420 [details]
Cleanup patch for 35530

[Lyon Chen]

Created attachment 57421 [details]
Patch for bug 35530.

[Darin Adler]

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".

[Lyon Chen]

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.

[Darin Adler]

Comment on attachment 57424 [details]
Patch for bug 35530 with the "E" in all enumeration values removed.

r=me

[Kent Tamura]

Comment on attachment 57420 [details]
Cleanup patch for 35530

OK.

[WebKit Commit Bot]

Comment on attachment 57420 [details]
Cleanup patch for 35530

Clearing flags on attachment: 57420

Committed r60421: <http://trac.webkit.org/changeset/60421>

[Nikolas Zimmermann]

Comment on attachment 57424 [details]
Patch for bug 35530 with the "E" in all enumeration values removed.

Set cq+, on Lyons request.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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.

[WebKit Commit Bot]

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

[Lyon Chen]

Created attachment 57501 [details]
Patch updated to 108b044c

Updated to commit till 108b044c.

[Kent Tamura]

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.

[Lyon Chen]

Created attachment 57503 [details]
Patch regenerated.

[Kent Tamura]

Comment on attachment 57503 [details]
Patch regenerated.

OK

[WebKit Commit Bot]

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

[Lyon Chen]

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.

[Eric Seidel (no email)]

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.

[Kent Tamura]

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.

[Kent Tamura]

Committed r60463: <http://trac.webkit.org/changeset/60463>

[Lyon Chen]

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!