[webkit-reviews] review granted: [Bug 22399] Add WML <anchor> support. : [Attachment 25349] Initial patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 22399: Add WML <anchor> support.
https://bugs.webkit.org/show_bug.cgi?id=22399

Attachment 25349: Initial patch
https://bugs.webkit.org/attachment.cgi?id=25349&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> +	   // <anchor> element don't have href attributes, but we still want to

> +	   // appear as link, so linkAttribute() has to return a non-null
value!

Bad grammar here. Also, this hack seems bad for accessibility - but that's
probably not a practical concern for WML.

> +    // TODO: Enable when WMLImageElement exists

We don't use TODO, please replace with FIXME. Also, it's not clear from the
comment what should be enabled when WMLImageElement exists!

> +    else if (m_innerURLElement->hasTagName(WMLNames::aTag))
> +	   urlString = m_innerURLElement->getAttribute(hrefAttr);

I don't see A element mentioned anywhere in ChangeLog, please do add that.

> +	  WMLElement::defaultEventHandler(event); // Skip
WMLAElement::defaultEventHandler, on purpose

Please explain the purpose.

> +    WMLTaskElement* m_task;

Does this merit an explanation that WML elements are never destroyed while the
document exists, so this cannot result in stale pointer? Maybe anyone working
on WML knows this though.

> +// #include "WMLSetvarElement.h"

Is there a need to check in commented out code? There is a lot in this file.
Not a big deal if you are going to uncomment soon.

> +	   /* TODO

We don't use TODO.


> +String substituteVariableReferences(const String& variableReference,
Document* document, WMLVariableEscapingMode escapeMode)
> +{
> +    // TODO!
> +    return variableReference;
> +}

You could use notImplemented() here.

r=me