If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Consolidate nsIPresContext

RESOLVED FIXED

Status

()

Core
Layout: Misc Code
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: Brian Ryner (not reading), Assigned: Brian Ryner (not reading))

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

13 years ago
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).
(Assignee)

Comment 1

13 years ago
Created attachment 154598 [details] [diff] [review]
patch
(Assignee)

Updated

13 years ago
Attachment #154598 - Flags: superreview?(roc)
Attachment #154598 - Flags: review?(bzbarsky)
Brian, I probably won't be able to get to this till a week from now....
+  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...
Blocks: 253489
+  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 :-)
No longer blocks: 253489
Blocks: 253489
(Assignee)

Comment 5

13 years ago
> 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.
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!
(Assignee)

Updated

13 years ago
Attachment #154598 - Flags: superreview?(roc)
Attachment #154598 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

13 years ago
checked in, along with follow-up to rename nsIPresContext to nsPresContext.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 8

13 years ago
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.
Yeah, I think there's no issue there.
You need to log in before you can comment on or make changes to this bug.