Bug 28155 – Remove platform-specific code from FontFastPath.cpp

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: Platform

http://trac.webkit.org/projects/webkit/changeset/46938 introduced WinCE-specific code to Font::glyphDataForCharacter(). The code seems misplaced, given that no other platform-specific code exists in FontFastPath.cpp. Whatever problem it is trying to solve should probably be addressed by platform-specific GlyphPageTreeNode code.

[Yong Li]

(In reply to comment #0)
> http://trac.webkit.org/projects/webkit/changeset/46938 introduced
> WinCE-specific code to Font::glyphDataForCharacter(). The code seems misplaced,
> given that no other platform-specific code exists in FontFastPath.cpp. Whatever
> problem it is trying to solve should probably be addressed by platform-specific
> GlyphPageTreeNode code.

Do you also think a lot of #if PLATFORM(...) should be removed from those platform-independent files? 

I can find tons of those.

for example, #if PLATFORM(MAC) in AccessibilityRenderObject.cpp

[Yong Li]

I understand we should try to avoid using PLATFORM(). but apparently it's not banned in webkit code.

[George Staikos]

Yes, the overall goal is to reduce use of PLATFORM().  This was discussed on webkit-dev a while ago.  Sometime to keep in mind though is that we really shouldn't go to effort of making "generic" systems that only one platform will ever use.  We should have at least two users before we genericize.

(In reply to comment #2)
> I understand we should try to avoid using PLATFORM(). but apparently it's not
> banned in webkit code.

I think a good rule to follow is:
“If you think you need to add platform-specific code to a file that currently has no platform-specific code, then think again”.

[George Staikos]

(In reply to comment #4)
> (In reply to comment #2)
> > I understand we should try to avoid using PLATFORM(). but apparently it's not
> > banned in webkit code.
> 
> I think a good rule to follow is:
> “If you think you need to add platform-specific code to a file that currently
> has no platform-specific code, then think again”.

He definitely did when we wrote it and we do try to follow the same rule.  Unfortunately WinCE is a different beast than you are probably used to.  Sometimes the peg isn't round.

[Yong Li]

(In reply to comment #4)
> (In reply to comment #2)
> > I understand we should try to avoid using PLATFORM(). but apparently it's not
> > banned in webkit code.
> 
> I think a good rule to follow is:
> “If you think you need to add platform-specific code to a file that currently
> has no platform-specific code, then think again”.

If I could find a way to implement the functionality without changing FontFastPath.cpp, I would definitely leave FontFastPath.cpp alone.

[Ahmad Saleem]

I am not able to find any comment referred in below commit:

http://trac.webkit.org/projects/webkit/changeset/46938

Marking this as "RESOLVED CONFIGURATION CHANGED".