anybody mind if I commit my changes?

[Leo Savernik]

Am Freitag, 8. August 2003 21:34 schrieb Dirk Mueller:
> On Fre, 08 Aug 2003, Leo Savernik wrote:
[...]
>
> I would appreciate if you could wait for the patch review. I'm sorry that I
> was rather busy the last few weeks, I hope it gets better soon.

Ok, I'll not commit yet but post the complete patch before I'm off for the 
weekend.
[...]
> a) cursorPos -> caretPos rename. Besides that I completely miss the point
> on why a good, established naming sheme has to be broken it doesn't look
> like all necessary places have been changed. at least khtml_part and
> dom_nodeimpl is missing in the diff

KHTML always uses the term "cursor" to specify the mouse cursor. To eliminate 
opportunities for confusion, I chose the term "caret" whenever I referred to 
the blinking thingy (as does Mozilla), and I already used it hundreds of 
times in the navigation code, so I'd like to keep it. However, I can, if you 
wish, rename caretPos back to cursorPos.

And btw, I never sent a full patch. It was only a diff of the rendering 
subdirectory. Given the nits you found in this small patch, I fear you'll 
jump through the ceiling if you see the full patch ;-)
>
> b) code style // FIXME: ... (LS) additions. Usually we use // ### instead

I'm okay using ### but I'd rather retain (LS) otherwise I might forget that I 
put this comment there.
>
> c) TextSlave -> TextRun rename. While I again miss the point of renaming
>    it to something that is remarkable similiar to BidiRun (which is however
>    a different thing), I'm wondering why it has to inherit InlineBox (which
>    is quite a huge class). There are tons of TextSlaves created, and this
>    is a nontrivial overhead IMHO. Also, for consistency I would suggest
>    a name line InlineTextBox instead (similiar to InlineFlowBox).

Don't ask me, that's what I ported over from safari. I used the Safari naming 
to make both branches more similar. And yes, it is heavy, but needed for 
building up and traversing lines of a paragraph. It's only because of line 
persistance why I ported it at all. This port of TextSlave/Run is extra-huge 
because it keeps some members that aren't there any more in Safari.
>
> d) Why is it named InlineBox anyway? CSS specs talk about Line boxes, not
>    Inline Boxes..

As far as I understand apple's changes, a css line box maps to a 
RootInlineBox. InlineBoxes are the least common denominator for any part of a 
render object that still fits on the same line (ie a text slave or a replaced 
element).
>
> e) C++ supports declaration of variables at any place in the function, so
>    make use of it. Don't put variables at the beginning of blocks, like
>    in RenderFlow around line 1010. It is confusing and might make
>    somebody accidentally use it uninitialized.
>
I can't follow you there, line 1010 is in RenderFlow::leftBottom(), and I 
certainly didn't touch that. Perhaps you meant another file?

> f)  bidi.cpp:
>
>      while( obj && obj != eor.obj ) {
>          if(!obj->isHidden()) {
>              //kdDebug(6041) << "appendRun: "<< start << "/" <<
> obj->length() <<endl;
> +	    Q_ASSERT(sruns != 0);
>              sruns->append( new BidiRun(start, obj->length(), obj, context,
> dir) );
>          }
>          start = 0;
>
> There is no point in this assert. if it is 0 then it will crash on the next
> line anyway. Please don't slow down innerloops.

Oops, sorry, this is a leftover from a debugging session. I already removed 
it.
>
> g) offsetForPoint should be const

done.
>
> h) TextRun::minOffset(): why does it return long btw?

because of
long m_startOffset;
long m_endOffset;
in KHTMLPartPrivate (I guess they did it to comply to the DOM-Range 
specification)
>
> i) isTextRun() etc should be const

I ported it over from WebCore as is. I'd like to leave it to keep the diff 
between the two projects smaller.
>
> j)
[...]
>      _x = _y = height = -1;
> +    width = 1;
>  }
>
> hmm height being initialized to -1, but width to 1. is that a typo?

No, that's intentional. The caret's width is always one. I added a comment.
>
> k)
[...]
> - *           (C) 2002 Apple Computer, Inc.
> + * Copyright (C) 2003 Apple Computer, Inc.
>
> Copyright is already in the first line, why does it have to be repeated
> here?

Apple did it this way. I simply copied it to reflect that I ported over some 
of their code.
>
> l) It amazes me that RenderFlow::constructLine takes a start and end
>    BidiIterator, but never actually makes use of it. Instead it does
>    a full iteration over the complete BidiRun.

I dunno, I'm only the porting guy. Ask apple.

mfg
	Leo