834107 - nsComputedDOMStyle.cpp's helper-method GetContainingBlockFor unnecessarily null-checks its argument

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

While doing some investigation for bug 832168, I noticed that nsComputedDOMStyle.cpp's helper-method "GetContainingBlockFor()" has a redundant null-check for its argument -- it's redundant because all of its callers have their own earlier null-checks.

So, we can drop that null-check and replace it with an assertion.

Comment 1

[Daniel Holbert [:dholbert]]

...and actually, without that null-check, GetContainingBlockFor(frame) just becomes a wrapper for frame->GetContainingBlock().

So we might as well just directly call frame->GetContainingBlock().

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> [...] "GetContainingBlockFor()" has a
> redundant null-check for its argument -- it's redundant because all of its
> callers have their own earlier null-checks.

Two of the caller's null-checks are obvious; the one non-obvious one is the call in GetAbsoluteOffset().  That one is guaranteed to have a non-null frame because it's only got one caller -- GetOffsetWidthFor() -- which takes us on a different path if we don't have a frame:
https://mxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp?rev=037363fa0258#3515

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Per prev comment, mOuterFrame is guaranteed to be null when we call GetAbsoluteOffset(), so this patch adds an assertion to that effect.

I also removed the "if (!container)" case in that function (i.e. "if we don't have a containing block"), because it looks like we basically assume GetContainingBlock() returns non-null everywhere else.[1]   From inspection, it looks like GetContainingBlock() is effectively guaranteed to return non-null, except for on the root frame, where it simply crashes instead.

I'll post a '-w' version of the patch for review, since a bunch of this patch is indentation-changes.

[1] (we do null-check it in one place in nsLayoutUtils, but even there we've got a NS_NOTREACHED() in the "no containing block" case).

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v1 (diff -w version)

Comment 5

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

Comment on attachment 705762 [details] [diff] [review]
fix v1 (diff -w version)

r=dbaron, though maybe we should document somewhere that any primary frame for an element has a containing block

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #5)
> though maybe we should document somewhere that any primary frame
> for an element has a containing block

Sure -- I can add a comment to the nsIContent::GetPrimaryFrame() documentation.

(Incidentally, I tried adding an assertion to enforce that mPrimaryFrame->GetContainingBlock() returns non-null, in nsIContent::SetPrimaryFrame() and GetPrimaryFrame().  But that wouldn't compile, because those function's impls are inlined in nsIContent.h, and that header only knows about nsIFrame as a forward-declared class -- it hasn't seen the class definition yet.)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: comment tweak

Here's a patch to add a comment, per the suggestion in comment 5. How does this sound?

Comment 8

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

Comment on attachment 714205 [details] [diff] [review]
comment tweak

I think it probably makes more sense as a comment above the declaration of nsIFrame::GetContainingBlock than in nsIContent.h.  but r=dbaron there

Comment 9

[Daniel Holbert [:dholbert]]

OK, I added:
   * NOTE: This is guaranteed to return a non-null pointer when invoked on any
   * frame other than the root frame.
to the GetContainingBlock documentation, and landed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/139cea84972b

(I left the root-frame behavior unspecified; currently rootFrame->GetContainingBlock() crashes, but we probably don't want to enshrine that behavior in documentation & we may want to fix it.)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/139cea84972b