Bug 44478 – [GTK] Add support for automatic hyphenation

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

The following tests show no hyphenation at all:
fast/text/hyphenate-first-word.html
fast/text/hyphenate-locale.html

[Philippe Normand]

Those seem to pass now. We can unflag them maybe?

[Martin Robinson]

The results still seem to be different (GTK+ is not hyphenating at all) from the Mac port. It could be that only the Mac port is hypenating properly now.

[Carlos Alberto Lopez Perez]

I think I can see the hypens working if i use the gtk minibrowser (from trunk@r174265) and open this test http://trac.webkit.org/export/174265/trunk/LayoutTests/fast/text/hyphens.html


What do you think? Should we unskip the hyphen tests?

Right now the following tests are skipped:

# [GTK] GTK+ does not support hyphenation
webkit.org/b/44478 fast/text/hyphenate-character.html [ Skip ]
webkit.org/b/44478 fast/text/hyphens.html [ Skip ]
webkit.org/b/44478 fast/text/hyphenate-first-word.html [ Skip ]
webkit.org/b/44478 fast/text/hyphenate-locale.html [ Skip ]
webkit.org/b/44478 fast/text/hyphen-min-preferred-width.html [ Skip ]

Running the tests, fail all of them, but I think they might need just a rebaseline.


At least fast/text/hyphenate-character.html and fast/text/hyphens.html needs to be rebaselined after r174233, and probably the others need also a rebaseline from previous revisions that affected them.

[Martin Robinson]

Is the auto test working? I believe in auto mode, a library is used to pick proper hyphenation locations. This support is missing entirely, if I understand correctly.

[Martin Robinson]

Created attachment 251182 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 251182 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251182&action=review

> Source/WebCore/PlatformGTK.cmake:186
> +    platform/text/gtk/HyphenationLibHyphen.cpp

I wonder why this is specific to GTK, I guess it could be platform/text/libhyphen/HyphenationLibHyphen.cpp

> Source/WebCore/platform/gtk/GtkUtilities.cpp:2
> + * Copyright (C) 2011, 2013 Igalia S.L.

2013?

> Source/WebCore/platform/gtk/GtkUtilities.cpp:101
> +CString getWebKitBuildDirectory()

This should be webkitBuildDirectory()

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:43
> +const char* const gDictionaryDirectories[] = {

This should be static

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:53
> +    return fileName.substring(5, fileName.length() - 5 - 4);

Can we give names to these magic numbers?

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:60
> +    static const char* gDictionaryFilePattern = "hyph_*.dic";
> +    Vector<String> filePaths = listDirectory(directoryPath, gDictionaryFilePattern);
> +    for (auto& filePath : filePaths)

You could simplify this

for (const auto& filePath : listDirectory(directoryPath, "hyph_*.dic"))

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:72
> +    if (g_file_test(dictionariesPath.get(), static_cast<GFileTest>(G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR))) {

G_FILE_TEST_IS_DIR already implies it exists.

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:77
> +    // Try alternative dictionaries path for people not using JHBuild.

Thanks!

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:78
> +    dictionariesPath.reset(g_build_filename(buildDirectory.data(), "webkitgtk-test-dicts", NULL));

nullptr

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:80
> +    if (g_file_test(dictionariesPath.get(), static_cast<GFileTest>(G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR)))
> +        scanDirectoryForDicionaries(dictionariesPath.get(), availableLocales);

In thi case we could probably avoid the file existence check, listDirectory will return an empty vector in that case.

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:117
> +    static RefPtr<HyphenationDictionary> createNull()

why createNull(), what's Null? I would use just create().

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:122
> +    static RefPtr<HyphenationDictionary> createWithDictionaryPath(const CString& dictPath)

And this could be also create, bur receiving a path, we don't need to use a different name. Also, since create() methods never return null, we could probably use Ref<HyphenationDictionary> instead of RefPtr

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:127
> +    HyphenDict* libhyphenDictionary()

This should be const. Maybe platformDictionary?

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:159
> +static AtomicStringKeyedMRUCache<RefPtr<HyphenationDictionary>>& hyphenDictionaryCache()

Isn't it possible to use std::unique_ptr directly for the AtomicStringKeyedMRUCache? do we need the wrapper class?

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:161
> +    DEPRECATED_DEFINE_STATIC_LOCAL(AtomicStringKeyedMRUCache<RefPtr<HyphenationDictionary>>, cache, ());

Would it be possible to use NeverDestroyed instead of DEPRECATED_DEFINE_STATIC_LOCAL?

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:183
> +size_t lastHyphenLocation(StringView string, size_t beforeIndex, const AtomicString&  localeIdentifier)

const AtomicString&  localeIdentifier <- there's an extra space there

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:203
> +    Vector<char> hyphenArray = Vector<char>(utf8StringCopy.length() - leadingSpaceBytes + 5);

Vector<char> hyphenArray(utf8StringCopy.length() - leadingSpaceBytes + 5);

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:221
> +            if (replacements[i])
> +                free(replacements[i]);

I think free is null safe.

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:224
> +        free(replacements);
> +    }

So, what are the replacements for? it seems we free them right after getting them?

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:227
> +    free(positions);
> +    free(removedCharacterCounts);

Same about these

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:230
> +        if (hyphenArrayData[i] & 1)

What's this? why & 1?

[Martin Robinson]

Comment on attachment 251182 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251182&action=review

>> Source/WebCore/PlatformGTK.cmake:186
>> +    platform/text/gtk/HyphenationLibHyphen.cpp
> 
> I wonder why this is specific to GTK, I guess it could be platform/text/libhyphen/HyphenationLibHyphen.cpp

I plan to keep this specific to GTK+ for the moment, because I may rely on pango in a future patch to canonicalize shorter locales. I considered "unix," but I think that gtk makes the most sense for now since we are already collecting things there. Maybe we can reapproach this if EFL wants to share.

>> Source/WebCore/platform/gtk/GtkUtilities.cpp:2
>> + * Copyright (C) 2011, 2013 Igalia S.L.
> 
> 2013?

Yes, this originates from utilities in WebKitTestRunner.

>> Source/WebCore/platform/gtk/GtkUtilities.cpp:101
>> +CString getWebKitBuildDirectory()
> 
> This should be webkitBuildDirectory()

Okay.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:43
>> +const char* const gDictionaryDirectories[] = {
> 
> This should be static

Okay.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:53
>> +    return fileName.substring(5, fileName.length() - 5 - 4);
> 
> Can we give names to these magic numbers?

Sure. Why not?

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:60
>> +    for (auto& filePath : filePaths)
> 
> You could simplify this
> 
> for (const auto& filePath : listDirectory(directoryPath, "hyph_*.dic"))

Nice.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:72
>> +    if (g_file_test(dictionariesPath.get(), static_cast<GFileTest>(G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR))) {
> 
> G_FILE_TEST_IS_DIR already implies it exists.

Good catch. This was carried over from the WebKitTestRunner code.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:80
>> +        scanDirectoryForDicionaries(dictionariesPath.get(), availableLocales);
> 
> In thi case we could probably avoid the file existence check, listDirectory will return an empty vector in that case.

Okay. Seems reasonable.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:117
>> +    static RefPtr<HyphenationDictionary> createNull()
> 
> why createNull(), what's Null? I would use just create().

This is for the purposes of specificity. This shouldn't be used to create any random dictionary, but only to create a null or empty dictionary. The code documents that the result is non-functional and really only a special case for the MRU cache. I'm not sure pedantically named constructors are a problem in this case. ;)

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:122
>> +    static RefPtr<HyphenationDictionary> createWithDictionaryPath(const CString& dictPath)
> 
> And this could be also create, bur receiving a path, we don't need to use a different name. Also, since create() methods never return null, we could probably use Ref<HyphenationDictionary> instead of RefPtr

See below for why I didn't use unique_ptr for AtomicStringKeyedMRUCache. This is the same issue. I don't mind changing this constructor to create, since the argument makes it more obvious.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:127
>> +    HyphenDict* libhyphenDictionary()
> 
> This should be const. Maybe platformDictionary?

I think libhyphenDictionary might be a better name, since platformFoo is used mainly in cases where the platform is unknown (platform-independent). In this case we do know the platform and its details, so the name can better document the difference between the member and the containing class.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:159
>> +static AtomicStringKeyedMRUCache<RefPtr<HyphenationDictionary>>& hyphenDictionaryCache()
> 
> Isn't it possible to use std::unique_ptr directly for the AtomicStringKeyedMRUCache? do we need the wrapper class?

I attempted this at first, but couldn't get it to work with AtomicStringKeyedMRUCache. The issue is that the get(...) method returns the same type as is stored in the cache, instead of a reference (T instead of T&). If I used std::unique_ptr that would attempt to copy the std::unique_ptr, which won't work. For the moment it seems to be built for holding RefPtrs. This can be fixed in a later patch, but I'd prefer not to address it here.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:161
>> +    DEPRECATED_DEFINE_STATIC_LOCAL(AtomicStringKeyedMRUCache<RefPtr<HyphenationDictionary>>, cache, ());
> 
> Would it be possible to use NeverDestroyed instead of DEPRECATED_DEFINE_STATIC_LOCAL?

Yes. I copied this from the CoreText implementation.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:183
>> +size_t lastHyphenLocation(StringView string, size_t beforeIndex, const AtomicString&  localeIdentifier)
> 
> const AtomicString&  localeIdentifier <- there's an extra space there

Thanks.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:203
>> +    Vector<char> hyphenArray = Vector<char>(utf8StringCopy.length() - leadingSpaceBytes + 5);
> 
> Vector<char> hyphenArray(utf8StringCopy.length() - leadingSpaceBytes + 5);

Ooh. Too much time away from WebKit C++!

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:221
>> +                free(replacements[i]);
> 
> I think free is null safe.

Okay.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:224
>> +    }
> 
> So, what are the replacements for? it seems we free them right after getting them?

We do not use them, but libhyphen might allocate them anyway. Based on the documentation and the example code included with the library, we need to free them if they are set. I've never seen libhyphen set them, but this is the contract specified in the interface.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:227
>> +    free(removedCharacterCounts);
> 
> Same about these

Ditto.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:230
>> +        if (hyphenArrayData[i] & 1)
> 
> What's this? why & 1?

Hyphenation locations are at all places where hyphenArrayData[i] is odd. I can add a comment explaining this.

[Martin Robinson]

Created attachment 251825 [details]
Patch

[Carlos Garcia Campos]

(In reply to comment #7)
> Comment on attachment 251182 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251182&action=review
> 
> >> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:117
> >> +    static RefPtr<HyphenationDictionary> createNull()
> > 
> > why createNull(), what's Null? I would use just create().
> 
> This is for the purposes of specificity. This shouldn't be used to create
> any random dictionary, but only to create a null or empty dictionary. The
> code documents that the result is non-functional and really only a special
> case for the MRU cache. I'm not sure pedantically named constructors are a
> problem in this case. ;)

Ah, I see.

> >> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:122
> >> +    static RefPtr<HyphenationDictionary> createWithDictionaryPath(const CString& dictPath)
> > 
> > And this could be also create, bur receiving a path, we don't need to use a different name. Also, since create() methods never return null, we could probably use Ref<HyphenationDictionary> instead of RefPtr
> 
> See below for why I didn't use unique_ptr for AtomicStringKeyedMRUCache.
> This is the same issue. I don't mind changing this constructor to create,
> since the argument makes it more obvious.
> 
> >> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:127
> >> +    HyphenDict* libhyphenDictionary()
> > 
> > This should be const. Maybe platformDictionary?
> 
> I think libhyphenDictionary might be a better name, since platformFoo is
> used mainly in cases where the platform is unknown (platform-independent).
> In this case we do know the platform and its details, so the name can better
> document the difference between the member and the containing class.
> 
> >> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:159
> >> +static AtomicStringKeyedMRUCache<RefPtr<HyphenationDictionary>>& hyphenDictionaryCache()
> > 
> > Isn't it possible to use std::unique_ptr directly for the AtomicStringKeyedMRUCache? do we need the wrapper class?
> 
> I attempted this at first, but couldn't get it to work with
> AtomicStringKeyedMRUCache. The issue is that the get(...) method returns the
> same type as is stored in the cache, instead of a reference (T instead of
> T&). If I used std::unique_ptr that would attempt to copy the
> std::unique_ptr, which won't work. For the moment it seems to be built for
> holding RefPtrs. This can be fixed in a later patch, but I'd prefer not to
> address it here.

Ok.

> >> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:224
> >> +    }
> > 
> > So, what are the replacements for? it seems we free them right after getting them?
> 
> We do not use them, but libhyphen might allocate them anyway. Based on the
> documentation and the example code included with the library, we need to
> free them if they are set. I've never seen libhyphen set them, but this is
> the contract specified in the interface.

According to the documentation you can pass NULL for those. And if the code is this one 

https://code.google.com/p/android-source-browsing/source/browse/hyphen.c?repo=platform--external--hyphenation 

It seems the values are null checked indeed

https://code.google.com/p/android-source-browsing/source/browse/hyphen.c?repo=platform--external--hyphenation#869

[Martin Robinson]

(In reply to comment #9)

> According to the documentation you can pass NULL for those. And if the code
> is this one 
> 
> https://code.google.com/p/android-source-browsing/source/browse/hyphen.
> c?repo=platform--external--hyphenation 
> 
> It seems the values are null checked indeed
> 
> https://code.google.com/p/android-source-browsing/source/browse/hyphen.
> c?repo=platform--external--hyphenation#869

You can see here in the example code where it passes pointers to null pointers and frees them anyway:

https://code.google.com/p/android-source-browsing/source/browse/example.c?repo=platform--external--hyphenation#144

I'm not sure if the null check is relevant or not, because inside libhyphen they are pointers to the null pointers, not simply the null pointers.

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #9)
> 
> > According to the documentation you can pass NULL for those. And if the code
> > is this one 
> > 
> > https://code.google.com/p/android-source-browsing/source/browse/hyphen.
> > c?repo=platform--external--hyphenation 
> > 
> > It seems the values are null checked indeed
> > 
> > https://code.google.com/p/android-source-browsing/source/browse/hyphen.
> > c?repo=platform--external--hyphenation#869
> 
> You can see here in the example code where it passes pointers to null
> pointers and frees them anyway:
> 
> https://code.google.com/p/android-source-browsing/source/browse/example.
> c?repo=platform--external--hyphenation#144

I think it's because it uses any of those, but if you don't use them at all, you can just pass NULL, or that's what I understand of the documentation and the code.

 rep:       NULL (only standard hyph.), or replacements (hyphenation points
            signed with `=' in replacements);
 pos:       NULL, or difference of the actual position and the beginning
            positions of the change in input words;
 cut:       NULL, or counts of the removed characters of the original words
            at hyphenation,


> I'm not sure if the null check is relevant or not, because inside libhyphen
> they are pointers to the null pointers, not simply the null pointers.

I don't get what you mean, it fist checks rep && pos && cut and then it checks !*rep && !*pos && !*cut before allocating them. Why do we want to allocate them if we are not using them? or am I missing something?

[Martin Robinson]

(In reply to comment #11)

> 
> > I'm not sure if the null check is relevant or not, because inside libhyphen
> > they are pointers to the null pointers, not simply the null pointers.
> 
> I don't get what you mean, it fist checks rep && pos && cut and then it
> checks !*rep && !*pos && !*cut before allocating them. Why do we want to
> allocate them if we are not using them? or am I missing something?

The example code never uses any of them, but always attempts to free them if they are set.

[Martin Robinson]

(In reply to comment #12)
> (In reply to comment #11)
> 
> > 
> > > I'm not sure if the null check is relevant or not, because inside libhyphen
> > > they are pointers to the null pointers, not simply the null pointers.
> > 
> > I don't get what you mean, it fist checks rep && pos && cut and then it
> > checks !*rep && !*pos && !*cut before allocating them. Why do we want to
> > allocate them if we are not using them? or am I missing something?
> 
> The example code never uses any of them, but always attempts to free them if
> they are set.

Additionally, if the code of libhyphen ever changes so that they are allocated, this will prevent a memory leak.

[Carlos Garcia Campos]

(In reply to comment #12)
> (In reply to comment #11)
> 
> > 
> > > I'm not sure if the null check is relevant or not, because inside libhyphen
> > > they are pointers to the null pointers, not simply the null pointers.
> > 
> > I don't get what you mean, it fist checks rep && pos && cut and then it
> > checks !*rep && !*pos && !*cut before allocating them. Why do we want to
> > allocate them if we are not using them? or am I missing something?
> 
> The example code never uses any of them, but always attempts to free them if
> they are set.

They are passed to single_hyphenations and used there.

[Carlos Garcia Campos]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > 
> > > 
> > > > I'm not sure if the null check is relevant or not, because inside libhyphen
> > > > they are pointers to the null pointers, not simply the null pointers.
> > > 
> > > I don't get what you mean, it fist checks rep && pos && cut and then it
> > > checks !*rep && !*pos && !*cut before allocating them. Why do we want to
> > > allocate them if we are not using them? or am I missing something?
> > 
> > The example code never uses any of them, but always attempts to free them if
> > they are set.
> 
> Additionally, if the code of libhyphen ever changes so that they are
> allocated, this will prevent a memory leak.

But the docs say you can pass NULL for those, I don't understand why you want to keep allocating memory we don't use at all.

[Martin Robinson]

(In reply to comment #15)

> But the docs say you can pass NULL for those, I don't understand why you
> want to keep allocating memory we don't use at all.

Passing null (as opposed to a pointer to null) causes libhyphen to crash, I believe. The documentation says "NULL (only standard hyph.)" for rep. I'm not sure what that means exactly, but it didn't work when I tried it.

It seems that Gecko [1] is not freeing this memory, so it's probably safe to do the same here. I worry about doing something different than example.c though.

1. https://dxr.mozilla.org/mozilla-central/source/intl/hyphenation/nsHyphenator.cpp

[Carlos Garcia Campos]

(In reply to comment #16)
> (In reply to comment #15)
> 
> > But the docs say you can pass NULL for those, I don't understand why you
> > want to keep allocating memory we don't use at all.
> 
> Passing null (as opposed to a pointer to null) causes libhyphen to crash, I
> believe.

Well, if it crashes there's nothing more to discuss.

> The documentation says "NULL (only standard hyph.)" for rep. I'm
> not sure what that means exactly, but it didn't work when I tried it.
> 
> It seems that Gecko [1] is not freeing this memory, so it's probably safe to
> do the same here. I worry about doing something different than example.c
> though.

I'm not proposing not to free the mem, if we have to pass a pointer (pointing to NULL), the function will allocate the memory and we will have to free it.

> 1.
> https://dxr.mozilla.org/mozilla-central/source/intl/hyphenation/nsHyphenator.
> cpp

[Martin Robinson]

(In reply to comment #17)

> I'm not proposing not to free the mem, if we have to pass a pointer
> (pointing to NULL), the function will allocate the memory and we will have
> to free it.

Okay. So what's the result here? Is the patch okay like this? If so, I can upload a rebased version.

[Carlos Garcia Campos]

Comment on attachment 251825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251825&action=review

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:43
> +#if USE(LIBHYPHEN)

This should be after the Hyphenation.h, and before all other includes.

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:61
> +    static const char* gDictionaryFilePattern = "hyph_*.dic";

Why not using the string literal directly? I think it's obvious in this case that it's the pattern and it's only used here.

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:251
> +#else
> +
> +bool canHyphenate(const AtomicString& /* localeIdentifier */)
> +{
> +    return false;
> +}
> +
> +size_t lastHyphenLocation(StringView, size_t /* beforeIndex */, const AtomicString& /* localeIdentifier */)
> +{
> +    ASSERT_NOT_REACHED();
> +    return 0;
> +}
> +
> +#endif // USE(LIBHYPHEN)

It's a bit weird to have this in a file called HyphenationLibHyphen.cpp. Why don't we build this only when libhyphen is present? otherwise we build the stubbed implementation instead of duplicating it here.

[Carlos Garcia Campos]

Comment on attachment 251825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251825&action=review

> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:62
> +    for (auto& filePath : listDirectory(directoryPath, gDictionaryFilePattern))

Btw, I also suggested to use const auto& here in my previous review.

[Martin Robinson]

Comment on attachment 251825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251825&action=review

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:43
>> +#if USE(LIBHYPHEN)
> 
> This should be after the Hyphenation.h, and before all other includes.

Okay.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:61
>> +    static const char* gDictionaryFilePattern = "hyph_*.dic";
> 
> Why not using the string literal directly? I think it's obvious in this case that it's the pattern and it's only used here.

Sure.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:62
>> +    for (auto& filePath : listDirectory(directoryPath, gDictionaryFilePattern))
> 
> Btw, I also suggested to use const auto& here in my previous review.

Okay.

>> Source/WebCore/platform/text/gtk/HyphenationLibHyphen.cpp:251
>> +#endif // USE(LIBHYPHEN)
> 
> It's a bit weird to have this in a file called HyphenationLibHyphen.cpp. Why don't we build this only when libhyphen is present? otherwise we build the stubbed implementation instead of duplicating it here.

This is to move toward a time when you can simply have one big source list and not have to do any conditional inclusion. I can use CMake though.

[Martin Robinson]

Created attachment 251963 [details]
Patch

[Martin Robinson]

Created attachment 251968 [details]
Patch

[Martin Robinson]

Comment on attachment 251968 [details]
Patch

Clearing flags on attachment: 251968

Committed r183584: <http://trac.webkit.org/changeset/183584>

[Martin Robinson]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #24)
> Comment on attachment 251968 [details]
> Patch
> 
> Clearing flags on attachment: 251968
> 
> Committed r183584: <http://trac.webkit.org/changeset/183584>

It seems the necessary library isn't installed on GTK bots:

-- Could NOT find HYPHEN (missing:  HYPHEN_INCLUDE_DIR HYPHEN_LIBRARIES) 
CMake Error at Source/cmake/OptionsGTK.cmake:385 (message):
  libhyphen is needed for USE_LIBHYPHEN.

- https://build.webkit.org/builders/GTK%20Linux%20ARM%20Release/builds/6966/steps/compile-webkit/logs/stdio
- https://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/52272/steps/compile-webkit/logs/stdio