PDF : Unused parameters cleanup

PDF : Unused parameters cleanup

I removed unused parameters in the PDFs wherever it was trivial to do so. A few constructors had to change signature in the process to reflect the changes.
Committed: https://code.google.com/p/skia/source/detail?r=7987

[sugoi, 2013-02-27 16:21:00]

https://codereview.appspot.com/7390056/diff/1/src/pdf/SkPDFUtils.cpp
File src/pdf/SkPDFUtils.cpp (right):

https://codereview.appspot.com/7390056/diff/1/src/pdf/SkPDFUtils.cpp#newcode117
src/pdf/SkPDFUtils.cpp:117: SkPoint lastMovePt = SkPoint::Make(0,0);
This is just to remove the "parameter can be used before initialization"
warning.

[Steve VanDeBogart, 2013-02-27 21:33:50]

On 2013/02/27 16:21:00, sugoi wrote:
> https://codereview.appspot.com/7390056/diff/1/src/pdf/SkPDFUtils.cpp
> File src/pdf/SkPDFUtils.cpp (right):
> 
>
https://codereview.appspot.com/7390056/diff/1/src/pdf/SkPDFUtils.cpp#newcode117
> src/pdf/SkPDFUtils.cpp:117: SkPoint lastMovePt = SkPoint::Make(0,0);
> This is just to remove the "parameter can be used before initialization"
> warning.

So it turns out it's a regression that relatedFontDescriptor isn't used.  If you
want to change the CL to ignore the unused variable in the Type1 and Type3
constructor...

[sugoi, 2013-02-27 21:39:35]

On 2013/02/27 21:33:50, Steve VanDeBogart wrote:
> On 2013/02/27 16:21:00, sugoi wrote:
> > https://codereview.appspot.com/7390056/diff/1/src/pdf/SkPDFUtils.cpp
> > File src/pdf/SkPDFUtils.cpp (right):
> > 
> >
>
https://codereview.appspot.com/7390056/diff/1/src/pdf/SkPDFUtils.cpp#newcode117
> > src/pdf/SkPDFUtils.cpp:117: SkPoint lastMovePt = SkPoint::Make(0,0);
> > This is just to remove the "parameter can be used before initialization"
> > warning.
> 
> So it turns out it's a regression that relatedFontDescriptor isn't used.  If
you
> want to change the CL to ignore the unused variable in the Type1 and Type3
> constructor...

I can fix the regression if you give me a few details about what you found out.

[Steve VanDeBogart, 2013-02-27 21:45:19]

On 2013/02/27 21:39:35, sugoi wrote:
> On 2013/02/27 21:33:50, Steve VanDeBogart wrote:
> > On 2013/02/27 16:21:00, sugoi wrote:
> > > https://codereview.appspot.com/7390056/diff/1/src/pdf/SkPDFUtils.cpp
> > > File src/pdf/SkPDFUtils.cpp (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/7390056/diff/1/src/pdf/SkPDFUtils.cpp#newcode117
> > > src/pdf/SkPDFUtils.cpp:117: SkPoint lastMovePt = SkPoint::Make(0,0);
> > > This is just to remove the "parameter can be used before initialization"
> > > warning.
> > 
> > So it turns out it's a regression that relatedFontDescriptor isn't used.  If
> you
> > want to change the CL to ignore the unused variable in the Type1 and Type3
> > constructor...
> 
> I can fix the regression if you give me a few details about what you found
out.

It'd be great if you want to take care of it.  If it's too hairy though, just
let me know and I'll figure it out.

If you print this [1] page with chrome, you'll find that the Type1 font is
included in the PDF three times instead of once.  I think the fix is to call
setFontDescriptor(relatedFontDescriptor) in the Type1 constructor.  Not sure
about Type3, it may actually be unused in that case.

[1] http://www.corp.google.com/~vandebo/no_crawl/font.html

[sugoi, 2013-02-27 21:57:14]

On 2013/02/27 21:45:19, Steve VanDeBogart wrote:
> It'd be great if you want to take care of it. If it's too hairy though, just
let me know and I'll figure it out.
> 
> If you print this [1] page with chrome, you'll find that the Type1 font is
included in the PDF three times instead of once.  I think the fix is to call
setFontDescriptor(relatedFontDescriptor) in the Type1 constructor.  Not sure
about Type3, it may actually be unused in that case.
> 
> [1] http://www.corp.google.com/%7Evandebo/no_crawl/font.html

1 ) Are you refering to the LMMono10-Regular font ?
2 ) Is there a way to test this within Skia ? I'm assuming it should be possible
to create a font test for this, right ?

[Steve VanDeBogart, 2013-02-27 22:01:29]

On 2013/02/27 21:57:14, sugoi wrote:
> On 2013/02/27 21:45:19, Steve VanDeBogart wrote:
> > It'd be great if you want to take care of it. If it's too hairy though, just
> let me know and I'll figure it out.
> > 
> > If you print this [1] page with chrome, you'll find that the Type1 font is
> included in the PDF three times instead of once.  I think the fix is to call
> setFontDescriptor(relatedFontDescriptor) in the Type1 constructor.  Not sure
> about Type3, it may actually be unused in that case.
> > 
> > [1] http://www.corp.google.com/%257Evandebo/no_crawl/font.html
> 
> 1 ) Are you refering to the LMMono10-Regular font ?
> 2 ) Is there a way to test this within Skia ? I'm assuming it should be
possible
> to create a font test for this, right ?

Yes, LMMono10-Regular is included three times.  Multiple font dictionaries is
ok.  The problem is the stream contents (search for /Length3).

You should be able to create a test in Skia -- simply draw more than 255 glyphs
with a Type1 font. The default font host doesn't handle Type1 fonts.  You might
be able to hack around it by just changing the .ttf match in SkFontHost_Linux.

[sugoi, 2013-02-28 15:42:51]

I'm reading the code and I'm trying to figure out how this should work and I'm
not sure I fully understand it. So :

Before my changes, there was an unused font descriptor parameter in the
constructors of SkPDFType1Font and SkPDFType3Font.

Currently, the way to set a font descriptor is through the constructor of
SkPDFType1Font, which calls populate() -> addFontDescriptor() ->
setFontDescriptor(), which seems to indicate that type 1 fonts are creating
their font descriptor rather than receiving it by argument in the constructor
(note that, as you mentioned, SkPDFType3Font does not use a font descriptor). If
I were to use the argument previously passed to the constructor, that would
effectively bypass the SkPDFType1Font::addFontDescriptor() function and return
early in SkPDFType1Font::populate(), which would most likely modify the behavior
of the font.

Also, this behavior comes from SkPDFFont::GetFontResource(), in which we either
return an existing font or create a new one if we can't find the desired font.
The only case where the relatedFontDescriptor would be used is when finding the
font for the given glyphID fails, but succeeds for glyphID == 0. Then, we could
use that given relatedFontDescriptor for SkPDFType1Font, but I'm not sure how
that helps, since we are already creating a new font anyway at this point.

Instead of using the relatedFontDescriptor inside SkPDFFont::GetFontResource(),
why aren't we adding kType1_Font support inside SkPDFFont::GetFontResource()
around line 782 ?

I'm guessing this has to be something that's special about type 1 fonts, but I'd
like to understand it if you have time to give me some context around this.
Thanks.

[Steve VanDeBogart, 2013-02-28 22:14:14]

On 2013/02/28 15:42:51, sugoi wrote:
> I'm reading the code and I'm trying to figure out how this should work and I'm
> not sure I fully understand it. So :

If it helps, this is the change where it regressed
https://codereview.appspot.com/4811049


> Before my changes, there was an unused font descriptor parameter in the
> constructors of SkPDFType1Font and SkPDFType3Font.
> 
> Currently, the way to set a font descriptor is through the constructor of
> SkPDFType1Font, which calls populate() -> addFontDescriptor() ->
> setFontDescriptor(), which seems to indicate that type 1 fonts are creating
> their font descriptor rather than receiving it by argument in the constructor
> (note that, as you mentioned, SkPDFType3Font does not use a font descriptor).
If
> I were to use the argument previously passed to the constructor, that would
> effectively bypass the SkPDFType1Font::addFontDescriptor() function and return
> early in SkPDFType1Font::populate(), which would most likely modify the
behavior
> of the font.

It only affects the behavior of the font if relatedFontDescriptor isn't Null,
see below.

> Also, this behavior comes from SkPDFFont::GetFontResource(), in which we
either
> return an existing font or create a new one if we can't find the desired font.
> The only case where the relatedFontDescriptor would be used is when finding
the
> font for the given glyphID fails, but succeeds for glyphID == 0. Then, we
could
> use that given relatedFontDescriptor for SkPDFType1Font, but I'm not sure how
> that helps, since we are already creating a new font anyway at this point.

This is a bit of background information
https://code.google.com/p/skia/wiki/PDFTheoryOfOperation#Fonts
There are three PDF objects used to embed a font.  The Font dictionary (/Type
/Font), which refers to a descendant font (Type 0) or a FontDescriptor (/Type
/FontDescriptor). A descendant font will refer to a FontDescriptor. A
FontDescriptor references a stream object containing the bytes of the font.
Truetype fonts use the descendant font form, Type1 and Type3 fonts do not.  In
PDF, Truetype fonts support up to 16k glyphs but Type1 and Type3 support only
256 glyphs. Of course, a Type1 or Type3 font may have more than 256 glyphs.  We
get around this by having multiple fonts (/Type /Font) that use the same
FontDescriptor and stream, but use different glyphs from the stream.  That is
exactly the part that is broken and instead we are including multiple
FontDescriptors and multiple copies of the stream.

When we search the canonical fonts with glyphID == 0, we're checking to see if
there's another font (/Type /Font) already using the needed FontDescriptor and
stream.

> Instead of using the relatedFontDescriptor inside
SkPDFFont::GetFontResource(),
> why aren't we adding kType1_Font support inside SkPDFFont::GetFontResource()
> around line 782 ?

Does the above answer this question?

> I'm guessing this has to be something that's special about type 1 fonts, but
I'd
> like to understand it if you have time to give me some context around this.
> Thanks.

[sugoi, 2013-03-04 15:41:23]

Thanks for the detailed explanation, I understand the problem much better now. I
uploaded a new version of the cl that should be closer to what you'd expect for
the fix. I'll also try to create a test for this.

[sugoi, 2013-03-04 16:49:07]

Actually, it seems creating the test is fairly complicated for someone who's not
too familiar with this code (like me), so edisonn offered to provide a test for
this. Thanks!

[Steve VanDeBogart, 2013-03-04 23:18:11]

Thanks for the nice clean up.  LGTM

https://codereview.appspot.com/7390056/diff/11001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.appspot.com/7390056/diff/11001/src/pdf/SkPDFFont.cpp#newco...
src/pdf/SkPDFFont.cpp:818: SkPDFFont* SkPDFFont::getFontSubset(const
SkPDFGlyphSet*) {
Is this what was decided for unused arguments? I thought Mike didn't like this.

https://codereview.appspot.com/7390056/diff/11001/src/pdf/SkPDFFont.cpp#newco...
src/pdf/SkPDFFont.cpp:1338: : SkPDFFont(info, typeface, relatedFontDescriptor) {
I looked into it a bit; Type3 fonts don't have font descriptors, so you can pass
NULL here and remove the arg from the constructor.

[sugoi, 2013-03-05 13:50:13]

I'll wait for the test before I commit this code. Thanks.

https://codereview.appspot.com/7390056/diff/11001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.appspot.com/7390056/diff/11001/src/pdf/SkPDFFont.cpp#newco...
src/pdf/SkPDFFont.cpp:818: SkPDFFont* SkPDFFont::getFontSubset(const
SkPDFGlyphSet*) {
For arguments that provide no useful information, we can just remove the
argument name. The thing Mike didn't want was to have something like :
void foo(int x, int y, int z)
become
void foo(int, int, int)
because, in this case, the argument name is what carries the important
information about what the argument does. It is acceptable to remove argument
names for stuff like :
int foo(SkMatrix matrix, SkCanvas canvas, SkRect rect, SkBitmap bitmap)
when the argument name does not convey much more information than the argument
type.

https://codereview.appspot.com/7390056/diff/11001/src/pdf/SkPDFFont.cpp#newco...
src/pdf/SkPDFFont.cpp:1338: : SkPDFFont(info, typeface, relatedFontDescriptor) {
Will do

[edisonn, 2013-03-05 16:45:07]

FYI, SkFontHost_Linux.cpp is only included now in NaCl, for which we do not
run tests




On Tue, Mar 5, 2013 at 8:50 AM, <sugoi@chromium.org> wrote:

> I'll wait for the test before I commit this code. Thanks.
>
>
>
> https://codereview.appspot.**com/7390056/diff/11001/src/**
>
pdf/SkPDFFont.cpp<https://codereview.appspot.com/7390056/diff/11001/src/pdf/SkPDFFont.cpp>
> File src/pdf/SkPDFFont.cpp (right):
>
> https://codereview.appspot.**com/7390056/diff/11001/src/**
>
pdf/SkPDFFont.cpp#newcode818<https://codereview.appspot.com/7390056/diff/11001/src/pdf/SkPDFFont.cpp#newcode818>
> src/pdf/SkPDFFont.cpp:818: SkPDFFont* SkPDFFont::getFontSubset(const
> SkPDFGlyphSet*) {
> For arguments that provide no useful information, we can just remove the
> argument name. The thing Mike didn't want was to have something like :
> void foo(int x, int y, int z)
> become
> void foo(int, int, int)
> because, in this case, the argument name is what carries the important
> information about what the argument does. It is acceptable to remove
> argument names for stuff like :
> int foo(SkMatrix matrix, SkCanvas canvas, SkRect rect, SkBitmap bitmap)
> when the argument name does not convey much more information than the
> argument type.
>
>
> https://codereview.appspot.**com/7390056/diff/11001/src/**
>
pdf/SkPDFFont.cpp#newcode1338<https://codereview.appspot.com/7390056/diff/11001/src/pdf/SkPDFFont.cpp#newcode1338>
> src/pdf/SkPDFFont.cpp:1338: : SkPDFFont(info, typeface,
> relatedFontDescriptor) {
> Will do
>
>
https://codereview.appspot.**com/7390056/<https://codereview.appspot.com/7390...
>

[sugoi, 2013-03-05 17:55:40]

Uploaded a new version with vandebo's comments fixed.

@edisonn: about the tests, are you saying that I should commit this cl without
the test since this code is untested  ?

[Steve VanDeBogart, 2013-03-05 18:28:57]

On 2013/03/05 17:55:40, sugoi wrote:
> Uploaded a new version with vandebo's comments fixed.
> 
> @edisonn: about the tests, are you saying that I should commit this cl without
> the test since this code is untested  ?

As I mentioned before, Chrome has its own font host implementation.  I'm not
sure there's a way to reproduce the conditions needed to test the regression you
fixed with stock Skia font hosts.

[sugoi, 2013-03-05 18:32:26]

On 2013/03/05 18:28:57, Steve VanDeBogart wrote:
> As I mentioned before, Chrome has its own font host implementation.  I'm not
sure there's a way to reproduce the conditions needed to test the regression you
fixed with stock Skia font hosts.

All right, thanks Steve. I'll commit this code so that it can be tested within
Chrome.

[sugoi, 2013-03-05 18:35:59]

Committed patchset #3 manually as r7987 (presubmit successful).

[reed1, 2013-03-05 18:54:39]

I have a CL out to pull in chrome's fonthost, so that skia will own all of
them... hopeful it will land by Thur/Friday.


On Tue, Mar 5, 2013 at 1:28 PM, <vandebo@chromium.org> wrote:

> On 2013/03/05 17:55:40, sugoi wrote:
>
>> Uploaded a new version with vandebo's comments fixed.
>>
>
>  @edisonn: about the tests, are you saying that I should commit this cl
>>
> without
>
>> the test since this code is untested  ?
>>
>
> As I mentioned before, Chrome has its own font host implementation.  I'm
> not sure there's a way to reproduce the conditions needed to test the
> regression you fixed with stock Skia font hosts.
>
>
>
https://codereview.appspot.**com/7390056/<https://codereview.appspot.com/7390...
>
> --
> You received this message because you are subscribed to the Google Groups
> "skia-review" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
skia-review+unsubscribe@**googlegroups.com<skia-review%2Bunsubscribe@googlegr...
> .
> To post to this group, send email to skia-review@googlegroups.com.
> Visit this group at
http://groups.google.com/**group/skia-review?hl=en<http://groups.google.com/g...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[bungeman, 2013-03-05 18:56:19]

This isn't the case anymore, we are using a full FontConfig FontHost
on Linux now in Skia.

On Tue, Mar 5, 2013 at 1:28 PM,  <vandebo@chromium.org> wrote:
> On 2013/03/05 17:55:40, sugoi wrote:
>>
>> Uploaded a new version with vandebo's comments fixed.
>
>
>> @edisonn: about the tests, are you saying that I should commit this cl
>
> without
>>
>> the test since this code is untested  ?
>
>
> As I mentioned before, Chrome has its own font host implementation.  I'm
> not sure there's a way to reproduce the conditions needed to test the
> regression you fixed with stock Skia font hosts.
>
>
> https://codereview.appspot.com/7390056/
>
> --
> You received this message because you are subscribed to the Google Groups
> "skia-review" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to skia-review+unsubscribe@googlegroups.com.
> To post to this group, send email to skia-review@googlegroups.com.
> Visit this group at http://groups.google.com/group/skia-review?hl=en.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>