[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