685154 - Cleanup nsIFrame::GetParentStyleContextFrame and related code

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

Essentially this change:

-  NS_IMETHOD GetParentStyleContextFrame(nsPresContext* aPresContext,
-                                        nsIFrame**      aProviderFrame,
-                                        PRBool*         aIsChild) = 0;
+  virtual nsIFrame* GetParentStyleContextFrame(PRBool* aIsChild) = 0;

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Shouldn't the failure nsresults at least be converted to assertions?

Comment 3

[Mats Palmgren (inactive)]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I would be fine with that.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 558804 [details] [diff] [review]
fix

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

I guess we'll have a new patch here

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 1, outparamdel nsIFrame::GetParentStyleContextFrame

Comment 7

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 8

[Mats Palmgren (inactive)]

Attachment: part1+2

Here's the full patch in case you prefer it for review.

Comment 9

[Mats Palmgren (inactive)]

BTW, is the nsTableOuterFrame::mInnerTableFrame really worth the space?
Wouldn't an accessor to mFrames.FirstChild() just as fast?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Do we guarantee that the first child is the inner?

We didn't use to, but maybe we do now.

Comment 11

[Mats Palmgren (inactive)]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

In that case, just nuke the member.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 16

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #12)
> In that case, just nuke the member.

Filed as bug 685901.

Comment 17

[Mats Palmgren (inactive)]

Attachment: part 1, outparamdel nsIFrame::GetParentStyleContextFrame

Remove the aIsChild parameter too.

Comment 18

[Mats Palmgren (inactive)]

Attachment: (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.

Comment 19

[Mats Palmgren (inactive)]

Attachment: full patch

Comment 20

[Mats Palmgren (inactive)]

> 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.

Comment 21

[Mats Palmgren (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/38ff711d4369
http://hg.mozilla.org/integration/mozilla-inbound/rev/79890feb4abe

Comment 22

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/79890feb4abe
https://hg.mozilla.org/mozilla-central/rev/38ff711d4369