[webkit-reviews] review granted: [Bug 25239] Add a settings entry to en/disable web font support : [Attachment 29546] patch 2 with the check moved upstream

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Apr 28 21:50:20 PDT 2009


mitz at webkit.org has granted Jungshik Shin <jshin at chromium.org>'s request for
review:
Bug 25239: Add a settings entry to en/disable web font support
https://bugs.webkit.org/show_bug.cgi?id=25239

Attachment 29546: patch 2 with the check moved upstream
https://bugs.webkit.org/attachment.cgi?id=29546&action=review

------- Additional Comments from mitz at webkit.org
Please rename the instance variable, setter and getter to *remoteFontsEnabled*
(in plural). I don't think "remote" is the best way to characterize these fonts
(they may come from file: or data: URLs) but I could not think of a better
name, and they are the opposite of local() fonts, I guess.

> +	   There's no behavior change. So, no new test is necessary.

You could have added WebKit SPI for changing the setting and a DumpRenderTree
LayoutTestController function for controlling it, making it testable. It is
fine if you don't want to do that, but I don't think you should make that
comment in the change log.

r=me with the renames.


More information about the webkit-reviews mailing list