Closed Bug 834107 Opened 8 years ago Closed 8 years ago
Computed DOMStyle .cpp's helper-method Get Containing Block For unnecessarily null-checks its argument
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
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. 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.  (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 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.)
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? → 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+
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.)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.