Bug 22399 - Add WML <anchor> support.
Summary: Add WML <anchor> support.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 20393
  Show dependency treegraph
 
Reported: 2008-11-21 06:01 PST by Nikolas Zimmermann
Modified: 2008-11-22 07:51 PST (History)
0 users

See Also:


Attachments
Initial patch (17.85 KB, patch)
2008-11-21 06:01 PST, Nikolas Zimmermann
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2008-11-21 06:01:09 PST
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.
Comment 1 Nikolas Zimmermann 2008-11-21 06:01:53 PST
Created attachment 25349 [details]
Initial patch
Comment 2 Alexey Proskuryakov 2008-11-21 06:48:59 PST
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
Comment 3 Nikolas Zimmermann 2008-11-21 06:56:28 PST
(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.
Comment 4 Nikolas Zimmermann 2008-11-22 07:49:28 PST
Landed in r38658.