[webkit-reviews] review granted: [Bug 22477] Simplify task registration and event handler construction in WML : [Attachment 25469] Initial patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 22477: Simplify task registration and event handler construction in WML
https://bugs.webkit.org/show_bug.cgi?id=22477

Attachment 25469: Initial patch
https://bugs.webkit.org/attachment.cgi?id=25469&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
>	       // 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