More fall cleaning of the rendering code. Patch to come.
Created attachment 11262 [details] patch
Created attachment 11263 [details] no-whitespace-changes version of the patch Diff made with svn diff -x -w
I actually had a bunch of comments on the first 2/3 of the patch before Tim reviewed it :-) In case you want to consider them: I interpret the "one line" guideline as "one line", not "one statement", so revert this: - - if (!result) { + + if (!result) // Allocate a new chunk from the arena ARENA_ALLOCATE(result, &m_pool, size); - } Please change 'b' to 'firstLine' in these: + virtual short lineHeight(bool b, bool isRootLineBox = false) const; + virtual short baselinePosition(bool b, bool isRootLineBox = false) const; For this: - virtual int maxTopMargin(bool positive) const { + virtual int maxTopMargin(bool positive) const + { if (positive) return m_maxTopPosMargin; - else - return m_maxTopNegMargin; + return m_maxTopNegMargin; } I think "return positive ? m_maxTopPosMargin : m_maxTopNegMargin;" is neater. Ditto for maxBottomMargin(). Leftover space after the &: + int skipWhitespace(BidiIterator& , BidiState&); 'stream' is redundant: + virtual void dump(TextStream* stream, DeprecatedString ind = "") const; I don't think you need parameter names at all in these: + RootInlineBox* lineAtIndex(int index); + int heightForLineCount(int lineCount); Same goes for 'renderer' here: + Position positionForRenderer(RenderObject* renderer, bool start = true) const; Is this comment still needed?! - // XXX Generalize to work with top and left as well. + // FIXME: Generalize to work with top and left as well. And what about this one? Duh. +#include "AXObjectCache.h" // For accessibility Index: WebCore/rendering/RenderFieldset.cpp Needs spaces around the minus sign: + int yOff = (legend->yPos() > 0) ? 0 : (legend->height()-borderTop()) / 2; Index: WebCore/rendering/RenderForeignObject.cpp I wanted to suggest a better name for the bool in: + virtual void computeAbsoluteRepaintRect(IntRect&, bool f); Apparently it means that the caller is fixed, but I can't see that it's being used anywhere. Weird. The change makes the comment redundant: - ASSERT(false); // Should never be reached. + ASSERT_NOT_REACHED(); // Should never be reached. I don't like the name 'needsLayout' here: - bool needlayout = false; + bool needsLayout = false; How about 'ensureLayout'? Double parentheses: + if ((newImage->imageSize().width() != intrinsicWidth() || newImage->imageSize().height() != intrinsicHeight() || imageSizeChanged)) { Missing space after if: + if(imageSizeChanged || m_width != oldwidth || m_height != oldheight)
Finally, I don't think the patch will merge following r17399 :(
Created attachment 11279 [details] updated patch
Comment on attachment 11279 [details] updated patch Great work! I have 3 comments which you should fix when landing. "its", not "it's": +// FIXME: move this to it's own file "IntRect &exposeRect" -> "IntRect& exposeRect": + IntRect getRectToExpose(const IntRect& visibleRect, const IntRect &exposeRect, const ScrollAlignment& alignX, const ScrollAlignment& alignY); 'b' is a bad name, but 'firstLine' is better than no name: - virtual short lineHeight(bool b, bool isRootLineBox=false) const; - virtual short baselinePosition(bool b, bool isRootLineBox=false) const; + virtual short lineHeight(bool, bool isRootLineBox = false) const; + virtual short baselinePosition(bool, bool isRootLineBox = false) const;
Landed in r17448.