Bug 22477 - Simplify task registration and event handler construction in WML
Summary: Simplify task registration and event handler construction in WML
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-24 19:54 PST by Nikolas Zimmermann
Modified: 2008-11-25 17:15 PST (History)
0 users

See Also:


Attachments
Initial patch (26.90 KB, patch)
2008-11-24 19:56 PST, Nikolas Zimmermann
sam: 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-24 19:54:39 PST
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.
Comment 1 Nikolas Zimmermann 2008-11-24 19:56:52 PST
Created attachment 25469 [details]
Initial patch
Comment 2 Sam Weinig 2008-11-25 16:54:54 PST
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
Comment 3 Nikolas Zimmermann 2008-11-25 17:07:50 PST
(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.
Comment 4 Nikolas Zimmermann 2008-11-25 17:15:14 PST
Landed in r38773.