Drawing fake bold text, line and normal text made normal text bolder on PDF.

Drawing fake bold text, line and normal text made normal text bolder on PDF.

When PDF uses fake bold text, it uses stroke and fill for the
text. After that, line is drawn. It creates new ContextEntry which
GraphicStateEntry has 0 fTextScaleX. Since fTextScaleX is 0,
GraphicStackState::updateDrawingState() doesn't update text fill mode or
the PDF page. When the normal text is drawn, it reuse the line's
ContextEntry because GraphicStateEntry::compareInitialState() returns
true. However, since the reused GraphicsStateEntry's fTextScaleX is 0,
the text fill mode is not updated to fill, and the text is drawn in
stroke and fill mode.

When the fTextScaleX is changed, we should not reuse GraphicStateEntry
even if ether of fTextScaleX is 0.

BUG=skia:2132
Committed: http://code.google.com/p/skia/source/detail?r=13369

[yuki.sekiguchi, 2014-02-05 08:28:29]

Hi reed,

Could you review my CL?

This is my first CL to skia, so if I'm doing wrong, please correct me.

[vandebo (ex-Chrome), 2014-02-05 22:07:08]

https://codereview.chromium.org/137923005/diff/1/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/137923005/diff/1/src/pdf/SkPDFDevice.cpp#newc...
src/pdf/SkPDFDevice.cpp:241: fTextScaleX == b.fTextScaleX &&
This will work, but I think it's a bit more specific than we need to be.

Being more specific requires knowing which entry is the current one, and which
one is the previous, which is fine - there's only one caller to this function =>
 Rename b to cur.  Then if the new entry doesn't care about text (i.e.
fTextScaleX == 0), we're ok, but if the new entry does care, it has to match
both.  So 241-242 becomes:
(fTextScaleX == 0 ||
 (fTextScaleX == cur.fTextScaleX && fTextFill == cur.fTextFill));

Sound right/work for you?

[yuki.sekiguchi, 2014-02-06 02:39:04]

On 2014/02/05 22:07:08, vandebo wrote:
> https://codereview.chromium.org/137923005/diff/1/src/pdf/SkPDFDevice.cpp
> File src/pdf/SkPDFDevice.cpp (right):
> 
>
https://codereview.chromium.org/137923005/diff/1/src/pdf/SkPDFDevice.cpp#newc...
> src/pdf/SkPDFDevice.cpp:241: fTextScaleX == b.fTextScaleX &&
> This will work, but I think it's a bit more specific than we need to be.
> 
> Being more specific requires knowing which entry is the current one, and which
> one is the previous, which is fine - there's only one caller to this function
=>
>  Rename b to cur.  Then if the new entry doesn't care about text (i.e.
> fTextScaleX == 0), we're ok, but if the new entry does care, it has to match
> both.  So 241-242 becomes:
> (fTextScaleX == 0 ||
>  (fTextScaleX == cur.fTextScaleX && fTextFill == cur.fTextFill));
> 
> Sound right/work for you?

SGTM.

Cur is more clear meaning, and reusing more GraphicStateEntry is better.

I tested reproduced code, and it works well.

[yuki.sekiguchi, 2014-02-06 02:40:50]

Hi vandebo,

I fixed your comment. Could you review this?

[vandebo (ex-Chrome), 2014-02-06 16:45:00]

On 2014/02/06 02:40:50, yuki.sekiguchi wrote:
> Hi vandebo,
> 
> I fixed your comment. Could you review this?

Have you submitted the CLA[1]? I didn't see it in the internal list, but I may
be looking in the wrong place.

[1]: https://developers.google.com/open-source/cla/corporate?csw=1

[yuki.sekiguchi, 2014-02-07 00:38:02]

On 2014/02/06 16:45:00, vandebo wrote:
> On 2014/02/06 02:40:50, yuki.sekiguchi wrote:
> > Hi vandebo,
> > 
> > I fixed your comment. Could you review this?
> 
> Have you submitted the CLA[1]? I didn't see it in the internal list, but I may
> be looking in the wrong place.
> 
> [1]: https://developers.google.com/open-source/cla/corporate?csw=1

Yes, we, ACCESS, have submitted the CLA.

We sent the signed agreement PDF to cla-submissions@google.com. However, no one
response to the mail.

I landed some patches to Blink, so someone in Blink team may know the CLA
status. My first patch was reviewed by ojan@, so he may know the status.

[hcm, 2014-02-07 18:51:59]

On 2014/02/07 00:38:02, yuki.sekiguchi wrote:
> On 2014/02/06 16:45:00, vandebo wrote:
> > On 2014/02/06 02:40:50, yuki.sekiguchi wrote:
> > > Hi vandebo,
> > > 
> > > I fixed your comment. Could you review this?
> > 
> > Have you submitted the CLA[1]? I didn't see it in the internal list, but I
may
> > be looking in the wrong place.
> > 
> > [1]: https://developers.google.com/open-source/cla/corporate?csw=1
> 
> Yes, we, ACCESS, have submitted the CLA.
> 
> We sent the signed agreement PDF to mailto:cla-submissions@google.com.
However, no one
> response to the mail.
> 
> I landed some patches to Blink, so someone in Blink team may know the CLA
> status. My first patch was reviewed by ojan@, so he may know the status.

Yuki, By chance could you or the person who submitted the email to
cla-submissions forward that to me (hcm@google.com) so I can follow up with the
open source legal team and make sure it gets registered.  Sorry for the trouble.
 If you guys do not find the email, just send me the PDF with the CLA.  I'll
work on that and you should be good to go for now with the AUTHORS file entry,
appreciate you doing that.
-Heather

[vandebo (ex-Chrome), 2014-02-07 19:12:23]

LGTM

[vandebo (ex-Chrome), 2014-02-07 19:12:28]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-02-07 19:12:34]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/yuki.sekiguchi@access-company.com/137...

[commit-bot: I haz the power, 2014-02-07 19:39:59]

Change committed as 13369