Add WML <timer> element support.
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.