Closed Bug 1247478 Opened 4 years ago Closed 4 years ago

nsDocument::FirstAdditionalAuthorSheet() is unclear about its lack-of-infallibility

Categories

(Core :: CSS Parsing and Computation, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file)

In Gecko (or layout/style at least), there's a convention that getter-functions that return pointers should include "Get" in the name if they might return null, or should omit "Get" if they promise to return non-null.

nsDocument::FirstAdditionalAuthorSheet() seems to be an exception to this convention right now:
4431 CSSStyleSheet*
4432 nsDocument::FirstAdditionalAuthorSheet()
4433 {
4434   return mAdditionalSheets[eAuthorSheet].SafeElementAt(0, nullptr);
4435 }

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=8de87e81529a#4431

Note the "SafeElementAt" call there -- that'll return nullptr (the second arg that was passed to it) if the array doesn't have enough entries (in this case, if the array is empty).

So, this FirstAdditionalAuthorSheet function is explicitly returning null (via the fallback value it passes to SafeElementAt) in some cases.

So, we should probably rename it to "GetFirstAdditionalAuthorSheet".

(Note that we're changing this function to return a StyleSheetHandle instead of a CSSStyleSheet* in bug 1244074 part 4. This bug is still valid, regardless, since StyleSheetHandle is still pointer-like, and the function is still allowing itself to return a default-constructed (null-ish) StyleSheetHandle.)

Marking this bug as depending on bug 1244074, because we shouldn't fix this until bug 1244074 has landed, to avoid bitrotting those patches.
(In reply to Daniel Holbert [:dholbert] from comment #0)
> So, we should probably rename it to "GetFirstAdditionalAuthorSheet".

Sounds good to me.
Here's a patch to do this rename.

(I rewrapped one contextual comment that was over 80 characters, too.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8723731 - Flags: review?(cam)
Attachment #8723731 - Flags: review?(cam) → review+
https://hg.mozilla.org/mozilla-central/rev/be17cdb8e8e0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.