Last Comment Bug 199412 - optimize nsTypedSelection::ScrollIntoView()
: optimize nsTypedSelection::ScrollIntoView()
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: leon.zhang
: Patty Mac
: Jet Villegas (:jet)
Mentors:
Depends on: 35296
Blocks: 188318
  Show dependency treegraph
 
Reported: 2003-03-26 23:03 PST by leon.zhang
Modified: 2003-04-19 18:27 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1.0 (3.62 KB, patch)
2003-04-02 02:44 PST, leon.zhang
no flags Details | Diff | Splinter Review
test patch (5.29 KB, patch)
2003-04-08 08:00 PDT, leon.zhang
no flags Details | Diff | Splinter Review
move cache from caret to selection (6.66 KB, patch)
2003-04-09 03:30 PDT, leon.zhang
sfraser_bugs: review-
sfraser_bugs: superreview-
Details | Diff | Splinter Review
new patch (12.68 KB, patch)
2003-04-10 06:01 PDT, leon.zhang
no flags Details | Diff | Splinter Review
new patch (12.68 KB, patch)
2003-04-10 06:12 PDT, leon.zhang
no flags Details | Diff | Splinter Review
patch 1(move GetPointFromOffset to nsISelectionPrivate.idl ) (11.40 KB, patch)
2003-04-11 04:18 PDT, leon.zhang
no flags Details | Diff | Splinter Review
patch 2 (9.65 KB, patch)
2003-04-11 04:53 PDT, leon.zhang
no flags Details | Diff | Splinter Review
new patch (13.04 KB, patch)
2003-04-17 04:16 PDT, leon.zhang
no flags Details | Diff | Splinter Review
new version (12.81 KB, patch)
2003-04-18 01:58 PDT, leon.zhang
sfraser_bugs: review+
sfraser_bugs: superreview+
Details | Diff | Splinter Review
final patch(smfr's comments + newest trunk) (12.19 KB, patch)
2003-04-19 02:47 PDT, leon.zhang
no flags Details | Diff | Splinter Review

Description leon.zhang 2003-03-26 23:03:14 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.1) Gecko/20020823 Netscape/7.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.1) Gecko/20020823 Netscape/7.0

we have cached offset of caret in text frame in bug 35296,and scrollintoview
always called after we have refreshed cached value, 
so we can use it cached value when: 
nsTypedSelection::ScrollIntoView() ->
nsTypedSelection::GetSelectionRegionRectAndScrollableView() ->
nsTypedSelection::GetPointFromOffset() -> nsTextFrame::GetPointFromOffset()

through this we can improve perf of editor about 2%.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Comment 1 leon.zhang 2003-03-26 23:04:17 PST
assigned to me
Comment 2 leon.zhang 2003-03-27 01:06:07 PST
After patche for bug 35206 has been committed, I will give a patch based on this
Comment 3 leon.zhang 2003-03-27 01:08:02 PST
typo:
After patch for bug 35296 has been committed, I will give a patch based on that bug.
Comment 4 leon.zhang 2003-04-02 02:44:56 PST
Created attachment 119172 [details] [diff] [review]
patch 1.0
Comment 5 Simon Fraser 2003-04-02 18:09:56 PST
I don't think it's appropriate to be using the caret as a cache storage object.
It would make much more sense for the selection to have the cache, with some
method that wraps GetPointFromOffset(), returning a cached value when possible.
Comment 6 leon.zhang 2003-04-08 08:00:41 PDT
Created attachment 119811 [details] [diff] [review]
test patch
Comment 7 leon.zhang 2003-04-09 03:30:05 PDT
Created attachment 119955 [details] [diff] [review]
move cache from caret to selection
Comment 8 Simon Fraser 2003-04-09 17:31:32 PDT
Comment on attachment 119955 [details] [diff] [review]
move cache from caret to selection

>Index: content/base/public/nsISelection.idl
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/public/nsISelection.idl,v

>+
>+    /**
>+     * Returns cache for nsIFrame::GetPointFromOffset.
>+     */
>+    [noscript] void GetOptimizeDrawCaret(out PRBool aOptimzeDrawCaret, out nsIFrame aFrame, out PRInt32 inOffset, out nsPoint aPoint);
>+
>+    /**
>+     * Sets cache for nsIFrame::GetPointFromOffset.
>+     */
>+    [noscript] void SetOptimizeDrawCaret(in PRBool aOptimzeDrawCaret, in nsIFrame aFrame, in PRInt32 inOffset, in nsPoint aPoint);
> };

I don't think we want to mess with nsISelection, which is a public interface.
nsISelectionPrivate would be a better API to put these on. Also, it's not
'OptimizeDrawCaret' draw caret any more. It's more like
GetCachedOffsetForFrame(). There should be no 'SetCachedOffset'.


>Index: content/base/src/nsSelection.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsSelection.cpp,v
>retrieving revision 3.142
>diff -u -r3.142 nsSelection.cpp
>--- content/base/src/nsSelection.cpp	12 Mar 2003 03:21:49 -0000	3.142
>+++ content/base/src/nsSelection.cpp	9 Apr 2003 10:23:17 -0000
>@@ -218,6 +218,9 @@
>   NS_IMETHOD    GetInterlinePosition(PRBool *aInterlinePosition);
>   NS_IMETHOD    SetInterlinePosition(PRBool aInterlinePosition);
>   NS_IMETHOD    SelectionLanguageChange(PRBool aLangRTL);
>+  NS_IMETHOD    GetOptimizeDrawCaret(PRBool *aOptimzeDrawCaret, nsIFrame **aFrame, PRInt32 *inOffset, nsPoint ** aPoint);
>+  NS_IMETHOD    SetOptimizeDrawCaret(PRBool aOptimzeDrawCaret, nsIFrame *aFrame, PRInt32 inOffset, nsPoint * aPoint);
>+
> 
> /*END nsISelection interface implementations*/
> 
>@@ -311,6 +314,12 @@
>   PRBool mTrueDirection;
>   nsCOMPtr<nsIEventQueue> mEventQueue;
>   PRBool mScrollEventPosted;
>+
>+  PRPackedBool mOptimizeDrawCaret;   // flag for optimizing draw caret
>+  PRPackedBool mCachedOffsetValid;   // whether the cached frame offset is valid
>+  nsPoint      mCachedFrameOffset;   // cached frame offset
>+  nsIFrame*    mLastCaretFrame;      // store the frame the caret was last drawn in.
>+  PRInt32      mLastContentOffset;
> };

is mOptimizeDrawCaret necessary? I'm also worried that you're adding about 20
bytes to each selection object. Maybe put these into a struct that is only
allocated if used?

>+NS_IMETHODIMP    
>+nsTypedSelection::GetOptimizeDrawCaret(PRBool *aOptimzeDrawCaret, nsIFrame **aFrame, PRInt32 *inOffset, nsPoint ** aPoint)
>+{

This method should be called GetCachedOffsetForFrame(). It should return the
cached offset if it's valid, otherwise call GetPointFromOffset() on the frame,
then cache the result.

>+NS_IMETHODIMP
>+nsTypedSelection::SetOptimizeDrawCaret(PRBool aOptimzeDrawCaret, nsIFrame *aFrame, PRInt32 inOffset, nsPoint * aPoint)
>+{
>+  mOptimizeDrawCaret = aOptimzeDrawCaret;
>+  mLastCaretFrame = aFrame;
>+  mLastContentOffset = inOffset;
>+  mCachedFrameOffset.x = aPoint->x;
>+  mCachedFrameOffset.y = aPoint->y;
>+  return NS_OK;
>+}

This method is not necessary.

> nsresult
> nsTypedSelection::StartAutoScrollTimer(nsIPresContext *aPresContext, nsIFrame *aFrame, nsPoint& aPoint, PRUint32 aDelay)
> {

This should just call GetCachedOffsetForFrame()

>Index: layout/base/src/nsCaret.cpp

>-    if (!mOptimizeDrawCaret || !mCachedOffsetValid) {
>-      mLastCaretFrame->GetPointFromOffset(presContext, mRendContext,mLastContentOffset, &mCachedFrameOffset);
>-      mCachedOffsetValid = mOptimizeDrawCaret;
>+    // if cache in nsSelection is available, apply it, else refresh it
>+    if (mOptimizeDrawCaret && mCachedOffsetValid) {
>+      nsPoint *cachedPos= &framePos;
>+      domSelection->GetOptimizeDrawCaret(nsnull, nsnull, nsnull, &cachedPos);

QI to nsISelectionPrivate, and call GetCachedOffsetForFrame().
Comment 9 leon.zhang 2003-04-10 06:01:40 PDT
Created attachment 120067 [details] [diff] [review]
new patch
Comment 10 leon.zhang 2003-04-10 06:12:04 PDT
Created attachment 120068 [details] [diff] [review]
new patch 

simon: 
I am not sure there exist memory leak when "delete mCachedOffsetForFrame" in
this patch? or what is a better solution? thx
Comment 11 Simon Fraser 2003-04-10 18:27:49 PDT
>Index: content/base/public/nsISelectionPrivate.idl
>===================================================================

>+
>+    /**
>+     * Returns cache for nsIFrame::GetPointFromOffset.
>+     */
>+    [noscript] void GetCachedOffsetForFrame(out PRBool aGetCachedOffset, out
nsIFrame aFrame, out PRInt32 inOffset, out nsPoint aPoint);

I don't see the need to return aGetCachedOffset to the caller. Also, aFrame
and inOffset should be [in] params. I think aPoint should be [inout]
so that you don't get a nsPoint** in the .h file.


>Index: content/base/src/nsSelection.cpp
>===================================================================

>+struct CachedOffsetForFrame {
>+  nsPoint      CachedFrameOffset;   // cached frame offset
>+  nsIFrame*    LastCaretFrame;      // store the frame the caret was last
drawn in.
>+  PRInt32      LastContentOffset;
>+};
>+

You could give this struct a constructor to initialize the fields.
Please rename the member variables using the mFoo convention.


>+  if (mCachedOffsetForFrame) {
>+    mCachedOffsetForFrame->LastCaretFrame = nsnull;
>+    delete mCachedOffsetForFrame;
>+  }

No need to null out LastCaretFrame just before deleting.

>+NS_IMETHODIMP    
>+nsTypedSelection::GetCachedOffsetForFrame(PRBool *aGetCachedOffset, nsIFrame
**aFrame, PRInt32 *inOffset, nsPoint ** aPoint)
>+{
>+  if (!mCachedOffsetForFrame) {
>+    mCachedOffsetForFrame = new CachedOffsetForFrame;
>+    mCachedOffsetForFrame->LastCaretFrame = nsnull;
>+    mCachedOffsetForFrame->LastContentOffset = nsnull;
>+    (mCachedOffsetForFrame->CachedFrameOffset).x =
(mCachedOffsetForFrame->CachedFrameOffset).y = 0;

The ctor on CachedOffsetForFrame should take care of this init.

>+    //
>+    // Retrieve the device context. We need one to create
>+    // a rendering context.
>+    //
>+
...
>+    nsCOMPtr<nsIRenderingContext> rendContext;
>+
>+    rv = deviceContext->CreateRenderingContext(closestView,
*getter_AddRefs(rendContext));
>+  
>+    if (NS_FAILED(rv))
>+      return rv;
>+
>+    if (!rendContext)
>+      return NS_ERROR_NULL_POINTER;

Since all this code is here, can we remove some code in nsCaret.cpp that
does similar stuff?

Comment 12 leon.zhang 2003-04-11 04:18:03 PDT
Created attachment 120191 [details] [diff] [review]
patch 1(move GetPointFromOffset to nsISelectionPrivate.idl )
Comment 13 leon.zhang 2003-04-11 04:53:51 PDT
Created attachment 120193 [details] [diff] [review]
patch 2
Comment 14 leon.zhang 2003-04-11 05:02:09 PDT
1) difference between patch 1 and patch 2 is:
   move GetPointFromOffset to nsISelectionPrivate.idl, so that when drawcaret
can call this function in patch 1. This is also for future "remove some code in
nsCaret.cpp that does similar stuff?" in comments #11.

2) other part of patch 1 is almost same as patch 2:
   do most of things in comment #11, and set a flag in nsTypedSelection, so that
we  can use our cached point if necessary. 
Comment 15 leon.zhang 2003-04-17 04:16:30 PDT
Created attachment 120813 [details] [diff] [review]
new patch

simon: 
1) I keep orginal "nsPointPtr", because when apply "nsPointRef", will meet
complile error: 
../../../dist/include/content\nsISelectionPrivate.h(95) : error C2528:
'<Unknown>' : pointer to reference is illegal

2) other codes are based on our discussion
Comment 16 Simon Fraser 2003-04-17 19:35:08 PDT
Here's how to do the idl:

Index: nsISelectionPrivate.idl
===================================================================
RCS file: /cvsroot/mozilla/content/base/public/nsISelectionPrivate.idl,v
retrieving revision 1.10
diff -u -r1.10 nsISelectionPrivate.idl
--- nsISelectionPrivate.idl	4 Apr 2003 02:36:12 -0000	1.10
+++ nsISelectionPrivate.idl	18 Apr 2003 02:34:30 -0000
@@ -45,9 +45,12 @@
 
 %{C++
 class nsIPresShell;
+class nsIFrame;
 %}
 
 [ptr] native nsIPresShell(nsIPresShell);
+[ptr] native nsIFrame(nsIFrame);
+[ref] native nsPointRef(nsPoint);
 
 [scriptable, uuid(2d5535e2-1dd2-11b2-8e38-d53ec833adf6)]
 interface nsISelectionPrivate : nsISupports
@@ -92,5 +95,9 @@
     
     /* Internal utility method to set the pres shell on a newly created
selection */
     [noscript] void setPresShell(in nsIPresShell aPresShell);
+
+
+    [noscript] void GetCachedFrameOffset(in nsIFrame aFrame, in PRInt32
aOffset, /* inout */ in nsPointRef aPoint);
+
 };
 
Comment 17 Simon Fraser 2003-04-17 19:57:36 PDT
Comment on attachment 120813 [details] [diff] [review]
new patch

>Index: content/base/public/nsISelectionPrivate.idl


>+    [noscript] attribute boolean CachedFrameOffsetValid;

Leading lowercase is better here: 'cachedFrameOffsetValid'

>+    /* GetCachedOffsetForFrame
>+     * Returns cached value for nsTextFrame::GetPointFromOffset.
>+     */
>+    [noscript] void GetCachedFrameOffset(in nsIFrame aFrame, in PRInt32 inOffset, inout nsPoint aPoint);

I showed you how to get an nsPoint&  :)

>Index: content/base/src/nsSelection.cpp

>+struct CachedOffsetForFrame {
>+  CachedOffsetForFrame() {
>+    mLastCaretFrame = nsnull;
>+    mCachedFrameOffset.x = mCachedFrameOffset.y = 0;
>+    mLastContentOffset = 0;
>+  };

Nice

>+
>+  PRPackedBool mCachedFrameOffsetValid; // cached frame offset is valid?
>+  nsPoint      mCachedFrameOffset;      // cached frame offset
>+  nsIFrame*    mLastCaretFrame;         // store the frame the caret was last drawn in.
>+  PRInt32      mLastContentOffset;      // store last content offset
>+};

You'd get better packing if you put the PRPackedBool at the end, since the
nsPoint might end up 8-byte aligned. Also, the ctor needs to init
mCachedFrameOffsetValid.

>+
>+  if (mCachedOffsetForFrame) {
>+    delete mCachedOffsetForFrame;
>+  }

It's always safe to call 'delete' on a null pointer, so remove the if ().

>+NS_IMETHODIMP
>+nsTypedSelection::GetCachedFrameOffsetValid(PRBool *aCachedFrameOffsetValid)
>+{
>+  if (!aCachedFrameOffsetValid)
>+    return NS_ERROR_NULL_POINTER;

Maybe use NS_ENSURE_ARG_POINTER(aCachedFrameOffsetValid);

>+  if (mCachedOffsetForFrame)
>+    *aCachedFrameOffsetValid = mCachedOffsetForFrame->mCachedFrameOffsetValid;
>+  else
>+    *aCachedFrameOffsetValid = PR_FALSE;
>+
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP    
>+nsTypedSelection::SetCachedFrameOffsetValid(PRBool aCachedFrameOffsetValid)
>+{
>+  if (!mCachedOffsetForFrame) {
>+    mCachedOffsetForFrame = new CachedOffsetForFrame;
>+  }
>+
>+  mCachedOffsetForFrame->mCachedFrameOffsetValid = aCachedFrameOffsetValid;
>+
>+  return NS_OK;
>+}

These look good.

>+NS_IMETHODIMP    
>+nsTypedSelection::GetCachedFrameOffset(nsIFrame *aFrame, PRInt32 inOffset, nsPoint ** aPoint)
>+{
>+  if (!mCachedOffsetForFrame) {
>+    mCachedOffsetForFrame = new CachedOffsetForFrame;
>+  }
>+
>+  if (mCachedOffsetForFrame->mCachedFrameOffsetValid && 
>+      (aFrame == mCachedOffsetForFrame->mLastCaretFrame) &&
>+      (inOffset == mCachedOffsetForFrame->mLastContentOffset))
>+  {
>+     // get cached frame offset
>+     if (aPoint && *aPoint) {
>+       (*aPoint)->x = (mCachedOffsetForFrame->mCachedFrameOffset).x;
>+       (*aPoint)->y = (mCachedOffsetForFrame->mCachedFrameOffset).y;
>+     }

This will change, but you should be able to assign the point in one go:
	*aPoint = mCachedOffsetForFrame->mCachedFrameOffset;

>+     else     
>+       return NS_ERROR_NULL_POINTER;
>+  } 
>+  else
>+  {
>+     // recalculate frame offset and cache it
>+     if (aPoint && *aPoint) {
>+       GetPointFromOffset(aFrame, inOffset, *aPoint);
>+       if (mCachedOffsetForFrame->mCachedFrameOffsetValid) {

Shouldn't the cached offset always be valid just after calling
GetPointFromOffset()? If necessary, someone might have to invalidate
it by calling SetCachedFrameOffsetValid.


>Index: layout/base/src/nsCaret.cpp
>===================================================================

>   if (!mDrawn)
>   {
>+    nsPoint   framePos(0, 0);
>+    nsPoint   *cachedPos= &framePos;
>     nsRect    caretRect = frameRect;
>+    PRBool    aCachedFrameOffsetValid = PR_FALSE;
>+    nsCOMPtr<nsISelection> domSelection = do_QueryReferent(mDomSelectionWeak);
>+    nsCOMPtr<nsISelectionPrivate> privateSelection = do_QueryInterface(domSelection);
> 
>-    // if use cache and cache is ready, apply it, else refresh it
>-    if (!mOptimizeDrawCaret || !mCachedOffsetValid) {
>-      mLastCaretFrame->GetPointFromOffset(presContext, mRendContext,mLastContentOffset, &mCachedFrameOffset);
>-      mCachedOffsetValid = mOptimizeDrawCaret;
>-    }
>+    // if cache in selection is available, apply it, else refresh it
>+    privateSelection->GetCachedFrameOffsetValid(&aCachedFrameOffsetValid);
>+    if (aCachedFrameOffsetValid)
>+      privateSelection->GetCachedFrameOffset(mLastCaretFrame,mLastContentOffset, &cachedPos);

Can't we just assume that that privateSelection->GetCachedFrameOffset() will
always return a valid offset, because it recomputed it if necessary? This
requires that SetCachedFrameOffsetValid(PR_FALSE) has been called
appropriately.

This goes back to the old issue of knowing when a frame got reflowed.

If we have to stay with this model of "assume it's invalid unless told
otherwise", then I think SetCachedFrameOffsetValid() needs to be renamed to
CanCacheFrameOffset().

> nsresult
>Index: editor/libeditor/base/nsEditor.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/editor/libeditor/base/nsEditor.cpp,v
>retrieving revision 1.390
>diff -u -r1.390 nsEditor.cpp
>--- editor/libeditor/base/nsEditor.cpp	2 Apr 2003 05:48:08 -0000	1.390
>+++ editor/libeditor/base/nsEditor.cpp	17 Apr 2003 11:08:55 -0000
>@@ -746,18 +746,16 @@
>   NS_PRECONDITION(mPlaceHolderBatch > 0, "zero or negative placeholder batch count when ending batch!");
>   if (mPlaceHolderBatch == 1)
>   {
>-    nsCOMPtr<nsIPresShell> ps = do_QueryReferent(mPresShellWeak);
>-    nsCOMPtr<nsICaret> caret;
>-    if (!ps)
>-      return NS_ERROR_FAILURE;
>-
>-    nsresult rv = ps->GetCaret(getter_AddRefs(caret));
>+    nsCOMPtr<nsISelection>selection;
>+    nsresult rv = GetSelection(getter_AddRefs(selection));
>     if (NS_FAILED(rv))
>       return rv;
> 
>-    // optimizing drawcaret starts
>-    if (caret) {
>-      caret->SetOptimizeDrawCaret(PR_TRUE);
>+    nsCOMPtr<nsISelectionPrivate>selPrivate(do_QueryInterface(selection));
>+
>+    // Set cache for frame offset valid
>+    if (selPrivate) {
>+      selPrivate->SetCachedFrameOffsetValid(PR_TRUE);

You need some comments here to state what your assumptions are about the order
in which things are called, so that you know whether the cached offset is
valid.
Comment 18 leon.zhang 2003-04-18 01:58:34 PDT
Created attachment 120940 [details] [diff] [review]
new version
Comment 19 Simon Fraser 2003-04-18 17:37:14 PDT
Comment on attachment 120940 [details] [diff] [review]
new version

>Index: content/base/public/nsISelectionPrivate.idl
>===================================================================
>+    /* GetCachedOffsetForFrame
>+     * Returns cached value for nsTextFrame::GetPointFromOffset.
>+     */
>+    [noscript] void GetCachedFrameOffset(in nsIFrame aFrame, in PRInt32 inOffset, in nsPointRef aPoint);

This could use leading lowercase, like other idl methods. It gets uppercased in
 the .h file.

>+struct CachedOffsetForFrame {
>+  CachedOffsetForFrame() {
>+    mCachedFrameOffset.x = mCachedFrameOffset.y = 0;
>+    mLastCaretFrame = nsnull;
>+    mLastContentOffset = 0;
>+    mCanCacheFrameOffset = PR_FALSE;
>+  };

This would be cleaner using initializers:

  CachedOffsetForFrame()
  : mCachedFrameOffset(0, 0) // nsPoint ctor
  , mLastCaretFrame(nsnull)
  , mLastContentOffset(0)
  , mCanCacheFrameOffset(PR_FALSE)
  {}


>+NS_IMETHODIMP
>+nsTypedSelection::GetCanCacheFrameOffset(PRBool *aCanCacheFrameOffset)
>+{ 
>+  if (mCachedOffsetForFrame && mCachedOffsetForFrame)

This line seems wrong.

>Index: layout/base/src/nsCaret.cpp
>===================================================================

>+    // if cache in selection is available, apply it, else refresh it
>+    privateSelection->GetCanCacheFrameOffset(&aCanCacheFrameOffset);
>+    if (aCanCacheFrameOffset)

You don't need to do that test here, since
nsTypedSelection::GetCachedFrameOffset() does it for you. So you can always
just go ahead and call GetCachedFrameOffset().

>Index: editor/libeditor/base/nsEditor.cpp
>===================================================================

>-    // optimizing drawcaret starts
>-    if (caret) {
>-      caret->SetOptimizeDrawCaret(PR_TRUE);
>+    nsCOMPtr<nsISelectionPrivate>selPrivate(do_QueryInterface(selection));
>+
>+    // Cache begin to be available only if reflow/view-refresh/scroll happen sequentially during current 
>+    // process resulted from changing content of DOM through UI (e.g. typing chars into input fields).
>+    // This soultion can prevent exceptions which cause similar sequence during other process(e.g. changing
>+    // content of DOM by means of javascript). See bugs 199412 35296
>+    if (selPrivate) {
>+      selPrivate->SetCanCacheFrameOffset(PR_TRUE);
>     }

I think this comments needs a bit of work. How about:

// By making the assumption that no reflow happens during the calls
// to EndUpdateViewBatch and ScrollSelectionIntoView, we are able to
// allow the selection to cache a frame offset which is used by the
// caret drawing code. We only enable this cache here; at other times,
// we have no way to know whether reflow invalidates it
// See bugs 35296 and 199412.

Fix those, and r/sr=sfraser
Comment 20 leon.zhang 2003-04-19 02:47:31 PDT
Created attachment 121053 [details] [diff] [review]
final patch(smfr's comments + newest trunk)

transfer r/sr to this one
Comment 21 leon.zhang 2003-04-19 02:55:41 PDT
The patch can decrease time consumed by nsTextFrame::GetPointFromOffset about
half: from 4.2% to 2.2%.
Comment 22 leon.zhang 2003-04-19 03:07:35 PDT
more testing data:
                 orignal codes      current patch    
nsPresShell::      5.71% / 5.59%     5.92% /5.92% 
SetCaretEnabled        *5.65%*           *5.92%*    

nsCaret::          10.42% / 10.44%   10.77% / 11.03%
SetCaretVisible        *11.43%*           *10.80%*    

nsCaret::          5.76% / 5.74%     6.23% / 6.26% 
DrawCaret             *5.75%*            *6.25%*    

nsTextFrame::      4.22% / 4.21%     2.22% / 2.24% 
GetPointFromOffset    *4.22%*            *2.23%*
Comment 23 leon.zhang 2003-04-19 03:10:15 PDT
testing data of other functions in table above change a little is related to
current data structure(cache moved from caret to selection)
Comment 24 leon.zhang 2003-04-19 03:18:29 PDT
Other functions(SetCaretEnabled,SetCaretVisible,DrawCaret) in table consumed a
little more cpu time than previous codes.  This result is related to current
code structure(cache has been moved from caret to selection so that both
selection and caret can use the same cache based on a better code stlyle).
Comment 25 leon.zhang 2003-04-19 18:09:15 PDT
Comment on attachment 121053 [details] [diff] [review]
final patch(smfr's comments + newest trunk)

checked in
Comment 26 leon.zhang 2003-04-19 18:27:08 PDT
fixed

Note You need to log in before you can comment on or make changes to this bug.