Bug 49066

Summary: Convert to and from DOMTimeStamp with converter functions
Product: WebKit Reporter: John Knottenbelt <jknotten>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andreip, commit-queue, dbates, eric, sam, steveblock
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 48518    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description John Knottenbelt 2010-11-05 04:50:23 PDT
At present, DOMTimeStamp is defined in WebCore/dom/Event.h, with a FIXME to move somewhere else. 

It should be moved into its own header and provide conversion functions to convert to and from seconds as provided by WTF::currentTime().

We should change existing conversions (multiply / divide by 1000) to use the conversion functions.
Comment 1 John Knottenbelt 2010-11-05 04:53:54 PDT
Created attachment 73055 [details]
Patch
Comment 2 Steve Block 2010-11-05 06:56:21 PDT
Comment on attachment 73055 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73055&action=review

LGTM, but would be good to get an OK from somebody else too.

> WebCore/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=49066

Add a line about LayoutTests - just say this is a refactoring only.
Comment 3 Andrei Popescu 2010-11-05 07:29:23 PDT
You forgot to update the build files for Visual Studio, XCode, etc.
Comment 4 John Knottenbelt 2010-11-05 09:23:08 PDT
Created attachment 73071 [details]
Patch
Comment 5 John Knottenbelt 2010-11-05 09:28:20 PDT
Created attachment 73072 [details]
Patch
Comment 6 John Knottenbelt 2010-11-05 09:29:01 PDT
Thanks, change log modified and build files attached.
Comment 7 Adam Barth 2010-11-05 10:26:53 PDT
Comment on attachment 73072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73072&action=review

Thanks!  Much easier to understand.  :)

> WebCore/platform/android/GeolocationServiceAndroid.cpp:175
> -    DOMTimeStamp currentTimeMillis = WTF::currentTime() * 1000.0;
> -    DOMTimeStamp maximumAgeMillis = 10 * 60 * 1000;  // 10 minutes
> +    DOMTimeStamp currentTimeMillis = convertSecondsToDOMTimeStamp(WTF::currentTime());
> +    DOMTimeStamp maximumAgeMillis = convertSecondsToDOMTimeStamp(10 * 60); // 10 minutes

Should we rename these variables?  currentTimeStamp, etc?
Comment 8 John Knottenbelt 2010-11-05 10:38:55 PDT
Created attachment 73079 [details]
Patch
Comment 9 John Knottenbelt 2010-11-05 10:41:12 PDT
Comment on attachment 73079 [details]
Patch

Renamed the variables in GeolocationServiceAndroid.cpp as per Adam's suggestion.
Comment 10 WebKit Commit Bot 2010-11-05 11:03:10 PDT
Comment on attachment 73079 [details]
Patch

Rejecting patch 73079 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
4/WebTextCompletionController.o /Projects/CommitQueue/WebKit/mac/WebView/WebTextCompletionController.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebKit.build/Debug/WebKit.build/Objects-normal/x86_64/WebView.o /Projects/CommitQueue/WebKit/mac/WebView/WebView.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2
(36 failures)

Use of uninitialized value $_[0] in join or string at /System/Library/Perl/5.10.0/File/Spec/Unix.pm line 81.

Full output: http://queues.webkit.org/results/5247018
Comment 11 Eric Seidel (no email) 2010-11-05 11:07:30 PDT
The perl-error is unrelated (I think).  But CCing Dan as this is not the first time I've seen this. It is something to do with File->catdir iirc.
Comment 12 John Knottenbelt 2010-11-08 07:28:35 PST
Looking at the build log, the it failed because DOMTimeStamp.h wasn't included in the Framework's private headers. Will include and try again.
Comment 13 John Knottenbelt 2010-11-08 07:35:54 PST
Created attachment 73238 [details]
Patch
Comment 14 John Knottenbelt 2010-11-08 07:58:58 PST
Comment on attachment 73238 [details]
Patch

In the WebCore Framework, I have now set the header role of DOMTimestamp.h to private. This allowed WebKit to build.
Comment 15 John Knottenbelt 2010-11-08 08:34:04 PST
Created attachment 73242 [details]
Patch
Comment 16 WebKit Commit Bot 2010-11-08 20:15:09 PST
Comment on attachment 73242 [details]
Patch

Clearing flags on attachment: 73242

Committed r71602: <http://trac.webkit.org/changeset/71602>
Comment 17 WebKit Commit Bot 2010-11-08 20:15:16 PST
All reviewed patches have been landed.  Closing bug.