|
|
Created:
3 years, 9 months ago by cbiesinger Modified:
3 years, 7 months ago CC:
chromium-reviews, ojan+watch_chromium.org, szager+layoutwatch_chromium.org, glebl+reviews_chromium.org, dgrogan+ng_chromium.org, atotic+reviews_chromium.org, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, zoltan1, jchaffraix+rendering, blink-reviews, Stephen Chennney Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFontCachePurgePreventer is needed when computing MinPreferredWidth
This makes little difference for now but will be relevant later
for LayoutNG.
Review-Url: https://codereview.chromium.org/2737173002
Cr-Commit-Position: refs/heads/master@{#473089}
Committed: https://chromium.googlesource.com/chromium/src/+/cd916cc43984ff94e84fd5716c8e5d6d04416ec6
Patch Set 1 #Patch Set 2 : fix crash #Patch Set 3 : fix mac DCHECK #Patch Set 4 : rebased without changes #Patch Set 5 : only change WebViewImpl for now #Messages
Total messages: 52 (29 generated)
The CQ bit was checked by cbiesinger@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM
lgtm
The CQ bit was checked by cbiesinger@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...
This new version fixes the stack overflow the old version had. It does make CanUseNewLayout() public to do so.
The CQ bit was unchecked by cbiesinger@chromium.org
The CQ bit was checked by cbiesinger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/2737173002/#ps20001 (title: "fix crash")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
cbiesinger@chromium.org changed reviewers: + kojii@chromium.org
Emil/Koji, can you help me understand why this fails on mac (only) with the following stack trace? Does NG not support computing preferred sizes for inline nodes outside of layout? 15:35:04.580 18754 worker/4 virtual/layout_ng/css2.1/20110323/word-spacing-characters-002.htm output stderr lines: 15:35:04.580 18754 [19123:771:0316/153458.748887:966904996735:FATAL:FontCache.cpp(293)] Check failed: m_purgePreventCount. 15:35:04.580 18754 0 Content Shell Framework 0x0000000109f100dc base::debug::StackTrace::StackTrace(unsigned long) + 28 15:35:04.580 18754 1 Content Shell Framework 0x0000000109f353d3 logging::LogMessage::~LogMessage() + 67 15:35:04.580 18754 2 Content Shell Framework 0x000000010c0b6712 blink::FontCache::fontDataFromFontPlatformData(blink::FontPlatformData const*, blink::ShouldRetain, bool) + 130 15:35:04.580 18754 3 Content Shell Framework 0x000000010c0da1c4 blink::FontCache::fallbackFontForCharacter(blink::FontDescription const&, int, blink::SimpleFontData const*, blink::FontFallbackPriority) + 1220 15:35:04.581 18754 4 Content Shell Framework 0x000000010c0caf9e blink::FontFallbackIterator::uniqueSystemFontForHintList(WTF::Vector<int, 0ul, WTF::PartitionAllocator> const&) + 542 15:35:04.581 18754 5 Content Shell Framework 0x000000010c0ca418 blink::FontFallbackIterator::next(WTF::Vector<int, 0ul, WTF::PartitionAllocator> const&) + 776 15:35:04.581 18754 6 Content Shell Framework 0x000000010c0ca623 blink::FontFallbackIterator::next(WTF::Vector<int, 0ul, WTF::PartitionAllocator> const&) + 1299 15:35:04.581 18754 7 Content Shell Framework 0x000000010c0e79b2 blink::HarfBuzzShaper::shapeSegment(blink::HarfBuzzShaper::RangeData*, blink::RunSegmenter::RunSegmenterRange, blink::ShapeResult*) const + 2834 15:35:04.581 18754 8 Content Shell Framework 0x000000010c0e9088 blink::HarfBuzzShaper::shape(blink::Font const*, blink::TextDirection, unsigned int, unsigned int) const + 4888 15:35:04.581 18754 9 Content Shell Framework 0x000000010d3b57f0 blink::NGInlineNode::ShapeText() + 160 15:35:04.581 18754 10 Content Shell Framework 0x000000010d3b64c2 blink::NGInlineNode::Layout(blink::NGLineBuilder*) + 66 15:35:04.581 18754 11 Content Shell Framework 0x000000010d3b65f4 blink::NGInlineNode::ComputeMinMaxContentSize() + 228 15:35:04.581 18754 12 Content Shell Framework 0x000000010d3a4810 blink::NGBlockLayoutAlgorithm::ComputeMinMaxContentSize() const + 384 15:35:04.581 18754 13 Content Shell Framework 0x000000010d3a9382 blink::NGBlockNode::ComputeMinMaxContentSize() + 882 15:35:04.581 18754 14 Content Shell Framework 0x000000010d3a4786 blink::NGBlockLayoutAlgorithm::ComputeMinMaxContentSize() const + 246 15:35:04.581 18754 15 Content Shell Framework 0x000000010d3a9382 blink::NGBlockNode::ComputeMinMaxContentSize() + 882 15:35:04.581 18754 16 Content Shell Framework 0x000000010d3a4786 blink::NGBlockLayoutAlgorithm::ComputeMinMaxContentSize() const + 246 15:35:04.581 18754 17 Content Shell Framework 0x000000010d3a9382 blink::NGBlockNode::ComputeMinMaxContentSize() + 882 15:35:04.581 18754 18 Content Shell Framework 0x000000010d3a4786 blink::NGBlockLayoutAlgorithm::ComputeMinMaxContentSize() const + 246 15:35:04.581 18754 19 Content Shell Framework 0x000000010d3a9382 blink::NGBlockNode::ComputeMinMaxContentSize() + 882 15:35:04.581 18754 20 Content Shell Framework 0x000000010d3a0c54 blink::LayoutNGBlockFlow::computeIntrinsicLogicalWidths(blink::LayoutUnit&, blink::LayoutUnit&) const + 196 15:35:04.581 18754 21 Content Shell Framework 0x000000010d20c758 blink::LayoutBlock::computePreferredLogicalWidths() + 312 15:35:04.581 18754 22 Content Shell Framework 0x000000010d242aaf blink::LayoutBox::minPreferredLogicalWidth() const + 47 15:35:04.581 18754 23 Content Shell Framework 0x000000010d20cfbd blink::LayoutBlock::computeChildPreferredLogicalWidths(blink::LayoutObject&, blink::LayoutUnit&, blink::LayoutUnit&) const + 125 15:35:04.581 18754 24 Content Shell Framework 0x000000010d20be7a blink::LayoutBlock::computeBlockPreferredLogicalWidths(blink::LayoutUnit&, blink::LayoutUnit&) const + 1018 15:35:04.581 18754 25 Content Shell Framework 0x000000010d20b756 blink::LayoutBlock::computeIntrinsicLogicalWidths(blink::LayoutUnit&, blink::LayoutUnit&) const + 70 15:35:04.581 18754 26 Content Shell Framework 0x000000010d20c758 blink::LayoutBlock::computePreferredLogicalWidths() + 312 15:35:04.581 18754 27 Content Shell Framework 0x000000010d242aaf blink::LayoutBox::minPreferredLogicalWidth() const + 47 15:35:04.581 18754 28 Content Shell Framework 0x000000010c4028e5 blink::WebViewImpl::contentsPreferredMinimumSize() + 389 15:35:04.581 18754 29 Content Shell Framework 0x000000010e00b177 content::RenderViewImpl::CheckPreferredSize() + 55
On 2017/03/17 at 20:37:28, cbiesinger wrote: > Emil/Koji, can you help me understand why this fails on mac (only) with the following stack trace? Does NG not support computing preferred sizes for inline nodes outside of layout? I think you're hitting this AF: > ASSERT(m_purgePreventCount) and that you need to put |FontCachePurgePreventer| somewhere. Mac calls |fontDataFromFontPlatformData| with |DoNotRetain| and that somewhere in higher stack needs to prevent purging. FrameView::layout() has |FontCachePurgePreventer|, but since this is outside of layout, I guess we should put this to |WebViewImpl::contentsPreferredMinimumSize()|?
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
sgtm. Koji, does this look good? schenney, can you review the web/ patch?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Source/web/WebViewImpl.cpp non-owner lgtm from font point of view.
schenney@chromium.org changed reviewers: + schenney@chromium.org
lgtm for web/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by cbiesinger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/2737173002/#ps40001 (title: "fix mac DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Ah... another case of a mac-specific inline failure due to this. Not entirely sure what to do about that. [16030:771:0320/132220.422492:1260577955277:FATAL:SubtreeLayoutScope.cpp(39)] Check failed: m_root.document().view()->isInPerformLayout(). 0 Content Shell Framework 0x0000000104f1999c base::debug::StackTrace::StackTrace(unsigned long) + 28 1 Content Shell Framework 0x0000000104f3ec93 logging::LogMessage::~LogMessage() + 67 2 Content Shell Framework 0x000000010835c40d blink::SubtreeLayoutScope::SubtreeLayoutScope(blink::LayoutObject&) + 109 3 Content Shell Framework 0x000000010828de80 blink::LayoutFlexibleBox::layoutBlock(bool) + 368 4 Content Shell Framework 0x0000000108221226 blink::LayoutBlock::layout() + 102 5 Content Shell Framework 0x00000001083c3056 blink::NGBlockNode::RunOldLayout(blink::NGConstraintSpace const&) + 950 6 Content Shell Framework 0x00000001083c2b57 blink::NGBlockNode::Layout(blink::NGConstraintSpace*, blink::NGBreakToken*) + 87 7 Content Shell Framework 0x00000001083da45f blink::NGLineBuilder::LayoutItem(blink::NGLayoutInlineItem const&) + 527 8 Content Shell Framework 0x00000001083da180 blink::NGLineBuilder::InlineSizeFromLayout(blink::NGLayoutInlineItem const&) + 32 9 Content Shell Framework 0x00000001083d9df9 blink::NGLineBuilder::SetEnd(unsigned int) + 681 10 Content Shell Framework 0x00000001083e0e7b blink::NGTextLayoutAlgorithm::LayoutInline(blink::NGLineBuilder*) + 299 11 Content Shell Framework 0x00000001083d12ed blink::NGInlineNode::Layout(blink::NGLineBuilder*) + 109 12 Content Shell Framework 0x00000001083d13ec blink::NGInlineNode::ComputeMinMaxContentSize() + 220 13 Content Shell Framework 0x00000001083bf510 blink::NGBlockLayoutAlgorithm::ComputeMinMaxContentSize() const + 384 14 Content Shell Framework 0x00000001083c3b3c blink::NGBlockNode::ComputeMinMaxContentSize() + 876 15 Content Shell Framework 0x00000001083bf486 blink::NGBlockLayoutAlgorithm::ComputeMinMaxContentSize() const + 246 16 Content Shell Framework 0x00000001083c3b3c blink::NGBlockNode::ComputeMinMaxContentSize() + 876 17 Content Shell Framework 0x00000001083bb954 blink::LayoutNGBlockFlow::computeIntrinsicLogicalWidths(blink::LayoutUnit&, blink::LayoutUnit&) const + 196 18 Content Shell Framework 0x0000000108226b28 blink::LayoutBlock::computePreferredLogicalWidths() + 312 19 Content Shell Framework 0x000000010825d0ff blink::LayoutBox::minPreferredLogicalWidth() const + 47 20 Content Shell Framework 0x000000010822738d blink::LayoutBlock::computeChildPreferredLogicalWidths(blink::LayoutObject&, blink::LayoutUnit&, blink::LayoutUnit&) const + 125 21 Content Shell Framework 0x0000000108226244 blink::LayoutBlock::computeBlockPreferredLogicalWidths(blink::LayoutUnit&, blink::LayoutUnit&) const + 1012 22 Content Shell Framework 0x0000000108225b26 blink::LayoutBlock::computeIntrinsicLogicalWidths(blink::LayoutUnit&, blink::LayoutUnit&) const + 70 23 Content Shell Framework 0x0000000108226b28 blink::LayoutBlock::computePreferredLogicalWidths() + 312 24 Content Shell Framework 0x000000010825d0ff blink::LayoutBox::minPreferredLogicalWidth() const + 47 25 Content Shell Framework 0x000000010741a346 blink::WebViewImpl::contentsPreferredMinimumSize() + 406 26 Content Shell Framework 0x00000001090356e7 content::RenderViewImpl::CheckPreferredSize() + 55
huh, this one is hard...wondering why this is mac specific, looks like to happen in all platforms. Not very familiar with lifecycles but probably RunOldLayout() while !isInPerformLayout() isn't good? Should NGLineBuilder::InlineSizeFromLayout() switch to computePreferredLogicalWidths() if old layout?
On second thought, this should happen in NGBlockNode too. When someone calls WebViewImpl::contentsPreferredMinimumSize(), and we have NG tree, and we have reverse hosting. NGBlockNode::ComputeMinMaxContentSize() calls RunOldLayout(), no?
On 2017/03/22 06:53:49, kojii wrote: > On second thought, this should happen in NGBlockNode too. > > When someone calls WebViewImpl::contentsPreferredMinimumSize(), and we have NG > tree, and we have reverse hosting. NGBlockNode::ComputeMinMaxContentSize() calls > RunOldLayout(), no? In practice it does not call RunOldLayout because the one layout algorithm implementation we have right now always returns a value from ComputeMinMaxContentSize, so we never fall back to actually calling Layout, but that is a good point, hmm. >huh, this one is hard...wondering why this is mac specific, looks like to happen >in all platforms. I believe the code that computes preferred sizes outside of layout only runs on Mac due to some UI feature they have. https://superuser.com/posts/30541/revisions >Not very familiar with lifecycles but probably RunOldLayout() while >!isInPerformLayout() isn't good? Should NGLineBuilder::InlineSizeFromLayout() >switch to computePreferredLogicalWidths() if old layout? That may be a good suggestion... let me look into that.
Description was changed from ========== [layoutng] Override computeIntrinsicLogicalWidths so we can use the NG code for it R=ikilpatrick@chromium.org,eae@chromium.org BUG=635619 ========== to ========== [layoutng] Override computeIntrinsicLogicalWidths so we can use the NG code for it R=ikilpatrick@chromium.org,eae@chromium.org BUG=635619 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ==========
The CQ bit was checked by cbiesinger@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 ========== [layoutng] Override computeIntrinsicLogicalWidths so we can use the NG code for it R=ikilpatrick@chromium.org,eae@chromium.org BUG=635619 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ========== to ========== FontCachePurgePreventer is needed when computing MinPreferredWidth This makes little difference for now but will be relevant later for LayoutNG. ==========
I decided to just submit the WebViewImpl change for now and do the larger change later.
The CQ bit was checked by cbiesinger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org, schenney@chromium.org, kojii@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/2737173002/#ps80001 (title: "only change WebViewImpl for now")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by cbiesinger@google.com
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": 80001, "attempt_start_ts": 1495160490539410, "parent_rev": "954090a0849f84a35bc8087e4d022d51366cc18b", "commit_rev": "cd916cc43984ff94e84fd5716c8e5d6d04416ec6"}
Message was sent while issue was closed.
Description was changed from ========== FontCachePurgePreventer is needed when computing MinPreferredWidth This makes little difference for now but will be relevant later for LayoutNG. ========== to ========== FontCachePurgePreventer is needed when computing MinPreferredWidth This makes little difference for now but will be relevant later for LayoutNG. Review-Url: https://codereview.chromium.org/2737173002 Cr-Commit-Position: refs/heads/master@{#473089} Committed: https://chromium.googlesource.com/chromium/src/+/cd916cc43984ff94e84fd5716c8e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/cd916cc43984ff94e84fd5716c8e... |