Summary: | Add WML <timer> element support | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
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-28 19:22:09 PST
Created attachment 25581 [details]
Initial patch
Created attachment 25582 [details]
Layout test results
Comment on attachment 25581 [details]
Initial patch
The comment
// The value of timeout must be a positive integral number, or ignore it
would probably be more clear as
// If the value of timeout is not a positive integer, ignore it
In WMLTimerElement::insertedIntoDocument(), you have both
ASSERT(parent);
and
if (!parent || !parent->isWMLElement())
You should decide if parent should ever be null and make a decision accordingly.
+ if (interval <= 0)
+ interval = m_value.toInt();
+
+ if (interval > 0)
+ m_timer.startOneShot(interval / 10.0f);
Can't the second conditional be an else?
Oops, I forgot to say: if you address those comments, r=me. > > // If the value of timeout is not a positive integer, ignore it Fixed. > > In WMLTimerElement::insertedIntoDocument(), you have both > > ASSERT(parent); > > and > > if (!parent || !parent->isWMLElement()) > > You should decide if parent should ever be null and make a decision > accordingly. Yeah, Holger also noted this while reviewing the <do> patch. I'll come up with a follow-up patch fixing these, as there are 7 other places which use the same piece of code. > > + if (interval <= 0) > + interval = m_value.toInt(); > + > + if (interval > 0) > + m_timer.startOneShot(interval / 10.0f); > > Can't the second conditional be an else? The interval may change inbetween? Thanks for the first review Created attachment 25583 [details]
Updated layout test results
(In reply to comment #5) > > In WMLTimerElement::insertedIntoDocument(), you have both > > > > ASSERT(parent); > > > > and > > > > if (!parent || !parent->isWMLElement()) > > > > You should decide if parent should ever be null and make a decision > > accordingly. > Yeah, Holger also noted this while reviewing the <do> patch. I'll come up with > a follow-up patch fixing these, as there are 7 other places which use the same > piece of code. If it occurs elsewhere, a followup patch is okay. Be sure to do it soon, though. > > + if (interval <= 0) > > + interval = m_value.toInt(); > > + > > + if (interval > 0) > > + m_timer.startOneShot(interval / 10.0f); > > > > Can't the second conditional be an else? > The interval may change inbetween? Wow, I'm stupid. Sorry about that. ;-) Created attachment 25584 [details]
Updated patch
Comment on attachment 25584 [details]
Updated patch
r=me
Comment on attachment 25583 [details]
Updated layout test results
r=me
Landed in r38838. |