In-te-grate hy-phen-ator to con-tent.

In-te-grate hy-phen-ator to con-tent.
This change integrates the Hyphenator class to content so Chrome can use it. This change basically consists of two parts:
* Adds a couple of IPC messages so a renderer asks a browser to open hyphenation dictionaries;
* Adds a HyphenatorMessageFilter class, which opens hyphenation dictionaries in a browser process;
* Changes the Hyphenator class to send IPC messages to open hyphenation dictionaries. 

BUG=47083
TEST=HyphenatorTest.SetDictionary,HyphenatorMessageFilterTest.OpenDictionary

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

[Hironori Bono, 2012-08-22 09:39:52]

Greetings John and Tony,

Sorry for suddenly sending a big change.
I have integrated the hyphen library to content so Chrome can use it. (This
change also modifies MockRenderProcessHost and MockRenderThread so I can write
unit tests.)
Unfortunately, this change has a couple of limitations:
* This change reads hyphenation dictionaries from the directory containing the
executable file. (I will create another change so Chrome can change the
directory.)
* A renderer does not wait for a browser to open hyphenation dictionaries. (This
is intentional to prevent hyphenation code from blocking rendering.)
Would it be possible to review this change and give me your thoughts?

Regards,

Hironori Bono

[tony, 2012-08-22 19:05:50]

http://codereview.chromium.org/10854245/diff/16019/content/browser/hyphenator...
File content/browser/hyphenator/hyphenator_message_filter.cc (right):

http://codereview.chromium.org/10854245/diff/16019/content/browser/hyphenator...
content/browser/hyphenator/hyphenator_message_filter.cc:32: :
render_process_host_(render_process_host),
Rather than passing in RenderProcessHost*, can we just pass in a
base::ProcessHandle (the result of render_process_host->GetHandle())?  Then you
don't have to modify MockRenderProcessHost.

http://codereview.chromium.org/10854245/diff/16019/content/browser/hyphenator...
File content/browser/hyphenator/hyphenator_message_filter.h (right):

http://codereview.chromium.org/10854245/diff/16019/content/browser/hyphenator...
content/browser/hyphenator/hyphenator_message_filter.h:32: // This class opens
the file "|dictionary_path_|/|locale|-1-0.dic".
Nit: This comment feels verbose.  I would just keep the first sentence since the
rest is implementation details.

http://codereview.chromium.org/10854245/diff/16019/content/browser/hyphenator...
content/browser/hyphenator/hyphenator_message_filter.h:56: // function sends an
IPC::nvalidPlatformFileForTransit value to tell the
Nit: typo: nvalidPlatformFileForTransit -> InvalidPlatformFileForTransit

http://codereview.chromium.org/10854245/diff/16019/content/public/test/mock_r...
File content/public/test/mock_render_process_host.cc (right):

http://codereview.chromium.org/10854245/diff/16019/content/public/test/mock_r...
content/public/test/mock_render_process_host.cc:107: return GetCurrentProcess();
Why only on Windows?

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
File content/renderer/hyphenator/hyphenator.cc (right):

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.cc:260: // by the MemoryMappFile class.
To prevent this class from closing this file,
Nit: typo: MemoryMappFile -> MemoryMappedFile

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
File content/renderer/hyphenator/hyphenator_unittest.cc (right):

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator_unittest.cc:18: // A mock message
listener that listens HyphenatorHost messages. This class
Nit: that listens _for_ HyphenatorHost messages

http://codereview.chromium.org/10854245/diff/16019/content/renderer/renderer_...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

http://codereview.chromium.org/10854245/diff/16019/content/renderer/renderer_...
content/renderer/renderer_webkitplatformsupport_impl.cc:754: // only when WebKit
finds a page that needs hyphenation.)
Nit: I would remove the sentence in ()s.  I don't think you need to explain or
justify lazy initialization.

[tony, 2012-08-22 19:06:33]

Seems OK overall. John should take a look too since it's been a long time since
I've written any IPC code and many things have changed.

[jam, 2012-08-23 21:34:29]

http://codereview.chromium.org/10854245/diff/16019/content/browser/renderer_h...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/10854245/diff/16019/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.cc:613:
channel_->AddFilter(new content::HyphenatorMessageFilter(this));
nit: content:: unnecessary (can also remove in the line above)

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
File content/renderer/hyphenator/hyphenator.cc (right):

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.cc:16: #include
"content/public/renderer/render_thread.h"
nit: not needed

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.cc:190: class MessageFilter : public
IPC::ChannelProxy::MessageFilter {
you don't need a message filter, since you're really dispatching this message on
the main renderer thread (a message filter would be helpful if you needed to
dispatch it on the IO thread).

you can make the Hyphenator class derive from RenderProcessObserver and register
with RenderThread. that way you can dispatch messages on the main thread

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.cc:200: // Called when a browser sends a
HyphenatorMsg_SetDictionary message, i.e.
nit: by convention we don't comment IPC message handlers. the comment for those
is in their _messages.h file

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.cc:271: bool
Hyphenator::Attach(content::RenderThread* thread, const string16& locale) {
ditto

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
File content/renderer/hyphenator/hyphenator.h (right):

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.h:42: bool Attach(content::RenderThread*
thread, const string16& locale);
ditto

[Hironori Bono, 2012-08-27 06:57:27]

Greetings Tony and John,

Thank you for your review and comments. I have applied your comments as much as
possible and uploaded a new change. Would it be possible to take another look?

Regards,

Hironori Bono

http://codereview.chromium.org/10854245/diff/16019/content/browser/hyphenator...
File content/browser/hyphenator/hyphenator_message_filter.cc (right):

http://codereview.chromium.org/10854245/diff/16019/content/browser/hyphenator...
content/browser/hyphenator/hyphenator_message_filter.cc:32: :
render_process_host_(render_process_host),
On 2012/08/22 19:05:50, tony wrote:
> Rather than passing in RenderProcessHost*, can we just pass in a
> base::ProcessHandle (the result of render_process_host->GetHandle())?  Then
you
> don't have to modify MockRenderProcessHost.

Thanks for your comment. Unfortunately, I cannot use
RenderProcessHost::GetHandle() here because it always returns kNullProcessHandle
in RenderProcessHostImpl::CreateMessageFilters(), the function which creates
this object. (RenderProcessHostImpl::CreateMessageFilters() seems to be called
while the renderer process is being created, i.e. its process handle is not
fixed.)

http://codereview.chromium.org/10854245/diff/16019/content/browser/hyphenator...
File content/browser/hyphenator/hyphenator_message_filter.h (right):

http://codereview.chromium.org/10854245/diff/16019/content/browser/hyphenator...
content/browser/hyphenator/hyphenator_message_filter.h:32: // This class opens
the file "|dictionary_path_|/|locale|-1-0.dic".
On 2012/08/22 19:05:50, tony wrote:
> Nit: This comment feels verbose.  I would just keep the first sentence since
the
> rest is implementation details.

Done. Thanks for noticing my redundant comments.

http://codereview.chromium.org/10854245/diff/16019/content/browser/hyphenator...
content/browser/hyphenator/hyphenator_message_filter.h:56: // function sends an
IPC::nvalidPlatformFileForTransit value to tell the
On 2012/08/22 19:05:50, tony wrote:
> Nit: typo: nvalidPlatformFileForTransit -> InvalidPlatformFileForTransit

Done. Thanks for noticing this typo.

http://codereview.chromium.org/10854245/diff/16019/content/browser/renderer_h...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/10854245/diff/16019/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.cc:613:
channel_->AddFilter(new content::HyphenatorMessageFilter(this));
On 2012/08/23 21:34:29, John Abd-El-Malek wrote:
> nit: content:: unnecessary (can also remove in the line above)

Done. Thanks for noticing these redundant namespaces.

http://codereview.chromium.org/10854245/diff/16019/content/public/test/mock_r...
File content/public/test/mock_render_process_host.cc (right):

http://codereview.chromium.org/10854245/diff/16019/content/public/test/mock_r...
content/public/test/mock_render_process_host.cc:107: return GetCurrentProcess();
On 2012/08/22 19:05:50, tony wrote:
> Why only on Windows?

Thanks for noticing it. This is just because I have just copied this
GetCurrentProcess call from MockRenderProcessHost::GetTransportDIB and this
function should not use such a platform-dependent function. I have replaced this
call with base::Process::current().handle() to make it platform-independent.

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
File content/renderer/hyphenator/hyphenator.cc (right):

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.cc:16: #include
"content/public/renderer/render_thread.h"
On 2012/08/23 21:34:29, John Abd-El-Malek wrote:
> nit: not needed

Done. I have removed redundant headers. (I do not need some other header files
because I used the RenderProcessObserver class.)

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.cc:190: class MessageFilter : public
IPC::ChannelProxy::MessageFilter {
On 2012/08/23 21:34:29, John Abd-El-Malek wrote:
> you don't need a message filter, since you're really dispatching this message
on
> the main renderer thread (a message filter would be helpful if you needed to
> dispatch it on the IO thread).
> 
> you can make the Hyphenator class derive from RenderProcessObserver and
register
> with RenderThread. that way you can dispatch messages on the main thread

Done. Thanks for your suggestion. The RenderProcessObserver class makes this
change much simpler.

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.cc:200: // Called when a browser sends a
HyphenatorMsg_SetDictionary message, i.e.
On 2012/08/23 21:34:29, John Abd-El-Malek wrote:
> nit: by convention we don't comment IPC message handlers. the comment for
those
> is in their _messages.h file

Done. Thanks for your comment. (I have removed comments in header files.)

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.cc:260: // by the MemoryMappFile class.
To prevent this class from closing this file,
On 2012/08/22 19:05:50, tony wrote:
> Nit: typo: MemoryMappFile -> MemoryMappedFile

Done. Thanks for noticing my typo.

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.cc:271: bool
Hyphenator::Attach(content::RenderThread* thread, const string16& locale) {
On 2012/08/23 21:34:29, John Abd-El-Malek wrote:
> ditto

Done. I have removed this redundant comment.

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
File content/renderer/hyphenator/hyphenator.h (right):

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.h:42: bool Attach(content::RenderThread*
thread, const string16& locale);
On 2012/08/23 21:34:29, John Abd-El-Malek wrote:
> ditto

Done. Thanks for noticing this redundant comment.

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
File content/renderer/hyphenator/hyphenator_unittest.cc (right):

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator_unittest.cc:18: // A mock message
listener that listens HyphenatorHost messages. This class
On 2012/08/22 19:05:50, tony wrote:
> Nit: that listens _for_ HyphenatorHost messages

Done. Thanks for fixing my typo.

http://codereview.chromium.org/10854245/diff/16019/content/renderer/renderer_...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

http://codereview.chromium.org/10854245/diff/16019/content/renderer/renderer_...
content/renderer/renderer_webkitplatformsupport_impl.cc:754: // only when WebKit
finds a page that needs hyphenation.)
On 2012/08/22 19:05:50, tony wrote:
> Nit: I would remove the sentence in ()s.  I don't think you need to explain or
> justify lazy initialization.

Done. Thanks for your suggestion.

[jam, 2012-08-27 16:31:10]

lgtm

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
File content/renderer/hyphenator/hyphenator.h (right):

http://codereview.chromium.org/10854245/diff/16019/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.h:42: bool Attach(content::RenderThread*
thread, const string16& locale);
On 2012/08/27 06:57:28, Hironori Bono wrote:
> On 2012/08/23 21:34:29, John Abd-El-Malek wrote:
> > ditto
> 
> Done. Thanks for noticing this redundant comment.

I had meant the "content::"

http://codereview.chromium.org/10854245/diff/27043/content/renderer/hyphenato...
File content/renderer/hyphenator/hyphenator.h (right):

http://codereview.chromium.org/10854245/diff/27043/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.h:52: // content::RenderProcessObserver
implementation.
nit: no need for "content::"

[tony, 2012-08-27 18:17:39]

LGTM2

http://codereview.chromium.org/10854245/diff/27043/content/common/hyphenator_...
File content/common/hyphenator_messages.h (right):

http://codereview.chromium.org/10854245/diff/27043/content/common/hyphenator_...
content/common/hyphenator_messages.h:13: // Opens the specified hyphnation
dictionary. This message is expected to be
Nit: typo hyphnation -> hyphenation

[commit-bot: I haz the power, 2012-08-28 06:53:58]

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

[Hironori Bono, 2012-08-28 06:54:37]

Greetings Tony and John,

Thank you for your review and comments. I have updated this change to apply your
comments and sent the updated change to the commit-queue.

Regards,

Hironori Bono

http://codereview.chromium.org/10854245/diff/27043/content/common/hyphenator_...
File content/common/hyphenator_messages.h (right):

http://codereview.chromium.org/10854245/diff/27043/content/common/hyphenator_...
content/common/hyphenator_messages.h:13: // Opens the specified hyphnation
dictionary. This message is expected to be
On 2012/08/27 18:17:39, tony wrote:
> Nit: typo hyphnation -> hyphenation

Done. Thanks for pointing out this typo.

http://codereview.chromium.org/10854245/diff/27043/content/renderer/hyphenato...
File content/renderer/hyphenator/hyphenator.h (right):

http://codereview.chromium.org/10854245/diff/27043/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.h:52: // content::RenderProcessObserver
implementation.
On 2012/08/27 16:31:10, John Abd-El-Malek wrote:
> nit: no need for "content::"

Done. Thanks for your suggestion.

[commit-bot: I haz the power, 2012-08-29 02:39:47]

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

[commit-bot: I haz the power, 2012-08-29 03:53:05]

Try job failure for 10854245-21044 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-29 04:10:01]

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

[commit-bot: I haz the power, 2012-08-29 05:34:53]

Try job failure for 10854245-21044 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-09-03 03:56:22]

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

[commit-bot: I haz the power, 2012-09-03 06:39:03]

Change committed as 154663