[webkit-reviews] review denied: [Bug 38701] [chromium] don't call fontconfig twice in complex text path : [Attachment 113611] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Kenichi Ishibashi
<bashi at chromium.org>'s request for review:
Bug 38701: [chromium] don't call fontconfig twice in complex text path
https://bugs.webkit.org/show_bug.cgi?id=38701

Attachment 113611: Patch
https://bugs.webkit.org/attachment.cgi?id=113611&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=113611&action=review


This approach seems reasonable to me. r- because we need to figure out how to
land this without breaking the chromium build.

> Source/WebKit/chromium/public/linux/WebFontInfo.h:36
>  #include "../WebCString.h"
> +#include "../linux/WebFontFamily.h"
>  #include "../linux/WebFontRenderStyle.h"

fishd may have a preference on whether the includes from the same dir should
just be "WebFontFamily.h" or "../linux/WebFontFamily.h". I don't have a strong
preference one way or another.

> Source/WebKit/chromium/public/linux/WebFontInfo.h:53
> -    WEBKIT_EXPORT static WebCString familyForChars(const WebUChar*
characters, size_t numCharacters, const char* preferredLocale);
> +    // Returns: the font family instance. The instance has an empty font
name if the request could not be satisfied.
> +    WEBKIT_EXPORT static void familyForChars(const WebUChar* characters,
size_t numCharacters, const char* preferredLocale, WebFontFamily*);

Is changing this function going to break the chromium build?  It looks like
this called from
content/browser/renderer_host/render_sandbox_host_linux.cc:270.  I think you
need to do a 3 sided patch to migrate to the new API.

> Source/WebKit/chromium/public/linux/WebSandboxSupport.h:55
> -    virtual WebString getFontFamilyForCharacters(const WebUChar* characters,
size_t numCharacters, const char* preferredLocale)  = 0;
> +    // Returns a WebFontFamily instance with the font name. The instance has
empty font name if the request cannot be satisfied.
> +    virtual void getFontFamilyForCharacters(const WebUChar* characters,
size_t numCharacters, const char* preferredLocale, WebFontFamily*)  = 0;

Is changing this function going to break the chromium build?  It looks like
this is implemented in content/renderer/renderer_webkitplatformsupport_impl.cc.


> Source/WebKit/chromium/src/PlatformSupport.cpp:441
> +    return;

Nit: remove return

> Source/WebKit/chromium/src/PlatformSupport.cpp:451
> +    return;

Nit: remove return

> Source/WebKit/chromium/src/linux/WebFontInfo.cpp:126
> +    return;

Nit: remove return

> LayoutTests/platform/chromium/test_expectations.txt:1969
> +BUGUKAI LINUX : fast/text/international/bold-bengali.html = PASS

This shouldn't be necessary.  The test should already be running on Linux. 
What are you trying to do here?