anybody mind if I commit my changes?

[Dirk Mueller]

On Fre, 08 Aug 2003, Leo Savernik wrote:

> Today I'd like to commit the caret-navigation changes to khtml I've done so 
> far. This would include the "Harmful safari merges" as well as my 
> caret-navigation-specific changes.

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. 

While quickly browsing the diff, I find a couple of oddities. Note this is 
not a complete list, just from a 10 minutes glance:


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

b) code style // FIXME: ... (LS) additions. Usually we use // ### instead
  
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). 

d) Why is it named InlineBox anyway? CSS specs talk about Line boxes, not 
   Inline Boxes..

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. 

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. 

g) offsetForPoint should be const

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

i) isTextRun() etc should be const

j) 

-void RenderObject::cursorPos(int /*offset*/, int &_x, int &_y, int &height)
+void RenderObject::caretPos(int /*offset*/, bool /*override*/, int &_x, int 
&_y, int &width, int &height)
 {
     _x = _y = height = -1;
+    width = 1;
 }

hmm height being initialized to -1, but width to 1. is that a typo?

k) 

  *
  * Copyright (C) 1999-2003 Lars Knoll (knoll at kde.org)
  *           (C) 2000-2003 Dirk Mueller (mueller at kde.org)
- *           (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?

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. 



-- 
Dirk