Closed Bug 852501 Opened 7 years ago Closed 7 years ago

Make frame construction infallible

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(19 files)

7.53 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
12.91 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
11.32 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
32.66 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
9.59 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
4.98 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
8.57 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
12.50 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
11.83 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
15.31 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
5.18 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
8.74 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
11.41 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
40.34 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
12.91 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
3.60 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
3.71 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
13.68 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
3.78 KB, patch
Details | Diff | Splinter Review
The error-handling is all messed up already, I bet, and chances are it's not needed.
Whiteboard: [need review]
Comment on attachment 726606 [details] [diff] [review]
part 2.  Make nsFrameConstructorState::AddChild infallible.

># HG changeset patch
># User Boris Zbarsky <bzbarsky@mit.edu>
># Date 1363696606 14400
># Node ID c8712338e1819ad9cb77bb58b3386b3090b03ded
># Parent  4963cbd7dc6daf359fb1f8dc4f46a37345233489
>Bug 852501 part 2.  Make nsFrameConstructorState::AddChild infallible.  r=dholbert
>
>diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp
>--- a/layout/base/nsCSSFrameConstructor.cpp
>+++ b/layout/base/nsCSSFrameConstructor.cpp
>@@ -823,31 +823,27 @@ public:
>    * @param aStyleContext the style context resolved for aContent
>    * @param aParentFrame the parent frame for the content if it were in-flow
>    * @param aCanBePositioned pass false if the frame isn't allowed to be
>    *        positioned
>    * @param aCanBeFloated pass false if the frame isn't allowed to be
>    *        floated
>    * @param aIsOutOfFlowPopup pass true if the frame is an out-of-flow popup
>    *        (XUL-only)
>-   * @throws NS_ERROR_OUT_OF_MEMORY if it happens.
>-   * @note If this method throws, that means that aNewFrame was not inserted
>-   *       into any frame lists.  Furthermore, this method will handle cleanup
>-   *       of aNewFrame (via calling Destroy() on it).
>    */
>-  nsresult AddChild(nsIFrame* aNewFrame,
>-                    nsFrameItems& aFrameItems,
>-                    nsIContent* aContent,
>-                    nsStyleContext* aStyleContext,
>-                    nsIFrame* aParentFrame,
>-                    bool aCanBePositioned = true,
>-                    bool aCanBeFloated = true,
>-                    bool aIsOutOfFlowPopup = false,
>-                    bool aInsertAfter = false,
>-                    nsIFrame* aInsertAfterFrame = nullptr);
>+  void AddChild(nsIFrame* aNewFrame,
>+                nsFrameItems& aFrameItems,
>+                nsIContent* aContent,
>+                nsStyleContext* aStyleContext,
>+                nsIFrame* aParentFrame,
>+                bool aCanBePositioned = true,
>+                bool aCanBeFloated = true,
>+                bool aIsOutOfFlowPopup = false,
>+                bool aInsertAfter = false,
>+                nsIFrame* aInsertAfterFrame = nullptr);
> 
>   /**
>    * Function to return the fixed-pos element list.  Normally this will just hand back the
>    * fixed-pos element list, but in case we're dealing with a transformed element that's
>    * acting as an abs-pos and fixed-pos container, we'll hand back the abs-pos list.  Callers should
>    * use this function if they want to get the list acting as the fixed-pos item parent.
>    */
>   nsAbsoluteItems& GetFixedItems()
>@@ -1117,17 +1113,17 @@ nsFrameConstructorState::GetGeometricPar
>   if (aStyleDisplay->mPosition == NS_STYLE_POSITION_FIXED &&
>       GetFixedItems().containingBlock) {
>     return GetFixedItems().containingBlock;
>   }
> 
>   return aContentParentFrame;
> }
> 
>-nsresult
>+void
> nsFrameConstructorState::AddChild(nsIFrame* aNewFrame,
>                                   nsFrameItems& aFrameItems,
>                                   nsIContent* aContent,
>                                   nsStyleContext* aStyleContext,
>                                   nsIFrame* aParentFrame,
>                                   bool aCanBePositioned,
>                                   bool aCanBeFloated,
>                                   bool aIsOutOfFlowPopup,
>@@ -1205,18 +1201,16 @@ nsFrameConstructorState::AddChild(nsIFra
>   }
> #endif
> 
>   if (aInsertAfter) {
>     frameItems->InsertFrame(nullptr, aInsertAfterFrame, aNewFrame);
>   } else {
>     frameItems->AddChild(aNewFrame);
>   }
>-  
>-  return NS_OK;
> }
> 
> void
> nsFrameConstructorState::ProcessFrameInsertions(nsAbsoluteItems& aFrameItems,
>                                                 ChildListID aChildListID)
> {
> #define NS_NONXUL_LIST_TEST (&aFrameItems == &mFloatedItems &&            \
>                              aChildListID == nsIFrame::kFloatList)    ||  \
>@@ -1945,21 +1939,17 @@ nsCSSFrameConstructor::ConstructTable(ns
>   else
>     innerFrame = NS_NewTableFrame(mPresShell, styleContext);
> 
>   InitAndRestoreFrame(aState, content, newFrame, nullptr, innerFrame);
> 
>   // Put the newly created frames into the right child list
>   SetInitialSingleChild(newFrame, innerFrame);
> 
>-  rv = aState.AddChild(newFrame, aFrameItems, content, styleContext,
>-                       aParentFrame);
>-  if (NS_FAILED(rv)) {
>-    return rv;
>-  }
>+  aState.AddChild(newFrame, aFrameItems, content, styleContext, aParentFrame);
> 
>   if (!mRootElementFrame) {
>     // The frame we're constructing will be the root element frame.
>     // Set mRootElementFrame before processing children.
>     mRootElementFrame = newFrame;
>   }
> 
>   nsFrameItems childItems;
>@@ -2950,21 +2940,18 @@ nsCSSFrameConstructor::ConstructSelectFr
>       // since the complete tree is required before we restore.
>       nsILayoutHistoryState *historyState = aState.mFrameState;
>       aState.mFrameState = nullptr;
>       // Initialize the combobox frame
>       InitAndRestoreFrame(aState, content,
>                           aState.GetGeometricParent(aStyleDisplay, aParentFrame),
>                           nullptr, comboboxFrame);
> 
>-      rv = aState.AddChild(comboboxFrame, aFrameItems, content, styleContext,
>-                           aParentFrame);
>-      if (NS_FAILED(rv)) {
>-        return rv;
>-      }
>+      aState.AddChild(comboboxFrame, aFrameItems, content, styleContext,
>+                      aParentFrame);
>       
>       nsIComboboxControlFrame* comboBox = do_QueryFrame(comboboxFrame);
>       NS_ASSERTION(comboBox, "NS_NewComboboxControlFrame returned frame that "
>                              "doesn't implement nsIComboboxControlFrame");
> 
>         // Resolve pseudo element style for the dropdown list
>       nsRefPtr<nsStyleContext> listStyle;
>       listStyle = mPresShell->StyleSet()->
>@@ -3064,21 +3051,18 @@ nsCSSFrameConstructor::InitializeSelectF
>   // We don't call InitAndRestoreFrame for scrollFrame because we can only
>   // restore the frame state after its parts have been created (in particular,
>   // the scrollable view). So we have to split Init and Restore.
> 
>   // Initialize the frame
>   scrollFrame->Init(aContent, geometricParent, nullptr);
> 
>   if (!aBuildCombobox) {
>-    nsresult rv = aState.AddChild(scrollFrame, aFrameItems, aContent,
>-                                  aStyleContext, aParentFrame);
>-    if (NS_FAILED(rv)) {
>-      return rv;
>-    }
>+    aState.AddChild(scrollFrame, aFrameItems, aContent,
>+                    aStyleContext, aParentFrame);
>   }
>       
>   if (aBuildCombobox) {
>     nsContainerFrame::CreateViewForFrame(scrollFrame, true);
>   }
> 
>   BuildScrollFrame(aState, aContent, aStyleContext, scrolledFrame,
>                    geometricParent, scrollFrame);
>@@ -3122,21 +3106,17 @@ nsCSSFrameConstructor::ConstructFieldSet
>   fieldsetContentStyle = mPresShell->StyleSet()->
>     ResolveAnonymousBoxStyle(nsCSSAnonBoxes::fieldsetContent, styleContext);
> 
>   nsIFrame* blockFrame = NS_NewBlockFrame(mPresShell, fieldsetContentStyle,
>                                           NS_BLOCK_FLOAT_MGR |
>                                           NS_BLOCK_MARGIN_ROOT);
>   InitAndRestoreFrame(aState, content, newFrame, nullptr, blockFrame);
> 
>-  nsresult rv = aState.AddChild(newFrame, aFrameItems, content, styleContext,
>-                                aParentFrame);
>-  if (NS_FAILED(rv)) {
>-    return rv;
>-  }
>+  aState.AddChild(newFrame, aFrameItems, content, styleContext, aParentFrame);
>   
>   // Process children
>   nsFrameConstructorSaveState absoluteSaveState;
>   nsFrameItems                childItems;
> 
>   newFrame->AddStateBits(NS_FRAME_CAN_HAVE_ABSPOS_CHILDREN);
>   if (newFrame->IsPositioned()) {
>     aState.PushAbsoluteContainingBlock(newFrame, absoluteSaveState);
>@@ -3695,21 +3675,18 @@ nsCSSFrameConstructor::ConstructFrameFro
>         maybeAbsoluteContainingBlockDisplay = blockDisplay;
>         maybeAbsoluteContainingBlock = blockFrame;
>       }
>       
>       // Our kids should go into the blockFrame
>       newFrame = blockFrame;
>     }
> 
>-    rv = aState.AddChild(frameToAddToList, aFrameItems, content, styleContext,
>-                         aParentFrame, allowOutOfFlow, allowOutOfFlow, isPopup);
>-    if (NS_FAILED(rv)) {
>-      return rv;
>-    }
>+    aState.AddChild(frameToAddToList, aFrameItems, content, styleContext,
>+                    aParentFrame, allowOutOfFlow, allowOutOfFlow, isPopup);
> 
> #ifdef MOZ_XUL
>     // Icky XUL stuff, sadly
> 
>     if (aItem.mIsRootPopupgroup) {
>       NS_ASSERTION(nsIRootBox::GetRootBox(mPresShell) &&
>                    nsIRootBox::GetRootBox(mPresShell)->GetPopupSetFrame() ==
>                      newFrame,
>@@ -4479,18 +4456,17 @@ nsCSSFrameConstructor::ConstructScrollab
>     // XXXbz any cleanup needed here?
>     return rv;
>   }
> 
>   NS_ASSERTION(blockItem.FirstChild() == scrolledFrame,
>                "Scrollframe's frameItems should be exactly the scrolled frame");
>   FinishBuildingScrollFrame(*aNewFrame, scrolledFrame);
> 
>-  rv = aState.AddChild(*aNewFrame, aFrameItems, content, styleContext,
>-                       aParentFrame);
>+  aState.AddChild(*aNewFrame, aFrameItems, content, styleContext, aParentFrame);
>   return rv;
> }
> 
> nsresult
> nsCSSFrameConstructor::ConstructNonScrollableBlock(nsFrameConstructorState& aState,
>                                                    FrameConstructionItem&   aItem,
>                                                    nsIFrame*                aParentFrame,
>                                                    const nsStyleDisplay*    aDisplay,
>@@ -4759,21 +4735,17 @@ nsCSSFrameConstructor::ConstructOuterSVG
>   // Create the anonymous inner wrapper frame
>   nsIFrame* innerFrame = NS_NewSVGOuterSVGAnonChildFrame(mPresShell, scForAnon);
> 
>   InitAndRestoreFrame(aState, content, newFrame, nullptr, innerFrame);
> 
>   // Put the newly created frames into the right child list
>   SetInitialSingleChild(newFrame, innerFrame);
> 
>-  rv = aState.AddChild(newFrame, aFrameItems, content, styleContext,
>-                       aParentFrame);
>-  if (NS_FAILED(rv)) {
>-    return rv;
>-  }
>+  aState.AddChild(newFrame, aFrameItems, content, styleContext, aParentFrame);
> 
>   if (!mRootElementFrame) {
>     // The frame we're constructing will be the root element frame.
>     // Set mRootElementFrame before processing children.
>     mRootElementFrame = newFrame;
>   }
> 
>   nsFrameItems childItems;
>@@ -10594,26 +10566,22 @@ nsCSSFrameConstructor::CreateFloatingLet
>   // Put the new float before any of the floats in the block we're doing
>   // first-letter for, that is, before any floats whose parent is
>   // containingBlock.
>   nsFrameList::FrameLinkEnumerator link(aState.mFloatedItems);
>   while (!link.AtEnd() && link.NextFrame()->GetParent() != containingBlock) {
>     link.Next();
>   }
> 
>-  rv = aState.AddChild(letterFrame, aResult, letterContent, aStyleContext,
>-                       aParentFrame, false, true, false, true,
>-                       link.PrevFrame());
>+  aState.AddChild(letterFrame, aResult, letterContent, aStyleContext,
>+                  aParentFrame, false, true, false, true,
>+                  link.PrevFrame());
> 
>   if (nextTextFrame) {
>-    if (NS_FAILED(rv)) {
>-      nextTextFrame->Destroy();
>-    } else {
>-      aResult.AddChild(nextTextFrame);
>-    }
>+    aResult.AddChild(nextTextFrame);
>   }
> }
> 
> /**
>  * Create a new letter frame for aTextFrame. The letter frame will be
>  * a child of aParentFrame.
>  */
> nsresult
>@@ -11145,24 +11113,19 @@ nsCSSFrameConstructor::ConstructBlock(ns
>     *aNewFrame = columnSetFrame;
> 
>     SetInitialSingleChild(columnSetFrame, blockFrame);
>   }
> 
>   blockFrame->SetStyleContextWithoutNotification(blockStyle);
>   InitAndRestoreFrame(aState, aContent, parent, nullptr, blockFrame);
> 
>-  nsresult rv = aState.AddChild(*aNewFrame, aFrameItems, aContent,
>-                                aStyleContext,
>-                                aContentParentFrame ? aContentParentFrame :
>-                                                      aParentFrame);
>-  if (NS_FAILED(rv)) {
>-    return rv;
>-  }
>-
>+  aState.AddChild(*aNewFrame, aFrameItems, aContent, aStyleContext,
>+                  aContentParentFrame ? aContentParentFrame :
>+                                        aParentFrame);
>   if (!mRootElementFrame) {
>     // The frame we're constructing will be the root element frame.
>     // Set mRootElementFrame before processing children.
>     mRootElementFrame = *aNewFrame;
>   }
> 
>   // We should make the outer frame be the absolute containing block,
>   // if one is required. We have to do this because absolute
>@@ -11175,16 +11138,17 @@ nsCSSFrameConstructor::ConstructBlock(ns>   // Process the child content
>   nsFrameItems childItems;
>+  nsresult rv;
>   rv = ProcessChildren(aState, aContent, aStyleContext, blockFrame, true,
>                        childItems, true, aPendingBinding);

Nit: this part ends up looking a little weird. (declaring rv uninitialized, and then initializing it on the next line)

Maybe change to:
  nsresult rv =
    ProcessChildren(...
but it's not a huge deal; r=me either way.
Attachment #726606 - Flags: review?(dholbert) → review+
Yikes, meant to delete all but that one last chunk of the quoted patch.  Sorry for unnecessarily contributing to the tallness of this bug. :)
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
> Nit: this part ends up looking a little weird.

Yeah, I was trying to keep the changes minimal.

That rv goes away in part 10, so I'm just as happy not messing with it too much.  ;)
Comment on attachment 726608 [details] [diff] [review]
part 4.  Make CreateContinuingFrame infallible.

>+nsIFrame*
> nsCSSFrameConstructor::CreateContinuingOuterTableFrame(nsIPresShell*    aPresShell,
>                                                        nsPresContext*  aPresContext,
>                                                        nsIFrame*        aFrame,
>                                                        nsIFrame*        aParentFrame,
>                                                        nsIContent*      aContent,
>-                                                       nsStyleContext*  aStyleContext,
>-                                                       nsIFrame**       aContinuingFrame)
>+                                                       nsStyleContext*  aStyleContext)
> {
[...]
>+    NS_RUNTIMEABORT("unexpected frame type");
>+    return nullptr;
>   }

I'm not sure it's useful to have the "return nullptr" there, since:
 (a) it won't ever be executed, so it's slightly deceptive to have it there.
 (b) it disagrees with the "This method never returns null." comment that you added in the header.

I'd probably prefer just the NS_RUNTIMEABORT w/ no explicit return.

If you'd like to keep the return statement, though, it'd probably be worth annotating it with a "// will never actually be executed" comment or something.
Attachment #726608 - Flags: review?(dholbert) → review+
Comment on attachment 726609 [details] [diff] [review]
part 5.  Make InitAndRestoreFrame infallible.

Observation: It looks like every caller to InitAndRestoreFrame passes nullptr as the "aPrevInFlow" argument. Perhaps we should drop that argument (in a separate bug)?

r=me on this, anyway.
Attachment #726609 - Flags: review?(dholbert) → review+
> I'd probably prefer just the NS_RUNTIMEABORT w/ no explicit return.

Done.

> Observation: It looks like every caller to InitAndRestoreFrame passes nullptr 

Nice catch.  Filed bug 852636.
Blocks: 852636
Comment on attachment 726684 [details] [diff] [review]
part 9.  Optimistically make ConstructFramesFromItemList infallible in the hope that this is the only thing that makes other things fallible.

>@@ -9897,23 +9897,23 @@ nsCSSFrameConstructor::ConstructFramesFr
>   for (FCItemIterator iter(aItems); !iter.IsDone(); iter.Next()) {
>     NS_ASSERTION(iter.item().DesiredParentType() == GetParentType(aParentFrame),
>                  "Needed pseudos didn't get created; expect bad things");
>   }
> #endif
> 
>   for (FCItemIterator iter(aItems); !iter.IsDone(); iter.Next()) {
>     nsresult rv = ConstructFramesFromItem(aState, iter, aParentFrame, aFrameItems);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    if (NS_FAILED(rv)) {
>+      NS_RUNTIMEABORT("Frame construction failure");
>+    }

This is a slightly scary change, but it looks like the abort gets removed in a later patch (part 15), so it looks like it should be OK.
Attachment #726684 - Flags: review?(dholbert) → review+
> This is a slightly scary change, but it looks like the abort gets removed in a later
> patch

Yep.  The idea was to break the dependency loop in an incremental way so you don't have to review a mega-patch that removes nsresult from all over the place at once.  ;)  And yeah, in the end ConstructFramesFromItem ends up infallible so the abort can go away.
(In reply to Boris Zbarsky (:bz) from comment #28)
> Yep.  The idea was to break the dependency loop in an incremental way so you
> don't have to review a mega-patch that removes nsresult from all over the
> place at once.  ;)

(very much appreciated, BTW. :) )
Comment on attachment 726689 [details] [diff] [review]
part 13.  Clean up ConstructSelectFrame a bit to get rid of unreachable codepaths.

r+ on part 13, based on the diff -w version (attachment 726699 [details] [diff] [review]) -- thanks for that.
Attachment #726689 - Flags: review?(dholbert) → review+
Comment on attachment 726695 [details] [diff] [review]
part 18.  Make ConstructDocElementFrame infallible.

>Bug 852501 part 18.  Make ConstructDocElementFrame infallible.  r=dholbert

Commit message nit: s/infallible/return a nsIFrame*/, probalby.

Because: for a function that returns a pointer to a constructed object, "infallible" indicates "guaranteed not to return null", at least to me. And in this case, ConstructDocElementFrame can still definitely return null -- it's got several explicit "return nullptr;" statements.

>@@ -2405,22 +2404,20 @@ nsCSSFrameConstructor::ConstructDocEleme
>       nsFrameItems frameItems;
>       contentFrame = ConstructOuterSVG(state, item, mDocElementContainingBlock,
>                                        styleContext->StyleDisplay(),
>                                        frameItems);
>-      if (!contentFrame || frameItems.IsEmpty())
>-        return NS_ERROR_FAILURE;
>-      *aNewFrame = frameItems.FirstChild();
>+      newFrame = frameItems.FirstChild();
>     } else {
>-      return NS_ERROR_FAILURE;
>+      return nullptr;

I think this 'else' case only gets visited when "aDocElement->IsSVG()" is true and "aDocElement->Tag() == nsGkAtoms::svg" is false.  I bet we don't ever expect that to happen, right...?  We should probably add a NS_ERROR here, to indicate that this is unexpected, so we'll notice if this gets visited.

(I know you're trying to keep these patches minimal, but I think this assertion-addition would be useful doing in this patch  (assuming you agree that this is an assert-worthy condition), because otherwise the patch converts an explicitly "error"-flavored rv to a more generic rv. A NS_ERROR would preserve the error-flavored-ness of this code.)

>+++ b/layout/base/nsCSSFrameConstructor.h
[...]
>+  // Construct the frames for the document element.  This can return
>+  // null if the document element is display:none.
>+  nsIFrame* ConstructDocElementFrame(Element*                 aDocElement,
>+                                     nsILayoutHistoryState*   aFrameState);

Note: It can also return null if the document element uses an XBL binding that's not yet available, or if we hit that probably-assert-worthy SVG-doc-but-no-SVG-element case. Maybe worth mentioning those here, too? (at least the XBL one)

r=me with the above addressed
Attachment #726695 - Flags: review?(dholbert) → review+
> Commit message nit: s/infallible/return a nsIFrame*/, probalby.

Done.

> I think this 'else' case only gets visited when "aDocElement->IsSVG()" is true and
> "aDocElement->Tag() == nsGkAtoms::svg" is false.

Yep.

> I bet we don't ever expect that to happen, right...?

Well, not commonly, but it's trivial to create such a document:

  data:text/xml,<foopy xmlns="http://www.w3.org/2000/svg"/>

so I'm not sure NS_ERROR is right for this situation at all.

> Maybe worth mentioning those here, too?

Done:

  // Construct the frames for the document element.  This can return null if the
  // document element is display:none, or if the document element has a
  // not-yet-loaded XBL binding, or if it's an SVG element that's not <svg>.
(In reply to Boris Zbarsky (:bz) from comment #32)
> Well, not commonly, but it's trivial to create such a document:
> 
>   data:text/xml,<foopy xmlns="http://www.w3.org/2000/svg"/>
> 
> so I'm not sure NS_ERROR is right for this situation at all.

Ah -- agreed, thanks for clarifying.  Might be worth adding a comment to that clause to briefly mention that possibility.  But your new header-comment covers it too, so this is fine w/ just that too.
Comment on attachment 726691 [details] [diff] [review]
part 14.  Make FrameFullConstructor implementations infallible.

>@@ -3557,23 +3534,19 @@ nsCSSFrameConstructor::ConstructFrameFro
>   if (bits & FCDATA_FUNC_IS_FULL_CTOR) {
>-    nsresult rv =
>+    newFrame =
>       (this->*(data->mFullConstructor))(aState, aItem, aParentFrame,
>-                                        display, aFrameItems, &newFrame);
>-    if (NS_FAILED(rv)) {
>-      return rv;
>-    }
>-
>+                                        display, aFrameItems);
>     primaryFrame = newFrame;

Could we assert that newFrame is non-null here? (since we're removing a failure-handling case, and since we're calling the functions invoked at this point "infallible")

Or, maybe we already effectively end up asserting that elsewhere. If so, never mind.

>-     @param aFrame out param handing out the frame that was constructed.  This
>-                   frame is what the caller will set as the frame on the content.
>+     @return the frame that was constructed.  This frame is what the caller
>+             will set as the frame on the content.
>   */
>-  typedef nsresult
>+  typedef nsIFrame*
>     (nsCSSFrameConstructor::* FrameFullConstructor)(nsFrameConstructorState& aState,

Might be worth adding "Guaranteed to be non-null" or something to that effect on the @return line there.

r=me regardless
Attachment #726691 - Flags: review?(dholbert) → review+
> Could we assert that newFrame is non-null here? 
..
> Might be worth adding "Guaranteed to be non-null" 

Both done.
\o/
Blocks: 427099
Depends on: 959311
You need to log in before you can comment on or make changes to this bug.