Remove the 2 different font stacks. This was not fixing the webkit issue...

[jeremiele]

....

What seems to be happening is that webkit is changing the font to the
default browser font when downloading a font and not respecting the font
stack. The new code requests a bogus font (sans-serif with quotes to avoid
the css generic font name keyword). This has the effect of falling back
to the default font for the default size. The word 'sans-serif' was chosen
because it also seems to match what Chrome linux requests by default (the
underlaying system library like pango, must match on sans-serif, whereas
it doesn't match anythin on something like 'foo' and would fail for most
cases).

[seanami]

Jeremie: I need to take a deeper look at this, but please don't merge it yet.

The double font stacks weren't meant to solve the webkit issue of fonts changing size more than once. They were meant to prevent all browsers from giving a false negative when the font that's loaded has identical metrics to a single fallback font (such as Liberation Sans and Arial), which means that the width won't change at all. So, you should back out the change to remove the double font stacks, as those are still necessary (and necessary in more browsers than just webkit).

It's good that you've gotten some more insight into the webkit issue, though. The part of webfontloader that's currently working around the webkit issue of size changing before the fonts are loaded is that we wait until we see a new width twice before saying that the fonts are active. That's the part that we would want to change in order to make a better workaround for the webkit early-positive issue.

[jeremiele]

Hi, Sean!

Thanks for the quick look.

On Wed, Nov 16, 2011 at 3:57 PM, Sean McBride <
reply@reply.github.com

wrote:

Jeremie: I need to take a deeper look at this, but please don't merge it
yet.

The double font stacks weren't meant to solve the webkit issue of fonts
changing size more than once. They were meant to prevent all browsers from
giving a false negative when the font that's loaded has identical metrics
to a single fallback font (such as Liberation Sans and Arial), which means
that the width won't change at all. So, you should back out the change to
remove the double font stacks, as those are still necessary (and necessary
in more browsers than just webkit).

The problem is that the double stack makes webkit fail since the stacks are
not properly respected on webkit. Wouldn't the double check work for every
other browser? As far as I can tell on webkit the double check doesn't
always work with the different stacks. For some reason the period of the
size being different can be longer that one cycle.

It's good that you've gotten some more insight into the webkit issue,
though. The part of webfontloader that's currently working around the
webkit issue of size changing before the fonts are loaded is that we wait
until we see a new width twice before saying that the fonts are active.
That's the part that we would want to change in order to make a better
workaround for the webkit early-positive issue.

If you are really against removing the double stack, could we then have
webkit specific code that would do what I've just done? That code has been
the only one that has worked successfully so far on webkit.

---

Reply to this email directly or view it on GitHub:
#38 (comment)

Jeremie

[seanami]

Yeah, I'm pretty against removing the double stack, as it will cause a regression in all browsers for the case of loading a font with identical metrics to the fallback. I promise to look into this more deeply later today and see if we can suggest an alternative.

It's good to know exactly what is causing Webkit's strange behavior (that it's ignoring the fallback stack while the web font is loading or rendering). The "wait to see the same size twice" thing was always a bit of a hack since it depends on timing. Now that we actually know what's going on, I think we can remove that "wait to see the size twice" thing and implement a better workaround.

At the moment, though, I can't think of a way to workaround the case where the font we're loading has the same metrics as the browser's default font. Essentially, we need to ignore an observed width that matches the browser's default font in webkit, but if that's actually the final width of the successfully rendered font as well, then we'll never know the difference between the font loading and the browser's default font being rendered.

I'm going to think about this some more and I'll get back to you today since it sounds like you guys would like to get a fix out soon. In the meantime, if you have any other ideas, chime in so we can brainstorm.

[jeremiele]

The case where the default font size is the same as the fallback is a
tricky one. I'll revert my change for now, and try to think/prototype
any other ideas I have.

On Thu, Nov 17, 2011 at 10:52 AM, Sean McBride
reply@reply.github.com
wrote:

Yeah, I'm pretty against removing the double stack, as it will cause a regression in all browsers for the case of loading a font with identical metrics to the fallback. I promise to look into this more deeply later today and see if we can suggest an alternative.

It's good to know exactly what is causing Webkit's strange behavior (that it's ignoring the fallback stack while the web font is loading or rendering). The "wait to see the same size twice" thing was always a bit of a hack since it depends on timing. Now that we actually know what's going on, I think we can remove that "wait to see the size twice" thing and implement a better workaround.

At the moment, though, I can't think of a way to workaround the case where the font we're loading has the same metrics as the browser's default font. Essentially, we need to ignore an observed width that matches the browser's default font in webkit, but if that's actually the final width of the successfully rendered font as well, then we'll never know the difference between the font loading and the browser's default font being rendered.

I'm going to think about this some more and I'll get back to you today since it sounds like you guys would like to get a fix out soon. In the meantime, if you have any other ideas, chime in so we can brainstorm.

---

Reply to this email directly or view it on GitHub:
#38 (comment)

Jeremie

[seanami]

Jeremie,

Alright, I thought about it a bit more, and here's an outline of what I think we should do:

• We should back out this change, as we need to keep the two font stacks for the case where the metrics are the same as one of the fallbacks.
• We should get rid of the code that I added to wait to see the if the new width was consistent for two measurements. That code was intended to work around this same webkit issue, and it's not good enough.
• We should add the following behavior, but only for Webkit:
  • When FontWatchRunner is initialized, in addition to originalSizeA_ and originalSizeB_, we should capture a browserDefaultSize_ using getDefaultFontSize_. This is the size that we'll expect to see when Webkit is ignoring the fallbacks during font loading.
  • When either sizeA or sizeB changes in check_ for the first time, we check if the new size is equal to browserDefaultSize_. If it is, we don't mark it as active and assume that one of the following will happen:
    • If the font is going to be active and has different metrics from the browser default, then either sizeA or sizeB will quickly change again to be different from the original size and different from browserDefaultSize_, so we just wait for that to happen before marking as active.
    • If the font is going to be inactive, then both sizeA and sizeB will change (how quickly?) back to the originally measured fallback sizes. We continue to wait, and when we hit the timeout, if the sizes are back to the original sizes, we mark the font as inactive.
    • If the font is going to be active and has identical metrics to the browser default, then sizeA and sizeB will never change again. They'll stay the same as browserDefaultSize_. In this case, we have to decide how long we want to wait before we mark the font as active (assuming that the font has the same metrics). The length of this waiting period depends on how long an inactive font will keep the browserDefaultSize_ while it attempts to load before going back to the original sizes when it fails. (Or does an inactive font ever change sizes during the loading process?) It might be short or we might have to wait all the way until the inactive timeout. We should experiment here.

This means that we'll get no modifications for other browsers that don't need them, and in Webkit we'll usually get active notifications as quickly as possible, unless the font happens to have the same metrics as the browser default, which is relatively rare. Even in that case, we can tune the waiting period based on the inactive behavior so that we get an accurate active as quickly as possible.

Does that all make sense? Do you want to alter this pull request to try to implement that approach and do some testing to see what the inactive behavior is like so we can tune how long to wait before marking a font that changes to browserDefaultSize_ and stays there as active in webkit?

[jeremiele]

Hey, Sean.

That's pretty much what I've implemented this morning.
I will update my pull request just so that you can look at the code
(it's just a few lines, the tests are not updated yet though). What I
have done is that I do not update lastObserved if the size is the same
as the default browser font and you're on webkit (same idea as you).
My thinking was to maybe for webkit always mark as active if we hit
the timeout (since we are today marking active early it wouldn't
really break the behavior people are experiencing...).

What do you think?

On Thu, Nov 17, 2011 at 2:55 PM, Sean McBride
reply@reply.github.com
wrote:

Jeremie,

Alright, I thought about it a bit more, and here's an outline of what I think we should do:

• We should back out this change, as we need to keep the two font stacks for the case where the metrics are the same as one of the fallbacks.
• We should get rid of the code that I added to wait to see the if the new width was consistent for two measurements. That code was intended to work around this same webkit issue, and it's not good enough.
• We should add the following behavior, but only for Webkit:
   - When FontWatchRunner is initialized, in addition to originalSizeA_ and originalSizeB_, we should capture a browserDefaultSize_ using getDefaultFontSize_. This is the size that we'll expect to see when Webkit is ignoring the fallbacks during font loading.
   - When either sizeA or sizeB changes in check_ for the first time, we check if the new size is equal to browserDefaultSize_. If it is, we don't mark it as active and assume that one of the following will happen:
     - If the font is going to be active and has different metrics from the browser default, then either sizeA or sizeB will quickly change again to be different from the original size and different from browserDefaultSize_, so we just wait for that to happen before marking as active.
     - If the font is going to be inactive, then both sizeA and sizeB will change (how quickly?) back to the originally measured fallback sizes. We continue to wait, and when we hit the timeout, if the sizes are back to the original sizes, we mark the font as inactive.
     - If the font is going to be active and has identical metrics to the browser default, then sizeA and sizeB will never change again. They'll stay the same as browserDefaultSize_. In this case, we have to decide how long we want to wait before we mark the font as active (assuming that the font has the same metrics). The length of this waiting period depends on how long an inactive font will keep the browserDefaultSize_ while it attempts to load before going back to the original sizes when it fails. (Or does an inactive font ever change sizes during the loading process?) It might be short or we might have to wait all the way until the inactive timeout. We should experiment here.

This means that we'll get no modifications for other browsers that don't need them, and in Webkit we'll usually get active notifications as quickly as possible, unless the font happens to have the same metrics as the browser default, which is relatively rare. Even in that case, we can tune the waiting period based on the inactive behavior so that we get an accurate active as quickly as possible.

Does that all make sense? Do you want to alter this pull request to try to implement that approach and do some testing to see what the inactive behavior is like so we can tune how long to wait before marking a font that changes to browserDefaultSize_ and stays there as active in webkit?

---

Reply to this email directly or view it on GitHub:
#38 (comment)

Jeremie

[seanami]

Nice, I think this approach will definitely work, but there are a few things we can do to optimize it further:

• We don't need to keep the lastObservedSizeA_ and lastObservedSizeB_ waiting bit around for any other browsers, since we know this issue only affects webkit. Removing the extra check iteration that these other browsers have to wait would speed things up for them in getting to active just that little bit sooner.
• Waiting until the timeout in order to mark active when the sizes are equal to webKitFallbackFontSize_ is definitely safe (as you say), but it means that they have to wait quite a while to get to active in cases where the metrics match this fallback. On the other hand, I'm not sure if there's anything foolproof that we can do to speed that up, so maybe it's best for now.

What about something like this within _check?

if ((this.originalSizeA_ != sizeA || this.originalSizeB_ != sizeB) &&
    (!this.webKitFallbackFontSize_ || (this.webKitFallbackFontSize_ != sizeA && this.webKitFallbackFontSize_ != sizeB))) {
  this.finish_(this.activeCallback_);
} else if (this.getTime_() - this.started_ >= 5000) {
  this.finish_(this.inactiveCallback_);
} else {
  this.asyncCheck_();
}

That way we can get rid of lastObservedSizeA_ and lastObservedSizeB_ entirely.

[jeremiele]

Ok, so I literrally went in the webkit code, and tried to figure which
default fallback font they were using.

I've done my best to put the default value for each browser/OS combination.
This code should work if users did not change the default values.

I've also been in touch with chrome developers and they are trying to
get a patch in WebKit but it looks like no one has reviewed the code
so far and it's not on top of their priority list...

I removed the lastObserved as you asked. And I mark the font as active
if on a webkit browser and we reach the 5 second timeout.

I still need to fix the tests, but I've pushed the code if you wanted
to take a glance at it.

On Thu, Nov 17, 2011 at 3:35 PM, Sean McBride
reply@reply.github.com
wrote:

Nice, I think this approach will definitely work, but there are a few things we can do to optimize it further:

• We don't need to keep the lastObservedSizeA_ and lastObservedSizeB_ waiting bit around for any other browsers, since we know this issue only affects webkit. Removing the extra check iteration that these other browsers have to wait would speed things up for them in getting to active just that little bit sooner.
• Waiting until the timeout in order to mark active when the sizes are equal to webKitFallbackFontSize_ is definitely safe (as you say), but it means that they have to wait quite a while to get to active in cases where the metrics match this fallback. On the other hand, I'm not sure if there's anything foolproof that we can do to speed that up, so maybe it's best for now.

What about something like this within _check?

if ((this.originalSizeA_ != sizeA || this.originalSizeB_ != sizeB) &&
   (!this.webKitFallbackFontSize_ || (this.webKitFallbackFontSize_ != sizeA && this.webKitFallbackFontSize_ != sizeB))) {
 this.finish_(this.activeCallback_);
} else if (this.getTime_() - this.started_ >= 5000) {
 this.finish_(this.inactiveCallback_);
} else {
 this.asyncCheck_();
}

That way we can get rid of lastObservedSizeA_ and lastObservedSizeB_ entirely.

---

Reply to this email directly or view it on GitHub:
#38 (comment)

Jeremie

[jeremiele]

I might actually also try to simply have a set with all the possible
sizes of last resort fallback font.
The downside would be that there would be potentially more fonts for
which we wouldn't detect early that the size is different (if they
happen to have the same size as any of the font in the set).
On the other hand that could make the code more robust. I'll explore
that tomorrow.

On Thu, Nov 17, 2011 at 9:33 PM, Jeremie Lenfant-Engelmann
jeremiele@google.com wrote:

Ok, so I literrally went in the webkit code, and tried to figure which
default fallback font they were using.

I've done my best to put the default value for each browser/OS combination.
This code should work if users did not change the default values.

I've also been in touch with chrome developers and they are trying to
get a patch in WebKit but it looks like no one has reviewed the code
so far and it's not on top of their priority list...

I removed the lastObserved as you asked. And I mark the font as active
if on a webkit browser and we reach the 5 second timeout.

I still need to fix the tests, but I've pushed the code if you wanted
to take a glance at it.

On Thu, Nov 17, 2011 at 3:35 PM, Sean McBride
reply@reply.github.com
wrote:

Nice, I think this approach will definitely work, but there are a few things we can do to optimize it further:

• We don't need to keep the lastObservedSizeA_ and lastObservedSizeB_ waiting bit around for any other browsers, since we know this issue only affects webkit. Removing the extra check iteration that these other browsers have to wait would speed things up for them in getting to active just that little bit sooner.
• Waiting until the timeout in order to mark active when the sizes are equal to webKitFallbackFontSize_ is definitely safe (as you say), but it means that they have to wait quite a while to get to active in cases where the metrics match this fallback. On the other hand, I'm not sure if there's anything foolproof that we can do to speed that up, so maybe it's best for now.

What about something like this within _check?

if ((this.originalSizeA_ != sizeA || this.originalSizeB_ != sizeB) &&
   (!this.webKitFallbackFontSize_ || (this.webKitFallbackFontSize_ != sizeA && this.webKitFallbackFontSize_ != sizeB))) {
 this.finish_(this.activeCallback_);
} else if (this.getTime_() - this.started_ >= 5000) {
 this.finish_(this.inactiveCallback_);
} else {
 this.asyncCheck_();
}

That way we can get rid of lastObservedSizeA_ and lastObservedSizeB_ entirely.

---

Reply to this email directly or view it on GitHub:
#38 (comment)

Jeremie

Jeremie

[jeremiele]

I've pushed a change. I now create a set including the size of all the
possible last resort fallback font on webkit, I can then test that the
current size of the font is not part of the set, if it is we basically
ignore and continue polling.
I've also updated the tests.

On Thu, Nov 17, 2011 at 11:55 PM, Jeremie Lenfant-Engelmann
jeremiele@google.com wrote:

I might actually also try to simply have a set with all the possible
sizes of last resort fallback font.
The downside would be that there would be potentially more fonts for
which we wouldn't detect early that the size is different (if they
happen to have the same size as any of the font in the set).
On the other hand that could make the code more robust. I'll explore
that tomorrow.

On Thu, Nov 17, 2011 at 9:33 PM, Jeremie Lenfant-Engelmann
jeremiele@google.com wrote:

Ok, so I literrally went in the webkit code, and tried to figure which
default fallback font they were using.

I've done my best to put the default value for each browser/OS combination.
This code should work if users did not change the default values.

I've also been in touch with chrome developers and they are trying to
get a patch in WebKit but it looks like no one has reviewed the code
so far and it's not on top of their priority list...

I removed the lastObserved as you asked. And I mark the font as active
if on a webkit browser and we reach the 5 second timeout.

I still need to fix the tests, but I've pushed the code if you wanted
to take a glance at it.

On Thu, Nov 17, 2011 at 3:35 PM, Sean McBride
reply@reply.github.com
wrote:

Nice, I think this approach will definitely work, but there are a few things we can do to optimize it further:

• We don't need to keep the lastObservedSizeA_ and lastObservedSizeB_ waiting bit around for any other browsers, since we know this issue only affects webkit. Removing the extra check iteration that these other browsers have to wait would speed things up for them in getting to active just that little bit sooner.
• Waiting until the timeout in order to mark active when the sizes are equal to webKitFallbackFontSize_ is definitely safe (as you say), but it means that they have to wait quite a while to get to active in cases where the metrics match this fallback. On the other hand, I'm not sure if there's anything foolproof that we can do to speed that up, so maybe it's best for now.

What about something like this within _check?

if ((this.originalSizeA_ != sizeA || this.originalSizeB_ != sizeB) &&
   (!this.webKitFallbackFontSize_ || (this.webKitFallbackFontSize_ != sizeA && this.webKitFallbackFontSize_ != sizeB))) {
 this.finish_(this.activeCallback_);
} else if (this.getTime_() - this.started_ >= 5000) {
 this.finish_(this.inactiveCallback_);
} else {
 this.asyncCheck_();
}

That way we can get rid of lastObservedSizeA_ and lastObservedSizeB_ entirely.

---

Reply to this email directly or view it on GitHub:
#38 (comment)

Jeremie

Jeremie

Jeremie

[jeremiele]

Hey, Sean.

Do you think you'll have the time to take a look at the change before
Thanksgiving?

Thanks!

On Fri, Nov 18, 2011 at 5:12 PM, Jeremie Lenfant-Engelmann
jeremiele@google.com wrote:

I've pushed a change. I now create a set including the size of all the
possible last resort fallback font on webkit, I can then test that the
current size of the font is not part of the set, if it is we basically
ignore and continue polling.
I've also updated the tests.

On Thu, Nov 17, 2011 at 11:55 PM, Jeremie Lenfant-Engelmann
jeremiele@google.com wrote:

I might actually also try to simply have a set with all the possible
sizes of last resort fallback font.
The downside would be that there would be potentially more fonts for
which we wouldn't detect early that the size is different (if they
happen to have the same size as any of the font in the set).
On the other hand that could make the code more robust. I'll explore
that tomorrow.

On Thu, Nov 17, 2011 at 9:33 PM, Jeremie Lenfant-Engelmann
jeremiele@google.com wrote:

Ok, so I literrally went in the webkit code, and tried to figure which
default fallback font they were using.

I've done my best to put the default value for each browser/OS combination.
This code should work if users did not change the default values.

I've also been in touch with chrome developers and they are trying to
get a patch in WebKit but it looks like no one has reviewed the code
so far and it's not on top of their priority list...

I removed the lastObserved as you asked. And I mark the font as active
if on a webkit browser and we reach the 5 second timeout.

I still need to fix the tests, but I've pushed the code if you wanted
to take a glance at it.

On Thu, Nov 17, 2011 at 3:35 PM, Sean McBride
reply@reply.github.com
wrote:

Nice, I think this approach will definitely work, but there are a few things we can do to optimize it further:

• We don't need to keep the lastObservedSizeA_ and lastObservedSizeB_ waiting bit around for any other browsers, since we know this issue only affects webkit. Removing the extra check iteration that these other browsers have to wait would speed things up for them in getting to active just that little bit sooner.
• Waiting until the timeout in order to mark active when the sizes are equal to webKitFallbackFontSize_ is definitely safe (as you say), but it means that they have to wait quite a while to get to active in cases where the metrics match this fallback. On the other hand, I'm not sure if there's anything foolproof that we can do to speed that up, so maybe it's best for now.

What about something like this within _check?

if ((this.originalSizeA_ != sizeA || this.originalSizeB_ != sizeB) &&
   (!this.webKitFallbackFontSize_ || (this.webKitFallbackFontSize_ != sizeA && this.webKitFallbackFontSize_ != sizeB))) {
 this.finish_(this.activeCallback_);
} else if (this.getTime_() - this.started_ >= 5000) {
 this.finish_(this.inactiveCallback_);
} else {
 this.asyncCheck_();
}

That way we can get rid of lastObservedSizeA_ and lastObservedSizeB_ entirely.

---

Reply to this email directly or view it on GitHub:
#38 (comment)

Jeremie

Jeremie

Jeremie

Jeremie

[seanami]

This now means that the fonts can never be inactive on webkit browsers, which isn't right. If the sizeA and sizeB are equal to originalSizeA and originalSizeB, then we know for sure that the font is inactive, and we should mark it as such.

I think we only want to mark active if sizeA and sizeB are true in the webKitLastResortFontSizes map.

[jeremiele]

On Tue, Nov 22, 2011 at 10:41 AM, Sean McBride
reply@reply.github.com
wrote:

     this.finish_(this.activeCallback_);
   } else if (this.getTime_() - this.started_ >= 5000) {

•    this.finish_(this.inactiveCallback_);
  +
•    // In order to handle the fact that a font could be the same size as the
•    // default browser font on a webkit browser, mark the font as active
•    // after 5 seconds.
•    this.finish_(this.webKitLastResortFontSizes_ ?
•        this.activeCallback_ : this.inactiveCallback_);

This now means that the fonts can never be inactive on webkit browsers, which isn't right. If the sizeA and sizeB are equal to originalSizeA and originalSizeB, then we know for sure that the font is inactive, and we should mark it as such.

I think we only want to mark active if sizeA and sizeB are true in the webKitLastResortFontSizes map.

Done.

---

Reply to this email directly or view it on GitHub:
https://github.com/typekit/webfontloader/pull/38/files#r245894

Jeremie

[seanami]

Thanks, that part looks good now.

[seanami]

Jeremie, sorry, I was out of town unexpectedly for a funeral. Back now. I left a few more comments for you. I'll keep watching for your responses so we can turn it around quickly and hopefully get a new version out by Thanksgiving.

[jeremiele]

Sorry about your loss :(

It's fine if we push the new version next week. It would just be good
to try to have the code reviewed and agreed upon soon.

Thanks for the comments!

On Tue, Nov 22, 2011 at 10:54 AM, Sean McBride
reply@reply.github.com
wrote:

Jeremie, sorry, I was out of town unexpectedly for a funeral. Back now. I left a few more comments for you. I'll keep watching for your responses so we can turn it around quickly and hopefully get a new version out by Thanksgiving.

---

Reply to this email directly or view it on GitHub:
#38 (comment)

Jeremie

[seanami]

General question: Currently, your code finds all of the last resort fallback widths for each FontWatchRunner that gets instantiated, and one of these is instantiated for each font that we're loading. If we're loading a lot of fonts at once, this will be a lot of extra width measurements. Have you looked at how much this slows things down when loading fonts? I'm not sure how expensive taking all of these measurements repeatedly will be.

[jeremiele]

This will have an impact on performances. This is why I'm creating
only one element in the first place and then change the style (so we
do not have that many insertion and removal of nodes in the DOM).
I'll get some measurements.

On Tue, Nov 22, 2011 at 2:05 PM, Sean McBride
reply@reply.github.com
wrote:

General question: Currently, your code finds all of the last resort fallback widths for each FontWatchRunner that gets instantiated, and one of these is instantiated for each font that we're loading. If we're loading a lot of fonts at once, this will be a lot of extra width measurements. Have you looked at how much this slows things down when loading fonts? I'm not sure how expensive taking all of these measurements repeatedly will be.

---

Reply to this email directly or view it on GitHub:
#38 (comment)

Jeremie

[jeremiele]

A few numbers, done extremely unscientifically. Running the script and
timing the watch method in fontwatcher (which creates all the
fontwatchrunners) 15 times with and without the new code with 3
entries in the WebFontConfig on Chrome linux:

Without the new code: 16ms on average
With the new code: 27ms on average

So on average we see an 11ms increase. As expected it does have an
impact. I do not think however that this is alarming, and again I'm
hoping that we can continue refine that code and maybe the webkit bug
will be fixed sometimes soon...

On Tue, Nov 22, 2011 at 3:22 PM, Jeremie Lenfant-Engelmann
jeremiele@google.com wrote:

This will have an impact on performances. This is why I'm creating
only one element in the first place and then change the style (so we
do not have that many insertion and removal of nodes in the DOM).
I'll get some measurements.

On Tue, Nov 22, 2011 at 2:05 PM, Sean McBride
reply@reply.github.com
wrote:

General question: Currently, your code finds all of the last resort fallback widths for each FontWatchRunner that gets instantiated, and one of these is instantiated for each font that we're loading. If we're loading a lot of fonts at once, this will be a lot of extra width measurements. Have you looked at how much this slows things down when loading fonts? I'm not sure how expensive taking all of these measurements repeatedly will be.

---

Reply to this email directly or view it on GitHub:
#38 (comment)

Jeremie

Jeremie

[jeremiele]

On Tue, Nov 22, 2011 at 3:50 PM, Jeremie Lenfant-Engelmann
jeremiele@google.com wrote:

A few numbers, done extremely unscientifically. Running the script and
timing the watch method in fontwatcher (which creates all the
fontwatchrunners)  15 times with and without the new code with 3
entries in the WebFontConfig on Chrome linux:

3 entries in the WebFontConfig meaning, the Google Module with 3
different font families:

WebFontConfig = {
google: { families: [ 'Mountains of Christmas', 'Lobster', 'Open Sans' ] }
};

Without the new code: 16ms on average
With the new code: 27ms on average

So on average we see an 11ms increase. As expected it does have an
impact. I do not think however that this is alarming, and again I'm
hoping that we can continue refine that code and maybe the webkit bug
will be fixed sometimes soon...

On Tue, Nov 22, 2011 at 3:22 PM, Jeremie Lenfant-Engelmann
jeremiele@google.com wrote:

This will have an impact on performances. This is why I'm creating
only one element in the first place and then change the style (so we
do not have that many insertion and removal of nodes in the DOM).
I'll get some measurements.

On Tue, Nov 22, 2011 at 2:05 PM, Sean McBride
reply@reply.github.com
wrote:

General question: Currently, your code finds all of the last resort fallback widths for each FontWatchRunner that gets instantiated, and one of these is instantiated for each font that we're loading. If we're loading a lot of fonts at once, this will be a lot of extra width measurements. Have you looked at how much this slows things down when loading fonts? I'm not sure how expensive taking all of these measurements repeatedly will be.

---

Reply to this email directly or view it on GitHub:
#38 (comment)

Jeremie

Jeremie

Jeremie

[seanami]

Hey Jeremie,

I think I prefer the style of your other webkit_fix_inheritance branch that uses straight inheritance on the FontWatchRunner.

There are definitely some nice aspects to the way that the different pieces are separated in this branch, but I feel like it adds a little bit too much structure around the current way that we do font watching when we may not do that in the future.

The webkit_fix_inheritance branch is a smaller, more straightforward change, and I think it puts us in a better place for exploring FontWatchRunners with totally different internals in the future.

I think if you just move the comment over about the Webkit code near your font list (as you have in the other branch) and update the tests, then that other branch would be pretty much ready to go.

What do you think?

[jeremiele]

I've open a pull request for the new branch. I've added tests and
updated the old one. I have moved the webkit FontWatchRunner under the
google module, because I've added google specific code.

We can continue the conversation on the other pull request on the other branch.

On Tue, Nov 29, 2011 at 9:56 AM, Sean McBride
reply@reply.github.com
wrote:

Hey Jeremie,

I think I prefer the style of your other webkit_fix_inheritance branch that uses straight inheritance on the FontWatchRunner.

There are definitely some nice aspects to the way that the different pieces are separated in this branch, but I feel like it adds a little bit too much structure around the current way that we do font watching when we may not do that in the future.

The webkit_fix_inheritance branch is a smaller, more straightforward change, and I think it puts us in a better place for exploring FontWatchRunners with totally different internals in the future.

I think if you just move the comment over about the Webkit code near your font list (as you have in the other branch) and update the tests, then that other branch would be pretty much ready to go.

What do you think?

---

Reply to this email directly or view it on GitHub:
#38 (comment)

Jeremie

[seanami]

This pull request is being closed in favor of the new work in #37.