Closed Bug 1203477 Opened 4 years ago Closed 4 years ago

intl/hyphenation/ should be split in two, to separate third-party code from Mozilla code

Categories

(Core :: Internationalization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: njn, Assigned: sajitk)

Details

Attachments

(1 file, 5 obsolete files)

intl/hyphenation/README.mozilla explains that the following files are Mozilla-specific:

  hnjalloc.h
  hnjstdio.cpp
  nsHyphenationManager.cpp
  nsHyphenator.cpp
  moz.build

Also, I'm pretty sure nsHyphenator.{h,cpp} should also be in that list.

The other files in that directory come from the Hypen library, which is part of Hunspell. They should be in a separate directory.
BTW, intl/hyphenation/moz.build currently has ALLOW_COMPILER_WARNINGS=True in it because hyphen.c (one of the third-party files) has a compile warning. Once the split is complete, ALLOW_COMPILER_WARNINGS=True would be used for the third-party directory, but not used in the Mozilla code directory.
sajitk: this bug is similar to bug 1200065. Are you interested in doing it?
Flags: needinfo?(sajitk)
Sure, I will get started on it. Thanks.
Flags: needinfo?(sajitk)
Great! Thank you.
Assignee: nobody → sajitk
Attached patch 1203477.patch (obsolete) — Splinter Review
Please find attached the patch file. I created "glue" and "hyphen" directories within /intl/hyphenation directory, and moved the hyphen library files into the "hyphen" and mozilla files to "glue". 
I think the README.mozilla file should be updated with this information, so if you agree with the approach, I will change the README.mozilla file as well.
Attachment #8659306 - Flags: review?(mh+mozilla)
Comment on attachment 8659306 [details] [diff] [review]
1203477.patch

Review of attachment 8659306 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/hyphenation/hyphen/moz.build
@@ +21,5 @@
>          '-Wno-sign-compare',
>          '-Wno-type-limits',
>      ]
>  
>  if not CONFIG['GNU_CXX']:

In intl/hyphenation/hyphen/moz.build you can replace all the stuff about warnings (the last 9 lines of the file) with a single line like this:

> ALLOW_COMPILER_WARNINGS = True
Also, there's a file called tools/rewriting/ThirdPartyPaths.txt which lists third-party code in the tree. The "intl/hyphenation/" entry should be changed to "intl/hyphenation/hyphen/", and the hunspell entries should be updated for the changes you did in bug 1200065. (Sorry I didn't think of this until just now.)
Attached patch 1203477.patch (obsolete) — Splinter Review
Patch updated with last comments. 
Regarding the bug 1200065 and the ThirdPartyPaths.txt, the third-party Hunspell code source directory did not change, so the change may not be needed. 
I have updated this file for the hyphen code.
Attachment #8659306 - Attachment is obsolete: true
Attachment #8659306 - Flags: review?(mh+mozilla)
Attachment #8659357 - Flags: review?(mh+mozilla)
> the third-party Hunspell code source directory did not change, so the change may not be
> needed. 

It's not strictly necessary, but the current entries are like this:

> extensions/spellcheck/hunspell/src/*.cxx
> extensions/spellcheck/hunspell/src/*.hxx

I think the "*.cxx" and "*.hxx" suffixes can now be removed because all the C++ code in that directory is now third-party. That will make these entries more like all the other ones in that file.

(Given that bug 1200065's patch hasn't landed yet, that change could still be done in that bug.)
Comment on attachment 8659357 [details] [diff] [review]
1203477.patch

Review of attachment 8659357 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/hyphenation/moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +DIRS += [
> +    'glue',
> +    'hyphen',

It might be better to remove this file entirely, and to add hyphenation/glue and hyphenation/hyphen to the parent moz.build.
Attachment #8659357 - Flags: review?(mh+mozilla) → feedback+
Attached patch 1203477.patch (obsolete) — Splinter Review
Removed moz.build file from the hyphenation directory, as per review comment.
Attachment #8659357 - Attachment is obsolete: true
Attachment #8659918 - Flags: review?(mh+mozilla)
Comment on attachment 8659918 [details] [diff] [review]
1203477.patch

Review of attachment 8659918 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/hyphenation/README.mozilla
@@ +20,5 @@
>  
> +  glue/hnjalloc.h
> +  glue/hnjstdio.cpp
> +  glue/nsHyphenationManager.cpp
> +  glue/nsHyphenator.cpp

I'd change this to just say "The source files in the glue/ directory are Mozilla-authored code...". It's more concise and won't become incorrect in the future if any files are added or removed.
Attached patch 1203477.patch (obsolete) — Splinter Review
Updated readme text.
Attachment #8659918 - Attachment is obsolete: true
Attachment #8659918 - Flags: review?(mh+mozilla)
Attachment #8660738 - Flags: review?(mh+mozilla)
Comment on attachment 8660738 [details] [diff] [review]
1203477.patch

Review of attachment 8660738 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/hyphenation/README.mozilla
@@ +5,2 @@
>  Hyphen library, part of the hunspell project. The various COPYING* and README*
>  files (except this README.mozilla) are likewise from the hunspell distribution

Come to think of it, it would be best to move those files as well.

@@ +15,5 @@
>  Hyphen package from upstream, so the original README.* files may refer to
>  additional files that are not present in the Mozilla source tree.
>  
>  
> +The source files in the /glue folder, as well as the build files 

You are adding trailing whitespaces in this file.
Attachment #8660738 - Flags: review?(mh+mozilla) → feedback+
Attached patch 1203477.patch (obsolete) — Splinter Review
The COPYING* and README* files are already moved to the new /hyphen folder.
Please find attached the patch without the trailing whitespace in the Readme.mozilla.

If OK, could you please also push to the try server?
Attachment #8660738 - Attachment is obsolete: true
Attachment #8661245 - Flags: review?(mh+mozilla)
Comment on attachment 8661245 [details] [diff] [review]
1203477.patch

Review of attachment 8661245 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/hyphenation/README.mozilla
@@ +7,1 @@
>  of Hyphen:

You can actually simplify all this by saying the hyphen/ directory comes from the hyphen library, part of the hunspell project, except moz.build.

@@ +16,5 @@
>  additional files that are not present in the Mozilla source tree.
>  
> +The source files in the /glue directory, as well as the build files
> +(Makefile.in and moz.build) are Mozilla-authored code, and the standard
> +MPL 2.0 applies to these, as noted in the comments within the files.

And you can remove this paragraph.
Attachment #8661245 - Flags: review?(mh+mozilla) → review+
Sorry for my ignorance, but in the treeherder output, it looks like an OS2 build did not go through. From the output , it does not look like any issue with the code that was changed. Could you please let me know if I can set teh checkin-needed flag after making the last change as in Comment 16
Flags: needinfo?(mh+mozilla)
Attached patch 1203477.patchSplinter Review
Attachment #8661245 - Attachment is obsolete: true
For good measure, I retriggered the job. Let's see how it goes the second time.
Flags: needinfo?(mh+mozilla)
Thank you. The last run seems to have worked. Requesting checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/77e7d5151129
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.