Linux: apply a different hyphen patch.

Linux: apply a different hyphen patch.

This will make it possible to use system hyphen.

BUG=176285
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184515

[Paweł Hajdan Jr., 2013-02-15 08:51:24]

FYI, I'm actually talking with upstream about this new patch. This will make it
easier to update hyphen in the future (our copy is not necessarily up to date).

[darin (slow to review), 2013-02-15 18:16:40]

https://codereview.chromium.org/12259025/diff/9001/content/renderer/hyphenato...
File content/renderer/hyphenator/hyphenator.h (right):

https://codereview.chromium.org/12259025/diff/9001/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.h:58: FILE* dictionary_file_;
nit: You could use ScopedStdioHandle here.

https://codereview.chromium.org/12259025/diff/9001/webkit/support/test_webkit...
File webkit/support/test_webkit_platform_support.cc (right):

https://codereview.chromium.org/12259025/diff/9001/webkit/support/test_webkit...
webkit/support/test_webkit_platform_support.cc:552: hyphen_dictionary_ =
hnj_hyphen_load(path.value().c_str());
Does this work on Windows?  FilePath::value() returns wstring on Windows.  It
looks like hnj_hyphen_load expects a |const char*|.  You should probably use the
FdopenPlatformFile approach here as well to avoid Unicode issues.  Don't try to
pass UTF-8 to fopen on Windows.  It won't always work.

[Paweł Hajdan Jr., 2013-02-19 13:00:12]

PTAL

https://codereview.chromium.org/12259025/diff/9001/content/renderer/hyphenato...
File content/renderer/hyphenator/hyphenator.h (right):

https://codereview.chromium.org/12259025/diff/9001/content/renderer/hyphenato...
content/renderer/hyphenator/hyphenator.h:58: FILE* dictionary_file_;
On 2013/02/15 18:16:40, darin wrote:
> nit: You could use ScopedStdioHandle here.

Done.

https://codereview.chromium.org/12259025/diff/9001/webkit/support/test_webkit...
File webkit/support/test_webkit_platform_support.cc (right):

https://codereview.chromium.org/12259025/diff/9001/webkit/support/test_webkit...
webkit/support/test_webkit_platform_support.cc:552: hyphen_dictionary_ =
hnj_hyphen_load(path.value().c_str());
On 2013/02/15 18:16:40, darin wrote:
> Does this work on Windows?  FilePath::value() returns wstring on Windows.  It
> looks like hnj_hyphen_load expects a |const char*|.  You should probably use
the
> FdopenPlatformFile approach here as well to avoid Unicode issues.  Don't try
to
> pass UTF-8 to fopen on Windows.  It won't always work.

Fixed. Thank you for advice about Windows quirks.

[darin (slow to review), 2013-02-21 04:08:44]

https://codereview.chromium.org/12259025/diff/18001/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

https://codereview.chromium.org/12259025/diff/18001/base/platform_file_posix....
base/platform_file_posix.cc:164: base::ThreadRestrictions::AssertIOAllowed();
nit: Does fdopen actually do any real blocking IO?  It seems like this should
basically be free.  Perhaps you are hoping to catch subsequent calls to fread?

https://codereview.chromium.org/12259025/diff/18001/base/platform_file_win.cc
File base/platform_file_win.cc (right):

https://codereview.chromium.org/12259025/diff/18001/base/platform_file_win.cc...
base/platform_file_win.cc:115: if (file == kInvalidPlatformFileValue)
nit: Is it really necessary to add this precondition?  Won't _open_osfhandle
simply fail when passed a invalid handle value?

nit: This function should also have the AssertIOAllowed() check.

https://codereview.chromium.org/12259025/diff/18001/third_party/hyphen/hyphen.c
File third_party/hyphen/hyphen.c (right):

https://codereview.chromium.org/12259025/diff/18001/third_party/hyphen/hyphen...
third_party/hyphen/hyphen.c:247: {
How about adding a patch on our end that asserts that this function is never
called?  Or, just exclude it with a #if 0 / #endif so that we get a link error
if someone ever tries to call it.  Again, I'm worried that someone will make the
mistake of using it and thereby have unicode file path bugs.

[Paweł Hajdan Jr., 2013-02-21 09:14:19]

PTAL

https://codereview.chromium.org/12259025/diff/18001/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

https://codereview.chromium.org/12259025/diff/18001/base/platform_file_posix....
base/platform_file_posix.cc:164: base::ThreadRestrictions::AssertIOAllowed();
On 2013/02/21 04:08:44, darin wrote:
> nit: Does fdopen actually do any real blocking IO?  It seems like this should
> basically be free.

Ah, right, I removed the assert then.

> Perhaps you are hoping to catch subsequent calls to fread?

No, nothing like that was my intention.

https://codereview.chromium.org/12259025/diff/18001/base/platform_file_win.cc
File base/platform_file_win.cc (right):

https://codereview.chromium.org/12259025/diff/18001/base/platform_file_win.cc...
base/platform_file_win.cc:115: if (file == kInvalidPlatformFileValue)
On 2013/02/21 04:08:44, darin wrote:
> nit: Is it really necessary to add this precondition?  Won't _open_osfhandle
> simply fail when passed a invalid handle value?

Good question. It does fail and takes entire process with it.

In Debug it looks like

f:\dd\vctools\crt_bld\self_x86\crt\src\fdopen.c(55) : Assertion failed: (filedes
>= 0 && (unsigned)filedes < (unsigned)_nhandle)

While the above is about _fdopen and not _open_osfhandle, I prefer not to mess
with surprising (to me) Windows semantics here.

> nit: This function should also have the AssertIOAllowed() check.

I removed one from POSIX version so I'm not adding one here for consistency.

https://codereview.chromium.org/12259025/diff/18001/third_party/hyphen/hyphen.c
File third_party/hyphen/hyphen.c (right):

https://codereview.chromium.org/12259025/diff/18001/third_party/hyphen/hyphen...
third_party/hyphen/hyphen.c:247: {
On 2013/02/21 04:08:44, darin wrote:
> How about adding a patch on our end that asserts that this function is never
> called?  Or, just exclude it with a #if 0 / #endif so that we get a link error
> if someone ever tries to call it.  Again, I'm worried that someone will make
the
> mistake of using it and thereby have unicode file path bugs.

I've added #if 0 to the .h file, so the failure is even faster.

[darin (slow to review), 2013-02-21 16:31:08]

LGTM

https://codereview.chromium.org/12259025/diff/25001/third_party/hyphen/hyphen.c
File third_party/hyphen/hyphen.c (right):

https://codereview.chromium.org/12259025/diff/25001/third_party/hyphen/hyphen...
third_party/hyphen/hyphen.c:250: f = fopen (fn, "r");
Just for completeness, or in case anyone asks, the right way to implement this
function on Windows is to prescribe that the given |fn| consists of UTF-8 code
points, then convert that to UTF-16, and then pass that string to _wfopen.

Tempting things that would be a mistake:  Assuming that |fn| is in the default
multi-byte code page, which is usually some ANSI encoding (not UTF-8), means
that this function would almost always work except in cases where the file
system has paths with code points that cannot be expressed using the default
multi-byte code page.  For example, if the user's logon name has interesting
characters in it, you can readily encounter this type of bug.

[Paweł Hajdan Jr., 2013-02-25 23:33:40]

Committed patchset #8 manually as r184515 (presubmit successful).