Last Thursday, I held a meeting a design and code review meeting
regarding the Bi-Di code submission from IBM. Thanks to all those who
attended and sent me feedback.
Here's a summary of where I think we are today.
1. Architecture
The overall design of the new code is fine, as far as we can tell.
There are plenty of things that need to get fixed, but the basic concept
is perfectly acceptable. However, there were a few issues that do need
to be addressed before we could include the code on the trunk.
A) platform-specific code
In general, we do not allow #ifdef PLATFORM code in XP modules. You
need to factor out the platform-specific portions of your code, and
isolate platform code in it's own module. Then the build system can do
the right thing at build time, without polluting the XP modules with
tons of #ifdef code.
Along these lines...it is absolutely *not* required that you implement
Bi-Di on all platforms. However, your implementation should strive to
be free of platform-specific assumptions, so that others can implement
it on their systems. Erik has volunteered to help validate your design
against other platforms (I think he volunteered to validate Linux
himself, and he "volunteered" Frank for Mac.)
B) illegal dependancies
You added a dependancy between layout and the view system that isn't
legal. Kevin Mcclusky can provide the details, but basically you are
making bad assumptions about frames in the view code. Kevin, please
elaborate.
C) misuse of interfaces
You have added concrete functions and member variables to several
interfaces. This is illegal. XPCOM interface are abstract contracts
that cannot include this sort of implementation. Also, you should not
have #ifdef blocks on an interface. An interface is a public contract
that sometime soon (probably Mozilla 1.0), will become immutable. It
cannot depend on compile-time switches. If you need optional additional
functionality, it has to be on a new interface that is optionally a
subclass of whatever concrete class needs to support the methods.
2. Documentation
One thing that makes reviewing a submission of this size very difficult
is a lack of documentation. Some of the individual code blocks are well
documented, but there is no overview to guide us. To get this code
successfully integrated into the branch, we need 4 levels of documentation:
A) an overview document. This need not be long, or formal. Just
something to help us understand the philosophy behind the changes.
Where are major pieces of data stored (such as knowing whether Bi-Di is
enabled, or required for a particular page?) What classes do which
portion of the work? What work exactly is being done (i.e., frame
reordering.) I don't think the overview document needs to be complete
and polished before the code can go in, but I do think something is
needed before the next round of reviews.
B) interface documentation. Though we're not always good at it, we do
try hard to get all major classes and public interfaces thoroughly
documented. It would be a big help if each new method had a comment
block that described what the method did, its arguments, it's return
value, and any possible side effects. We urge people to use a javadoc
syntax, because there are tools that automatically build documentation
from such comments. See nsIFrame.h for an example of a fairly-well
documented interface.
C) code-level documentation. For the most part, the submission was
pretty good about including appropriate code-level comments. More is
better, of course. In particular, documenting the use of member
variables inside of classes is very helpful.
D) adhering to coding conventions. Parts orf the submission were very
poor at sticking to the mozilla coding conventions. This makes the code
much more difficult to read. Please see
http://www.mozilla.org/newlayout/doc/codingconventions.html
3. Performance
One of the biggest concerns is the impact on clients that are not
interested in providing Bi-Di support. Let's break this down into
several categories:
A) code size
Clearly, clients that are not interested in supporting Bi-Di should not
have to pay a significant penalty for the additional code required for
Bi-Di. The two ways we can think to minimize the impact are to factor
as much as possible into a separate library, or to leave significant
code chunks in #ifdef BIDI blocks. I'd like to urge people to think
about which code could reasonably be factored into it's own library,
since the support costs for #ifdef code is high.
B) memory usage
Reading the code, it doesn't look like the Bi-Di code adds any
significant amount of bloat. We'll have to take measurements once it's
integrated to validate, but so far, it looks good.
C) performance
Most reviewers are less concerned with the performance of the code when
Bi-Di is required, than the impact of the code when Bi-Di is not needed
to lay out a page. There seemed to be a few areas where Bi-Di code was
being executed unnecessarily. These could probably be fixed by simply
checking whether anything on the page warrented Bi-Di calculation before
executing the new code.
4. Implementation problems
There are plenty of minor problems that need to get fixed. Too many to
put in a newsgroup posting! But here are some general trends:
A) memory leaks
There are a few places where you leak objects because of early returns
in a method. Using nsCOMPtr would prevent this.
B) null pointer checks
There are many places where pointers are used without first being
checked for null. These include new allocations, method parameters, and
returned out-parameters from function calls. At a minimum, assertions
need to be added to validate the pointer. And unless you're guaranteed
the pointer must be valid, you should put in a null pointer check and
return an error if null.
C) 64-bit compatibility
Chris Waterson noticed some code that seemed to make bad assumptions
about 32-bit pointers. We already have one 64-bit system, and in
general we strive to avoid assumptions about the hardware. Chris, could
you elaborate on the specifics here?
I'll foward individual comments separately.
Steve
