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

work on removing |aPresContext| parameters

RESOLVED FIXED

Status

()

Core
Layout: Misc Code
P2
normal
RESOLVED FIXED
15 years ago
4 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

See bug 190735 comment 25 and bug 190735 comment 26.

As a first pass, I'd like to add some inline accessors to nsIPresContext and
nsIPresShell (probably some other places too, but these will help with removing
|aPresContext| parameters from nsIFrameManager methods.

Does this seem like the direction we want to go?
Created attachment 124864 [details] [diff] [review]
add inline accessors to nsIPresShell and nsIPresContext
Created attachment 125049 [details] [diff] [review]
add inline accessors to nsIPresShell and nsIPresContext

slightly more accessors
Attachment #124864 - Attachment is obsolete: true
Comment on attachment 125049 [details] [diff] [review]
add inline accessors to nsIPresShell and nsIPresContext

Is this the direction we want to go?
Attachment #125049 - Flags: superreview?(bz-bugspam)
Attachment #125049 - Flags: review?(roc+moz)
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [patch]
Target Milestone: --- → mozilla1.5alpha
Comment on attachment 125049 [details] [diff] [review]
add inline accessors to nsIPresShell and nsIPresContext

The direction looks fine (great, even).  Two questions:

1)  What happens if the prescontext's mShell pointer is null and one of the
other accessors is called?

2)  Why the move from nsCOMPtrs to raw pointers?
> 1)  What happens if the prescontext's mShell pointer is null and one of the
> other accessors is called?

The same thing that happens to the few hundred places currently in the code
where we do things like:

  nsCOMPtr<nsIPresShell> shell;
  aPresContext->GetShell(getter_AddRefs(shell));
  shell->do_something();

> 2)  Why the move from nsCOMPtrs to raw pointers?

So that I don't have to include headers, which could vastly increase
dependencies (slow down compilation and likely mess up REQUIRES).  (And the
ability to forward-declare nsCOMPtrs wouldn't solve this, since the inline
functions use them.)
Comment on attachment 125049 [details] [diff] [review]
add inline accessors to nsIPresShell and nsIPresContext

>Index: base/src/nsPresContext.cpp

>+  *aManager = GetEventStateManager();
>+  if (!*aManager)
>+    return NS_ERROR_UNEXPECTED;

NS_ERROR_OUT_OF_MEMORY seems more appropriate here...

>Index: html/base/src/nsPresShell.cpp

>+  mDocument = aDocument;
>+  NS_IF_ADDREF(mDocument);

We already assert that the document is non-null, so this may as well be
NS_ADDREF.

sr=me with those minor nits.
Attachment #125049 - Flags: superreview?(bz-bugspam) → superreview+
Comment on attachment 125049 [details] [diff] [review]
add inline accessors to nsIPresShell and nsIPresContext

Yummy
Attachment #125049 - Flags: review?(roc+moz) → review+
Comment on attachment 125049 [details] [diff] [review]
add inline accessors to nsIPresShell and nsIPresContext

Patch checked in to trunk, 2003-06-19 11:16 -0700.
Created attachment 126053 [details] [diff] [review]
remove |aPresContext| and |aPresShell| parameters from nsIFrameManager methods
Attachment #126053 - Flags: superreview?(bzbarsky)
Attachment #126053 - Flags: review?(bzbarsky)
Blocks: 190735
Created attachment 126064 [details] [diff] [review]
add nsIFrame::GetPresContext and clean up some nsRuleNode/nsStyleContext signatures

This adds nsIFrame::GetPresContext as I suggested in bug 190735 comment 29.  It
also cleans up some related signatures on nsStyleContext and nsRuleNode, and
removes an unused method from nsRuleNode.
Attachment #126064 - Flags: superreview?(roc+moz)
Attachment #126064 - Flags: review?(roc+moz)
Comment on attachment 126053 [details] [diff] [review]
remove |aPresContext| and |aPresShell| parameters from nsIFrameManager methods

r+sr=me.
Attachment #126053 - Flags: superreview?(bzbarsky)
Attachment #126053 - Flags: superreview+
Attachment #126053 - Flags: review?(bzbarsky)
Attachment #126053 - Flags: review+
Comment on attachment 126053 [details] [diff] [review]
remove |aPresContext| and |aPresShell| parameters from nsIFrameManager methods

Patch checked in to trunk, 2003-06-19 16:52 -0700.
Comment on attachment 126064 [details] [diff] [review]
add nsIFrame::GetPresContext and clean up some nsRuleNode/nsStyleContext signatures

fabulous, r+sr=roc
Attachment #126064 - Flags: superreview?(roc+moz)
Attachment #126064 - Flags: superreview+
Attachment #126064 - Flags: review?(roc+moz)
Attachment #126064 - Flags: review+
Comment on attachment 126064 [details] [diff] [review]
add nsIFrame::GetPresContext and clean up some nsRuleNode/nsStyleContext signatures

Patch checked in to trunk, 2003-06-19 18:22 -0700.
Created attachment 126591 [details] [diff] [review]
fix regression from attachment 126064 [details] [diff] [review]

This bug made inspector crash sometimes.
Attachment #126591 - Flags: superreview?(bzbarsky)
Attachment #126591 - Flags: review?(bzbarsky)
Attachment #126591 - Flags: superreview?(roc+moz)
Attachment #126591 - Flags: superreview?(bzbarsky)
Attachment #126591 - Flags: review?(roc+moz)
Attachment #126591 - Flags: review?(bzbarsky)
Comment on attachment 126591 [details] [diff] [review]
fix regression from attachment 126064 [details] [diff] [review]

looks familiar :-)
Attachment #126591 - Flags: superreview?(roc+moz)
Attachment #126591 - Flags: superreview+
Attachment #126591 - Flags: review?(roc+moz)
Attachment #126591 - Flags: review+
Comment on attachment 126591 [details] [diff] [review]
fix regression from attachment 126064 [details] [diff] [review]

Patch checked in to trunk, 2003-06-27 14:06 -0700.
Created attachment 127579 [details] [diff] [review]
add frame manager accessors, clean up nsIFrame::(Get|Set)View
Attachment #127579 - Flags: superreview?(roc+moz)
Attachment #127579 - Flags: review?(roc+moz)
Comment on attachment 127579 [details] [diff] [review]
add frame manager accessors, clean up nsIFrame::(Get|Set)View

+	  f && (f->GetStateBits() & NS_FRAME_HAS_CHILD_WITH_VIEW);

Shouldnt't that be !(f->GetStateBits() & NS_FRAME_HAS_CHILD_WITH_VIEW
?

With that change, r+sr=roc
Attachment #127579 - Flags: superreview?(roc+moz)
Attachment #127579 - Flags: superreview+
Attachment #127579 - Flags: review?(roc+moz)
Attachment #127579 - Flags: review+
Comment on attachment 127579 [details] [diff] [review]
add frame manager accessors, clean up nsIFrame::(Get|Set)View

Patch checked in to trunk (with correction), 2003-07-11 17:46/48/50 -0700.
Whiteboard: [patch]
Target Milestone: mozilla1.5alpha → Future
QA Contact: nobody → layout.misc-code
Should this bug still be open?  It looks like all the work was done and checked in years ago.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: Future → ---
You need to log in before you can comment on or make changes to this bug.