253470 - Consolidate nsIPresContext

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Brian Ryner (not reading)]

I have a patch which consolidates the 3 types of PresContexts (Galley, Print,
PrintPreview) onto a single nsIPresContext implementation.  nsIPresContext no
longer has any virtual methods when called from inside layout.  When called from
outside layout, I implemented passthrough virtual functions for the small amount
of functionality that's needed (that isn't inlined).

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Brian, I probably won't be able to get to this till a week from now....

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  NS_HIDDEN_(void) SetImageAnimationModeInternal(PRUint16 aMode);

This can be moved out of the #ifdef. Personally I'd have just made the external
callers all change to call External, and just had SetImageAnimationMode be
local, but I can deal with this. Ditto for the other methods you made external...

+  NS_HIDDEN_(void) SetPageDim(nsRect* aRect);

Really this should be a const nsRect&.

+  /* Convenience method for converting one pixel to twips */

Should say "for converting one pixel *value* to twips"

more coming...

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  nsCAutoString         mCharset;

I think you don't want an auto string as a class member?

+nsIPresContext::GetPageDim(nsRect* aActualRect, nsRect* aAdjRect)

I think this should be broken up into two methods, GetPageDimActual and
GetPageDimAdjusted, which each return an nsRect. Anyway, as written,
+    *aAdjRect = mPageDim;
this should be inside the if (aAdjRect && aActualRect)

+  nscoord IntPixelsToTwips(nscoord aPixels) const

I think this should be called IntScaledPixelsToTwips.

The rest looks good. Let me know how you plan to address these and I'll apply
the stamp :-)

Comment 5

[Brian Ryner (not reading)]

> I think this should be broken up into two methods, GetPageDimActual and
> GetPageDimAdjusted, which each return an nsRect. Anyway, as written,

It's a toss-up... 2 out of 4 callers want both rects; one call is more efficient
than two.  I'm going to leave and just tweak the null check.

I applied all the rest of your suggestions.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Very well. I still think GetPageDim would be better as two functions, but I'll
yield on that point especially because this is existing code not new code.
r+sr=roc, check this thing in!

Comment 7

[Brian Ryner (not reading)]

checked in, along with follow-up to rename nsIPresContext to nsPresContext.

Comment 8

[Darin Fisher]

In the checkin with comment: "Change nsIPresContext to nsPresContext globally,
follow-up to bug 253470. rs=roc", it looks like a number of interfaces were
modified, but the UUIDs of those interfaces were not bumped.  Should they have
been bumped?  nsPresContext -> nsIPresContext doesn't sound like it would throw
off any vtables, so maybe not an issue.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, I think there's no issue there.