Bug 96804 - Make computeLogicalHeight virtual and updateLogicalHeight non-virtual.
Summary: Make computeLogicalHeight virtual and updateLogicalHeight non-virtual.
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
Keywords: InRadar
Depends on: 97049 97263 97475 97486 98677 99348
  Show dependency treegraph
Reported: 2012-09-14 10:17 PDT by Tony Chang
Modified: 2022-09-16 11:59 PDT (History)
13 users (show)

See Also:

Patch (1.08 KB, patch)
2022-09-15 07:31 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2012-09-14 10:17:33 PDT
In bug 94982, the code for computeLogicalHeight was made const and put int a non-virtual function.  This got rid of some hacks where we would store the old height, call computeLogicalHeight, then restore the old height.

However, in the new code, it's hard to use computeLogicalHeight because it's not virtual so if you call it on a pointer and your pointer overrides updateLogicalHeight (e.g., it's an iframe), it'll do the wrong thing.  See RenderBlock::computeBlockPreferredLogicalWidths for an example of where we would like to use childBox->computeLogicalHeight(..), but it crashes because sometimes childBox is a RenderView, which overrides updateLogicalHeight().

I think long term, we want to make computeLogicalHeight virtual and move code from the subclass updateLogicalHeight into it and then make updateLogicalHeight non-virtual.
Comment 1 Tony Chang 2012-10-15 13:57:28 PDT
computeLogicalHeight is now virtual, however, I can't make updateLogicalHeight virtual because of RenderMultiColumnSet, which sets a member variable during updateLogicalHeight.

It looks like that variable is the same as logicalHeight right now, but the comments suggest that there is a future use for it.  I'd like to remove RenderMultiColumnSet::m_computedColumnHeight.
Comment 2 Dave Hyatt 2012-12-06 13:37:25 PST
Yeah, you're not going to be able to remove this. It's going to be needed.
Comment 3 Morten Stenshorne 2013-05-22 01:39:28 PDT
FYI: the RenderMultiColumnSet::updateLogicalHeight() override is going away with the fix for https://bugs.webkit.org/show_bug.cgi?id=116033 , but in the meantime (after this bug was reported) RenderRegion::updateLogicalHeight() has appeared.
Comment 4 Rob Buis 2022-09-15 07:31:00 PDT
Created attachment 462358 [details]
Comment 5 Tim Nguyen (:ntim) 2022-09-16 10:51:34 PDT
Comment 6 EWS 2022-09-16 11:58:03 PDT
Committed 254564@main (c096c71bf576): <https://commits.webkit.org/254564@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 462358 [details].
Comment 7 Radar WebKit Bug Importer 2022-09-16 11:59:19 PDT