[webkit-reviews] review denied: [Bug 20393] Add WML support to WebKit : [Attachment 23850] WML directory patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied George Staikos <staikos at kde.org>'s
request for review:
Bug 20393: Add WML support to WebKit
https://bugs.webkit.org/show_bug.cgi?id=20393

Attachment 23850: WML directory patch
https://bugs.webkit.org/attachment.cgi?id=23850&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
Ok, here are a few thoughts.

I'm not really sure how we go about landing such a large hunk of code. 
Obviously the parts that touch the core code will need to be wrapped in
ENABLE(WML).

I'm not "sure" that WebKit wants WML support, but I also don't see reasons to
suggest that it does not.  I sorta thought WML was dead.

200k is too large of a patch for me to really review fully, but I'll at least
provide some thoughts on the code.

Minor style violations:
69     }
 70	else
(I hate that we even have to care about these!	Tools should take care of this
for us!)

I'm not really a fan of the "Helper" paradigm.	Helpers tend to be
misc-buckets.  It seems this one is too.

WMLHelper::tokenizer()->
So I guess this is basically just a global shared tokenizer?  We haven't
handled error reporting for SVG either, but I think normally the way you access
the tokenizer is through your document object.	Which would mean we don't need
this global here.

WMLHelper::currentBrwContext()->
What's a BrwContext?  It's not clear from the code.  I'm not sure I found the
right WML spec, but I searched for Brw and didn't find it mentioned anywhere.

I'm not sure this belongs on "WMLHelper" either:
WMLHelper::hasVariableRef(
Why not WMLElement?

Minor style:
 43	: HTMLAnchorElement(name, doc), m_task(0)
Those are generally 1-per-line.  Again, its *stupid* that you, the programmer,
should have to care about this... a script should handle this...

Whoever wrote this seems to like to add extra lines at the end of classes too:
 private:
 42 
 43	WMLTaskElement* m_task;
 44 
 45 };

FYI, there is a patch posted to change this syntax:
4 generateFactory="1"
 5 generateWrapperFactory="1"

I would think normal assignment should be sufficient here?
    for (VariableMap::iterator it = vars.begin(); it != vars.end(); ++it) { 
 77	    m_variables.set(it->first, it->second); 
 78

So does WMLBrwContext have the same lifetime as the page?  aka, the WebView?
 41 WMLBrwContext::WMLBrwContext(Page* page)

Why is this safe?
void WMLBrwContext::setNeedCheckDeckAccess(bool need)
 124 {
 125	 if (m_hasDeckAccess && need) {
 126	     WMLHelper::tokenizer()->reportError(MultiAccessElementsError);
 127
Are we guarenteed to still be parsing at that time?  In the HTML/XML world,
your tokenizer might not still be alive once you're done parsing.

So it seems Brw is actually an abbreviation for "Browser" and Brw is actually
just an extension of Page, to handle a few WML-specific attributes.  Seems this
all should go on Page as part of some WMLController sorta thing.

 57	, m_hasIntrEvent(false)
no need to abbreviate here.

Seems this should be an OwnPtr:
 73	WMLIntrinsicEventHandler* m_evntHandler;

Generally we encourage longer/more-descriptive naming:
 75	WMLIntrEventType etype = Unknown;

eventType in this case.

WebKit, for better or worse, is against { } on single line ifs:
    if (attr->name() == titleAttr) {
 78	    m_title = WMLHelper::substituteVarRef(attr->value(), document());
 79	} else if (attr->name() == onenterforwardAttr) {
 80
again, it's stupid that you should even have to think about that... since a
script shoudl do that for you.

Instead of using an enum and then an if:
 73 void WMLCardElement::parseMappedAttribute(MappedAttribute* attr)

should be changed to have 
 92	if (etype != Unknown) {

be a function which is called. 

 95	    Element* go = WMLElementFactory::createWMLElement(qName,
document(), false); 
Should use a RefPtr, no?  createWMLElement shoudl return a PassRefPtr

 94	    QualifiedName qName("go", "go", "");
I don't think you meant for that to be "go:go", also why not use:
WMLNames::goTag?

 97	    go->setAttributeNS("", "href", href, ec);
setAttribute?

 100	     ev.task = static_cast<WMLGoElement*>(go);
One could have called WMLElementFactory::createGoElement() directly, or?

Why is visability tracked by hand?
 120 void WMLCardElement::setVisibility(bool isVisible) 
instead of using css styling?

Seems odd to clear the old timer on error:
75     // only one timer is allowed in a card 
 176	 if (!m_evtTimer) {
 177	     m_evtTimer = timer;
 178	 } else {
 179	     m_evtTimer = 0;
 180	     WMLHelper::tokenizer()->reportError(MultiTimerElementsError);
 181	 }

Please use early-return instead of a long if:
void WMLCardElement::registerDoElement(WMLDoElement* e)
 190 {
 191	 if (e) {
 192	     Vector<WMLDoElement*>::iterator it = m_doVecs.begin();
 193

Please use more descriptive variable names:
 193	     WMLDoElement* doe = 0;

Same issue as before about safely of accessing tokenizer, and that it probably
should be accessed through the document() instead of via this global:
	    if (doe->doName() == e->doName()) {
 198		    
WMLHelper::tokenizer()->reportError(SameNameDoElementsError);
 199


We use full english sentence names instead of abbreviations when possible:
 235 void WMLCardElement::handleIntrEventIfNeeded()

eventType:
 242	 WMLIntrEventType etype;

Same as before:
 235 void WMLCardElement::handleIntrEventIfNeeded()

instead of using an enum, and then if's this should have used function calls. 
Seems like a strange paradigm to me.

I got about 1/5th of the way through teh patch,but already there are plenty of
comments to act on.

Thanks for the patch, but I think we should digest it in smaller chunks. 
Landing large sections of code which were developed outside of the main tree is
*hard*	(I know, believe me... I'm doing ti righ tnow for Chrome), but I do
think there is value in the review process.

If you wanted to land this all on a branch and then work to update the branch
over a set of reviews, that would be fine with me too.