221666 - remove redundant copies of nsCompressedCharMap.* and move them into intl/unicharutil

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Jungshik Shin]

Two copies of nsCompressedCharMap.(cpp|h) are present in the tree, one in
gfx/src(gfx/public) and the other in gfx/src/shared, one of which has to be
deleted. Moreover, to avoid making 'intl' depend on 'gfx' (see bug 221024), we
have to move them out of gfx and into intl/unicharutil

Comment 1

[Jungshik Shin]

Attachment: patch

In addition to the above patch, gfx/src/nsCompressedCharMap.cpp and
gfx/public/nsCompressedCharMap.h have to be moved to intl/unicharutils/src and
intl/unicharutils/public, respecitvely. Besides,
gfx/src/shared/nsCompressedCharMap.* have to be deleted (The references to two
files in Makefile.in in that directory were removed a long time ago).

Comment 2

[Jungshik Shin]

Comment on attachment 132916 [details] [diff] [review]
patch 

This is a tree cleanup plus a bit of reorganization

Comment 3

[Jungshik Shin]

Comment on attachment 132916 [details] [diff] [review]
patch 

This doesn't work because there are several 'extern's declared in
nsCompressedCharMap.h.

Comment 4

[Jungshik Shin]

Attachment: patch v2a

To avoid making 'intl' depend on gfx, I split nsCompressedCharMap.h into two
parts, one with macros for accessing nsCompressCharMap(used outside gfx) and
the other with nsCompressCharMap class definition and utility functions (used
only in gfx) to hanld nsCompressCharMap. The former is put in intl/unicharutil
(nsCCMapDefines.h) and the latter is left where it's now(gfx/public).

Comment 5

[Jungshik Shin]

Moving nsCompressedCharMap.* out of gfx appears to be an unnecessary hassle.
Instead, I just want to remove(cvs delete) the unused copy of
nsCompressedCharMap.* in gfx/shared/src. No cvs surgery is necessary because
gfx/src and gfx/public have them (more updated ones). 

Because the patch would be only made up of lines beginning with '-', I'm not
gonna upload it. smontagu and blizzard, can you mark your 'endorsement' here,
instead (if necessary)? 

Comment 6

[Simon Montagu :smontagu]

r=smontagu for the cvs remove.

Comment 7

[rbs]

> Two copies of nsCompressedCharMap.cpp

There are _three_ copies :-( Another one gets copied in gfx/src/windows.
I don't quite get it why you couldn't get attachment 132916 [details] [diff] [review] to work. It looks so
mcuh neater. What happens if you remove s/extern//g ?

Comment 8

[rbs]

... or by pushing them as |static| helpers inside nsCompressCharMap.

Comment 9

[Jungshik Shin]

> What happens if you remove s/extern//g 
  With those replacements,  C++ files wouldn't get even compiled, would they?
Only declaration but no definition..

>  pushing them as |static| helpers inside nsCompressCharMap

  That should work except that I have to go around to change a dozen or so
files. I'd do that if there's a guarantted that I can get a quick review :-)
Some of my large patches took almost a year to get in ... 

Comment 10

[rbs]

>a quick review

I have had some misgivings about these multiple copies for a while. If your
patch doesn't change the functionality, I will give you a quick review.

Comment 11

[Jungshik Shin]

I mistakenly thought that functions defined  in intl/unicharutils/(src|public)
would be available if 'unicharutil' is added to REQUIRED list in Makefile.in. It
turned out that they're only for nsCOM-ified methods. I didn't realized it until
I looked into unicharutil_s.lib for CCMap-related funtions. None of them was
there (that's why I got the link error). 
By moving nsCompressedCharMap.* into intl/unicharutils/util (files of which get
compiled into unicharutil_s.lib), I was able to go through the link w/o a
problem. In short, rbs was right. I don't need any mass-change at all. Just
moving nsCompressedCharMap.* into intl/unicharutils/util works. 

  

Comment 12

[Jungshik Shin]

Attachment: a new patch

In addition to this patch, nsCompressedCharMap.* under gfx (two copies) have to
be cvs-removed (the third copy mentioned by rbs is generated at the compile
time and the necessary changes for Makefile.in's are included in the patch).
Moreover, the newer of two copies (in gfx/src and gfx/public) have to be moved
to intl/unicharutil/util. 

BTW, who would be a 'CVS surgeon' to preserve the revision history after this
move?

Comment 13

[Jungshik Shin]

Comment on attachment 133854 [details] [diff] [review]
a new patch

Asking for r/sr. Here's the summary of the additional file movement for review.

  1. delete gfx/shared/nsCompressedCharMap.*
  2. move gfx/public/nsCompressedCharMap.h and gfx/src/nsCompressedCharMap.cpp
to intl/unicharutil/util

Comment 14

[Simon Montagu :smontagu]

Comment on attachment 133854 [details] [diff] [review]
a new patch

r=smontagu.

Comment 15

[rbs]

> BTW, who would be a 'CVS surgeon' to preserve the revision history after this
> move? 

Cc:ing [e-mail] for help with this. leaf, care to chime in?

Comment 16

[rbs]

Comment on attachment 133854 [details] [diff] [review]
a new patch

sr=rbs

Comment 17

[Jungshik Shin]

thanks for r/sr. 

To leaf,
what should I do about files I'm gonna move? Can I check in first at the new
location or do you have to move files doing the cvs surgery? 

Comment 18

[Daniel (Leaf) Nunes]

Just to make extra sure i'm doing the right thing:
gfx/public/nsCompressedCharMap.h -> intl/unicharutil/util/nsCompressedCharMap.h
gfx/src/nsCompressedCharMap.cpp -> intl/unicharutil/util/nsCompressedCharMap.cpp

Meaning, no "public" and "src" directories under "util".

I'll make the copy when i get confirmation that this is what you're looking for.
Once i perform the copy, you can cvs remove the old file locations at your leisure.

Comment 19

[Jungshik Shin]

leaf, thanks for your help. What you wrote is exactly what I want. There's no
'src/public' directory at the path of the new location. 

Comment 20

[Jungshik Shin]

ping leaf. Have you scheduled your operation? :-)

Comment 21

[Jungshik Shin]

fix checked into the trunk with the cvs operation done by brendan. 

Comment 22

[Simon Montagu :smontagu]

*** Bug 194823 has been marked as a duplicate of this bug. ***