Bug 22550

Summary: Add WML <timer> element support
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: XMLAssignee: 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 Flags
Initial patch
zwarich: review+
Layout test results
none
Updated layout test results
zwarich: review+
Updated patch zwarich: review+

Nikolas Zimmermann
Reported 2008-11-28 19:22:09 PST
Add WML <timer> element support.
Attachments
Initial patch (15.21 KB, patch)
2008-11-28 19:32 PST, Nikolas Zimmermann 		zwarich: review+
DetailsFormatted DiffDiff
Layout test results (53 bytes, patch)
2008-11-28 19:32 PST, Nikolas Zimmermann 		no flags
DetailsFormatted DiffDiff
Updated layout test results (3.73 KB, patch)
2008-11-28 19:55 PST, Nikolas Zimmermann 		zwarich: review+
DetailsFormatted DiffDiff
Updated patch (18.83 KB, patch)
2008-11-28 19:57 PST, Nikolas Zimmermann 		zwarich: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Nikolas Zimmermann
Comment 1 2008-11-28 19:32:17 PST
Created attachment 25581 [details] Initial patch
Nikolas Zimmermann
Comment 2 2008-11-28 19:32:38 PST
Created attachment 25582 [details] Layout test results
Cameron Zwarich (cpst)
Comment 3 2008-11-28 19:43:37 PST
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?
Cameron Zwarich (cpst)
Comment 4 2008-11-28 19:44:02 PST
Oops, I forgot to say: if you address those comments, r=me.
Nikolas Zimmermann
Comment 5 2008-11-28 19:53:40 PST
> > // 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
Nikolas Zimmermann
Comment 6 2008-11-28 19:55:40 PST
Created attachment 25583 [details] Updated layout test results
Cameron Zwarich (cpst)
Comment 7 2008-11-28 19:57:15 PST
(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. ;-)
Nikolas Zimmermann
Comment 8 2008-11-28 19:57:23 PST
Created attachment 25584 [details] Updated patch
Cameron Zwarich (cpst)
Comment 9 2008-11-28 19:58:22 PST
Comment on attachment 25584 [details] Updated patch r=me
Cameron Zwarich (cpst)
Comment 10 2008-11-28 19:58:32 PST
Comment on attachment 25583 [details] Updated layout test results r=me
Nikolas Zimmermann
Comment 11 2008-11-28 20:00:38 PST
Landed in r38838.
Note You need to log in before you can comment on or make changes to this bug.