Bug 22549 – Add WML <do> element support.

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Nikolas Zimmermann]

Add <do> element support, a general task element container element. Until now only <anchor> supported <go>/<prev>/<refresh> task child elements, <do> provides a way to register a task to a <template>/<card> element.

[Nikolas Zimmermann]

Created attachment 25578 [details]
Initial patch

[Nikolas Zimmermann]

Created attachment 25579 [details]
Updated patch

Incorporate comments from Holger.

[Nikolas Zimmermann]

Created attachment 25580 [details]
Layout test results

[Holger Freyther]

Comment on attachment 25580 [details]
Layout test results

Okay.

[Holger Freyther]

Comment on attachment 25579 [details]
Updated patch


> [phone]  Nikolas Zimmermann  <[e-mail]>
> +
> +        Reviewed by Holger Freyther

^^ haha ;)

> +void WMLDoElement::defaultEventHandler(Event* event)

I assume that you look at the right events?! But if you don't I'm pretty sure you will find that out sooner than later.

> +
> +    Node* parent = parentNode();
> +    ASSERT(parent);
> +
> +    if (!parent || !parent->isWMLElement())
> +        return;

Before landing decide if the parentNode might be zero or not and then remove the assert or the !parent from the if?

> +    bool m_isOption;

maybe call it m_isOptional? but that is optional...


from a grep in the source tree it looks like you uncommented all doTag users...

[Nikolas Zimmermann]

> > +
> > +        Reviewed by Holger Freyther
> 
> ^^ haha ;)
Psst :-)

> 
> > +void WMLDoElement::defaultEventHandler(Event* event)
> 
> I assume that you look at the right events?! But if you don't I'm pretty sure
> you will find that out sooner than later.
Yes, we'll find out soon - it worked this way in the internal codebase, so maybe it's correct as is already :-)

> 
> > +
> > +    Node* parent = parentNode();
> > +    ASSERT(parent);
> > +
> > +    if (!parent || !parent->isWMLElement())
> > +        return;
> 
> Before landing decide if the parentNode might be zero or not and then remove
> the assert or the !parent from the if?
Ok, as we discussed on IRC, there are 6 other places with exactly the same snippet. Will clean that up in another patch for all of them.
 
> > +    bool m_isOption;
> 
> maybe call it m_isOptional? but that is optional...
Done.

> from a grep in the source tree it looks like you uncommented all doTag users...
Yes.

Thanks for the review, landing soon.

[Nikolas Zimmermann]

Landed in r38837.