Cleanup nsIFrame::GetParentStyleContextFrame and related code

RESOLVED FIXED in mozilla9

Status

()

Core
Layout
--
trivial
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Mats Palmgren (vacation - back in August), Assigned: Mats Palmgren (vacation - back in August))

Tracking

unspecified
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Comment hidden (empty)
Created attachment 558804 [details] [diff] [review]
fix

Essentially this change:

-  NS_IMETHOD GetParentStyleContextFrame(nsPresContext* aPresContext,
-                                        nsIFrame**      aProviderFrame,
-                                        PRBool*         aIsChild) = 0;
+  virtual nsIFrame* GetParentStyleContextFrame(PRBool* aIsChild) = 0;
Attachment #558804 - Flags: review?(roc)
Shouldn't the failure nsresults at least be converted to assertions?
nsFrame::DoGetParentStyleContextFrame already has a NS_NOTREACHED.

I can add one in nsTableOuterFrame::GetParentStyleContextFrame if you like,
in this block:

  if (!mInnerTableFrame) {
    *aIsChild = PR_FALSE;
    return this;
  }

OTOH, I think it would be better to remove all null checks of
mInnerTableFrame and instead make nsCSSFrameConstructor::ConstructTable
return a null (outer) table frame if its inner couldn't be created for
some reason.  nsCSSFrameConstructor::CreateContinuingOuterTableFrame
already does that.
I would be fine with that.
Comment on attachment 558804 [details] [diff] [review]
fix

Review of attachment 558804 [details] [diff] [review]:
-----------------------------------------------------------------

I guess we'll have a new patch here
Attachment #558804 - Flags: review?(roc)
Summary: Outparamdel nsIFrame::GetParentStyleContextFrame → Cleanup nsIFrame::GetParentStyleContextFrame and related code
Created attachment 559141 [details] [diff] [review]
part 1, outparamdel nsIFrame::GetParentStyleContextFrame
Attachment #558804 - Attachment is obsolete: true
Attachment #559141 - Flags: review?(roc)
Created attachment 559142 [details] [diff] [review]
part 2, Make nsCSSFrameConstructor::ConstructTable return a null frame if creating the inner table frame fails

Make nsCSSFrameConstructor::ConstructTable return a null frame if
creating the inner table frame fails.  Simplify some code since we can
now depend on the invariant that a properly created outer table frame
always has a non-null inner table frame.
Attachment #559142 - Flags: review?(roc)
Created attachment 559143 [details] [diff] [review]
part1+2

Here's the full patch in case you prefer it for review.
BTW, is the nsTableOuterFrame::mInnerTableFrame really worth the space?
Wouldn't an accessor to mFrames.FirstChild() just as fast?
Do we guarantee that the first child is the inner?

We didn't use to, but maybe we do now.
We assert that it is:
http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableOuterFrame.cpp#242
except for when the given child list is empty, which is the case I'm removing
in part 2.
In that case, just nuke the member.
Comment on attachment 559141 [details] [diff] [review]
part 1, outparamdel nsIFrame::GetParentStyleContextFrame

Review of attachment 559141 [details] [diff] [review]:
-----------------------------------------------------------------

Why even bother with the aIsChild parameter? Callers can easily check whether the returned frame's GetParent() is equal to the frame we asked about.
Comment on attachment 559142 [details] [diff] [review]
part 2, Make nsCSSFrameConstructor::ConstructTable return a null frame if creating the inner table frame fails

Review of attachment 559142 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSFrameConstructor.cpp
@@ +1911,5 @@
>      innerFrame = NS_NewTableFrame(mPresShell, styleContext);
> +
> +  if (!innerFrame) {
> +    newFrame->Destroy();
> +    return NS_ERROR_OUT_OF_MEMORY;

Shouldn't frame allocation just be infallible?
> Shouldn't frame allocation just be infallible?

Ideally, yes.  But we're not there yet.  I'd love for that to happen; if allocation from presshell arenas were infallible we could simplify a lot of code...
Attachment #559142 - Flags: review?(roc) → review+
(In reply to Boris Zbarsky (:bz) from comment #12)
> In that case, just nuke the member.

Filed as bug 685901.
Created attachment 559496 [details] [diff] [review]
part 1, outparamdel nsIFrame::GetParentStyleContextFrame

Remove the aIsChild parameter too.
Attachment #559141 - Attachment is obsolete: true
Attachment #559143 - Attachment is obsolete: true
Attachment #559141 - Flags: review?(roc)
Attachment #559496 - Flags: review?(roc)
Created attachment 559497 [details] [diff] [review]
(part 1b before foding it into part1)

Here's the addendum for removing aIsChild before folding it into part 1 -
in case you want to review just that bit.
Created attachment 559498 [details] [diff] [review]
full patch
> Shouldn't frame allocation just be infallible?

As Boris said, let's attack OOM handling in the frame constructor systematically
in a specific bug for that purpose.
Attachment #559496 - Flags: review?(roc) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/38ff711d4369
http://hg.mozilla.org/integration/mozilla-inbound/rev/79890feb4abe
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
https://hg.mozilla.org/mozilla-central/rev/79890feb4abe
https://hg.mozilla.org/mozilla-central/rev/38ff711d4369
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.