During implementation for ruby support I came across a few places that I took the liberty of cleaning up the code and/or simplifying it. None of the attached patches add functionality, and there was no change in layout test results for me (but please still read individual patch descriptions).
Created attachment 30963 [details] CSSParser.cpp/.h - move parsing of 'attr(X)' to own method move parsing of 'attr(X)' to its own method, in order to make re-using it easier. (E.g., CSS3 'ruby-span' would also use 'attr(X)')
Created attachment 30964 [details] add macro for primitive values Like the other HANDLE_... macros used I added 2 more that also include the handling of primitive values. Using that allows to reduce the line count quite a bit. NOTE: There are some CSS properties that do not query whether a value is actually a primitive value, but do an assignment unconditionally. AFAICT, this would result in the asignment of a default value if the passed-in value is not, in fact, primitive. As these cases should only occur with malformed input anyway, and as the UA is supposed to ignore erroneous values, I believe my changes are correct in these cases. There was no change in layout test results.
Created attachment 30965 [details] RenderBlock.cpp/.h : simplify handleSpecialChild, comment correction This patch simplifies the handleSpecialChild and its related methods (handlePositionedChild, handleFloatingChild, handleRunInChild). It also corrects a typo in a clarifying remark.
Created attachment 30966 [details] RenderTable.cpp - remove superfluous if statements This patch removes 3 instances of superfluous if statements (the same condition has already been queried at the beginning of the block)
Created attachment 30967 [details] RenderTableSection.cpp : correct comment
Created attachment 30968 [details] CSSParser.cpp/.h - move parsing of 'attr(X)' to own method (fixed .patch) fixed the .patch format, otherwise same patch as in comment #1
Created attachment 30973 [details] ChangeLog entry Please adapt the ChangeLog depending on which patches make/don't make it in.
These are all unrelated cleanups and thus should have separate ChangeLogs. It is also preferable to add the ChangeLog with the patch.
Comment on attachment 30964 [details] add macro for primitive values This needs a ChangeLog. Otherwise this looks great. Since you don't have commit-bit yet, please upload a patch with a ChangeLog and I'll r+ it.
Comment on attachment 30973 [details] ChangeLog entry Please put the ChangeLogs with the patches you want reviewed. If they are distinct such that they should be reviewed separately, they should have separate ChangeLogs too.
Comment on attachment 30965 [details] RenderBlock.cpp/.h : simplify handleSpecialChild, comment correction Looks great! r- for lack of ChangeLog. These are too different to put all in one patch so they need separate ChangeLogs.
Comment on attachment 30966 [details] RenderTable.cpp - remove superfluous if statements I think we should just turn these into ASSERTS in case we ever change the Renderer inheritance for TableSections. Also, we might as well fix the style violation while were here: + if (!m_head) { + m_head = static_cast<RenderTableSection*>(child); + } else { r- for lack of ChangeLog and nits above.
Comment on attachment 30967 [details] RenderTableSection.cpp : correct comment If you were a committer already I would just r+ this and you could add a ChangeLog when landing. I think this could be combined with any one of the other patches, but r- since this can't land as-is w/o changelog.
Comment on attachment 30968 [details] CSSParser.cpp/.h - move parsing of 'attr(X)' to own method (fixed .patch) Looks great! r- for lack of ChangeLog. :(
These are fantastic patches btw. I am a *huge* fan of cleanup and think it's a great way to learn a new source base. Welcome to WebKit! I look forward to you updated patches.
Your posted ChangeLog is great, but since these are so distinct it's best to land them separately. Again, if you were already a committer this kind of review would be easier as it would not be blocked on posting a perfect landable patch for someone else to land.
It's a nit, but it'd be nice to have a more descriptive bug description...