Bug 22399 – Add WML <anchor> support.

• Component: XML
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

WML <anchor> support missing at the moment. <anchor> is similar to <a>.

WML anchor element is used like this: <anchor>Foobar<prev/></anchor>, similar to <a onclick="history.back()">Foobar</a> in HTML.

[Nikolas Zimmermann]

Created attachment 25349 [details]
Initial patch

[Alexey Proskuryakov]

Comment on attachment 25349 [details]
Initial patch

> +        // <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

[Nikolas Zimmermann]

(In reply to comment #2)
> (From update of attachment 25349 [details] [review])
> > +        // <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.
Fixed.

> 
> > +    // 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!
Maybe it shouldn't be available at all, we don't need image drag support for WML. Removing the whole ENABLE(WML) block.
 
> > +    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.
a is already in svn.

> 
> > +       WMLElement::defaultEventHandler(event); // Skip WMLAElement::defaultEventHandler, on purpose
> 
> Please explain the purpose.
Okay.

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

Yes that's the assumption. We could use insertedIntoDocument/removedFromDocument to register/deregister from the parent, but we need at least a working WML prototype usable for testing these issues first.

> 
> > +// #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.
In the next patch this is included :-)

> 
> > +        /* TODO
> 
> We don't use TODO.
> 
> 
> > +String substituteVariableReferences(const String& variableReference, Document* document, WMLVariableEscapingMode escapeMode)
> > +{
> > +    // TODO!
> > +    return variableReference;
> > +}
> 
> You could use notImplemented() here.
It's already used in code that's not included in this patch. Just variable substitution has not been ported yet. I'll replace all instances of TODO to FIXME.

Thanks for the review.

[Nikolas Zimmermann]

Landed in r38658.