98032 - Possible optimization in nsBoxToBlockAdaptor::Reflow(...)

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[rbs]

The nsBoxToBlockAdaptor::Reflow(...) is computing the 'ascent' of the block (see 
the excerpt below) -- it isn't cheap.  However, this information is _already_ 
available in the block code in its |mAscent| member variable. Furthermore, the 
block code computes it in a correct way -- just taking the max ascent of the 
fonts isn't enough, there could be an hierarchy of things like <sup>, <sub>, 
etc. The |mAscent| isn'it exposed. There used to be a nsBlockFrame::GetAscent() 
method, but it was removed. However, since nsBoxToBlockAdaptor needs the block's 
ascent, maybe that function could be re-instated.

nsresult
nsBoxToBlockAdaptor::Reflow(...)

[...]

    // ok we need the max ascent of the items on the line. So to do this
    // ask the block for its line iterator. Get the max ascent.
    nsresult rv;
    nsCOMPtr<nsILineIterator> lines = do_QueryInterface(mFrame, &rv);
    if (NS_SUCCEEDED(rv) && lines) 
    {
      nsIFrame* firstFrame = nsnull;
      PRInt32 framesOnLine;
      nsRect lineBounds;
      PRUint32 lineFlags;
      lines->GetLine(0, &firstFrame, &framesOnLine, lineBounds, &lineFlags);

      if (firstFrame) {
        nsRect rect(0,0,0,0);
        firstFrame->GetRect(rect);
     
        const nsStyleFont* font;
        firstFrame->GetStyleData(eStyleStruct_Font,
                            (const nsStyleStruct*&) font);
        nsIRenderingContext& rc = *aReflowState.rendContext;
        rc.SetFont(font->mFont);
        nsIFontMetrics* fm;
        rv = rc.GetFontMetrics(fm);
        if (NS_SUCCEEDED(rv) && (nsnull != fm)) {
          fm->GetMaxAscent(aDesiredSize.ascent);
          NS_RELEASE(fm);
        }
        rv = NS_OK;
        aDesiredSize.ascent += rect.y;

      }
    }

[...]

}

Comment 1

[John Morrison]

Well, let's send this on over to Mr. BoxToBlockAdaptor hisself. -> evaughan.

Comment 2

[Chris Waterson]

Sounds good to me, although I'd be a bit nervous about using the ``line
iterator''; IIRC, it copies all of the lines into an array when initialized. Is
there another way to get the first frame?

Comment 3

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

Can't you just use FirstChild?  (Either way, though, there's a problem with
ignorable whitespace -- I haven't been doing anything about that bug rbs filed
on me a while ago.)

Comment 4

[rbs]

No need to worry about the line iterator since all that chunk of code would 
become a [nsBlockFrame]mFrame->GetAscent(). I seem to recollect that when 
setting its |mAscent|, the block code avoids empty ignorable starting lines, or 
perhaps did I forgot about that precaution when removing GetAscent()?!

Comment 5

[rbs]

>perhaps did I forgot about that precaution when removing GetAscent()?!

False alarm. (I remember my testings were okay:-) Yep, ignorable whitespace are 
collapsed _inside_ the first line, and the case where the first line is itself a 
block is a different story.

Comment 6

[Eric Vaughan]

Sounds goods. I'll try making a public method and see what happens.

Comment 7

[rbs]

Remnants of the previous method can be seen here:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsBlockFrame.h&root=/cvsroot&subdir=mozilla/layout/html/base/src&command=DIFF_FRAMESET&rev1=3.126&rev2=3.127http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsBlockFrame.cpp&root=/cvsroot&subdir=mozilla/layout/html/base/src&command=DIFF_FRAMESET&rev1=3.435&rev2=3.436

Comment 8

[rbs]

(Another try -- using a double \n\n to work around this unexpected Mozilla bug)

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsBlockFrame.h&root=/cvsroot&subdir=mozilla/layout/html/base/src&command=DIFF_FRAMESET&rev1=3.126&rev2=3.127

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsBlockFrame.cpp&root=/cvsroot&subdir=mozilla/layout/html/base/src&command=DIFF_FRAMESET&rev1=3.435&rev2=3.436

Comment 9

[Eric Vaughan]

patch in #110328

Comment 10

[rbs]

I had a quick look at that patch and didn't see where the newly added 
nsBlockFrame::GetAscent() was called.

Comment 11

[Eric Vaughan]

Fixed. This is still called but it is cached.