187117 - [FIX]GetFirstInFlow/GetLastInFlow should live on nsIFrame

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

We have loops in all sorts of places to do what GetFirstInFlow/GetLastInFlow 
do...  I believe these should be virtual methods on nsIFrame, with the 
implementation in nsIFrame being just "return this;".  The nsSplittableFrame 
can override them as it does now.  And we can replace all those loops with the 
much faster code that nsSplittableFrame uses (or that nsIFrame will have) 
without having to resort to icky casts...

Thoughts?

Comment 1

[Boris Zbarsky [:bzbarsky]]

I meant to take this....

Comment 2

[Boris Zbarsky [:bzbarsky]]

.

Comment 3

[Boris Zbarsky [:bzbarsky]]

And yes, that will require virtual dispatch for this method.... I'm not sure 
how big of a hit that would be in the existing use cases....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: something like this

Comment 5

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

Comment on attachment 110747 [details] [diff] [review]
something like this

sr=dbaron, although I'm surprised there aren't more places where you could use
this

Comment 6

[Boris Zbarsky [:bzbarsky]]

I was somewhat suprised too.  A lot of callers actually _do_ something to all
the in-flow frames, so have to iterate.  I basically looked at all places
calling GetNextInFlow/GetPrevInFlow and converted the ones that looked
convertable...

I've filed bug 187924 on making GetNextInFlow/GetPrevInFlow more usable; as I
work on that I may find more places where GetFirstInFlow/GetNextInFlow could be
used.... (but I doubt it).

Comment 7

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

why NS_IMETHOD_(nsIFrame*) instead of just nsIFrame*? There's no need to pretend
that nsIFrame is an XPCOM interface.

Comment 8

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

|NS_IMETHOD_(x)| is just |virtual x|, except on Windows, where it adds some
calling convention thing.  (|NS_IMETHOD| is equivalent to |NS_IMETHOD_(nsresult)|)

Comment 9

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

Oh, never mind.  I guess you just meant |virtual nsIFrame*|.

Comment 10

[Boris Zbarsky [:bzbarsky]]

exactly.  I could certainly just make it |virtual nsIFrame*|, but I'm not sure
about the calling conventions business.  I'd be happier to de-COMify nsIFrame
all in one swoop.

Comment 11

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

Feel free to deCOMify nsIFrame, but in the meantime at least we don't need to
make it worse.

Comment 12

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

Comment on attachment 110747 [details] [diff] [review]
something like this

Since I'm a softie, I'll give the r= and let bz decide what the return type
should be.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fix checked in.  I made it |virtual nsIFrame*|; comment 11 convinced me.  ;)

Comment 14

[Aleksey Nogin]

This check-in has added a warning on brad TBox:

+layout/html/base/src/nsTextFrame.cpp:969
+ `class nsContinuingTextFrame * firstInFlow' might be used uninitialized in
this function

See also bug 59652, bug 179819 and bug 59675 for more on the "uninitialized"
warnings.

Comment 15

[Boris Zbarsky [:bzbarsky]]

That warning is completely bogus, if you just look at the code in question.  I
considered writing that in a different way when I wrote the code, but that is
simply the simplest and most straightforward way to write the code.

I suppose I could add a "= nsnull" to shut up the compiler, but I think it would
just obfuscate the code in this case....

If you see a good way to rewrite this code to avoid the idiocy of gcc, please
feel free to tell me.

Comment 16

[Aleksey Nogin]

I agree that it is bogus. Here are couple alternatives:

---
NS_IMETHODIMP_(nsIFrame*)
nsContinuingTextFrame::GetFirstInFlow() const
{
  nsContinuingTextFrame* firstInFlow = (nsContinuingTextFrame*)this;
  while (firstInFlow->mPrevInFlow)  {
    firstInFlow = (nsContinuingTextFrame*)firstInFlow->mPrevInFlow;
  }
  return firstInFlow;
}
---
NS_IMETHODIMP_(nsIFrame*)
nsContinuingTextFrame::GetFirstInFlow() const
{
  nsContinuingTextFrame* prevInFlow;
  nsContinuingTextFrame* firstInFlow = (nsContinuingTextFrame*)this;
  while ((prevInFlow = (nsContinuingTextFrame*)firstInFlow->mPrevInFlow))  {
    firstInFlow = prevInFlow;
  }
  return firstInFlow;
}

Comment 17

[Aleksey Nogin]

P.S. If you end up fixing it, can you also fix two other instances of the
(almost) same code:

layout/html/base/src/nsSplittableFrame.cpp:108
 `class nsSplittableFrame * firstInFlow' might be used uninitialized in this
function

layout/html/base/src/nsSplittableFrame.cpp:120
 `class nsSplittableFrame * lastInFlow' might be used uninitialized in this function

Thanks!