Bug 22122 - Use a stack-based object to make pushLayoutState() code safer
Summary: Use a stack-based object to make pushLayoutState() code safer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-06 20:50 PST by Simon Fraser (smfr)
Modified: 2008-11-07 11:17 PST (History)
2 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 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.
Comment 1 Simon Fraser (smfr) 2008-11-06 20:57:05 PST
Created attachment 24961 [details]
Patch
Comment 2 Simon Fraser (smfr) 2008-11-06 20:57:51 PST
LayoutStateMaintainer could use a better name. LayoutStateStack (but it's not actually the stack)?
Comment 3 mitz 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 :-)
Comment 4 Simon Fraser (smfr) 2008-11-06 21:34:09 PST
Created attachment 24963 [details]
Patch with explicit push(), allowing use in RenderTableSection
Comment 5 Simon Fraser (smfr) 2008-11-06 21:41:41 PST
Created attachment 24964 [details]
Patch with scope, rather than explicit pop()
Comment 6 Simon Fraser (smfr) 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.
Comment 7 Simon Fraser (smfr) 2008-11-06 21:43:12 PST
Note that second patch is with --ignore-whitespace, to avoid showing all the indentation.
Comment 8 Simon Fraser (smfr) 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.
Comment 9 mitz 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
Comment 10 Simon Fraser (smfr) 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)