Chromium Code Reviews
Help | Chromium Project | Sign in
(4)

Issue 2737173002: [layoutng] Override computeIntrinsicLogicalWidths so we can use the NG code for it

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 2 weeks ago by cbiesinger
Modified:
1 month 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.

Description

[layoutng] Override computeIntrinsicLogicalWidths so we can use the NG code for it R=ikilpatrick@chromium.org,eae@chromium.org BUG=635619

Patch Set 1 #

Patch Set 2 : fix crash #

Patch Set 3 : fix mac DCHECK #

Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -1 line) Patch
M third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.h View 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc View 1 2 chunks +17 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_block_node.h View 1 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/web/WebViewImpl.cpp View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 37 (19 generated)
cbiesinger
1 month, 2 weeks ago (2017-03-08 19:21:34 UTC) #1
eae
LGTM
1 month, 2 weeks ago (2017-03-09 01:23:17 UTC) #6
ikilpatrick
lgtm
1 month, 2 weeks ago (2017-03-09 21:49:50 UTC) #7
cbiesinger
This new version fixes the stack overflow the old version had. It does make CanUseNewLayout() ...
1 month, 1 week ago (2017-03-16 21:25:42 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2737173002/20001
1 month, 1 week ago (2017-03-16 21:26:21 UTC) #14
ikilpatrick
lgtm
1 month, 1 week ago (2017-03-16 21:28:39 UTC) #15
commit-bot: I haz the power
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_ng/builds/408794)
1 month, 1 week ago (2017-03-16 23:21:53 UTC) #17
cbiesinger
Emil/Koji, can you help me understand why this fails on mac (only) with the following ...
1 month, 1 week ago (2017-03-17 20:37:28 UTC) #19
kojii
On 2017/03/17 at 20:37:28, cbiesinger wrote: > Emil/Koji, can you help me understand why this ...
1 month ago (2017-03-20 17:01:18 UTC) #20
cbiesinger
sgtm. Koji, does this look good? schenney, can you review the web/ patch?
1 month ago (2017-03-20 19:13:50 UTC) #22
kojii
Source/web/WebViewImpl.cpp non-owner lgtm from font point of view.
1 month ago (2017-03-20 19:17:19 UTC) #24
Stephen Chennney
lgtm for web/
1 month ago (2017-03-20 19:19:55 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2737173002/40001
1 month ago (2017-03-20 19:28:24 UTC) #31
commit-bot: I haz the power
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_ng/builds/410538)
1 month ago (2017-03-20 21:07:47 UTC) #33
cbiesinger
Ah... another case of a mac-specific inline failure due to this. Not entirely sure what ...
1 month ago (2017-03-21 19:12:22 UTC) #34
kojii
huh, this one is hard...wondering why this is mac specific, looks like to happen in ...
1 month ago (2017-03-22 04:32:45 UTC) #35
kojii
On second thought, this should happen in NGBlockNode too. When someone calls WebViewImpl::contentsPreferredMinimumSize(), and we ...
1 month ago (2017-03-22 06:53:49 UTC) #36
cbiesinger
1 month ago (2017-03-24 16:30:23 UTC) #37
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46