Bug 49618 - Move CharacterNames.h into WTF directory
Summary: Move CharacterNames.h into WTF directory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Patrick R. Gansterer
URL:
Keywords:
Depends on:
Blocks: 49581
  Show dependency treegraph
 
Reported: 2010-11-16 13:19 PST by Patrick R. Gansterer
Modified: 2011-01-29 09:12 PST (History)
5 users (show)

See Also:


Attachments
Patch (37.35 KB, patch)
2010-11-18 02:25 PST, Patrick R. Gansterer
ddkilzer: review+
ddkilzer: commit-queue-
Details | Formatted Diff | Diff
Patch (50.51 KB, patch)
2011-01-29 08:47 PST, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2010-11-16 13:19:37 PST
see patch
Comment 1 Alexey Proskuryakov 2010-11-16 15:59:54 PST
This was prompted by bug 49581, where 0xFFFD was hardcoded. I'm very much unsure if all other characters need to be in WTF just because of U+FFFD.
Comment 2 Darin Adler 2010-11-16 16:05:07 PST
Alexey has a point. The CharacterNames.h header contains all the character names we use in WebCore, which is a tiny fraction of all the Unicode character names. It might be a little strange to have that exact set of characters in a WTF header.

But on the other hand, I don’t think it is all that helpful to have two of these headers, so maybe moving this header is indeed a good idea.

If we do move it, we have to make sure we do Maciej’s “using” technique so we don’t have to prefix all the uses of the constants with WTF.
Comment 3 Alexey Proskuryakov 2010-11-16 16:36:32 PST
Perhaps 0xFFFD could go to Unicode.h.
Comment 4 Patrick R. Gansterer 2010-11-18 02:25:09 PST
Created attachment 74219 [details]
Patch

AFIK CQ won't keep the history of moved files, so we need to commit in manually.
Comment 5 Darin Adler 2010-11-19 11:04:24 PST
Comment on attachment 74219 [details]
Patch

I suggested this change.

But Alexey has called into doubt the wisdom of moving a set of character names into WTF that just happens to be the set needed for WebCore. Alexey, do you still think that’s a bad idea? I don’t see harm in it, even though it’s a bit conceptually impure.
Comment 6 Alexey Proskuryakov 2010-11-19 12:04:15 PST
It's rather hard to pinpoint obvious harm from moving anything from WebCore to WTF.

I like to think of WTF as something that's as minimal as possible, and only contains code necessary for both JSC and WebCore. And ideographicFullStop is not one of those.
Comment 7 Darin Adler 2010-11-19 12:07:20 PST
(In reply to comment #6)
> I like to think of WTF as something that's as minimal as possible, and only contains code necessary for both JSC and WebCore. And ideographicFullStop is not one of those.

I think of WTF as something that has low level things suitable for sharing.

It seems to me that the names of Unicode characters fits into the same conceptual level as the string classes and ICU-style functions provided in WTF. The only thing unusual about CharacterNames.h is that it’s an arbitrary subset. I’d love to have symbolic names for all Unicode characters available in some header.
Comment 8 David Kilzer (:ddkilzer) 2011-01-22 22:39:15 PST
Comment on attachment 74219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74219&action=review

r- to use fewer using statements in CharacterNames.h.

> JavaScriptCore/wtf/unicode/CharacterNames.h:112
> +using WTF::Unicode::blackSquare;
> +using WTF::Unicode::bullet;
> +using WTF::Unicode::carriageReturn;
> +using WTF::Unicode::ethiopicPrefaceColon;
> +using WTF::Unicode::hebrewPunctuationGeresh;
> +using WTF::Unicode::hebrewPunctuationGershayim;
> +using WTF::Unicode::horizontalEllipsis;
> +using WTF::Unicode::hyphen;
> +using WTF::Unicode::hyphenMinus;
> +using WTF::Unicode::ideographicComma;
> +using WTF::Unicode::ideographicFullStop;
> +using WTF::Unicode::ideographicSpace;
> +using WTF::Unicode::leftDoubleQuotationMark;
> +using WTF::Unicode::leftSingleQuotationMark;
> +using WTF::Unicode::leftToRightEmbed;
> +using WTF::Unicode::leftToRightMark;
> +using WTF::Unicode::leftToRightOverride;
> +using WTF::Unicode::minusSign;
> +using WTF::Unicode::newlineCharacter;
> +using WTF::Unicode::noBreakSpace;
> +using WTF::Unicode::objectReplacementCharacter;
> +using WTF::Unicode::popDirectionalFormatting;
> +using WTF::Unicode::replacementCharacter;
> +using WTF::Unicode::rightDoubleQuotationMark;
> +using WTF::Unicode::rightSingleQuotationMark;
> +using WTF::Unicode::rightToLeftEmbed;
> +using WTF::Unicode::rightToLeftMark;
> +using WTF::Unicode::rightToLeftOverride;
> +using WTF::Unicode::softHyphen;
> +using WTF::Unicode::space;
> +using WTF::Unicode::whiteBullet;
> +using WTF::Unicode::yenSign;
> +using WTF::Unicode::zeroWidthJoiner;
> +using WTF::Unicode::zeroWidthNonJoiner;
> +using WTF::Unicode::zeroWidthSpace;

Can't you just use this instead?

using WTF::Unicode;
Comment 9 Alexey Proskuryakov 2011-01-22 23:03:25 PST
Actually, this was correct. For one thing, using declaration and using directive achieve different effect (see e.g. <http://www.devx.com/tips/Tip/12534>). And of course, we don't want to bring the whole WTF:Unicode into any file that includes CharacterNames.h.
Comment 10 Alexey Proskuryakov 2011-01-22 23:18:41 PST
This is a better link: <http://www.drdobbs.com/184401782;jsessionid=IC0T3CABSTWYZQE1GHPSKH4ATMY32JVN>.
Comment 11 Patrick R. Gansterer 2011-01-23 06:56:43 PST
Comment on attachment 74219 [details]
Patch

Setting back to r?

@ap, darin: Any consensus about this change?
Comment 12 David Kilzer (:ddkilzer) 2011-01-23 08:30:20 PST
Comment on attachment 74219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74219&action=review

r=me

I'll be the tie-breaker.  It's just a header full of constants, so it's easy enough to move back to WebCore if there is a compelling reason to do so.  I like the idea of preventing code duplication, even if it's only 1 character constant.  And it seems to fit in with JavaScriptCore/wtf/unicode just as well as WebCore/platform/text (if not a little more so).

>> JavaScriptCore/wtf/unicode/CharacterNames.h:112
>> +using WTF::Unicode::zeroWidthSpace;
> 
> Can't you just use this instead?
> 
> using WTF::Unicode;

Per @ap, you don't want to do this because that will make every method and constant in WTF::Unicode available without specifying a namespace, so this code is fine as-is.
Comment 13 David Kilzer (:ddkilzer) 2011-01-23 10:02:42 PST
(In reply to comment #8)
> (From update of attachment 74219 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74219&action=review
> 
> r- to use fewer using statements in CharacterNames.h.
> 
> > JavaScriptCore/wtf/unicode/CharacterNames.h:112
> > +using WTF::Unicode::blackSquare;
> > +using WTF::Unicode::bullet;
> > +using WTF::Unicode::carriageReturn;
> > +using WTF::Unicode::ethiopicPrefaceColon;
> > +using WTF::Unicode::hebrewPunctuationGeresh;
> > +using WTF::Unicode::hebrewPunctuationGershayim;
> > +using WTF::Unicode::horizontalEllipsis;
> > +using WTF::Unicode::hyphen;
> > +using WTF::Unicode::hyphenMinus;
> > +using WTF::Unicode::ideographicComma;
> > +using WTF::Unicode::ideographicFullStop;
> > +using WTF::Unicode::ideographicSpace;
> > +using WTF::Unicode::leftDoubleQuotationMark;
> > +using WTF::Unicode::leftSingleQuotationMark;
> > +using WTF::Unicode::leftToRightEmbed;
> > +using WTF::Unicode::leftToRightMark;
> > +using WTF::Unicode::leftToRightOverride;
> > +using WTF::Unicode::minusSign;
> > +using WTF::Unicode::newlineCharacter;
> > +using WTF::Unicode::noBreakSpace;
> > +using WTF::Unicode::objectReplacementCharacter;
> > +using WTF::Unicode::popDirectionalFormatting;
> > +using WTF::Unicode::replacementCharacter;
> > +using WTF::Unicode::rightDoubleQuotationMark;
> > +using WTF::Unicode::rightSingleQuotationMark;
> > +using WTF::Unicode::rightToLeftEmbed;
> > +using WTF::Unicode::rightToLeftMark;
> > +using WTF::Unicode::rightToLeftOverride;
> > +using WTF::Unicode::softHyphen;
> > +using WTF::Unicode::space;
> > +using WTF::Unicode::whiteBullet;
> > +using WTF::Unicode::yenSign;
> > +using WTF::Unicode::zeroWidthJoiner;
> > +using WTF::Unicode::zeroWidthNonJoiner;
> > +using WTF::Unicode::zeroWidthSpace;
> 
> Can't you just use this instead?
> 
> using WTF::Unicode;

(In reply to comment #9)
> Actually, this was correct. For one thing, using declaration and using directive achieve different effect (see e.g. <http://www.devx.com/tips/Tip/12534>). And of course, we don't want to bring the whole WTF:Unicode into any file that includes CharacterNames.h.

Another approach would be to put all the characters into another nested namespace like WTF::Unicode::CharacterName, then you could use:

using WTF::Unicode::CharacterName;

I just hate the maintenance nightmare of one "using WTF::Unicode::foo;" line for every "foo" character definition.
Comment 14 Alexey Proskuryakov 2011-01-23 11:33:36 PST
> Another approach would be to put all the characters into another nested namespace
> like WTF::Unicode::CharacterName

The end result would still be slightly different from having separate using declarations. A using declaration creates a synonym of the name in the current scope, so you can reference WTF names as e.g. ::blackSquare. A using directive just makes the names from a namespace usable in the current scope without adding synonyms.

This difference can have extremely subtle effects on dependent name lookup, and although I can't easily think of any case where it would cause problems for our usage of Unicode character names, it probably makes sense to be consistent with the rest of WTF.
Comment 15 David Kilzer (:ddkilzer) 2011-01-23 12:45:48 PST
(In reply to comment #14)
> > Another approach would be to put all the characters into another nested namespace
> > like WTF::Unicode::CharacterName
> 
> The end result would still be slightly different from having separate using declarations. A using declaration creates a synonym of the name in the current scope, so you can reference WTF names as e.g. ::blackSquare. A using directive just makes the names from a namespace usable in the current scope without adding synonyms.
> 
> This difference can have extremely subtle effects on dependent name lookup, and although I can't easily think of any case where it would cause problems for our usage of Unicode character names, it probably makes sense to be consistent with the rest of WTF.

Okay.
Comment 16 Patrick R. Gansterer 2011-01-23 12:49:01 PST
@ap: Patch has r+. Can I commit it now?
Comment 17 Alexey Proskuryakov 2011-01-23 13:50:12 PST
Just to put what I don't like about this move in one place:
- seems arbitrary to have things like blackSquare in WTF;
- adding new named characters will be harder - since it's WebCore that uses most of them, you now have to write two ChangeLogs instead of one for each patch that adds a named character;
- you now have to add two lines for every character (actual definition and using declaration);
- with a using declaration, there is a slightly larger chance of an unresolvable name conflict with some system header than with a name in WebCore namespace (when we could use ::name and WebCore::name to resolve the ambiguity). But we can and should rename the character in case of a conflict anyway.

None of these are huge issues, and it certainly would be good to have a name for U+FFFD in WTF and JavaScriptCore.
Comment 18 Patrick R. Gansterer 2011-01-23 13:56:30 PST
(In reply to comment #17)
> Just to put what I don't like about this move in one place:
> - seems arbitrary to have things like blackSquare in WTF;
> - adding new named characters will be harder - since it's WebCore that uses most of them, you now have to write two ChangeLogs instead of one for each patch that adds a named character;
> - you now have to add two lines for every character (actual definition and using declaration);
> - with a using declaration, there is a slightly larger chance of an unresolvable name conflict with some system header than with a name in WebCore namespace (when we could use ::name and WebCore::name to resolve the ambiguity). But we can and should rename the character in case of a conflict anyway.

Is this a yes or no?

> None of these are huge issues, and it certainly would be good to have a name for U+FFFD in WTF and JavaScriptCore.

I already added one in UTF8.cpp: http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/unicode/UTF8.cpp?rev=74855#L37
Comment 19 Alexey Proskuryakov 2011-01-23 14:18:23 PST
> Is this a yes or no?

Of course you can land if you didn't change your mind. There's an official r+, and all interested parties had a chance to comment over the last two months!
Comment 20 Patrick R. Gansterer 2011-01-23 14:50:26 PST
@darin: Can you please decide if I should land or discard this patch, since you requested this change in https://bugs.webkit.org/show_bug.cgi?id=49581#c3?
Comment 21 Darin Adler 2011-01-23 16:21:53 PST
It’s a close call--Alexey’s list gives the specific reasons why--but I think on balance it’s good to move the character names. You should go for it.
Comment 22 Patrick R. Gansterer 2011-01-29 08:47:26 PST
Created attachment 80563 [details]
Patch
Comment 23 WebKit Commit Bot 2011-01-29 08:49:14 PST
Comment on attachment 80563 [details]
Patch

Rejecting attachment 80563 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-..." exit_code: 2

Last 500 characters of output:
TreeAsText.cpp
patching file Source/WebCore/rendering/break_lines.cpp
patching file Source/WebCore/rendering/mathml/RenderMathMLOperator.h
patching file Source/WebCore/websockets/WebSocketHandshake.cpp
patching file Source/WebCore/wml/WMLTableElement.cpp
patching file Source/WebKit/chromium/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/chromium/src/ChromeClientImpl.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7597553
Comment 24 Patrick R. Gansterer 2011-01-29 09:11:43 PST
Comment on attachment 80563 [details]
Patch

Clearing flags on attachment: 80563

Committed r77062: <http://trac.webkit.org/changeset/77062>
Comment 25 Patrick R. Gansterer 2011-01-29 09:12:03 PST
All reviewed patches have been landed.  Closing bug.