[webkit-reviews] review denied: [Bug 98395] Implement FontLoader interface : [Attachment 188984] Updated patch, fix test failure

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Kunihiko Sakamoto
<ksakamoto at chromium.org>'s request for review:
Bug 98395: Implement FontLoader interface
https://bugs.webkit.org/show_bug.cgi?id=98395

Attachment 188984: Updated patch, fix test failure
https://bugs.webkit.org/attachment.cgi?id=188984&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=188984&action=review


This patch looks great.  Thank you for taking the time to speak with me this
afternoon.

For non-chromium ports, this will be off by default (the #define will be
false).  For Chromium, this will be enabled, but we need to guard it with a
feature flag.

In order to test your feature, you will need to create a setting and expose it
via internal settings, similar to what we did with the threaded HTML parser.  I
recommend that you post and land the feature flag and setting first.  That will
ensure that your tests require the setting to expose this to the web.

Once we believe this feature is stable and ready for release we'll change the
runtime flag to default to true.  A little while later, we'll remove the flag. 
Unfortunately this process is not quite as refined as we'd like, so adding the
setting is a bit of boiler plate code, but much less code than you've already
written for this.

Thank you again for your work here.  Many websites will be very happy to see
this!

I've marked this r- for now, as we should land this with the real names and
with the runtime flag.	However I'm happy to r+ any follow-up patch immediately
with those fixes.

> Source/WebCore/css/CSSFontFace.cpp:172
> +    Document* document =
(*m_segmentedFontFaces.begin())->fontSelector()->document();

I don't think the () are necessary?  They don't hurt, but I don't think c++
requires them.