Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't reload the entire font range when a glyph is missing in a font #9375

Merged
merged 9 commits into from
Mar 23, 2020
Merged

Don't reload the entire font range when a glyph is missing in a font #9375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f4d22e7
Ignore missing glyph when font range is already loaded
oterral Mar 4, 2020
b078eca
Add test
oterral Mar 4, 2020
e21bbec
lint
oterral Mar 4, 2020
0fc9d66
Fix tests
oterral Mar 4, 2020
640824a
Remove line
oterral Mar 4, 2020
41f017c
Remove line
oterral Mar 4, 2020
f08da01
Lint
oterral Mar 5, 2020
9568dac
Add ranges flow definition
oterral Mar 5, 2020
6621b71
Use helpers function
oterral Mar 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/render/glyph_manager.js
View file
Open in desktop
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
Open in desktop
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) => {
sgolbabaei marked this conversation as resolved.
Show resolved Hide resolved
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