Bug 27734 - WINCE PORT: font related changes
Summary: WINCE PORT: font related changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 28021 (view as bug list)
Depends on:
Blocks: 23154
  Show dependency treegraph
 
Reported: 2009-07-27 14:14 PDT by Yong Li
Modified: 2009-09-02 09:41 PDT (History)
5 users (show)

See Also:


Attachments
the patch (16.67 KB, patch)
2009-07-27 14:18 PDT, Yong Li
no flags Details | Formatted Diff | Diff
updated patch with clearer comments, updated handling of negative spacing, and a few style fixes (17.81 KB, patch)
2009-08-03 15:55 PDT, Joe Mason
mitz: review-
Details | Formatted Diff | Diff
remove the change to setLetterSpacing() (16.45 KB, patch)
2009-08-04 08:38 PDT, Yong Li
no flags Details | Formatted Diff | Diff
include changes to CachedFont.cpp (19.86 KB, patch)
2009-08-05 14:07 PDT, Yong Li
no flags Details | Formatted Diff | Diff
remove unnecessary lines in FontCache.h (19.68 KB, patch)
2009-08-05 14:41 PDT, Yong Li
eric: review-
Details | Formatted Diff | Diff
use shared buffer in custom font data (11.02 KB, patch)
2009-08-07 11:30 PDT, Joe Mason
eric: review-
Details | Formatted Diff | Diff
build fixes and minor bug fixes (4.99 KB, application/octet-stream)
2009-08-07 11:40 PDT, Joe Mason
no flags Details
pass unrecognized glyphs to GDI to handle (2.82 KB, patch)
2009-08-07 11:44 PDT, Joe Mason
no flags Details | Formatted Diff | Diff
store only width for GlyphAdvance, to save space on memory-constrained devices (2.80 KB, patch)
2009-08-07 11:49 PDT, Joe Mason
no flags Details | Formatted Diff | Diff
build fixes and minor bug fixes (4.99 KB, patch)
2009-08-07 11:50 PDT, Joe Mason
eric: review-
Details | Formatted Diff | Diff
just replace the string of PLATFORM(FOO) defines (3.26 KB, patch)
2009-08-12 15:08 PDT, Joe Mason
no flags Details | Formatted Diff | Diff
build fixes and minor bug fixes, with more comments (5.28 KB, patch)
2009-08-12 22:49 PDT, Joe Mason
abarth: review+
eric: commit-queue-
Details | Formatted Diff | Diff
use shared buffer in custom font data (refactored) (8.47 KB, patch)
2009-08-13 08:40 PDT, Joe Mason
abarth: review+
eric: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2009-07-27 14:14:23 PDT
patch will follow
Comment 1 Yong Li 2009-07-27 14:18:59 PDT
Created attachment 33569 [details]
the patch
Comment 2 Eric Seidel (no email) 2009-07-28 10:49:24 PDT
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?
Comment 3 Yong Li 2009-07-28 11:01:55 PDT
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.
Comment 4 Joe Mason 2009-08-03 07:59:04 PDT
What is the point of the change to setGlyphDataForCharacter in FontFastPath.cpp?
Comment 5 Yong Li 2009-08-03 13:28:35 PDT
(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.
Comment 6 Joe Mason 2009-08-03 14:44:38 PDT
Thanks - I'll update this patch with your suggestion about letterSpacing and to make that more clear.
Comment 7 Joe Mason 2009-08-03 15:55:02 PDT
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 8 mitz 2009-08-03 16:02:50 PDT
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.
Comment 9 Yong Li 2009-08-04 08:38:36 PDT
Created attachment 34065 [details]
remove the change to setLetterSpacing()
Comment 10 Yong Li 2009-08-04 08:39:27 PDT
(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
Comment 11 Yong Li 2009-08-05 13:49:01 PDT
*** Bug 28021 has been marked as a duplicate of this bug. ***
Comment 12 Yong Li 2009-08-05 14:07:47 PDT
Created attachment 34173 [details]
include changes to CachedFont.cpp

layering violations removed according to Dan Bernstein's suggestion
Comment 13 Yong Li 2009-08-05 14:41:12 PDT
Created attachment 34178 [details]
remove unnecessary lines in FontCache.h
Comment 14 Eric Seidel (no email) 2009-08-06 19:48:50 PDT
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.
Comment 15 Yong Li 2009-08-07 07:53:39 PDT
(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.
Comment 16 Joe Mason 2009-08-07 11:30:27 PDT
Created attachment 34297 [details]
use shared buffer in custom font data
Comment 17 Joe Mason 2009-08-07 11:40:52 PDT
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
Comment 18 Joe Mason 2009-08-07 11:44:59 PDT
Created attachment 34305 [details]
pass unrecognized glyphs to GDI to handle
Comment 19 Joe Mason 2009-08-07 11:49:40 PDT
Created attachment 34306 [details]
store only width for GlyphAdvance, to save space on memory-constrained devices

Last of the split patches.
Comment 20 Joe Mason 2009-08-07 11:50:36 PDT
Created attachment 34307 [details]
build fixes and minor bug fixes

uploaded last patch with wrong content type somehow
Comment 21 Eric Seidel (no email) 2009-08-07 13:41:38 PDT
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.
Comment 22 Eric Seidel (no email) 2009-08-07 13:42:10 PDT
Comment on attachment 34305 [details]
pass unrecognized glyphs to GDI to handle

LGTM.
Comment 23 Eric Seidel (no email) 2009-08-07 13:43:15 PDT
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.
Comment 24 Eric Seidel (no email) 2009-08-07 13:45:21 PDT
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.
Comment 25 George Staikos 2009-08-07 13:46:29 PDT
(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.
Comment 26 Yong Li 2009-08-07 14:16:36 PDT
(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.
Comment 27 Adam Barth 2009-08-07 18:24:23 PDT
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
Comment 28 Adam Barth 2009-08-07 18:25:13 PDT
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
Comment 29 Joe Mason 2009-08-12 15:08:21 PDT
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).
Comment 30 Joe Mason 2009-08-12 22:49:08 PDT
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.
Comment 31 Yong Li 2009-08-13 06:57:43 PDT
Joe: you may want to edit "details" of your last upload and make it a "patch".
Comment 32 Joe Mason 2009-08-13 08:40:31 PDT
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.
Comment 33 Yong Li 2009-08-18 12:43:23 PDT
PATCH: just replace the string of PLATFORM(FOO)

 landed @ 47441
Comment 34 Eric Seidel (no email) 2009-08-18 14:11:40 PDT
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
Comment 35 Eric Seidel (no email) 2009-08-18 14:26:46 PDT
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
Comment 36 Eric Seidel (no email) 2009-08-18 14:39:48 PDT
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.
Comment 37 Eric Seidel (no email) 2009-08-18 14:40:43 PDT
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 eric@webkit.org...
Comment 38 Yong Li 2009-08-18 14:41:37 PDT
(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 eric@webkit.org...

I've landed it. see comment #33.

Sorry for not mentioning the patch number.
Comment 39 Eric Seidel (no email) 2009-08-18 14:43:08 PDT
(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 eric@webkit.org...
> 
> 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. :)
Comment 40 Yong Li 2009-08-18 14:48:42 PDT
(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 eric@webkit.org...
> > 
> > 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
Comment 41 Eric Seidel (no email) 2009-08-18 15:08:52 PDT
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.
Comment 42 Yong Li 2009-08-18 15:17:24 PDT
(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
Comment 43 Eric Seidel (no email) 2009-08-18 15:25:41 PDT
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+
Comment 44 Brent Fulgham 2009-08-26 09:37:30 PDT
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.
Comment 45 Brent Fulgham 2009-08-26 09:38:41 PDT
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.
Comment 46 Adam Barth 2009-09-01 18:26:08 PDT
Comment on attachment 34716 [details]
build fixes and minor bug fixes, with more comments

This patch appears correct.
Comment 47 Adam Barth 2009-09-01 18:28:13 PDT
Comment on attachment 34747 [details]
use shared buffer in custom font data (refactored)

This patch also appears correct.
Comment 48 Eric Seidel (no email) 2009-09-01 18:29:01 PDT
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
Comment 49 Eric Seidel (no email) 2009-09-01 18:29:17 PDT
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.
Comment 50 Eric Seidel (no email) 2009-09-02 00:50:04 PDT
Build failed with the patch applied.  Sadly the commit-queue doesn't currently save build logs.
Comment 51 Eric Seidel (no email) 2009-09-02 00:50:34 PDT
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.
Comment 52 Yong Li 2009-09-02 09:34:35 PDT
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
Comment 53 Yong Li 2009-09-02 09:39:25 PDT
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
Comment 54 Yong Li 2009-09-02 09:41:29 PDT
no pending patch. close it now