Bug 39654 – Backslash is transcoded into yen sign even when non japanese font is specified

• Component: Text
• Status: RESOLVED FIXED

[Shinichiro Hamaji]

Created attachment 56983 [details]
test case

Bug 24906 introduced font based transcoding. In Bug 24906, I put a FIXME comment for the case where a web author specifies non-Japanese fonts to avoid yen sign glyph.

[Shinichiro Hamaji]

Created attachment 56993 [details]
Patch v1

[Shinichiro Hamaji]

Created attachment 57007 [details]
Patch v1 - rebased

[Alexey Proskuryakov]

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?

[Shinichiro Hamaji]

Created attachment 57438 [details]
Patch v2

[Shinichiro Hamaji]

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.

[Kent Tamura]

Comment on attachment 57438 [details]
Patch v2

Looks OK.

[Alexey Proskuryakov]

This patch has a review and can be landed, is there a reason it's commit-queue-?

[Kent Tamura]

(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.

[Shinichiro Hamaji]

Committed r63950: <http://trac.webkit.org/changeset/63950>

[WebKit Review Bot]

http://trac.webkit.org/changeset/63950 might have broken SnowLeopard Intel Release (Tests)

[Shinichiro Hamaji]

(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.