Bug 60371

Summary: Warning fix on PluginPackage.cpp.
Product: WebKit Reporter: Alexis Menard (darktears) <menard>
Component: Plug-insAssignee: Alexis Menard (darktears) <menard>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ademar, apavlov, commit-queue, darin, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Alexis Menard (darktears) 2011-05-06 07:15:31 PDT
Warning fix on PluginPackage.cpp.
Comment 1 Alexis Menard (darktears) 2011-05-06 07:16:41 PDT
Created attachment 92581 [details]
Patch
Comment 2 Alexis Menard (darktears) 2011-05-06 07:17:21 PDT
Is there a better way to fix this?
Comment 3 Antonio Gomes 2011-05-06 07:18:14 PDT
Comment on attachment 92581 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Warning fix on conversion from time_t to unsigned.

you could say on what platform and compiler it warns :)
Comment 4 Alexis Menard (darktears) 2011-05-06 07:51:49 PDT
Created attachment 92583 [details]
Patch
Comment 5 Darin Adler 2011-05-06 09:51:07 PDT
Comment on attachment 92583 [details]
Patch

This is probably not a good fix. If time_t is not an unsigned then we probably aren’t hashing the whole thing. I think we need a better fix here rather than just quieting the warning.
Comment 6 Alexis Menard (darktears) 2011-05-09 15:51:58 PDT
(In reply to comment #5)
> (From update of attachment 92583 [details])
> This is probably not a good fix. If time_t is not an unsigned then we probably aren’t hashing the whole thing. I think we need a better fix here rather than just quieting the warning.

Hi Darin,

time_t can be a integer (because time() can return -1 when the time can't be read) or a real floating type in some OS. Apparently some embedded systems also have a unsigned 32 bit version of it (so dates prior to 1970 are not represented).

unsigned on my 64 bits machine is 4 bytes.

The StringHasher::hashMemory<sizeof(hashCodes)>(hashCodes); always takes care of the size of the data going in. So I believe we can just modify hasCodes to store a time_t array or a uint64_t?
Comment 7 Darin Adler 2011-05-09 16:48:34 PDT
(In reply to comment #6)
> The StringHasher::hashMemory<sizeof(hashCodes)>(hashCodes); always takes care of the size of the data going in. So I believe we can just modify hasCodes to store a time_t array or a uint64_t?

Yes, I think we can change this to be a struct with two members of the appropriate types rather than an array.
Comment 8 Eric Seidel (no email) 2011-05-09 20:40:03 PDT
Comment on attachment 92583 [details]
Patch

OK.
Comment 9 Eric Seidel (no email) 2011-05-09 20:40:50 PDT
Comment on attachment 92583 [details]
Patch

Hmm.. Sounds liek Darin would like a differnt fix.
Comment 10 Alexis Menard (darktears) 2011-05-10 06:00:58 PDT
Created attachment 92942 [details]
Patch
Comment 11 Alexis Menard (darktears) 2011-05-10 06:03:03 PDT
Created attachment 92943 [details]
Patch
Comment 12 WebKit Commit Bot 2011-05-10 08:25:19 PDT
Comment on attachment 92943 [details]
Patch

Clearing flags on attachment: 92943

Committed r86153: <http://trac.webkit.org/changeset/86153>
Comment 13 WebKit Commit Bot 2011-05-10 08:25:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 WebKit Commit Bot 2011-05-10 09:11:18 PDT
The commit-queue encountered the following flaky tests while processing attachment 92943 [details]:

http/tests/xmlhttprequest/encode-request-url-2.html bug 51765 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 15 WebKit Review Bot 2011-05-10 10:20:13 PDT
http://trac.webkit.org/changeset/86153 might have broken Leopard Intel Debug (Tests)
The following tests are not passing:
fast/lists/003-vertical.html
fonts/cursive.html
Comment 16 Alexis Menard (darktears) 2011-05-10 12:13:07 PDT
Anyone with a Leopard Debug build around to help please? I don't have a Mac machine.
Comment 17 Ademar Reis 2011-05-10 13:16:55 PDT
Revision r86153 cherry-picked into qtwebkit-2.2 with commit 2317327 <http://gitorious.org/webkit/qtwebkit/commit/2317327>