Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(123)

Issue 2737173002: FontCachePurgePreventer is needed when computing MinPreferredWidth (Closed)

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.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -0 lines) Patch
M third_party/WebKit/Source/web/WebViewImpl.cpp View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 52 (29 generated)
cbiesinger
3 years, 9 months ago (2017-03-08 19:21:34 UTC) #1
eae
LGTM
3 years, 9 months ago (2017-03-09 01:23:17 UTC) #6
ikilpatrick
lgtm
3 years, 9 months 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() ...
3 years, 9 months 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
3 years, 9 months ago (2017-03-16 21:26:21 UTC) #14
ikilpatrick
lgtm
3 years, 9 months 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)
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months ago (2017-03-20 17:01:18 UTC) #20
cbiesinger
sgtm. Koji, does this look good? schenney, can you review the web/ patch?
3 years, 9 months ago (2017-03-20 19:13:50 UTC) #22
kojii
Source/web/WebViewImpl.cpp non-owner lgtm from font point of view.
3 years, 9 months ago (2017-03-20 19:17:19 UTC) #24
Stephen Chennney
lgtm for web/
3 years, 9 months 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
3 years, 9 months 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)
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months ago (2017-03-22 06:53:49 UTC) #36
cbiesinger
On 2017/03/22 06:53:49, kojii wrote: > On second thought, this should happen in NGBlockNode too. ...
3 years, 9 months ago (2017-03-24 16:30:23 UTC) #37
cbiesinger
I decided to just submit the WebViewImpl change for now and do the larger change ...
3 years, 7 months ago (2017-05-18 19:20:52 UTC) #42
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/80001
3 years, 7 months ago (2017-05-18 19:22:03 UTC) #45
commit-bot: I haz the power
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_ng/builds/448939)
3 years, 7 months ago (2017-05-19 01:51:40 UTC) #47
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/80001
3 years, 7 months ago (2017-05-19 02:22:33 UTC) #49
commit-bot: I haz the power
3 years, 7 months ago (2017-05-19 04:58:06 UTC) #52
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/cd916cc43984ff94e84fd5716c8e...

Powered by Google App Engine
This is Rietveld 408576698