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

RESOLVED FIXED in mozilla21

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla21
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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.
...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().
(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
Created attachment 705761 [details] [diff] [review]
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).
Created attachment 705762 [details] [diff] [review]
fix v1 (diff -w version)
Attachment #705762 - Flags: review?(dbaron)
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
Attachment #705762 - Flags: review?(dbaron) → review+
(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.)
Created attachment 714205 [details] [diff] [review]
comment tweak

Here's a patch to add a comment, per the suggestion in comment 5. How does this sound?
Attachment #714205 - Flags: review?(dbaron)
Flags: needinfo?
Flags: needinfo? → needinfo?(dbaron)
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
Attachment #714205 - Flags: review?(dbaron) → review+
Flags: needinfo?(dbaron)
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.)
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/139cea84972b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.