Bug 49618 – Move CharacterNames.h into WTF directory

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

see patch

[Alexey Proskuryakov]

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.

[Darin Adler]

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.

[Alexey Proskuryakov]

Perhaps 0xFFFD could go to Unicode.h.

[Patrick R. Gansterer]

Created attachment 74219 [details]
Patch

AFIK CQ won't keep the history of moved files, so we need to commit in manually.

[Darin Adler]

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.

[Alexey Proskuryakov]

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.

[Darin Adler]

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

[David Kilzer (:ddkilzer)]

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;

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

This is a better link: <http://www.drdobbs.com/184401782;jsessionid=IC0T3CABSTWYZQE1GHPSKH4ATMY32JVN>.

[Patrick R. Gansterer]

Comment on attachment 74219 [details]
Patch

Setting back to r?

@ap, darin: Any consensus about this change?

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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

[Alexey Proskuryakov]

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

[David Kilzer (:ddkilzer)]

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

[Patrick R. Gansterer]

@ap: Patch has r+. Can I commit it now?

[Alexey Proskuryakov]

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.

[Patrick R. Gansterer]

(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

[Alexey Proskuryakov]

> 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!

[Patrick R. Gansterer]

@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?

[Darin Adler]

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.

[Patrick R. Gansterer]

Created attachment 80563 [details]
Patch

[WebKit Commit Bot]

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

[Patrick R. Gansterer]

Comment on attachment 80563 [details]
Patch

Clearing flags on attachment: 80563

Committed r77062: <http://trac.webkit.org/changeset/77062>

[Patrick R. Gansterer]

All reviewed patches have been landed.  Closing bug.