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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

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.)

Comment 1

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

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.

Comment 2

[Daniel Holbert [:dholbert]]

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.)

Comment 3

[Daniel Holbert [:dholbert]]

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".

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v1 (renaming to ReferenceRenderingContext)

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.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v1: renaming to ReferenceRenderingContext(), now w/ 8 lines of context

Comment 6

[Daniel Holbert [:dholbert]]

[needinfo=dbaron on comment 2 - 3]

Comment 7

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

I'm happy with Create; maybe worth checking with roc/matt/tn to see if they're ok with it?

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v2: Rename to CreateReferenceRenderingContext() (and rename nsTextFrame.cpp's local version to CreateReferenceThebesContext)

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.

Comment 9

[Daniel Holbert [:dholbert]]

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.)

Comment 10

[Daniel Holbert [:dholbert]]

(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.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ff06ac4a434

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 13

[Daniel Holbert [: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?)

Comment 14

[Daniel Holbert [: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.

Comment 15

[Daniel Holbert [:dholbert]]

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.

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Good info, thanks.  Yes, we have a few options, and we'll track them in bug 1168934.