Skip to content

Commit

Permalink
Don't reload the entire font range when a glyph is missing in a font (#…
Browse files Browse the repository at this point in the history 
…9375)

* Ignore missing glyph when font range is already loaded

* Add test

* lint

* Fix tests

* Remove line

* Remove line

* Lint

* Add ranges flow definition

* Use helpers function
  • Loading branch information
@oterral
oterral authored Mar 23, 2020
1 parent 6ffd74e commit a2afce6
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 17 deletions.
10 changes: 9 additions & 1 deletion src/render/glyph_manager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ type Entry = {
// null means we've requested the range, but the glyph wasn't included in the result.
glyphs: {[id: number]: StyleGlyph | null},
requests: {[range: number]: Array<Callback<{[_: number]: StyleGlyph | null}>>},
ranges: {[range: number]: boolean | null},
tinySDF?: TinySDF
};

Expand Down Expand Up @@ -52,7 +53,8 @@ class GlyphManager {
if (!entry) {
entry = this.entries[stack] = {
glyphs: {},
requests: {}
requests: {},
ranges: {}
};
}

Expand All @@ -75,6 +77,11 @@ class GlyphManager {
return;
}

if (entry.ranges[range]) {
callback(null, {stack, id, glyph});
return;
}

let requests = entry.requests[range];
if (!requests) {
requests = entry.requests[range] = [];
Expand All @@ -86,6 +93,7 @@ class GlyphManager {
entry.glyphs[+id] = response[+id];
}
}
entry.ranges[range] = true;
}
for (const cb of requests) {
cb(err, response);
Expand Down
56 changes: 40 additions & 16 deletions test/unit/render/glyph_manager.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,27 @@ for (const glyph of parseGlyphPBF(fs.readFileSync('./test/fixtures/0-255.pbf')))
glyphs[glyph.id] = glyph;
}

test('GlyphManager requests 0-255 PBF', (t) => {
const identityTransform = (url) => ({url});
t.stub(GlyphManager, 'loadGlyphRange').callsFake((stack, range, urlTemplate, transform, callback) => {
const identityTransform = (url) => ({url});

const createLoadGlyphRangeStub = (t) => {
return t.stub(GlyphManager, 'loadGlyphRange').callsFake((stack, range, urlTemplate, transform, callback) => {
t.equal(stack, 'Arial Unicode MS');
t.equal(range, 0);
t.equal(urlTemplate, 'https://localhost/fonts/v1/{fontstack}/{range}.pbf');
t.equal(transform, identityTransform);
setImmediate(() => callback(null, glyphs));
});
};

const manager = new GlyphManager(identityTransform);
const createGlyphManager = (font) => {
const manager = new GlyphManager(identityTransform, font);
manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf');
return manager;
};

test('GlyphManager requests 0-255 PBF', (t) => {
createLoadGlyphRangeStub(t);
const manager = createGlyphManager();

manager.getGlyphs({'Arial Unicode MS': [55]}, (err, glyphs) => {
t.ifError(err);
Expand All @@ -28,13 +37,33 @@ test('GlyphManager requests 0-255 PBF', (t) => {
});
});

test('GlyphManager doesn\'t request twice 0-255 PBF if a glyph is missing', (t) => {
const stub = createLoadGlyphRangeStub(t);
const manager = createGlyphManager();

manager.getGlyphs({'Arial Unicode MS': [0.5]}, (err) => {
t.ifError(err);
t.equal(manager.entries['Arial Unicode MS'].ranges[0], true);
t.equal(stub.calledOnce, true);

// We remove all requests as in getGlyphs code.
delete manager.entries['Arial Unicode MS'].requests[0];

manager.getGlyphs({'Arial Unicode MS': [0.5]}, (err) => {
t.ifError(err);
t.equal(manager.entries['Arial Unicode MS'].ranges[0], true);
t.equal(stub.calledOnce, true);
t.end();
});
});
});

test('GlyphManager requests remote CJK PBF', (t) => {
t.stub(GlyphManager, 'loadGlyphRange').callsFake((stack, range, urlTemplate, transform, callback) => {
setImmediate(() => callback(null, glyphs));
});

const manager = new GlyphManager((url) => ({url}));
manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf');
const manager = createGlyphManager();

manager.getGlyphs({'Arial Unicode MS': [0x5e73]}, (err, glyphs) => {
t.ifError(err);
Expand All @@ -59,8 +88,7 @@ test('GlyphManager does not cache CJK chars that should be rendered locally', (t
return new Uint8ClampedArray(900);
}
});
const manager = new GlyphManager((url) => ({url}), 'sans-serif');
manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf');
const manager = createGlyphManager('sans-serif');

//Request char that overlaps Katakana range
manager.getGlyphs({'Arial Unicode MS': [0x3005]}, (err, glyphs) => {
Expand All @@ -86,8 +114,7 @@ test('GlyphManager generates CJK PBF locally', (t) => {
}
});

const manager = new GlyphManager((url) => ({url}), 'sans-serif');
manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf');
const manager = createGlyphManager('sans-serif');

manager.getGlyphs({'Arial Unicode MS': [0x5e73]}, (err, glyphs) => {
t.ifError(err);
Expand All @@ -104,8 +131,7 @@ test('GlyphManager generates Katakana PBF locally', (t) => {
}
});

const manager = new GlyphManager((url) => ({url}), 'sans-serif');
manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf');
const manager = createGlyphManager('sans-serif');

// Katakana letter te
manager.getGlyphs({'Arial Unicode MS': [0x30c6]}, (err, glyphs) => {
Expand All @@ -123,8 +149,7 @@ test('GlyphManager generates Hiragana PBF locally', (t) => {
}
});

const manager = new GlyphManager((url) => ({url}), 'sans-serif');
manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf');
const manager = createGlyphManager('sans-serif');

//Hiragana letter te
manager.getGlyphs({'Arial Unicode MS': [0x3066]}, (err, glyphs) => {
Expand All @@ -144,8 +169,7 @@ test('GlyphManager caches locally generated glyphs', (t) => {
}
});

const manager = new GlyphManager((url) => ({url}), 'sans-serif');
manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf');
const manager = createGlyphManager('sans-serif');

// Katakana letter te
manager.getGlyphs({'Arial Unicode MS': [0x30c6]}, (err, glyphs) => {
Expand Down

0 comments on commit a2afce6

Please sign in to comment.