Bug 26198 – Misc. code cleanup and simplification

• Status: RESOLVED
• Resolution: INVALID
• Component: WebCore Misc.

[Roland Steiner]

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).

[Roland Steiner]

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)')

[Roland Steiner]

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.

[Roland Steiner]

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.

[Roland Steiner]

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)

[Roland Steiner]

Created attachment 30967 [details]
RenderTableSection.cpp : correct comment

[Roland Steiner]

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

[Roland Steiner]

Created attachment 30973 [details]
ChangeLog entry

Please adapt the ChangeLog depending on which patches make/don't make it in.

[Sam Weinig]

These are all unrelated cleanups and thus should have separate ChangeLogs.  It is also preferable to add the ChangeLog with the patch.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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. :(

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Jeremy Orlow]

It's a nit, but it'd be nice to have a more descriptive bug description...