Closed
Bug 253470
Opened 21 years ago
Closed 21 years ago
Consolidate nsIPresContext
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
People
(Reporter: bryner, Assigned: bryner)
References
Details
Attachments
(1 file)
178.06 KB,
patch
|
Details | Diff | Splinter Review |
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•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #154598 -
Flags: superreview?(roc)
Attachment #154598 -
Flags: review?(bzbarsky)
![]() |
||
Comment 2•21 years ago
|
||
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...
+ 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
Assignee | ||
Comment 5•21 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•21 years ago
|
Attachment #154598 -
Flags: superreview?(roc)
Attachment #154598 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•21 years ago
|
||
checked in, along with follow-up to rename nsIPresContext to nsPresContext.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 8•21 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.
Updated•7 years ago
|
Product: Core → Core Graveyard
Updated•7 years ago
|
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•