| Summary: | Document::minimumLayoutDelay() should return unsigned, rather than int | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Steve Block <steveblock> | ||||
| Component: | WebCore Misc. | Assignee: | Steve Block <steveblock> | ||||
| Status: | ASSIGNED --- | ||||||
| Severity: | Normal | CC: | darin, ddkilzer, steveblock, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Attachments: |
|
||||||
|
Description
Steve Block
2010-01-07 03:34:21 PST
Created attachment 46061 [details] Patch 1 for Bug 33317 style-queue ran check-webkit-style on attachment 46061 [details] without any errors.
Comment on attachment 46061 [details] Patch 1 for Bug 33317 The other changes seem ideal, but I am slightly worried about elapsedTime. Are you sure we won't change behavior here by turning "moving backward in time" cases that might occur due to time changes into "move forward by MAX_UINT" cases? (In reply to comment #3) > (From update of attachment 46061 [details]) > The other changes seem ideal, but I am slightly worried about elapsedTime. Are > you sure we won't change behavior here by turning "moving backward in time" > cases that might occur due to time changes into "move forward by MAX_UINT" > cases? Good point. Since the result of elapsedTime is always intended to be non-negative, presumably the right thing to do is to have it return the following? currentTime() > m_startTime ? currentTime() - m_startTime : 0 This is a behavior change from "moving backward in time" to "no movement in time", but makes it more explicit. (In reply to comment #4) > (In reply to comment #3) > Good point. Since the result of elapsedTime is always intended to be > non-negative, presumably the right thing to do is to have it return the > following? > > currentTime() > m_startTime ? currentTime() - m_startTime : 0 > > This is a behavior change from "moving backward in time" to "no movement in > time", but makes it more explicit. Sure, seems good. The key is probably to find a way to test it. Comment on attachment 46061 [details] Patch 1 for Bug 33317 r- for two reasons: 1. This patch/changelog doesn't mention why this needs to be done only what (and I personally hate unsigned -- been burned in the past by subtle bugs of the nature that Darin mentioned). If there was a "why", perhaps we could discuss alternatives :) 2. Is there a way to address Darin Adler's last comment? > 1. This patch/changelog doesn't mention why this needs to be done only what The only motivation was Eric's comment on the bug in which I added minimumLayoutDelay. See https://bugs.webkit.org/show_bug.cgi?id=32875#c14 and later comments in that bug. > 2. Is there a way to address Darin Adler's last comment? I haven't yet looked into testing. |