Summary: | Backslash is transcoded into yen sign even when non japanese font is specified | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shinichiro Hamaji <hamaji> | ||||||||||
Component: | Text | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, ap, darin, eric, mitz, tkent, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Shinichiro Hamaji
2010-05-25 00:14:57 PDT
Created attachment 56993 [details]
Patch v1
Created attachment 57007 [details]
Patch v1 - rebased
Comment on attachment 57007 [details] Patch v1 - rebased > @@ -70,8 +70,8 @@ Font::Font(const FontDescription& fd, short letterSpacing, short wordSpacing) > , m_letterSpacing(letterSpacing) > , m_wordSpacing(wordSpacing) > , m_isPlatformFont(false) > - , m_needsTranscoding(fontTranscoder().needsTranscoding(family().family().string())) > { > + m_needsTranscoding = fontTranscoder().needsTranscoding(*this); > } In general, it isn't a great pattern to pass "this" to external functions from a constructor. The object may still be in some transitional state, and although it's valid C++, it may catch the programmer by surprise: - subclass constructors haven't been invoked yet, and virtual methods table hasn't been swapped to final one yet; - a reference counted object can still have a refcount of 0, so taking a temporary RefPtr will destroy it from within the constructor; - post-constructor "init" functions that someone else wrote to avoid these problems haven't run yet; - etc. I'm still unclear on the "specified font" concept. The test doesn't check multiple font names (e.g. '"some-windows-only-font", "Times"', or "some-windows-only-font", "MS PGothic"). Is "specified font" inherited from parent nodes? Created attachment 57438 [details]
Patch v2
Thanks for your review! > In general, it isn't a great pattern to pass "this" to external functions from a constructor. The object may still be in some transitional state, and although it's valid > C++, it may catch the programmer by surprise: > - subclass constructors haven't been invoked yet, and virtual methods table hasn't been swapped to final one yet; > - a reference counted object can still have a refcount of 0, so taking a temporary RefPtr will destroy it from within the constructor; > - post-constructor "init" functions that someone else wrote to avoid these problems haven't run yet; > - etc. Yeah, I totally agree. Fortunately, I could easily remove the "this" by using FontDescription instead of Font in this case. > I'm still unclear on the "specified font" concept. Basically, whenever a webpage specifies a font name by font-family or font, the font is considered "specified". If a non Japanese font is "specified", we guess the author of the web page intended to show backslashes even with Japanese encodings. If a generic font such as serif, sans-serif, etc. is specified, the font is considered "unspecified" because IE and Firefox choose locale specific fonts. We check only the first font family. I guess this is OK for now as most fonts would have U+005C. This limitation should be fixed if isSpecifiedFont will be used for other purpose. I doubt it will happen though. I updated the comment on m_isSpecifiedFont. I hope the updated comment is clearer a bit. I'm not sure if the name "isSpecifiedFont" is good. Suggestions of a better name will be really appreciated. > The test doesn't check multiple font names (e.g. '"some-windows-only-font", "Times"', or "some-windows-only-font", "MS PGothic"). Very good point. Actually, "Times, serif" was incorrect with the previous patch (the second font calls setIsSpecifiedFont). Now I fixed this issue and added some test cases. Thanks for catching this! > Is "specified font" inherited from parent nodes? Yes, just like font-family. Comment on attachment 57438 [details]
Patch v2
Looks OK.
This patch has a review and can be landed, is there a reason it's commit-queue-? (In reply to comment #7) > This patch has a review and can be landed, is there a reason it's commit-queue-? The patch can not be applied cleanly. Committed r63950: <http://trac.webkit.org/changeset/63950> http://trac.webkit.org/changeset/63950 might have broken SnowLeopard Intel Release (Tests) (In reply to comment #10) > http://trac.webkit.org/changeset/63950 might have broken SnowLeopard Intel Release (Tests) Sorry for this. I think I should have treated font-family "-webkit-*" as unspecified. For now, I'll land a failing test and post a fix soon later. |