[Webkit-unassigned] [Bug 20393] Add WML support to WebKit

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20393





------- Comment #2 from mrowe at apple.com  2008-08-15 06:42 PDT -------
(From update of attachment 22813)
I've just skimmed over the patch briefly, but have a few comments.

ASCIICType.h currently uses character literals for character codes of printable
characters, and hex literals for non-printable characters.  New code should
follow this style.

The naming on some new functions and variables uses inconsistent case.  For
example, defaultWmlStyle vs isWMLDocument.  The latter style is preferred.

It seems bad that knowledge of WML is sprinkled throughout XMLTokenizer.  There
also appears to be code (lines 2134 through 2143) outside of any functions,
which would not compile.

Including code that requires a third-party library (WBXML) directly in
ResourceLoader doesn't seem like a great idea either.

Destructors in new classes appear to be inconsistently declared as virtual, and
many are declared and then left empty.  Empty destructors should be omitted.

There are numerous coding style issues with the patch as well.  I would prefer
to see them addressed before the code is landed, primarily because I do not
foresee this code getting as much love and maintenance as the rest of the code
base going forward.

The primary issues that I see are:

1) Missing space between "if" and the opening parenthesis.
2) Unnecessary parentheses around some expressions in the conditions of "if"
statements.
3) Many new .cpp files have incorrect ordering of include files.  The order
should be config.h, the associated header file, a blank line, and then the rest
of the includes in sorted order.  See existing .cpp files for an example.
4) One-line "if" statements should not have braces.
5) The placement of braces in function declarations is inconsistent.  Braces
should be placed on their own line in function declarations.
6) The constant 0 should be used rather than NULL.
7) Enum members should user InterCaps with an initial capital letter, rather
than ALLCAPS.

All that said, I love the pile of layout tests!  Are expected results also
going to be included?


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.