Summary: | [New Multicolumn] Table cells and list items need to work as multicolumn blocks | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dave Hyatt <hyatt> | ||||||
Component: | Layout and Rendering | Assignee: | Dave Hyatt <hyatt> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, esprehn+autocc, glenn, kondapallykalyan | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Dave Hyatt
2014-01-21 12:46:26 PST
Created attachment 221778 [details]
Patch
Created attachment 221783 [details]
Patch
Attachment 221783 [details] did not pass style-queue:
ERROR: Source/WebCore/rendering/RenderMultiColumnFlowThread.h:64: One space before end of line comments [whitespace/comments] [5]
ERROR: Source/WebCore/rendering/RenderMultiColumnFlowThread.h:66: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 2 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 221783 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221783&action=review > Source/WebCore/rendering/RenderMultiColumnBlock.cpp:43 > + RenderMultiColumnFlowThread* flowThread = new RenderMultiColumnFlowThread(document(), RenderStyle::createAnonymousStyleWithDisplay(&style(), BLOCK)); > + flowThread->initializeStyle(); > + RenderBlockFlow::addChild(flowThread); > + setMultiColumnFlowThread(flowThread); Trying to work out the lifetime / ownership of the flowThread. We create it here, and set it as both a child and as a member of the RenderBlockFlow rareData. Does that mean it gets deleted as a result of the RenderBlockFlow's normal child deleting and we assume that corresponds with the rare data being torn down so we don't worry about the dangling pointer? (In reply to comment #4) > (From update of attachment 221783 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221783&action=review > > > Source/WebCore/rendering/RenderMultiColumnBlock.cpp:43 > > + RenderMultiColumnFlowThread* flowThread = new RenderMultiColumnFlowThread(document(), RenderStyle::createAnonymousStyleWithDisplay(&style(), BLOCK)); > > + flowThread->initializeStyle(); > > + RenderBlockFlow::addChild(flowThread); > > + setMultiColumnFlowThread(flowThread); > > Trying to work out the lifetime / ownership of the flowThread. We create it here, and set it as both a child and as a member of the RenderBlockFlow rareData. Does that mean it gets deleted as a result of the RenderBlockFlow's normal child deleting and we assume that corresponds with the rare data being torn down so we don't worry about the dangling pointer? Correct. It's torn down by anonymous child cleanup. Note I didn't change the ownership. I just moved it from one class to another. Comment on attachment 221783 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221783&action=review > Source/WebCore/rendering/RenderBlockFlow.cpp:2895 > + RenderBlockFlowRareData& rareData = ensureRareBlockFlowData(); Can flowThread be null and thus not require the creation of rareData? > Source/WebCore/rendering/RenderBlockFlow.h:154 > + RenderMultiColumnFlowThread* m_multiColumnFlowThread; Could be a reference? There are lots of unchecked dereferences. > Source/WebCore/rendering/RenderMultiColumnBlock.cpp:95 > + multiColumnFlowThread()->setInBalancingPass(true); // Prevent re-entering this method (and recursion into layout). This should use TemporaryChange<> or some other early-return-proof class. > Source/WebCore/rendering/RenderMultiColumnFlowThread.h:48 > + bool computeColumnCountAndWidth(); Add a comment to say what the return values means. > Source/WebCore/rendering/RenderMultiColumnFlowThread.h:65 > + LayoutUnit m_columnWidth; // since a multi-column block that is split across variable width pages or regions will have different column counts and widths in each. since a -> A > Source/WebCore/rendering/RenderMultiColumnFlowThread.h:68 > + bool m_inBalancingPass; // Set when relayouting for column balancing. Comment doesn't add anything. |