Bug 22122

Summary:

Use a stack-based object to make pushLayoutState() code safer

Product:

WebKit

Reporter:

Simon Fraser (smfr) <simon.fraser>

Component:

Layout and Rendering

Assignee:

Simon Fraser (smfr) <simon.fraser>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

hyatt, mitz

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	none
Patch with explicit push(), allowing use in RenderTableSection	mitz: review+
Patch with scope, rather than explicit pop()	none

Simon Fraser (smfr)

Reported 2008-11-06 20:50:39 PST

6 different renderer classes have code that does: view()->pushLayoutState(this,...) ... view()->popLayoutState(); or, in some cases: if (some condition) view()->pushLayoutState(this,...) else view()->disableLayoutState(); ... if (some condition which we hope is the same as before) view()->popLayoutState(); else view()->enableLayoutState(); There is only one place where we assert that the push/pops matched, and that's at the outer level. This is crying out for a stack-based class.

Attachments
Patch (8.38 KB, patch) 2008-11-06 20:57 PST, Simon Fraser (smfr)	no flags	Details Formatted Diff Diff
Patch with explicit push(), allowing use in RenderTableSection (11.64 KB, patch) 2008-11-06 21:34 PST, Simon Fraser (smfr)	mitz: review+	Details Formatted Diff Diff
Patch with scope, rather than explicit pop() (13.56 KB, patch) 2008-11-06 21:41 PST, Simon Fraser (smfr)	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Simon Fraser (smfr)

Comment 1 2008-11-06 20:57:05 PST

Created attachment 24961 [details] Patch

Simon Fraser (smfr)

Comment 2 2008-11-06 20:57:51 PST

LayoutStateMaintainer could use a better name. LayoutStateStack (but it's not actually the stack)?

mitz

Comment 3 2008-11-06 21:14:24 PST

Comment on attachment 24961 [details] Patch + RenderView* m_view; + + bool m_push; + bool m_popped; There should be a single space between type and variable name. I like the idea of a stack-based object, but I think it would be better to go all the way and instead of explicit pop (and push) add scope around where you want the state pushed/disabled. I'm interested in Hyatt's opinion as well :-)

Simon Fraser (smfr)

Comment 4 2008-11-06 21:34:09 PST

Created attachment 24963 [details] Patch with explicit push(), allowing use in RenderTableSection

Simon Fraser (smfr)

Comment 5 2008-11-06 21:41:41 PST

Created attachment 24964 [details] Patch with scope, rather than explicit pop()

Simon Fraser (smfr)

Comment 6 2008-11-06 21:42:29 PST

The second patch gets a bit messy; have to move local variables outside the scope. I prefer the explicit pop() version.

Simon Fraser (smfr)

Comment 7 2008-11-06 21:43:12 PST

Note that second patch is with --ignore-whitespace, to avoid showing all the indentation.

Simon Fraser (smfr)

Comment 8 2008-11-07 10:20:25 PST

Comment on attachment 24964 [details] Patch with scope, rather than explicit pop() hyatt doesn't like this one.

mitz

Comment 9 2008-11-07 11:13:08 PST

Comment on attachment 24963 [details] Patch with explicit push(), allowing use in RenderTableSection > +}; > + > + > } // namespace WebCore > > #endif // RenderView_h Extra newline up there. r=me

Simon Fraser (smfr)

Comment 10 2008-11-07 11:17:40 PST

Committed r38227 M WebCore/rendering/RenderFlexibleBox.cpp M WebCore/rendering/RenderTableRow.cpp M WebCore/rendering/RenderBlock.cpp M WebCore/rendering/RenderTableSection.cpp M WebCore/rendering/RenderContainer.cpp M WebCore/rendering/RenderTable.cpp M WebCore/rendering/RenderView.h M WebCore/ChangeLog r38227 = feaef2ebff2eea5fc2b9d46149a82e020dc88477 (trunk)

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