Adds a hy-phen-ator.

Adds a hy-phen-ator.
This change adds a project file for the hyphen library and a Hyphenator class, which encapsulates the library. (This class is not integrated into Chrome, though.) 

BUG=47083
TEST=HyphenatorTest.HyphenateWords

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146964

[Hironori Bono, 2012-03-02 06:59:37]

Greetings Tony,

I split my hyphenation change into a few changes because it becomes huge. (This
is the first part of it.) Would it be possible to review this change?

Regards,

Hironori Bono

[tony, 2012-03-05 18:59:00]

This is cool!  Can you provide some perf times for loading the dictionary and
time for say running against a page with a lot of text?

Should the hyphenator class be in content? If it were in content, it could be
used by content_shell.

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
File chrome/renderer/hyphenator/hyphenator.cc (right):

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:62: bool UTF16Position::Append(char c)
{
Can we use U8_FWD_1 in unicode/utf8.h for this instead?  I guess we would need
to use U16_IS_SINGLE from utf16.h to determine if we add 1 or 2 to position.

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:88: // A class that encapsulates a
hyphenation query. THis class owns resources
Nit: THis -> This

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:110: //   GetHyphenationPoints(dict,
word, &hyphens);
This snippet doesn't seem to match the class.

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:120: std::string word_;
Nit: Maybe name this word_utf8_ to make the encoding more clear?

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:144: hyphens_.reset(new
char[word_.length() + 5]);
Why 5? Is that the max number of hyphens that will be added?  Can we make this
number a constant?

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:165: int result =
hnj_hyphen_hyphenate2(dictionary,
Nit: result -> error_code?

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:172: &cut_);
It looks like we can just pass in NULL since we don't use rep_ pos_ or cut_. 
Then we don't have to worry about deleting the memory.

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:180: // converted hyphenation points to
the output vector.
It would be a nice TODO to switch the hyphen library to use utf-16.

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:185: if (hyphens_[i] & 1)
hyphens and hyphens_ make this code hard to follow.  Maybe hyphen_offsets and
hyphen_vector?

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
File chrome/renderer/hyphenator/hyphenator.h (right):

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.h:29: //   scoped_ptr<Hyphenator>
hyphenator(new Hyphenator(file));
How slow is it to instantiate a new Hyphenator?  E.g., how slow will it be to
navigate to a few pages that use hyphenation in sequence?  Roughly how much
memory does Hyphenator use?

I wonder if we can just lazy init Hyphenator and keep it around as long as the
renderer is alive.  That is, changing domains will normally cause it to be
deleted.

The current patch is OK for now, just some things to think about.

http://codereview.chromium.org/9545017/diff/9016/third_party/hyphen/hyphen.gyp
File third_party/hyphen/hyphen.gyp (right):

http://codereview.chromium.org/9545017/diff/9016/third_party/hyphen/hyphen.gy...
third_party/hyphen/hyphen.gyp:34: # Local Variables:
Please remove the emacs/vim stuff.

[Hironori Bono, 2012-03-12 09:11:25]

Greetings Tony,

Many thanks for your review and comments. Also sorry for my slow response. In my
personal opinion, integrating hyphenation to content is a good idea.
Nevertheless, I would like to hear opinions of other Chromium developers because
I do not have deep knowledge about it. (I have sent  an e-mail about it. I will
update this change after hearing them.

Regards,

Hironori Bono

[Hironori Bono, 2012-06-28 09:32:49]

Greeting Tony,

Sorry for my slow update. I have finally moved this Hyphenator class to content
and updated it.

Greetings John and Avi,

I have split my Hyphenation change to several changes to keep each change small.
It is definitely helpful to review this change and give me your comments. (As
written in this Description, this change just added a wrapper class and its unit
test.)

Regards,

Hironori Bono

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
File chrome/renderer/hyphenator/hyphenator.cc (right):

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:62: bool UTF16Position::Append(char c)
{
Thanks for your advice. Yes, it is better to use U8_ and U16_ macros. I have
re-implemented this function to use them.

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:88: // A class that encapsulates a
hyphenation query. THis class owns resources
On 2012/03/05 18:59:00, tony wrote:
> Nit: THis -> This

Done.

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:110: //   GetHyphenationPoints(dict,
word, &hyphens);
On 2012/03/05 18:59:00, tony wrote:
> This snippet doesn't seem to match the class.

Done. Thanks for noticing it. I have updated this snippet.

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:120: std::string word_;
On 2012/03/05 18:59:00, tony wrote:
> Nit: Maybe name this word_utf8_ to make the encoding more clear?

Done. Thanks for you suggestion.

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:144: hyphens_.reset(new
char[word_.length() + 5]);
On 2012/03/05 18:59:00, tony wrote:
> Why 5? Is that the max number of hyphens that will be added?  Can we make this
> number a constant?

This is a magic number written in "hyphen.h"
<http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/hyphen/hyphen....>.
I used this number as written in this comment since this header does not have
macros for it. I have added a comment to describe it.

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:165: int result =
hnj_hyphen_hyphenate2(dictionary,
On 2012/03/05 18:59:00, tony wrote:
> Nit: result -> error_code?

Done.

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:172: &cut_);
On 2012/03/05 18:59:00, tony wrote:
> It looks like we can just pass in NULL since we don't use rep_ pos_ or cut_. 
> Then we don't have to worry about deleting the memory.

This function assumes these pointers are non-NULL and it crashes when we use
NULL for them.

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.cc:185: if (hyphens_[i] & 1)
On 2012/03/05 18:59:00, tony wrote:
> hyphens and hyphens_ make this code hard to follow.  Maybe hyphen_offsets and
> hyphen_vector?

Done, I have renamed them. (I could not figure out which was what when I updated
this change.)

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
File chrome/renderer/hyphenator/hyphenator.h (right):

http://codereview.chromium.org/9545017/diff/9016/chrome/renderer/hyphenator/h...
chrome/renderer/hyphenator/hyphenator.h:29: //   scoped_ptr<Hyphenator>
hyphenator(new Hyphenator(file));
Thanks for your comment. When I created a page-cycler test, hyphenation takes 4%
more time and 41MB more memory for now. (I have sent detailed data via a direct
e-mail.)

[tony, 2012-06-28 20:06:44]

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
File content/renderer/hyphenator/hyphenator.cc (right):

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.cc:101: //   }
I don't think the code example is that useful as a comment.  Someone can just
look at the existing code that uses it (e.g., ComputeLastHyphenLocation).

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.cc:147: free(rep_[i]);
Nit: Should we be using hnj_free here and below?

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.cc:206: if
(!rule_map_->Initialize(rule_file_))
If this is in the sandboxed renderer process, how will it be able to open this
file? How does spell checking handle this?

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
File content/renderer/hyphenator/hyphenator.h (right):

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.h:24: // dictionary, it takes lots of
memory. A renderer should create this object
Nit: Would be nice to give actual size estimates here.

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
File content/renderer/hyphenator/hyphenator_unittest.cc (right):

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator_unittest.cc:38: void Hyphenate(const
string16& word, string16* hyphenated_word) {
Nit: I would probably just return a string16 with the result.

http://codereview.chromium.org/9545017/diff/41002/third_party/hyphen/hyphen.gyp
File third_party/hyphen/hyphen.gyp (right):

http://codereview.chromium.org/9545017/diff/41002/third_party/hyphen/hyphen.g...
third_party/hyphen/hyphen.gyp:34: # Local Variables:
Please remove these.  We have helper files in src/tools/{emacs,vim} if you need
this for gyp files.

[jochen (gone - plz use gerrit), 2012-07-02 15:33:01]

content drive-by comments

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
File content/renderer/hyphenator/hyphenator.cc (right):

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.cc:51: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.cc:114: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
File content/renderer/hyphenator/hyphenator.h (right):

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.h:19: 
all code in content/ should go into the namespace content

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.h:35: class CONTENT_EXPORT Hyphenator {
why is this a CONTENT_EXPORT class if it's not in content/public?

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.h:67: };
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
File content/renderer/hyphenator/hyphenator_unittest.cc (right):

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator_unittest.cc:21: bool Initialize() {
should be void SetUp()

[Hironori Bono, 2012-07-10 09:22:09]

Greetings Tony and Jochen,

Thanks for your review and comments. Even though I have applied some of your
comments, I cannot remove CONTENT_EXPORT because this class is an internal class
used by content_unittests and removing this macro causes a build break. Would it
be possible to give me good ideas to avoid this break?

Regards,

Hironori Bono

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
File content/renderer/hyphenator/hyphenator.cc (right):

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.cc:51: };
On 2012/07/02 15:33:01, jochen wrote:
> DISALLOW_COPY_AND_ASSIGN

Done. Thanks for noticing it.

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.cc:101: //   }
On 2012/06/28 20:06:44, tony wrote:
> I don't think the code example is that useful as a comment.  Someone can just
> look at the existing code that uses it (e.g., ComputeLastHyphenLocation).

Done. I have removed this redundant example.

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.cc:114: };
On 2012/07/02 15:33:01, jochen wrote:
> DISALLOW_COPY_AND_ASSIGN

Done. Thanks for noticing it.

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.cc:147: free(rep_[i]);
On 2012/06/28 20:06:44, tony wrote:
> Nit: Should we be using hnj_free here and below?

Thanks for your comment. The hyphen library uses hnj_malloc/hnj_free only for
allocating internal objects, i.e. it uses malloc for allocating these objects
<http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/hyphen/hyphen....>.
So, I would like to use free for deleting them.

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.cc:206: if
(!rule_map_->Initialize(rule_file_))
On 2012/06/28 20:06:44, tony wrote:
> If this is in the sandboxed renderer process, how will it be able to open this
> file? How does spell checking handle this?

Our spellchecker opens dictionary in a browser process and sends its handle to a
renderer. Even though this change expects to use the same design for
hyphenation, it is definitely helpful to give me your suggestions.

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
File content/renderer/hyphenator/hyphenator.h (right):

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.h:19: 
On 2012/07/02 15:33:01, jochen wrote:
> all code in content/ should go into the namespace content

Done. Thanks for your comment. (I did not notice it.)

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.h:35: class CONTENT_EXPORT Hyphenator {
On 2012/07/02 15:33:01, jochen wrote:
> why is this a CONTENT_EXPORT class if it's not in content/public?

Even though this class is not a public one, it is used by content_unittests.
That is, this CONTENT_EXPORT is just for avoiding build breaks in building
content_unittests on shared builders. Is it possible to give me better ideas?

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.h:67: };
On 2012/07/02 15:33:01, jochen wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done. Thanks for noticing it.

[jochen (gone - plz use gerrit), 2012-07-10 09:46:06]

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
File content/renderer/hyphenator/hyphenator.h (right):

http://codereview.chromium.org/9545017/diff/41002/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.h:35: class CONTENT_EXPORT Hyphenator {
On 2012/07/10 09:22:09, Hironori Bono wrote:
> On 2012/07/02 15:33:01, jochen wrote:
> > why is this a CONTENT_EXPORT class if it's not in content/public?
> 
> Even though this class is not a public one, it is used by content_unittests.
> That is, this CONTENT_EXPORT is just for avoiding build breaks in building
> content_unittests on shared builders. Is it possible to give me better ideas?

Usually, we have different macros for this case (see
http://www.chromium.org/developers/how-tos/component-build at the bottom).
content/ currently does not, so this is fine. I filed http://crbug.com/136495
for this

[tony, 2012-07-10 17:30:00]

LGTM

http://codereview.chromium.org/9545017/diff/24010/content/renderer/hyphenator...
File content/renderer/hyphenator/hyphenator.cc (right):

http://codereview.chromium.org/9545017/diff/24010/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.cc:201: if
(!rule_map_->Initialize(rule_file_))
Oh, I see, rule_file_ is a PlatformFile which has already been opened. That
works.

[jam, 2012-07-11 00:16:35]

lgtm

my personal preference would be to inline the classes in the cc file, because I
find that easier to read. but totally up to you, since this is just a personal
preference and not in the style guide

http://codereview.chromium.org/9545017/diff/24010/content/renderer/hyphenator...
File content/renderer/hyphenator/hyphenator.cc (right):

http://codereview.chromium.org/9545017/diff/24010/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.cc:7: #include <string>
nit: not needed cause of all the string includes below

[commit-bot: I haz the power, 2012-07-13 06:53:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hbono@chromium.org/9545017/29006

[commit-bot: I haz the power, 2012-07-13 06:53:28]

Failed to apply patch for content/content_renderer.gypi:
While running patch -p0 --forward --force;
patching file content/content_renderer.gypi
Hunk #2 FAILED at 83.
1 out of 2 hunks FAILED -- saving rejects to file
content/content_renderer.gypi.rej

[commit-bot: I haz the power, 2012-07-13 07:17:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hbono@chromium.org/9545017/47003

[Hironori Bono, 2012-07-13 08:11:35]

Greetings Jochen, Tony and John,

Thanks for your review and comments. I have applied you comment and sent the
updated one to the commit-queue.

Regards,

Hironori Bono

http://codereview.chromium.org/9545017/diff/24010/content/renderer/hyphenator...
File content/renderer/hyphenator/hyphenator.cc (right):

http://codereview.chromium.org/9545017/diff/24010/content/renderer/hyphenator...
content/renderer/hyphenator/hyphenator.cc:7: #include <string>
On 2012/07/11 00:16:35, John Abd-El-Malek wrote:
> nit: not needed cause of all the string includes below

Done. Thanks for catching it.

[commit-bot: I haz the power, 2012-07-13 08:52:53]

Try job failure for 9545017-47003 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, steps "interactive_ui_tests, browser_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-17 05:49:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hbono@chromium.org/9545017/48002

[commit-bot: I haz the power, 2012-07-17 06:46:48]

Try job failure for 9545017-48002 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-17 07:52:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hbono@chromium.org/9545017/48002

[commit-bot: I haz the power, 2012-07-17 08:57:03]

Change committed as 146964

[Agrawal, 2012-07-17 17:48:58]

FYI

HyphenatorTest.HyphenateWords fails on Android.

http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Linux%20Android/...



On Tue, Jul 17, 2012 at 1:57 AM, <commit-bot@chromium.org> wrote:

> Change committed as 146964
>
>
https://chromiumcodereview.**appspot.com/9545017/<https://chromiumcodereview....
>