Embed fonts information into resulting PDF file for printing.

Embed fonts information into resulting PDF file for printing.

BUG=9847
TEST=printing on linux should have right font in pdf

Patch contributed by Min-Yu Huang <minyu.huang@gmail.com>;

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=25974

[tony, 2009-09-09 23:48:18]

Do the unittests still pass?

http://codereview.chromium.org/196071/diff/1/3
File printing/pdf_ps_metafile_linux.cc (right):

http://codereview.chromium.org/196071/diff/1/3#newcode27
Line 27: FT_Library ft_library = NULL;  // handle to FreeType library.
Nit: Comment above the variable.  Also, please prefix with g_ since it's global
to the file.

http://codereview.chromium.org/196071/diff/1/3#newcode28
Line 28: FT_Error ft_error = 0;  // FreeType error.
Does this need to be global?  Why not just declare it when you need it?

http://codereview.chromium.org/196071/diff/1/3#newcode35
Line 35: FontInfo() { memset(&font_stream, 0, sizeof(FontInfo)); }
I think you can just do a = {0} when initializing to set the initial values to
0.

http://codereview.chromium.org/196071/diff/1/3#newcode39
Line 39: MapFontId2FontInfo font_cache;
Nit: prefix this with a g_ also.

http://codereview.chromium.org/196071/diff/1/3#newcode129
Line 129: ft_error = FT_Init_FreeType(&ft_library);
Should we check to see if ft_library is already initialized or does
FT_Init_FreeType do that for us?  E.g., what happens if you print twice?

http://codereview.chromium.org/196071/diff/1/3#newcode339
Line 339: // Destroys resoreces for current page.
Nit: resoreces

http://codereview.chromium.org/196071/diff/1/3#newcode380
Line 380: DLOG(ERROR) << "Connot set font face in Cairo!";
Why does this happen?  Should this be a NOTREACHED?

http://codereview.chromium.org/196071/diff/1/4
File skia/ext/vector_platform_device_linux.cc (right):

http://codereview.chromium.org/196071/diff/1/4#newcode264
Line 264: // NOTE: The style is either kFill_Style or kStroke_Style.
You don't need to prefix your comments with "NOTE: ".  They are all notes :)

http://codereview.chromium.org/196071/diff/1/4#newcode284
Line 284: context_, paint.getTypeface()->uniqueID())) {
What happens if this fails?  Do we not get text?  Maybe this should be a
NOTREACHED also (or SkFAIL())?

[myhuang, 2009-09-10 00:21:58]

http://codereview.chromium.org/196071/diff/1/3
File printing/pdf_ps_metafile_linux.cc (right):

http://codereview.chromium.org/196071/diff/1/3#newcode27
Line 27: FT_Library ft_library = NULL;  // handle to FreeType library.
On 2009/09/09 23:48:18, tony wrote:
> Nit: Comment above the variable.  Also, please prefix with g_ since it's
global
> to the file.

Done.

http://codereview.chromium.org/196071/diff/1/3#newcode28
Line 28: FT_Error ft_error = 0;  // FreeType error.
On 2009/09/09 23:48:18, tony wrote:
> Does this need to be global?  Why not just declare it when you need it?

Done.

http://codereview.chromium.org/196071/diff/1/3#newcode35
Line 35: FontInfo() { memset(&font_stream, 0, sizeof(FontInfo)); }
On 2009/09/09 23:48:18, tony wrote:
> I think you can just do a = {0} when initializing to set the initial values to
> 0.

Done.

http://codereview.chromium.org/196071/diff/1/3#newcode39
Line 39: MapFontId2FontInfo font_cache;
On 2009/09/09 23:48:18, tony wrote:
> Nit: prefix this with a g_ also.

Done.

http://codereview.chromium.org/196071/diff/1/3#newcode129
Line 129: ft_error = FT_Init_FreeType(&ft_library);
On 2009/09/09 23:48:18, tony wrote:
> Should we check to see if ft_library is already initialized or does
> FT_Init_FreeType do that for us?  E.g., what happens if you print twice?

Done.

http://codereview.chromium.org/196071/diff/1/3#newcode339
Line 339: // Destroys resoreces for current page.
On 2009/09/09 23:48:18, tony wrote:
> Nit: resoreces

Done.

http://codereview.chromium.org/196071/diff/1/3#newcode380
Line 380: DLOG(ERROR) << "Connot set font face in Cairo!";
On 2009/09/09 23:48:18, tony wrote:
> Why does this happen?  Should this be a NOTREACHED?

Done.

http://codereview.chromium.org/196071/diff/1/4
File skia/ext/vector_platform_device_linux.cc (right):

http://codereview.chromium.org/196071/diff/1/4#newcode264
Line 264: // NOTE: The style is either kFill_Style or kStroke_Style.
On 2009/09/09 23:48:18, tony wrote:
> You don't need to prefix your comments with "NOTE: ".  They are all notes :)

Done.

http://codereview.chromium.org/196071/diff/1/4#newcode284
Line 284: context_, paint.getTypeface()->uniqueID())) {
On 2009/09/09 23:48:18, tony wrote:
> What happens if this fails?  Do we not get text?  Maybe this should be a
> NOTREACHED also (or SkFAIL())?

Done.

[M-A Ruel, 2009-09-10 00:33:00]

http://codereview.chromium.org/196071/diff/4001/5002
File printing/pdf_ps_metafile_linux.cc (right):

http://codereview.chromium.org/196071/diff/4001/5002#newcode37
Line 37: MapFontId2FontInfo g_font_cache;
You can't make a global with a constructor. All these globals makes it look like
you should use a Singleton<>.

http://codereview.chromium.org/196071/diff/4001/5002#newcode365
Line 365: // Select the font associated with |font_id| in |context|.
the comment is redundant.

http://codereview.chromium.org/196071/diff/4001/5002#newcode379
Line 379: NOTREACHED() << "Connot set font face in Cairo!";
Cannot

http://codereview.chromium.org/196071/diff/4001/5002#newcode415
Line 415: (cairo_destroy_func_t)FT_Done_Face);
use c++ style cast

http://codereview.chromium.org/196071/diff/4001/5003
File skia/ext/vector_platform_device_linux.cc (right):

http://codereview.chromium.org/196071/diff/4001/5003#newcode276
Line 276: constY :
+2

http://codereview.chromium.org/196071/diff/4001/5003#newcode284
Line 284: context_, paint.getTypeface()->uniqueID())) {
+4

[myhuang, 2009-09-10 18:18:39]

http://codereview.chromium.org/196071/diff/4001/5002
File printing/pdf_ps_metafile_linux.cc (right):

http://codereview.chromium.org/196071/diff/4001/5002#newcode37
Line 37: MapFontId2FontInfo g_font_cache;
On 2009/09/10 00:33:00, Marc-Antoine Ruel wrote:
> You can't make a global with a constructor. All these globals makes it look
like
> you should use a Singleton<>.

Done.

http://codereview.chromium.org/196071/diff/4001/5002#newcode365
Line 365: // Select the font associated with |font_id| in |context|.
On 2009/09/10 00:33:00, Marc-Antoine Ruel wrote:
> the comment is redundant.

Done.

http://codereview.chromium.org/196071/diff/4001/5002#newcode379
Line 379: NOTREACHED() << "Connot set font face in Cairo!";
On 2009/09/10 00:33:00, Marc-Antoine Ruel wrote:
> Cannot

Done.

http://codereview.chromium.org/196071/diff/4001/5002#newcode415
Line 415: (cairo_destroy_func_t)FT_Done_Face);
On 2009/09/10 00:33:00, Marc-Antoine Ruel wrote:
> use c++ style cast

Done.

http://codereview.chromium.org/196071/diff/4001/5003
File skia/ext/vector_platform_device_linux.cc (right):

http://codereview.chromium.org/196071/diff/4001/5003#newcode276
Line 276: constY :
On 2009/09/10 00:33:00, Marc-Antoine Ruel wrote:
> +2

Done.

http://codereview.chromium.org/196071/diff/4001/5003#newcode284
Line 284: context_, paint.getTypeface()->uniqueID())) {
On 2009/09/10 00:33:00, Marc-Antoine Ruel wrote:
> +4

Done.

[M-A Ruel, 2009-09-10 18:23:03]

lgtm with small nits, will commit later.

http://codereview.chromium.org/196071/diff/2005/3006
File printing/pdf_ps_metafile_linux.cc (right):

http://codereview.chromium.org/196071/diff/2005/3006#newcode432
Line 432: g_font_cache->operator[](font_id) = new_font_info;
you can use the less awkward notation
(*g_font_cache)[font_id] = new_font_info;

http://codereview.chromium.org/196071/diff/2005/3007
File skia/ext/vector_platform_device_linux.cc (right):

http://codereview.chromium.org/196071/diff/2005/3007#newcode262
Line 262: const uint16_t* glyphIDs = static_cast<const uint16_t*>(text);
glyph_ids

http://codereview.chromium.org/196071/diff/2005/3007#newcode270
Line 270: uint16_t glyphID = glyphIDs[i];
glyph_id

http://codereview.chromium.org/196071/diff/2005/3007#newcode292
Line 292: uint16_t glyphID = glyphIDs[i];
same

[myhuang, 2009-09-10 20:42:33]

http://codereview.chromium.org/196071/diff/2005/3006
File printing/pdf_ps_metafile_linux.cc (right):

http://codereview.chromium.org/196071/diff/2005/3006#newcode432
Line 432: g_font_cache->operator[](font_id) = new_font_info;
On 2009/09/10 18:23:03, Marc-Antoine Ruel wrote:
> you can use the less awkward notation
> (*g_font_cache)[font_id] = new_font_info;

Done.

http://codereview.chromium.org/196071/diff/2005/3007
File skia/ext/vector_platform_device_linux.cc (right):

http://codereview.chromium.org/196071/diff/2005/3007#newcode262
Line 262: const uint16_t* glyphIDs = static_cast<const uint16_t*>(text);
On 2009/09/10 18:23:03, Marc-Antoine Ruel wrote:
> glyph_ids

Done.

http://codereview.chromium.org/196071/diff/2005/3007#newcode270
Line 270: uint16_t glyphID = glyphIDs[i];
On 2009/09/10 18:23:03, Marc-Antoine Ruel wrote:
> glyph_id

Done.

http://codereview.chromium.org/196071/diff/2005/3007#newcode292
Line 292: uint16_t glyphID = glyphIDs[i];
On 2009/09/10 18:23:03, Marc-Antoine Ruel wrote:
> same

Done.