Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(278)

Issue 23519006: pdf: write only ToUnicode mappings needed by the font, trimming anything out of [firstChar, lastCha… (Closed)

Created:
7 years, 3 months ago by edisonn
Modified:
7 years, 1 month ago
CC:
skia-review_googlegroups.com, reed1, epoger
Visibility:
Public.

Description

pdf: write only ToUnicode mappings needed by the font, trimming anything out of [firstChar, lastChar] interval. (checked in from another client) - closed

Patch Set 1 #

Total comments: 10

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Total comments: 3

Patch Set 8 : #

Patch Set 9 : #

Total comments: 12

Patch Set 10 : #

Patch Set 11 : #

Total comments: 9

Patch Set 12 : #

Patch Set 13 : #

Patch Set 14 : #

Patch Set 15 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+75 lines, -25 lines) Patch
M src/pdf/SkPDFFont.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 7 chunks +34 lines, -20 lines 0 comments Download
M tests/ToUnicode.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 6 chunks +41 lines, -5 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
edisonn
part1 of a probably long series of updates to make poppler warnings go away.
7 years, 3 months ago (2013-09-04 21:25:56 UTC) #1
vandebo (ex-Chrome)
Just nits. https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp#newcode411 src/pdf/SkPDFFont.cpp:411: while (i < bfchar.count() && bfchar[i].fGlyphId < ...
7 years, 3 months ago (2013-09-04 22:07:53 UTC) #2
edisonn
https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/1/src/pdf/SkPDFFont.cpp#newcode411 src/pdf/SkPDFFont.cpp:411: while (i < bfchar.count() && bfchar[i].fGlyphId < firstGlypthID) { ...
7 years, 3 months ago (2013-09-06 18:54:38 UTC) #3
vandebo (ex-Chrome)
https://codereview.chromium.org/23519006/diff/23001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/23001/src/pdf/SkPDFFont.cpp#newcode497 src/pdf/SkPDFFont.cpp:497: for (int i = 0; i < count + ...
7 years, 3 months ago (2013-09-06 21:22:55 UTC) #4
edisonn
https://codereview.chromium.org/23519006/diff/23001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/23001/src/pdf/SkPDFFont.cpp#newcode497 src/pdf/SkPDFFont.cpp:497: for (int i = 0; i < count + ...
7 years, 3 months ago (2013-09-09 14:36:52 UTC) #5
vandebo (ex-Chrome)
https://codereview.chromium.org/23519006/diff/23001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/23001/src/pdf/SkPDFFont.cpp#newcode497 src/pdf/SkPDFFont.cpp:497: for (int i = 0; i < count + ...
7 years, 3 months ago (2013-09-09 16:04:50 UTC) #6
edisonn
https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp#newcode507 src/pdf/SkPDFFont.cpp:507: i < count && On 2013/09/09 16:04:50, vandebo wrote: ...
7 years, 3 months ago (2013-09-09 16:48:41 UTC) #7
vandebo (ex-Chrome)
https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp#newcode507 src/pdf/SkPDFFont.cpp:507: i < count && On 2013/09/09 16:48:41, edisonn wrote: ...
7 years, 3 months ago (2013-09-09 17:13:10 UTC) #8
edisonn
https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/32001/src/pdf/SkPDFFont.cpp#newcode507 src/pdf/SkPDFFont.cpp:507: i < count && On 2013/09/09 17:13:11, vandebo wrote: ...
7 years, 3 months ago (2013-09-09 17:32:10 UTC) #9
edisonn
optimized the code to do the minimum amount of work, this include processing last glyph ...
7 years, 3 months ago (2013-09-09 19:05:22 UTC) #10
vandebo (ex-Chrome)
https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp#newcode496 src/pdf/SkPDFFont.cpp:496: const int limit = SkMin32(lastGlyphID, glyphToUnicode.count()) + 2; These ...
7 years, 3 months ago (2013-09-09 20:00:28 UTC) #11
edisonn
https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp File src/pdf/SkPDFFont.cpp (right): https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp#newcode496 src/pdf/SkPDFFont.cpp:496: const int limit = SkMin32(lastGlyphID, glyphToUnicode.count()) + 2; It ...
7 years, 3 months ago (2013-09-10 14:50:11 UTC) #12
vandebo (ex-Chrome)
7 years, 3 months ago (2013-09-12 22:23:39 UTC) #13
If you like patch set 13, I'm fine with it - LGTM.

https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/23519006/diff/38001/src/pdf/SkPDFFont.cpp#new...
src/pdf/SkPDFFont.cpp:496: const int limit = SkMin32(lastGlyphID,
glyphToUnicode.count()) + 2;
On 2013/09/10 14:50:11, edisonn wrote:
> It does not work with the new tests I added. Please apply the ToUnitode.cpp
> test.
> the issue is that count requires a < operator, while lastGlyph requires a <=
> operator. 
> 
> To make it work I can use SkMin32(lastGlyphID + 1, glyphToUnicode.count());
> 
> This works and makes the code simpler, but I do not like it because the code
is

Fair, I think this can be fixed by using another variable or two and working on
variable names...

const int lastUnicodeIndex = glyphToUnicode.count() - 1;
const int endIndex = SkMin32(lastGlyphID, lastUnicodeIndex) + 1;

// Loop around once more after endIndex in order to close any open range
for (int i ...

?

> based on a technicality, and the idea why we use +1 is lost in the code, and
the
> readability is lost from the code, it is like a compiled code for perf and
> beauty
> 
>  
> On 2013/09/09 20:00:29, vandebo wrote:
> > These changes seem to work for me and are closer to what's already here:
> > 
> > const int limit = SkMin32(lastGlyphID, glyphToUnicode.count());
>

Powered by Google App Engine
This is Rietveld 408576698