Summary: | More rendering code cleaning | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | mitz | ||||||||
Priority: | P2 | ||||||||||
Version: | 420+ | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.4 | ||||||||||
Attachments: |
|
Description
Sam Weinig
2006-10-27 16:02:00 PDT
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;
|