Last Comment Bug 74946 - bidi: more changes and files for layout directory
: bidi: more changes and files for layout directory
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla0.9
Assigned To: Erik van der Poel
: Frank Tang
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2001-04-05 19:21 PDT by Erik van der Poel
Modified: 2005-03-17 13:34 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
mozilla\layout\base\public\nsITextFrame.h (1.34 KB, text/plain)
2001-04-05 19:22 PDT, Erik van der Poel
no flags Details
mozilla\layout\html\base\src\nsBidiFrames.cpp (2.49 KB, text/plain)
2001-04-05 19:23 PDT, Erik van der Poel
no flags Details
mozilla\layout\html\base\src\nsBidiFrames.h (1.67 KB, text/plain)
2001-04-05 19:24 PDT, Erik van der Poel
no flags Details
diffs for the modified files mentioned earlier (34.31 KB, patch)
2001-04-05 19:26 PDT, Erik van der Poel
no flags Details | Diff | Splinter Review

Description Erik van der Poel 2001-04-05 19:21:23 PDT
This bug report will be used to attach some new files and diffs from the IBM
bidi project. They are all under the mozilla/layout directory. Here are the new
files:

  ? base/public/nsITextFrame.h
  ? html/base/src/nsBidiFrames.cpp
  ? html/base/src/nsBidiFrames.h

And here are the files that are being modified:

  M base/src/nsBidiPresUtils.cpp
  M html/base/src/nsLineLayout.cpp
  M html/base/src/nsTextFrame.cpp

Marc, nsITextFrame is the new interface that we discussed a while ago. This
cleans up the somewhat odd subclassing that we reviewed earlier.
Comment 1 Erik van der Poel 2001-04-05 19:22:46 PDT
Created attachment 29899 [details]
mozilla\layout\base\public\nsITextFrame.h
Comment 2 Erik van der Poel 2001-04-05 19:23:55 PDT
Created attachment 29900 [details]
mozilla\layout\html\base\src\nsBidiFrames.cpp
Comment 3 Erik van der Poel 2001-04-05 19:24:43 PDT
Created attachment 29901 [details]
mozilla\layout\html\base\src\nsBidiFrames.h
Comment 4 Erik van der Poel 2001-04-05 19:26:09 PDT
Created attachment 29902 [details] [diff] [review]
diffs for the modified files mentioned earlier
Comment 5 Marc Attinasi 2001-04-10 10:24:43 PDT
I'd suggest changing the use of 'long' to PRInt32 - I'm not used to seeing
non-abstracted types in Layout code! Also, the block that is commented as a
non-BIDI change should either be proven to be incorrect and thus just fixed (eg.
not in an #ifedf IBMBIDI) or have a bug opened on it so we can figure out if it
is wrong and fix it, or correct and clarify why it needs to be the way it is.
I'll do that if you like. sr=attinasi
Comment 6 Erik van der Poel 2001-04-10 12:21:53 PDT
Just to make sure we are talking about the same thing, Marc, are you referring
to the following block of code?

   if (inContentOffset < mContentOffset) //could happen with floaters!
   {
     result = GetPrevInFlow(outChildFrame);
+#ifdef IBMBIDI // Not a Bidi change; I just thought this line was wrong
+    if (NS_SUCCEEDED(result) && *outChildFrame)
+#else
     if (NS_SUCCEEDED(result) && outChildFrame)
+#endif
       return (*outChildFrame)->GetChildFrameContainingOffset(inContentOffset, 
inHint,
         outFrameContentOffset,outChildFrame);
Comment 7 Marc Attinasi 2001-04-10 14:09:05 PDT
Erik, yes that is the code I was referring to - thanks for checking. Looking at
the full source just now, I cannot see how the old version could be possibly be
correct, so I'd be inclined to just change it with no #ifdef's. Note that
outChildFrame is a **, and every place else that GetPrevInFlow is used, the
check is on the value returned from the method, not on the address of the
returned value (using the word 'return' lightly here to mean an output argument).
Comment 8 Frank Tang 2001-04-11 12:29:11 PDT
mark this as moz0.9
Comment 9 Erik van der Poel 2001-04-11 16:26:50 PDT
r=erik@netscape.com
Comment 10 Erik van der Poel 2001-04-11 16:41:30 PDT
New files and fixes checked in.
Comment 11 Lina Kemmel 2001-04-15 05:37:01 PDT
Marc: 'long' has the same size as 'void*', while using PRInt32 may be 
problematical for a 64-bit platform.
Comment 12 Erik van der Poel 2001-04-16 11:45:06 PDT
Lina, what size are the things that we are storing inside the void*? Are they
8-bit? In that case PRInt32 or even PRUint8 should be OK, no?
Comment 13 Lina Kemmel 2001-04-17 05:05:09 PDT
Erik, we'are storing also 'void*' there.
Cannot we replace 'long' by 'PRWord' (defined in prtypes.h)?
(By the way, I'm talking here about the type of the last argument of 
GetBidiProperty(); as for the callers of this method, they could safely 
use 'PRInt32', indeed, but then need to specify explicitly the size of this 
type - sizeof(PRInt32).)
Comment 14 Erik van der Poel 2001-04-17 11:48:01 PDT
PRWord isn't a good choice either, since it isn't used much in the code base
either. I think we should just use whatever type those things are supposed to
be, e.g. nsBidiLevel, then pass sizeof(nsBidiLevel) to GetBidiProperty().
Comment 15 Frank Tang 2001-04-18 10:59:01 PDT
code landing bug. change QA to ftang
v=ftang

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