Bug 53476

Summary: Fix some Visual Studio compiler warnings
Product: WebKit Reporter: Darin Fisher (:fishd, Google) <fishd>
Component: WebKit Misc.Assignee: Darin Fisher (:fishd, Google) <fishd>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
v1 patch none

Darin Fisher (:fishd, Google)
Reported 2011-01-31 23:45:42 PST
Chromium builds WebKit without C4244 suppressed, and we trip over that in a number of places. This warning is about converting from one type to a smaller type. I see this a bunch for conversions from "int" to "float".
Attachments
v1 patch (7.91 KB, patch)
2011-01-31 23:48 PST, Darin Fisher (:fishd, Google)
no flags
Darin Fisher (:fishd, Google)
Comment 1 2011-01-31 23:48:13 PST
Created attachment 80721 [details] v1 patch
Eric Seidel (no email)
Comment 2 2011-02-01 02:42:58 PST
Comment on attachment 80721 [details] v1 patch View in context: https://bugs.webkit.org/attachment.cgi?id=80721&action=review looks fine. > Source/WebKit/chromium/src/WebFrameImpl.cpp:689 > + for (size_t sourceIndex = 0; j < forms->length(); ++sourceIndex) { I don't understand why renaming i to j here, but OK... > Source/WebKit/chromium/src/WebViewImpl.cpp:1619 > + float zoomFactor = static_cast<float>(zoomLevelToZoomFactor(m_zoomLevel)); Maybe this should have float in its name some where?
WebKit Commit Bot
Comment 3 2011-02-01 04:42:28 PST
Comment on attachment 80721 [details] v1 patch Clearing flags on attachment: 80721 Committed r77242: <http://trac.webkit.org/changeset/77242>
WebKit Commit Bot
Comment 4 2011-02-01 04:42:32 PST
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 5 2011-02-01 05:36:16 PST
The commit-queue encountered the following flaky tests while processing attachment 80721 [details]: animations/play-state-suspend.html bug 50959 (author: cmarrin@apple.com) The commit-queue is continuing to process your patch.
Darin Fisher (:fishd, Google)
Comment 6 2011-02-01 08:41:38 PST
(In reply to comment #2) > (From update of attachment 80721 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80721&action=review > > looks fine. > > > Source/WebKit/chromium/src/WebFrameImpl.cpp:689 > > + for (size_t sourceIndex = 0; j < forms->length(); ++sourceIndex) { > > I don't understand why renaming i to j here, but OK... Sorry, I forgot to mention this case. It is not the same class of warning. In this case, the compiler is complaining because "i" is re-declared. The compiler (VS2005) sees the "for (size_t i = 0;...)" above, and then it sees us using "i" again outside of the loop. The compiler thinks we want old-style scoping of for-loop initializers, even though "i" is re-declared outside of the for-loop!!! Just changing the variable name seemed like the simplest way to make the compiler happy. > > > Source/WebKit/chromium/src/WebViewImpl.cpp:1619 > > + float zoomFactor = static_cast<float>(zoomLevelToZoomFactor(m_zoomLevel)); > > Maybe this should have float in its name some where? Are you suggesting that zoomFactor be renamed to zoomFactorFloat? I hadn't considered naming the variable like that. Is that conventional in WebKit?
Eric Seidel (no email)
Comment 7 2011-02-01 12:36:48 PST
(In reply to comment #6) > > > Source/WebKit/chromium/src/WebViewImpl.cpp:1619 > > > + float zoomFactor = static_cast<float>(zoomLevelToZoomFactor(m_zoomLevel)); > > > > Maybe this should have float in its name some where? > > Are you suggesting that zoomFactor be renamed to zoomFactorFloat? I hadn't considered naming the variable like that. Is that conventional in WebKit? I don't know whether folks use typeFooBar or fooBarType. But I've seen that done in rendering code or places where we need both types for different functiosn and want to keep them straight. In this case it's slightly strange to have a float local when we have a funtion which returns a double and a m_zoomLevel double. but I udnerstand why you did it. I guess other ports use double? Anyway, I think it's fine as is. :)
Note You need to log in before you can comment on or make changes to this bug.