Bug 22477 – Simplify task registration and event handler construction in WML

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

[Nikolas Zimmermann]

A WMLTaskElement checks wheter it's parent is an <anchor>/<do>/<onevent> element, and calls registerTask(this) on the parent.
Add a isWMLTaskElement() function to WMLElement, making it possible to cast to WMLTaskElement directly.

Add WMLEventHandlingElement class, inheriting from WMLElement, to serve as common base WMLTemplate/Option/CardElement, centralizing the eventHandler() / createEventHandlerIfNeeded() implementation. Add a isWMLEventHandlingElement() function to WMLElement, so WMLOnEventElement can create event handlers, without knowing wheter it's a <template>/<option> or <card> element.

Add complete <onevent> support.

[Nikolas Zimmermann]

Created attachment 25469 [details]
Initial patch

[Sam Weinig]

Comment on attachment 25469 [details]
Initial patch

>              // Force frame loader to load the URL with fragment identifier
> -            if (FrameLoader* loader = page->mainFrame() ? page->mainFrame()->loader() : 0)
> -                loader->setForceReloadWmlDeck(true);
> +            if (Frame* frame = pageState->page()->mainFrame())
> +                if (FrameLoader* loader = frame->loader())
> +                    loader->setForceReloadWmlDeck(true);
The outer if needs braces.

>  private:
> -    void setVisibility(bool isVisible);
> +    bool isVisible() const { return m_isVisible; }
> +    void setVisibility(bool isVisible) { m_isVisible = isVisible; }
I think this would be better as setVisible(bool).

> +void WMLOnEventElement::parseMappedAttribute(MappedAttribute* attr)
> +{
> +    if (attr->name() == HTMLNames::typeAttr) {
> +        const AtomicString& value = attr->value();
> +        if (containsVariableReference(value)) {
> +            // FIXME: error reporting
> +            // WMLHelper::tokenizer()->reportError(InvalidVariableReferenceError);
I don't think this commented out code adds much. If this is just about alerting the user, please use the InspectorController hanging off the Page.

> +    RefPtr<WMLIntrinsicEvent> event = WMLIntrinsicEvent::createWithTask(task);
> +    eventHandlingElement->eventHandler()->registerIntrinsicEvent(m_type, event);
You can use event.releaseRef() to avoid some churn here.

r=me

[Nikolas Zimmermann]

(In reply to comment #2)
> (From update of attachment 25469 [details] [review])
> >              // Force frame loader to load the URL with fragment identifier
> > -            if (FrameLoader* loader = page->mainFrame() ? page->mainFrame()->loader() : 0)
> > -                loader->setForceReloadWmlDeck(true);
> > +            if (Frame* frame = pageState->page()->mainFrame())
> > +                if (FrameLoader* loader = frame->loader())
> > +                    loader->setForceReloadWmlDeck(true);
> The outer if needs braces.
Fixed,
 
> >  private:
> > -    void setVisibility(bool isVisible);
> > +    bool isVisible() const { return m_isVisible; }
> > +    void setVisibility(bool isVisible) { m_isVisible = isVisible; }
> I think this would be better as setVisible(bool).
Ok, I'll rename.
 
> > +void WMLOnEventElement::parseMappedAttribute(MappedAttribute* attr)
> > +{
> > +    if (attr->name() == HTMLNames::typeAttr) {
> > +        const AtomicString& value = attr->value();
> > +        if (containsVariableReference(value)) {
> > +            // FIXME: error reporting
> > +            // WMLHelper::tokenizer()->reportError(InvalidVariableReferenceError);
> I don't think this commented out code adds much. If this is just about alerting
> the user, please use the InspectorController hanging off the Page.
I'd like to keep it, as most other classes also contain these statements. It could also say "// FIXME: Report 'InvalidVariableReferenceError'", it's just to remind me of adding error support :-)
 
> > +    RefPtr<WMLIntrinsicEvent> event = WMLIntrinsicEvent::createWithTask(task);
> > +    eventHandlingElement->eventHandler()->registerIntrinsicEvent(m_type, event);
> You can use event.releaseRef() to avoid some churn here.
Fixed.

Going to land soon, thanks for the review.

[Nikolas Zimmermann]

Landed in r38773.