[webkit-reviews] review denied: [Bug 48610] [Chromium] Implement hyphenation for Chromium : [Attachment 126480] Add stub methods.

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Hironori Bono
<hbono at chromium.org>'s request for review:
Bug 48610: [Chromium] Implement hyphenation for Chromium
https://bugs.webkit.org/show_bug.cgi?id=48610

Attachment 126480: Add stub methods.
https://bugs.webkit.org/attachment.cgi?id=126480&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=126480&action=review


> Source/WebCore/platform/chromium/PlatformSupport.h:192
> +    static size_t lastHyphenLocation(const UChar* characters, size_t length,
size_t index, const String& locale);

nit: this function could probably benefit from having a verb, like "compute"...
computeLastHyphenLocation.  what is the meaning of the "index" parameter?

> Source/WebCore/platform/text/chromium/Hyphenation.cpp:39
> +size_t lastHyphenLocation(const UChar* characters, size_t length, size_t
beforeIndex, const AtomicString& localeIdentifier)

it seems like this should also be named computeLastHyphenLocation

> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:168
> +    virtual bool canHyphenate(const WebKit::WebString& locale) { return
false; }

nit: no need for WebKit:: prefix

> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:172
> +

nit: two new lines above section headers

> Source/WebKit/chromium/src/PlatformSupport.cpp:539
> +bool PlatformSupport::canHyphenate(const String& locale)

Why isn't the hyphenation engine directly callable by WebKit?  WebKit uses ICU
directly.  How is this API implemented on the Chromium side?