Summary: | Simplify task registration and event handler construction in WML | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||
Component: | XML | Assignee: | Nikolas Zimmermann <zimmermann> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | ||||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Mac | ||||||
OS: | OS X 10.5 | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 20393 | ||||||
Attachments: |
|
Description
Nikolas Zimmermann
2008-11-24 19:54:39 PST
Created attachment 25469 [details]
Initial patch
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 (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. |