Making Unicode character names consistent

Making Unicode character names consistent.

Changing Unicode character naming schemes from [name], [name]Character
and character[Name] to character[Name]

BUG=484044
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195297

[h.joshi, 2015-05-04 10:08:44]

Patchset #4 (id:60001) has been deleted

[h.joshi, 2015-05-05 08:46:18]

h.joshi@samsung.com changed reviewers:
+ eae@chromium.org

[h.joshi, 2015-05-05 08:47:09]

Pls review

[h.joshi, 2015-05-05 10:27:20]

Patchset #5 (id:100001) has been deleted

[eae, 2015-05-05 16:44:49]

You'll need a reviewer in Source/OWNERS for this. pdr might be able to help.

Thanks for doing this, generally I think I'd prefer [name]Character though,
reads a bit better.

[jsbell, 2015-05-05 17:29:52]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2015-05-05 17:29:53]

not lgtm - some of the changes are incorrect

I agree with eae@ that 'nameCharacter' is more readable, and would also reduce
the size of the CL since 'replacementCharacter' appears to be the most common
reference.

Also... another approach would be to eliminate the 'using' statements from
Unicode.h and simply have code using the constants write 'Unicode::foo' instead
of 'fooCharacter'. If that were done, you'd still want
'Unicode::replacementCharacter' since that particular code point includes
'character' as part of the name.

https://codereview.chromium.org/1119663002/diff/140001/Source/core/paint/BoxP...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1119663002/diff/140001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:665: // Return the amount of characterSpace to
leave between image tiles for the background-repeat: characterSpace property.
This comment change is bogus

https://codereview.chromium.org/1119663002/diff/140001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:669: int characterSpace = -1;
This change is incorrect - it's a local variable, not the shared constant.

https://codereview.chromium.org/1119663002/diff/140001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:799: int characterSpace =
getSpaceBetweenImageTiles(positioningAreaSize.width(),
geometry.tileSize().width());
Ditto.

https://codereview.chromium.org/1119663002/diff/140001/Source/core/paint/BoxP...
Source/core/paint/BoxPainter.cpp:820: int characterSpace =
getSpaceBetweenImageTiles(positioningAreaSize.height(),
geometry.tileSize().height());
Ditto.

https://codereview.chromium.org/1119663002/diff/140001/Source/platform/fonts/...
File Source/platform/fonts/Character.h (right):

https://codereview.chromium.org/1119663002/diff/140001/Source/platform/fonts/...
Source/platform/fonts/Character.h:101: // According to specification all
characterSpace characters should be replaced with 0x0020 space character.
This comment change is incorrect.

https://codereview.chromium.org/1119663002/diff/140001/Source/platform/fonts/...
File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/1119663002/diff/140001/Source/platform/fonts/...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:855: // Add a characterSpace as
pre-context to the buffer. This prevents showing dotted-circle
While technically correct, I would not make this comment change.

https://codereview.chromium.org/1119663002/diff/140001/Source/platform/fonts/...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:1100: // Do not put emphasis
marks on characterSpace, separator, and control characters.
This comment change is incorrect.

[h.joshi, 2015-05-11 05:53:06]

Patchset #7 (id:160001) has been deleted

[h.joshi, 2015-05-11 07:06:30]

h.joshi@samsung.com changed reviewers:
+ pdr@chromium.org

[h.joshi, 2015-05-11 07:06:33]

Used "nameCharacter" format for all. With this CL size is also reduced.
Suggested comments are also fixed.

@jsbell: Pls review.
+Adding @pdr also

https://codereview.chromium.org/1119663002/diff/140001/Source/platform/fonts/...
File Source/platform/fonts/Character.h (right):

https://codereview.chromium.org/1119663002/diff/140001/Source/platform/fonts/...
Source/platform/fonts/Character.h:101: // According to specification all
characterSpace characters should be replaced with 0x0020 space character.
On 2015/05/05 17:29:53, jsbell wrote:
> This comment change is incorrect.

Done.

https://codereview.chromium.org/1119663002/diff/140001/Source/platform/fonts/...
File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/1119663002/diff/140001/Source/platform/fonts/...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:855: // Add a characterSpace as
pre-context to the buffer. This prevents showing dotted-circle
On 2015/05/05 17:29:53, jsbell wrote:
> While technically correct, I would not make this comment change.

Done.

https://codereview.chromium.org/1119663002/diff/140001/Source/platform/fonts/...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:1100: // Do not put emphasis
marks on characterSpace, separator, and control characters.
On 2015/05/05 17:29:53, jsbell wrote:
> This comment change is incorrect.

Done.

[jsbell, 2015-05-11 16:28:02]

Looks much better, thanks - lgtm to unblock later reviews

(Personally, I'd still prefer a namespace prefix to a suffix, but this is MUCH
more readable than what was there before, so let's go forward with it.)

Two minor nits...

https://codereview.chromium.org/1119663002/diff/180001/Source/core/layout/lin...
File Source/core/layout/line/InlineFlowBox.cpp (right):

https://codereview.chromium.org/1119663002/diff/180001/Source/core/layout/lin...
Source/core/layout/line/InlineFlowBox.cpp:443: // If we encounter any
spaceCharacter after this inline block then ensure it is treated as the space
between two words.
I believe this comment change is incorrect (but I'm not familiar with the
surrounding code). I'd revert it.

https://codereview.chromium.org/1119663002/diff/180001/Source/platform/fonts/...
File Source/platform/fonts/FontFallbackList.cpp (right):

https://codereview.chromium.org/1119663002/diff/180001/Source/platform/fonts/...
Source/platform/fonts/FontFallbackList.cpp:40: using namespace WTF;
Are these using declarations necessary?

[h.joshi, 2015-05-12 04:20:14]

@jsbell: Thank you
Fixed nit comments.
For "namespace prefix to a suffix", was think to make the changes but with those
changes CL size was increasing so moved back to initial suggested approach.

https://codereview.chromium.org/1119663002/diff/180001/Source/core/layout/lin...
File Source/core/layout/line/InlineFlowBox.cpp (right):

https://codereview.chromium.org/1119663002/diff/180001/Source/core/layout/lin...
Source/core/layout/line/InlineFlowBox.cpp:443: // If we encounter any
spaceCharacter after this inline block then ensure it is treated as the space
between two words.
On 2015/05/11 16:28:02, jsbell wrote:
> I believe this comment change is incorrect (but I'm not familiar with the
> surrounding code). I'd revert it.

Done.

https://codereview.chromium.org/1119663002/diff/180001/Source/platform/fonts/...
File Source/platform/fonts/FontFallbackList.cpp (right):

https://codereview.chromium.org/1119663002/diff/180001/Source/platform/fonts/...
Source/platform/fonts/FontFallbackList.cpp:40: using namespace WTF;
Sorry for this, removed now. Was checking something else and they got added to
this CL.

[h.joshi, 2015-05-12 09:09:26]

@pdr: Pls review

[pdr., 2015-05-12 13:04:56]

On 2015/05/12 at 09:09:26, h.joshi wrote:
> @pdr: Pls review

LGTM

[h.joshi, 2015-05-12 15:15:18]

@pdr: Thank you

[h.joshi, 2015-05-12 15:15:26]

The CQ bit was checked by h.joshi@samsung.com

[h.joshi, 2015-05-12 15:15:26]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1119663002/#ps200001
(title: "Fixing nit comments")

[commit-bot: I haz the power, 2015-05-12 15:15:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119663002/200001

[commit-bot: I haz the power, 2015-05-12 15:18:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-12 15:18:42]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/55098)

[h.joshi, 2015-05-13 05:10:26]

The CQ bit was checked by h.joshi@samsung.com

[h.joshi, 2015-05-13 05:10:26]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1119663002/#ps220001
(title: "Rebase patch")

[commit-bot: I haz the power, 2015-05-13 05:10:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1119663002/220001

[commit-bot: I haz the power, 2015-05-13 06:33:13]

Committed patchset #9 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195297