Fix ToUnicode generation bug

Fix ToUnicode generation bug

BUG=Chromium:104062
TEST=ToUnicode

[arthurhsu, 2011-12-16 22:05:57]

Partial solution of http://code.google.com/p/chromium/issues/detail?id=104062

[Steve VanDeBogart, 2011-12-19 19:38:05]

Skia doesn't require the BUG= and TEST= lines.  But in this case, I would put
BUG=Chromium:104062

http://codereview.appspot.com/5492061/diff/1/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/5492061/diff/1/src/pdf/SkPDFFont.cpp#newcode480
src/pdf/SkPDFFont.cpp:480: if (glyphToUnicode[i] && (subset == NULL ||
subset->has(i))) {
As is, this code is fairly convoluted.  I think it can be made much simpler. 
There are a couple predicates in this code:
Is the glyphid part of the subset (in_subset)
Is the glyphid next in the range and allowed to be added (in_range)
Is the range currently empty (range_empty)
In terms of these predicates, I think the code can be:

if ((!in_subset || !in_range) && !range_empty) {
  if (range size > 1)
    output range
  else
    output char
  empty range
}
if (in_subset) {
  add to range
}

Plus a flushing of the range outside the loop.  Or you can extend the loop count
by one and use the in loop flush.  Checking for an empty range can be
accomplished by using an invalid glyph id like in SkAdvancedTypefaceMetrics.

[arthurhsu, 2011-12-19 22:55:58]

http://codereview.appspot.com/5492061/diff/1/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/5492061/diff/1/src/pdf/SkPDFFont.cpp#newcode480
src/pdf/SkPDFFont.cpp:480: if (glyphToUnicode[i] && (subset == NULL ||
subset->has(i))) {
On 2011/12/19 19:38:05, Steve VanDeBogart wrote:
> As is, this code is fairly convoluted.  I think it can be made much simpler. 
> There are a couple predicates in this code:
> Is the glyphid part of the subset (in_subset)
> Is the glyphid next in the range and allowed to be added (in_range)
> Is the range currently empty (range_empty)
> In terms of these predicates, I think the code can be:
> 
> if ((!in_subset || !in_range) && !range_empty) {
>   if (range size > 1)
>     output range
>   else
>     output char
>   empty range
> }
> if (in_subset) {
>   add to range
> }
> 
> Plus a flushing of the range outside the loop.  Or you can extend the loop
count
> by one and use the in loop flush.  Checking for an empty range can be
> accomplished by using an invalid glyph id like in SkAdvancedTypefaceMetrics.

Done.

[Steve VanDeBogart, 2012-01-03 18:36:30]

Committed as r2944

http://codereview.appspot.com/5492061/diff/4001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/5492061/diff/4001/src/pdf/SkPDFFont.cpp#newcode481
src/pdf/SkPDFFont.cpp:481: bool inSubset = (i != count) ? subset->has(i) :
false;
bool inSubset = (i < count) && (subset == NULL || subset->has(i));

http://codereview.appspot.com/5492061/diff/4001/src/pdf/SkPDFFont.cpp#newcode486
src/pdf/SkPDFFont.cpp:486: bool inRange = (i == count) ? false :
just do: i < count &&

http://codereview.appspot.com/5492061/diff/4001/src/pdf/SkPDFFont.cpp#newcode487
src/pdf/SkPDFFont.cpp:487: ((i == currentRangeEntry.fEnd + 1) &&
nit: unnecessary parenthesis

http://codereview.appspot.com/5492061/diff/4001/src/pdf/SkPDFFont.cpp#newcode508
src/pdf/SkPDFFont.cpp:508: rangeEmpty = false;
nit: put this inside the if(rangeEmpty) body

[arthurhsu, 2012-01-03 19:07:31]

http://codereview.appspot.com/5492061/diff/4001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/5492061/diff/4001/src/pdf/SkPDFFont.cpp#newcode481
src/pdf/SkPDFFont.cpp:481: bool inSubset = (i != count) ? subset->has(i) :
false;
On 2012/01/03 18:36:31, Steve VanDeBogart wrote:
> bool inSubset = (i < count) && (subset == NULL || subset->has(i));

Done.

http://codereview.appspot.com/5492061/diff/4001/src/pdf/SkPDFFont.cpp#newcode486
src/pdf/SkPDFFont.cpp:486: bool inRange = (i == count) ? false :
On 2012/01/03 18:36:31, Steve VanDeBogart wrote:
> just do: i < count &&

Done.

http://codereview.appspot.com/5492061/diff/4001/src/pdf/SkPDFFont.cpp#newcode487
src/pdf/SkPDFFont.cpp:487: ((i == currentRangeEntry.fEnd + 1) &&
On 2012/01/03 18:36:31, Steve VanDeBogart wrote:
> nit: unnecessary parenthesis 

Done.

http://codereview.appspot.com/5492061/diff/4001/src/pdf/SkPDFFont.cpp#newcode508
src/pdf/SkPDFFont.cpp:508: rangeEmpty = false;
On 2012/01/03 18:36:31, Steve VanDeBogart wrote:
> nit: put this inside the if(rangeEmpty) body

Done.