Closed Bug 996351 Opened 6 years ago Closed 6 years ago

Rename nsPresShell::GetReferenceRenderingContext (replacing "Get" with "Create") to indicate that it's infallible

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 2 obsolete files)

nsPresShell::GetReferenceRenderingContext is infallible, though it's not clear from the name (and a lot of callers have unnecessary null checks).

Filing this bug on renaming it to simply ReferenceRenderingContext(), to indicate that it's guaranteed to return non-null, and dropping null-checks in the callers.

(Note: The patch here will layer on top of my patches for bug 996326 and bug 996319; setting dependencies.)
I'm a little worried the name change might make people think that it's a thing that's already there lying around rather than a new object infallibly created for the caller.
We already kind of have that problem -- "Get" kind of implies that it's an already-existing thing.

Maybe we should rename this to "CreateReferenceRenderingContext" to address Comment 1?

(This matches the name of a method on nsDeviceContext, which is called by GetReferenceRenderingContext... Not sure if making the name the same would increase or decrease confusion.)
Note that generally, people shouldn't be calling the nsDeviceContext version directly (instead, they want to call the nsPresShell version, which is a wrapper for it with a check to enforce the "only used during printing" assumption on the nsDeviceContext version).

This is why I suspect it might not be too bad to have the names be the same w/ "Create".
OS: Linux → All
Hardware: x86_64 → All
FWIW, here's a patch to rename this to ReferenceRenderingContext. I can adjust this to use a "Create" prefix (or something else), once we settle on what the correct naming should be.
[needinfo=dbaron on comment 2 - 3]
Flags: needinfo?(dbaron)
Summary: Drop "Get" from nsPresShell::GetReferenceRenderingContext, to indicate that it's infallible → Rename nsPresShell::GetReferenceRenderingContext (perhaps dropping "Get") to indicate that it's infallible
I'm happy with Create; maybe worth checking with roc/matt/tn to see if they're ok with it?
Flags: needinfo?(dbaron)
Cool. This renames to use "Create".  Tagging roc for feedback via r?.

Note that nsTextFrame.cpp has its own version of this function that returns a reference to a gfxContext.  This patch renames that function to CreateReferenceThebesContext(), so that we only have two functions of the same name (in nsDeviceContext & nsPresShell, per comment 2-3), instead or 3.
Attachment #8406526 - Attachment is obsolete: true
Attachment #8406543 - Flags: review?(roc)
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff06ac4a434

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #7)
> I'm happy with Create; maybe worth checking with roc/matt/tn to see if
> they're ok with it?

I'm satisfied knowing that at least you & roc are both OK with it. [hence the landing ^]

If matt, tn, or anyone else happen to have concerns with the new name, we can easily address them with another renaming. (And the patch that I've just landed will still have had some value, for removing null-checks that are unnecessary once you know that this function is infallible, & for updating the documentation to indicate this infallibility.)
Flags: in-testsuite-
(In reply to Daniel Holbert [:dholbert] from comment #2)
> (This matches the name of a method on nsDeviceContext, which is called by
> GetReferenceRenderingContext... Not sure if making the name the same would
> increase or decrease confusion.)

Just realized this comment ^^ was incorrect - the nsDeviceContext method is CreateRenderingContext (no "Reference").  So, the names are different, which is probably good.
Summary: Rename nsPresShell::GetReferenceRenderingContext (perhaps dropping "Get") to indicate that it's infallible → Rename nsPresShell::GetReferenceRenderingContext (replacing "Get" with "Create") to indicate that it's infallible
https://hg.mozilla.org/mozilla-central/rev/0ff06ac4a434
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Sorry for digging up an old bug, but the assumption that GetReferenceRenderingContext (now CreateReferenceRenderingContext) is infallible is incorrect if the device context size is invalid, as in 0 (e.g., bug 1019063.)

Do you recall where the original assertion that this function always succeeds was based on?
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(dholbert)
(In reply to Milan Sreckovic [:milan] from comment #12)
> Do you recall where the original assertion that this function always
> succeeds was based on?

Code inspection, I think?

(It may be that I didn't dig thoroughly enough -- or perhaps the failure case was added after this bug was fixed?)
Flags: needinfo?(dholbert)
Ah, I misunderstood the question / problem-scenario. IIUC, it was & still is infallible, but we're now proposing to make it not-infallible, because:
 - it depends on nsDeviceContext::CreateRenderingContext (currently infallible)
 - which depends on CreateDrawTargetForSurface
 - and CreateDrawTargetForSurface can fail, which means its callers need to be able to handle that failure.
Anyway, more directly answering comment 12: IIRC we formalized this nsPresShell method's infallibility here because some code *already* assumed it was infallible, and code inspection revealed that it actually was infallible (i.e. it had no null return cases).

If we need to make it fallible, we'll need to add failure-handling cases all over the place, I think. :-/  (Adding them back in some cases, and add them where they never existed in others).

If it's possible to abstract away the failure (e.g. by creating a dummy rendering context or something?), that might be cleaner, but I'm not sure.
Good info, thanks.  Yes, we have a few options, and we'll track them in bug 1168934.
Flags: needinfo?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.