Bug 96804

Summary:

Make computeLogicalHeight virtual and updateLogicalHeight non-virtual.

Product:

WebKit

Reporter:

Tony Chang <tony>

Component:

Layout and Rendering

Assignee:

Rob Buis <rbuis>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

changseok, donggwan.kim, esprehn+autocc, ews-watchlist, glenn, hyatt, kondapallykalyan, leviw, mstensho, ntim, pdr, rbuis, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

97049, 97263, 97475, 97486, 98677, 99348

Bug Blocks:

Attachments:

Description	Flags
Patch	none

Tony Chang

Reported 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.

Attachments
Patch (1.08 KB, patch) 2022-09-15 07:31 PDT, Rob Buis	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Tony Chang

Comment 1 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.

Dave Hyatt

Comment 2 2012-12-06 13:37:25 PST

Yeah, you're not going to be able to remove this. It's going to be needed.

Morten Stenshorne

Comment 3 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.

Rob Buis

Comment 4 2022-09-15 07:31:00 PDT

Created attachment 462358 [details] Patch

Tim Nguyen (:ntim)

Comment 5 2022-09-16 10:51:34 PDT

r=me

EWS

Comment 6 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].

Radar WebKit Bug Importer

Comment 7 2022-09-16 11:59:19 PDT

<rdar://problem/100036927>

Note You need to log in before you can comment on or make changes to this bug.