Created attachment 80721 [details] v1 patch

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?

Comment on attachment 80721 [details] v1 patch Clearing flags on attachment: 80721 Committed r77242: <http://trac.webkit.org/changeset/77242>

All reviewed patches have been landed. Closing bug.

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.

(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?

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