Bug 33317 - Document::minimumLayoutDelay() should return unsigned, rather than int
Summary: Document::minimumLayoutDelay() should return unsigned, rather than int
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Steve Block
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-07 03:34 PST by Steve Block
Modified: 2010-06-10 20:36 PDT (History)
4 users (show)

See Also:


Attachments
Patch 1 for Bug 33317 (13.69 KB, patch)
2010-01-07 10:10 PST, Steve Block
levin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2010-01-07 03:34:21 PST
Document::minimumLayoutDelay() should return unsigned, rather than int. Correspondingly, the variables and helper functions used in this method should be updated to use unsigned. See Bug 32875.
Comment 1 Steve Block 2010-01-07 10:10:03 PST
Created attachment 46061 [details]
Patch 1 for Bug 33317
Comment 2 WebKit Review Bot 2010-01-07 10:12:18 PST
style-queue ran check-webkit-style on attachment 46061 [details] without any errors.
Comment 3 Darin Adler 2010-01-07 10:14:07 PST
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?
Comment 4 Steve Block 2010-01-07 10:31:52 PST
(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.
Comment 5 Darin Adler 2010-01-10 20:52:24 PST
(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 6 David Levin 2010-01-21 22:48:24 PST
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?
Comment 7 Steve Block 2010-01-22 02:11:38 PST
> 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.