Bug 27734 – WINCE PORT: font related changes

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Yong Li]

patch will follow

[Yong Li]

Created attachment 33569 [details]
the patch

[Eric Seidel (no email)]

Comment on attachment 33569 [details]
the patch

I don't see why this would be done here:
#if PLATFORM(WINCE)
 95     // We do not support negative letter spacing.
 96     void setLetterSpacing(short s) { m_letterSpacing = s > 0 ? s : 0; }
 97 #else
9398     void setLetterSpacing(short s) { m_letterSpacing = s; }
 99 #endif

Why don't the call-sites which don't support negative letter spacing make the adjustment?

[Yong Li]

some websites use negative letter spacing (-1) to make text display more compact, and this works perfect on desktop browsers. but on windows mobile devices, the text are already very compact (no extra space). as the result, the characters overlap on each other when letter spacing is -1.

letter spacing is used by GlyphBuffer to position glyphs. seems hard to find a better way to do this.

maybe it should better be "m_letterSpacing = s < 0 : s + 1 : s;", in case that web sites intend to display overlapping characters.

[Joe Mason]

What is the point of the change to setGlyphDataForCharacter in FontFastPath.cpp?

[Yong Li]

(In reply to comment #4)
> What is the point of the change to setGlyphDataForCharacter in
> FontFastPath.cpp?

When we fail to link a font to the character, the changed code blocks will be reached. We are supposed to draw a square (missingGlyphData()) for this character. But on WINCE, we don't have real glyph. We use the character value as its fake glyph, and let GDI display the character directly. Our missingGplyData() returns a null character, which is not suitable for GDI to display. Also, sometimes we cannot map a font for the character on WINCE, but GDI may still be able to display the character, probably because the font package is not installed correctly. So in FontFastPath.cpp, we just always set the glyph to be same as the character, and let GDI solve it.

[Joe Mason]

Thanks - I'll update this patch with your suggestion about letterSpacing and to make that more clear.

[Joe Mason]

Created attachment 34012 [details]
updated patch with clearer comments, updated handling of negative spacing, and a few style fixes

This obsoletes Yong's patch (apparently I can't make it obsolete myself, and he's on vacation).

Comment on attachment 34012 [details]
updated patch with clearer comments, updated handling of negative spacing, and a few style fixes

> +#if PLATFORM(WINCE)
> +    // Some sites set the spacing to -1 to make text more compact.  But WinMobile already
> +    // compacts the text to save on screen space, so this makes it overlap.  If the spacing is
> +    // negative, widen is slightly to counteract this.
> +    void setLetterSpacing(short s) { m_letterSpacing = s < 0 ? s + 1 : s; }
> +#else

I don’t think this is the right place to make this platform-specific adjustment. It will also affect computed style.

[Yong Li]

Created attachment 34065 [details]
remove the change to setLetterSpacing()

[Yong Li]

(In reply to comment #8)
> (From update of attachment 34012 [details])
> > +#if PLATFORM(WINCE)
> > +    // Some sites set the spacing to -1 to make text more compact.  But WinMobile already
> > +    // compacts the text to save on screen space, so this makes it overlap.  If the spacing is
> > +    // negative, widen is slightly to counteract this.
> > +    void setLetterSpacing(short s) { m_letterSpacing = s < 0 ? s + 1 : s; }
> > +#else
> 
> I don’t think this is the right place to make this platform-specific
> adjustment. It will also affect computed style.

This change has been removed from the patch

[Yong Li]

*** Bug 28021 has been marked as a duplicate of this bug. ***

[Yong Li]

Created attachment 34173 [details]
include changes to CachedFont.cpp

layering violations removed according to Dan Bernstein's suggestion

[Yong Li]

Created attachment 34178 [details]
remove unnecessary lines in FontCache.h

[Eric Seidel (no email)]

Comment on attachment 34178 [details]
remove unnecessary lines in FontCache.h

Please collapse this line into one nicely named define:

 35 #if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) && PLATFORM(WIN_OS)) || PLATFORM(WINCE)

like STORE_FONT_POINTER or whatever the heck the ifdefed code does.

Please add more indetifying information than just "comment above":
 179         // see comment above
otherwise if someone adds a comment between your two comments the latter comment will be confusing.

// See comment about WINCE GDI handling near setGlyphDataForCharacter above.
or something.

Does WINCE only support horrizontal text?
3 #elif PLATFORM(WINCE)
 64 typedef float GlyphBufferAdvance;
6065 #else
A comment is need there to explain why it's just a float.

No need for arg name:
 51     FontCustomPlatformData* createFontCustomPlatformData(const SharedBuffer* buffer);

Can you split this out?

r- mostly for the unexplained advances change.

[Yong Li]

(In reply to comment #14)
> (From update of attachment 34178 [details])
> Please collapse this line into one nicely named define:
> 
>  35 #if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) &&
> PLATFORM(WIN_OS)) || PLATFORM(WINCE)
> 
> like STORE_FONT_POINTER or whatever the heck the ifdefed code does.
> 
> Please add more indetifying information than just "comment above":
>  179         // see comment above
> otherwise if someone adds a comment between your two comments the latter
> comment will be confusing.
> 
> // See comment about WINCE GDI handling near setGlyphDataForCharacter above.
> or something.
> 
> Does WINCE only support horrizontal text?
> 3 #elif PLATFORM(WINCE)
>  64 typedef float GlyphBufferAdvance;
> 6065 #else
> A comment is need there to explain why it's just a float.
> 
> No need for arg name:
>  51     FontCustomPlatformData* createFontCustomPlatformData(const
> SharedBuffer* buffer);
> 
> Can you split this out?
> 
> r- mostly for the unexplained advances change.

cannot find any platform crossing code that sets non-zero FloatSize::height() for 2D GlyphBufferAdvance. CoreTextController.cpp uses 2D GlyphBufferAdvance, but GlyphBufferAdvance is already defined to CGSize for PLATFORM(CG). We'll add comments for it.

[Joe Mason]

Created attachment 34297 [details]
use shared buffer in custom font data

[Joe Mason]

Created attachment 34302 [details]
build fixes and minor bug fixes

This patch includes declaring comInitialize and comUninitialize, which were added in bug 27509 - the header changes must have been missed in that checkin

[Joe Mason]

Created attachment 34305 [details]
pass unrecognized glyphs to GDI to handle

[Joe Mason]

Created attachment 34306 [details]
store only width for GlyphAdvance, to save space on memory-constrained devices

Last of the split patches.

[Joe Mason]

Created attachment 34307 [details]
build fixes and minor bug fixes

uploaded last patch with wrong content type somehow

[Eric Seidel (no email)]

Comment on attachment 34297 [details]
use shared buffer in custom font data

What does this mean?
STORE_FONT_CUSTOM_PLATFORM_DATA

Not sufficient explanation:
 74 // Need to use #define instead of typedef due to conflict

Not a valid variable name:
 353     const sfntHeader* sfnt = reinterpret_cast<const sfntHeader*>(fontData->data());

Seems like this shoudl be a static inline function:
69     size_t nameTableSize = ((offsetof(nameTable, nameRecords) + nameRecordCount * sizeof(nameRecord) + fontName.length() * sizeof(UChar)) & ~3) + 4;

This function needs more commentary:
 347 bool renameFont(SharedBuffer* fontData, const String& fontName)

What is it trying to do?  re-write the in-memory font representation?

Put this in a separate static inline, please:
380     // Write the new 'name' table after the original font data.
 381     nameTable* name = reinterpret_cast<nameTable*>(data + originalDataSize);
 382     name->format = 0;
 383     name->count = nameRecordCount;
 384     name->stringOffset = offsetof(nameTable, nameRecords) + nameRecordCount * sizeof(nameRecord);
 385     for (unsigned i = 0; i < nameRecordCount; ++i) {
 386         name->nameRecords[i].platformID = 3;
 387         name->nameRecords[i].encodingID = 1;
 388         name->nameRecords[i].languageID = 0x0409;
 389         name->nameRecords[i].offset = 0;
 390         name->nameRecords[i].length = fontName.length() * sizeof(UChar);
 391     }
 392 

This code is not legible.  Please make it more understandable to casual readers.

[Eric Seidel (no email)]

Comment on attachment 34305 [details]
pass unrecognized glyphs to GDI to handle

LGTM.

[Eric Seidel (no email)]

Comment on attachment 34306 [details]
store only width for GlyphAdvance, to save space on memory-constrained devices

That is much more readable, thank you.  Ideally this would have been a separate define which was not WINCE specific.  Like LOW_MEMORY_DEVICE.  But this is OK.

[Eric Seidel (no email)]

Comment on attachment 34307 [details]
build fixes and minor bug fixes

Are you sure that min<int> will compile on other platforms?  I feel like we've dealt with the min/max issue on windows before in other ways.  Please grep the source to see how it's dealt with elsewhere.

This needs a why comment:
29 #if !PLATFORM(WINCE)
227230     mutable SCRIPT_CACHE m_scriptCache;
228231     mutable SCRIPT_FONTPROPERTIES* m_scriptFontProperties;
229232 #endif
your note about it saving space in the ChangeLog is a fine comment, just put it in teh soruce code too.

[George Staikos]

(In reply to comment #23)
> (From update of attachment 34306 [details])
> That is much more readable, thank you.  Ideally this would have been a separate
> define which was not WINCE specific.  Like LOW_MEMORY_DEVICE.  But this is OK.

   We agree but this refactoring has to happen later with discussion with other device WebKit folks.

[Yong Li]

(In reply to comment #21)
> (From update of attachment 34297 [details])
> What does this mean?
> STORE_FONT_CUSTOM_PLATFORM_DATA
> 
> Not sufficient explanation:
>  74 // Need to use #define instead of typedef due to conflict

Conflict with "Fixed" defined in Length.h. It's not a problem if the PCH doesn't include Length.h. Probably we should remove this change from this patch.

> 
> Not a valid variable name:
>  353     const sfntHeader* sfnt = reinterpret_cast<const
> sfntHeader*>(fontData->data());
> 
> Seems like this shoudl be a static inline function:
> 69     size_t nameTableSize = ((offsetof(nameTable, nameRecords) +
> nameRecordCount * sizeof(nameRecord) + fontName.length() * sizeof(UChar)) & ~3)
> + 4;
> 
> This function needs more commentary:
>  347 bool renameFont(SharedBuffer* fontData, const String& fontName)
> 
> What is it trying to do?  re-write the in-memory font representation?
> 
> Put this in a separate static inline, please:
> 380     // Write the new 'name' table after the original font data.
>  381     nameTable* name = reinterpret_cast<nameTable*>(data +
> originalDataSize);
>  382     name->format = 0;
>  383     name->count = nameRecordCount;
>  384     name->stringOffset = offsetof(nameTable, nameRecords) +
> nameRecordCount * sizeof(nameRecord);
>  385     for (unsigned i = 0; i < nameRecordCount; ++i) {
>  386         name->nameRecords[i].platformID = 3;
>  387         name->nameRecords[i].encodingID = 1;
>  388         name->nameRecords[i].languageID = 0x0409;
>  389         name->nameRecords[i].offset = 0;
>  390         name->nameRecords[i].length = fontName.length() * sizeof(UChar);
>  391     }
>  392 
> 
> This code is not legible.  Please make it more understandable to casual
> readers.

The function is almost a clone of an existing function renameAndActivateFont(), except that renameFont() directly appends data to the source buffer, rather than activating it.

[Adam Barth]

Comment on attachment 34305 [details]
pass unrecognized glyphs to GDI to handle

Clearing review flag on attachment: 34305

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/FontFastPath.cpp
Committed r46938
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/FontFastPath.cpp
r46938 = 49ec5481f262af6b17f440a371e3f48f35b49342 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46938

[Adam Barth]

Comment on attachment 34306 [details]
store only width for GlyphAdvance, to save space on memory-constrained devices

Clearing review flag on attachment: 34306

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/GlyphBuffer.h
Committed r46939
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/GlyphBuffer.h
r46939 = 9d5648701aeec22afb43bc9ff38be1acd7fc2bd5 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46939

[Joe Mason]

Created attachment 34697 [details]
just replace the string of PLATFORM(FOO) defines

Let's get the easy one out of the way - this is just the STORE_FONT_CUSTOM_PLATFORM_DATA define (with an added explanatory comment).

[Joe Mason]

Created attachment 34716 [details]
build fixes and minor bug fixes, with more comments

> Are you sure that min<int> will compile on other platforms?  I feel like we've
> dealt with the min/max issue on windows before in other ways.  Please grep the
> source to see how it's dealt with elsewhere.

The more common methods of dealing with the windows min/max macros won't work here because the two parameters to min have different types (int and UChar32), so Windows gives an error about ambiguous template parameters.  min<int> will definitely compile on other platforms (it's just giving the template parameter explicitly instead of letting the compiler work it out), but I changed the fix anyway because I felt it was clearer - now it explicitly casts the UChar32 to an int.

> This needs a why comment:
> 29 #if !PLATFORM(WINCE)
> 227230     mutable SCRIPT_CACHE m_scriptCache;
> 228231     mutable SCRIPT_FONTPROPERTIES* m_scriptFontProperties;
> 229232 #endif
> your note about it saving space in the ChangeLog is a fine comment, just put it
> in teh soruce code too.

Done.

[Yong Li]

Joe: you may want to edit "details" of your last upload and make it a "patch".

[Joe Mason]

Created attachment 34747 [details]
use shared buffer in custom font data (refactored)

I moved the shared code into a helper method, and removed some build fixes that are no longer necessary due to changes in Forward.h.  Now it's clear that all the code the Eric objected to was already in the existing function.

[Yong Li]

PATCH: just replace the string of PLATFORM(FOO)

 landed @ 47441

[Eric Seidel (no email)]

Comment on attachment 34697 [details]
just replace the string of PLATFORM(FOO) defines

Rejecting patch 34697 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'rebase']"  exit_code: 1  cwd: None

[Eric Seidel (no email)]

Comment on attachment 34697 [details]
just replace the string of PLATFORM(FOO) defines

Sorry, we hit https://bugs.webkit.org/show_bug.cgi?id=28436

[Eric Seidel (no email)]

Comment on attachment 34697 [details]
just replace the string of PLATFORM(FOO) defines

Rejecting patch 34697 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=34697 from bug 27734 failed to download and apply.

[Eric Seidel (no email)]

patching file WebCore/loader/CachedFont.cpp
Reversed (or previously applied) patch detected!  Assume -R? [n] 
Apply anyway? [n] 
Skipping patch.
6 out of 6 hunks ignored -- saving rejects to file WebCore/loader/CachedFont.cpp.rej
patch -p0 "WebCore/loader/CachedFont.cpp" returned 1.  Pass --force to ignore patch failures.
Logging in as [e-mail]...

[Yong Li]

(In reply to comment #37)
> patching file WebCore/loader/CachedFont.cpp
> Reversed (or previously applied) patch detected!  Assume -R? [n] 
> Apply anyway? [n] 
> Skipping patch.
> 6 out of 6 hunks ignored -- saving rejects to file
> WebCore/loader/CachedFont.cpp.rej
> patch -p0 "WebCore/loader/CachedFont.cpp" returned 1.  Pass --force to ignore
> patch failures.
> Logging in as [e-mail]...

I've landed it. see comment #33.

Sorry for not mentioning the patch number.

[Eric Seidel (no email)]

(In reply to comment #38)
> (In reply to comment #37)
> > patching file WebCore/loader/CachedFont.cpp
> > Reversed (or previously applied) patch detected!  Assume -R? [n] 
> > Apply anyway? [n] 
> > Skipping patch.
> > 6 out of 6 hunks ignored -- saving rejects to file
> > WebCore/loader/CachedFont.cpp.rej
> > patch -p0 "WebCore/loader/CachedFont.cpp" returned 1.  Pass --force to ignore
> > patch failures.
> > Logging in as [e-mail]...
> 
> I've landed it. see comment #33.
> 
> Sorry for not mentioning the patch number.

Open bugs with r+ and cq+'d patches will cause the commit-queue to try and land those patches. :)  Please clear the flags or close the bug after landing. :)

[Yong Li]

(In reply to comment #39)
> (In reply to comment #38)
> > (In reply to comment #37)
> > > patching file WebCore/loader/CachedFont.cpp
> > > Reversed (or previously applied) patch detected!  Assume -R? [n] 
> > > Apply anyway? [n] 
> > > Skipping patch.
> > > 6 out of 6 hunks ignored -- saving rejects to file
> > > WebCore/loader/CachedFont.cpp.rej
> > > patch -p0 "WebCore/loader/CachedFont.cpp" returned 1.  Pass --force to ignore
> > > patch failures.
> > > Logging in as [e-mail]...
> > 
> > I've landed it. see comment #33.
> > 
> > Sorry for not mentioning the patch number.
> 
> Open bugs with r+ and cq+'d patches will cause the commit-queue to try and land
> those patches. :)  Please clear the flags or close the bug after landing. :)

seems I'm not authorized for changing that. I should ask a reviewer to do that, though

[Eric Seidel (no email)]

Oh, you're able to clear flags.  You just can't set r+ or cq+.  Well, you can set them, it's just that the commit-queue will notice and remove them. :)

Now that you're a committer and have added yourself to:
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py
you can set commit-queue+ on any patch.

[Yong Li]

(In reply to comment #41)
> Oh, you're able to clear flags.  You just can't set r+ or cq+.  Well, you can
> set them, it's just that the commit-queue will notice and remove them. :)
> 
> Now that you're a committer and have added yourself to:
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py
> you can set commit-queue+ on any patch.

actually I did try and failed. I guess it's because this time I'm not the owner of the patch

[Eric Seidel (no email)]

If I remember correctly, you tried setting r+ before.  You're not a reviewer.  You are a committer however, so you are allowed to set commit-queue+

[Brent Fulgham]

Comment on attachment 34697 [details]
just replace the string of PLATFORM(FOO) defines

Clearing so this does not show up in the commit queue anymore.  This was landed already.

[Brent Fulgham]

Comment on attachment 34697 [details]
just replace the string of PLATFORM(FOO) defines

Clearing review/commit flags since already landed and it is showing up in the commit queue list.

[Adam Barth]

Comment on attachment 34716 [details]
build fixes and minor bug fixes, with more comments

This patch appears correct.

[Adam Barth]

Comment on attachment 34747 [details]
use shared buffer in custom font data (refactored)

This patch also appears correct.

[Eric Seidel (no email)]

Comment on attachment 34716 [details]
build fixes and minor bug fixes, with more comments

Rejecting patch 34716 from commit-queue.  This patch will require manual commit.

WebKitTools/Scripts/build-webkit failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 34747 [details]
use shared buffer in custom font data (refactored)

Rejecting patch 34747 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=34747 from bug 27734 failed to download and apply.

[Eric Seidel (no email)]

Build failed with the patch applied.  Sadly the commit-queue doesn't currently save build logs.

[Eric Seidel (no email)]

Hunk #1 FAILED at 27.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/loader/CachedFont.cpp.rej
patch -p0 "WebCore/loader/CachedFont.cpp" returned 1.  Pass --force to ignore patch failures.

[Yong Li]

Committed r47974
        M       WebCore/ChangeLog
        M       WebCore/platform/graphics/GlyphPageTreeNode.cpp
        M       WebCore/platform/graphics/SimpleFontData.cpp
        M       WebCore/platform/graphics/FontCache.h
        M       WebCore/platform/graphics/SimpleFontData.h

[Yong Li]

Committed r47975
        M       WebCore/ChangeLog
        M       WebCore/platform/graphics/wince/FontCustomPlatformData.cpp
        M       WebCore/platform/graphics/wince/FontCustomPlatformData.h
        M       WebCore/platform/graphics/opentype/OpenTypeUtilities.cpp
        M       WebCore/platform/graphics/opentype/OpenTypeUtilities.h

[Yong Li]

no pending patch. close it now