Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2722493002:
FontFaceSet.ready should not be resolved if there's pending layouts (Closed)
|
Created:
3 years, 9 months ago by Kunihiko Sakamoto Modified:
3 years, 9 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, chromium-reviews, dglazkov+blink, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFontFaceSet.ready should not be resolved if there's pending layouts
Currently, FontFaceSet.ready returns resolved promise if there's no
loading fonts. But there may be pending stylesheet changes or layout
operations, which may cause font loads. So, this patch adds a
Document::updateStyleAndLayout() call in FontFaceSet::ready() before
returning a resolved promise.
This is a speculative fix to deflake css3/fonts/font-style-matching-* tests.
BUG=516680, 610300
Review-Url: https://codereview.chromium.org/2722493002
Cr-Commit-Position: refs/heads/master@{#454845}
Committed: https://chromium.googlesource.com/chromium/src/+/42bd5bf1a4db228077a4b44c70bff956470f0a75
Patch Set 1 #Patch Set 2 : add comments #
Messages
Total messages: 30 (13 generated)
The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== FontFaceSet.ready should be resolved after layout operations BUG=516680,610300 ========== to ========== FontFaceSet.ready should be resolved after layout operations Currently, FontFaceSet.ready returns resolved promise if there's no loading fonts. But there may be pending stylesheet changes or layout operations, which may cause font loads. So, this patch adds a Document::updateStyleAndLayout() call in FontFaceSet::ready() before returning a resolved promise. This should deflake font-style-matching-* tests. BUG=516680, 610300 ==========
Description was changed from ========== FontFaceSet.ready should be resolved after layout operations Currently, FontFaceSet.ready returns resolved promise if there's no loading fonts. But there may be pending stylesheet changes or layout operations, which may cause font loads. So, this patch adds a Document::updateStyleAndLayout() call in FontFaceSet::ready() before returning a resolved promise. This should deflake font-style-matching-* tests. BUG=516680, 610300 ========== to ========== FontFaceSet.ready should be resolved after layout operations Currently, FontFaceSet.ready returns resolved promise if there's no loading fonts. But there may be pending stylesheet changes or layout operations, which may cause font loads. So, this patch adds a Document::updateStyleAndLayout() call in FontFaceSet::ready() before returning a resolved promise. This should deflake css3/fonts/font-style-matching-* tests. BUG=516680, 610300 ==========
Description was changed from ========== FontFaceSet.ready should be resolved after layout operations Currently, FontFaceSet.ready returns resolved promise if there's no loading fonts. But there may be pending stylesheet changes or layout operations, which may cause font loads. So, this patch adds a Document::updateStyleAndLayout() call in FontFaceSet::ready() before returning a resolved promise. This should deflake css3/fonts/font-style-matching-* tests. BUG=516680, 610300 ========== to ========== FontFaceSet.ready should not be resolved if there's pending layouts Currently, FontFaceSet.ready returns resolved promise if there's no loading fonts. But there may be pending stylesheet changes or layout operations, which may cause font loads. So, this patch adds a Document::updateStyleAndLayout() call in FontFaceSet::ready() before returning a resolved promise. This should deflake css3/fonts/font-style-matching-* tests. BUG=516680, 610300 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ksakamoto@chromium.org changed reviewers: + drott@chromium.org, kojii@chromium.org
Hmm...I'm not very familiar with Promise, but isn't this updating layout when returning a Promise, not when resolving the Promise? The intention is the latter, correct? From my instinct, adding it before |m_resolver->resolve| looks better to me, but again, I'm not very familiar with it. Maybe we should ask binding team to review, since ScriptPromisePropertyBase is in binding?
drott@chromium.org changed reviewers: + haraken@chromium.org
Kunihiko, thanks very much for attacking this problem. I think it will grealy help actually help, even with more layout tests then the font-style-matching-* ones. I briefly discussed this problem with haraken@ briefly at BlinkOn: His take was that we should call ScriptPromiseResolver::resolve later than we currently do. I suspect we currently call it as soon as loading is complete, but the arrival-signal from the loader does trigger the relayout and we are not finished with relayout yet, when we already resolve the promise, resulting in failing/racy tests. Is updateStyleAndLayout() synchronous and blocking, completing only when layout is complete? In that case, I tend to agree with Koji, that before the resolve() call should be the right place.
(Added some comments in FontFaceSet::ready(), PTAL.) Thanks for the comments. I think there are two separate issues: (1) When font loading is complete, the ready promise may be resolved *before* we finish relayout with updated fonts. The needsLayout() check in fireDoneEventIfPossible() [1] attempts to catch that, but probably this isn't enough. (2) Once the ready promise is resolved, subsequent call to FontFaceSet::ready() will return the same (resolved) promise [2]. But once another font load is required by style/layout changes, it must return a fresh pending promise. [3] Currently, FontFaceSet::ready() returns resolved promise even if there are pending style / layout changes. Adding updateStyleAndLayout() just before resolve() will fix (1), but won't fix (2). My change is for (2). Can we land this and see if it will fix the font-style-matching-* flakiness? If it didn't, we can try adding updateStyleAndLayout() before resolving the promise. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/FontF... [2] https://bugs.chromium.org/p/chromium/issues/detail?id=510434#c15 [3] https://drafts.csswg.org/css-font-loading/#switch-the-fontfaceset-to-loading
LGTM, understood we were talking about different cases (talked offline.) This patch fixes the problem ksatamoto@ is trying to fix, and the side effect is low. Can you change the last line of the description to something like: This is a speculative fix to deflake css3/fonts/font-style-matching-* tests. so that sheriff can understand if it's still flaky?
Description was changed from ========== FontFaceSet.ready should not be resolved if there's pending layouts Currently, FontFaceSet.ready returns resolved promise if there's no loading fonts. But there may be pending stylesheet changes or layout operations, which may cause font loads. So, this patch adds a Document::updateStyleAndLayout() call in FontFaceSet::ready() before returning a resolved promise. This should deflake css3/fonts/font-style-matching-* tests. BUG=516680, 610300 ========== to ========== FontFaceSet.ready should not be resolved if there's pending layouts Currently, FontFaceSet.ready returns resolved promise if there's no loading fonts. But there may be pending stylesheet changes or layout operations, which may cause font loads. So, this patch adds a Document::updateStyleAndLayout() call in FontFaceSet::ready() before returning a resolved promise. This is a speculative fix to deflake css3/fonts/font-style-matching-* tests. BUG=516680, 610300 ==========
On 2017/02/28 06:43:08, kojii wrote: > LGTM, understood we were talking about different cases (talked offline.) This > patch fixes the problem ksatamoto@ is trying to fix, and the side effect is low. > > Can you change the last line of the description to something like: > This is a speculative fix to deflake css3/fonts/font-style-matching-* tests. > so that sheriff can understand if it's still flaky? CL description updated.
LGTM on my side.
Thanks for the explanations, Kunihiko. I somewhat understand the explanations, but I am not 100% sure. Koji said, he talked with you, so he probably has a better understanding now. Can you help with the question I had: > Is updateStyleAndLayout() synchronous and blocking, completing only when layout is complete? Are you planning to address 1)?
On 2017/02/28 07:40:40, drott wrote: > Thanks for the explanations, Kunihiko. I somewhat understand the explanations, > but I am not 100% sure. Koji said, he talked with you, so he probably has a > better understanding now. > > Can you help with the question I had: > > Is updateStyleAndLayout() synchronous and blocking, completing only when > layout is complete? Yes. > Are you planning to address 1)? If we have repro cases, I'm happy to do it. Do you have concrete test cases that fail because of 1), not 2) ?
On 2017/02/28 07:50:48, Kunihiko Sakamoto wrote: > On 2017/02/28 07:40:40, drott wrote: > > Thanks for the explanations, Kunihiko. I somewhat understand the explanations, > > but I am not 100% sure. Koji said, he talked with you, so he probably has a > > better understanding now. > > > > Can you help with the question I had: > > > Is updateStyleAndLayout() synchronous and blocking, completing only when > > layout is complete? > > Yes. > > > Are you planning to address 1)? > > If we have repro cases, I'm happy to do it. > Do you have concrete test cases that fail because of 1), not 2) ? (I couldn't reproduce font-style-matching failure locally, so I'm not sure which bug caused that.)
On 2017/02/28 at 07:56:39, ksakamoto wrote: > On 2017/02/28 07:50:48, Kunihiko Sakamoto wrote: > > On 2017/02/28 07:40:40, drott wrote: > > > Thanks for the explanations, Kunihiko. I somewhat understand the explanations, > > > but I am not 100% sure. Koji said, he talked with you, so he probably has a > > > better understanding now. > > > > > > Can you help with the question I had: > > > > Is updateStyleAndLayout() synchronous and blocking, completing only when > > > layout is complete? > > > > Yes. > > > > > Are you planning to address 1)? > > > > If we have repro cases, I'm happy to do it. > > Do you have concrete test cases that fail because of 1), not 2) ? > > (I couldn't reproduce font-style-matching failure locally, so I'm not sure which bug caused that.) I think if the hypothesis that the tests are flaky because of a race condition between fonts.ready resolved and relayout complete, it might be possible to make a long-layout page to trigger this issue. For example by creating large multi-screen long page, all characters depending on loading a web font, then measure timing of arrival of web font, document.fonts.ready resolved event, and layout complete.
On 2017/02/28 at 12:00:11, drott wrote: > I think if the hypothesis that the tests are flaky because of a race condition between fonts.ready resolved and relayout complete, it might be possible to make a long-layout page to trigger this issue. For example by creating large multi-screen long page, all characters depending on loading a web font, then measure timing of arrival of web font, document.fonts.ready resolved event, and layout complete. Wrote up a summary here: https://docs.google.com/a/chromium.org/document/d/11v0QEy8rvoNlFadAc4o3aaz9DM... In my understanding, I was thinking the root cause is 3, then drott@ brought up 2 in crbug.com/516680, and this CL is about 1. Given now we have 3 possible issues, I can't determine which is causing crbug.com/610300. So I prefer to land this and see if crbug.com/610300 is fixed. On the other hand, I'm certain that 3 is not resolved yet. I'm not certain about 2, but probably easier to discuss on docs?
drott@, are you happy to land this and continue the discussion on the doc and moz people?
On 2017/03/03 at 08:04:32, kojii wrote: > drott@, are you happy to land this and continue the discussion on the doc and moz people? I am okay landing this, but I would prefer we would investigate towards finding a local reproduction.
On 2017/03/06 09:11:23, drott wrote: > On 2017/03/03 at 08:04:32, kojii wrote: > > drott@, are you happy to land this and continue the discussion on the doc and > moz people? > > I am okay landing this, but I would prefer we would investigate towards finding > a local reproduction. Let me land this. This will at least fix (2) in #13, which is reproducible by the test case added to fontfaceset-ready.html.
The CQ bit was checked by ksakamoto@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488792112607210,
"parent_rev": "71934ae71dae5027bfe88908baf44bb0f81a59cb", "commit_rev":
"42bd5bf1a4db228077a4b44c70bff956470f0a75"}
Message was sent while issue was closed.
Description was changed from ========== FontFaceSet.ready should not be resolved if there's pending layouts Currently, FontFaceSet.ready returns resolved promise if there's no loading fonts. But there may be pending stylesheet changes or layout operations, which may cause font loads. So, this patch adds a Document::updateStyleAndLayout() call in FontFaceSet::ready() before returning a resolved promise. This is a speculative fix to deflake css3/fonts/font-style-matching-* tests. BUG=516680, 610300 ========== to ========== FontFaceSet.ready should not be resolved if there's pending layouts Currently, FontFaceSet.ready returns resolved promise if there's no loading fonts. But there may be pending stylesheet changes or layout operations, which may cause font loads. So, this patch adds a Document::updateStyleAndLayout() call in FontFaceSet::ready() before returning a resolved promise. This is a speculative fix to deflake css3/fonts/font-style-matching-* tests. BUG=516680, 610300 Review-Url: https://codereview.chromium.org/2722493002 Cr-Commit-Position: refs/heads/master@{#454845} Committed: https://chromium.googlesource.com/chromium/src/+/42bd5bf1a4db228077a4b44c70bf... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/42bd5bf1a4db228077a4b44c70bf... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
