Bug 28155 - Remove platform-specific code from FontFastPath.cpp
Summary: Remove platform-specific code from FontFastPath.cpp
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-10 10:45 PDT by mitz
Modified: 2023-05-13 04:04 PDT (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2009-08-10 10:45:35 PDT
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.
Comment 1 Yong Li 2009-08-10 10:50:48 PDT
(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
Comment 2 Yong Li 2009-08-10 10:54:17 PDT
I understand we should try to avoid using PLATFORM(). but apparently it's not banned in webkit code.
Comment 3 George Staikos 2009-08-10 11:00:46 PDT
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.
Comment 4 mitz 2009-08-10 11:02:09 PDT
(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”.
Comment 5 George Staikos 2009-08-10 11:05:46 PDT
(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.
Comment 6 Yong Li 2009-08-10 11:14:12 PDT
(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.
Comment 7 Ahmad Saleem 2023-05-13 04:04:12 PDT
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".