Bug 53476 – Fix some Visual Studio compiler warnings

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

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".

[Darin Fisher (:fishd, Google)]

Created attachment 80721 [details]
v1 patch

[Eric Seidel (no email)]

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 on attachment 80721 [details]
v1 patch

Clearing flags on attachment: 80721

Committed r77242: <http://trac.webkit.org/changeset/77242>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 80721 [details]:

animations/play-state-suspend.html bug 50959 (author: [e-mail])
The commit-queue is continuing to process your patch.

[Darin Fisher (:fishd, Google)]

(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)]

(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. :)