208190 - work on removing |aPresContext| parameters

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

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

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?

Comment 1

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

Attachment: add inline accessors to nsIPresShell and nsIPresContext

Comment 2

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

Attachment: add inline accessors to nsIPresShell and nsIPresContext

slightly more accessors

Comment 3

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

Comment on attachment 125049 [details] [diff] [review]
add inline accessors to nsIPresShell and nsIPresContext

Is this the direction we want to go?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

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

> 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 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

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

Comment on attachment 125049 [details] [diff] [review]
add inline accessors to nsIPresShell and nsIPresContext

Yummy

Comment 8

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

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.

Comment 9

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

Attachment: remove |aPresContext| and |aPresShell| parameters from nsIFrameManager methods

Comment 10

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

Attachment: 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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126053 [details] [diff] [review]
remove |aPresContext| and |aPresShell| parameters from nsIFrameManager methods

r+sr=me.

Comment 12

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

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 13

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

Comment on attachment 126064 [details] [diff] [review]
add nsIFrame::GetPresContext and clean up some nsRuleNode/nsStyleContext signatures

fabulous, r+sr=roc

Comment 14

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

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.

Comment 15

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

Attachment: fix regression from attachment 126064

This bug made inspector crash sometimes.

Comment 16

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

Comment on attachment 126591 [details] [diff] [review]
fix regression from attachment 126064 [details] [diff] [review]

looks familiar :-)

Comment 17

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

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.

Comment 18

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

Attachment: add frame manager accessors, clean up nsIFrame::(Get|Set)View

Comment 19

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

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

Comment 20

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

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.

Comment 21

[Zack Weinberg (:zwol)]

Should this bug still be open?  It looks like all the work was done and checked in years ago.