Bug 143313

Summary: REGRESSION (r182121): SVG animation macros cause warnings in MSVC
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: SVGAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, sabouhallawa, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed fix darin: review+

Description Alexey Proskuryakov 2015-04-01 12:00:39 PDT 
MSVC warns about what sounds like a real bug in SVG code:

C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\svg\SVGCursorElement.h(64): warning C4373: 'WebCore::SVGCursorElement::setExternalResourcesRequiredBaseValue': virtual function overrides 'WebCore::SVGExternalResourcesRequired::setExternalResourcesRequiredBaseValue', previous versions of the compiler did not override when parameters only differed by const/volatile qualifiers (..\css\CSSAllInOne.cpp) [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\svg\SVGFontElement.h(95): warning C4373: 'WebCore::SVGFontElement::setExternalResourcesRequiredBaseValue': virtual function overrides 'WebCore::SVGExternalResourcesRequired::setExternalResourcesRequiredBaseValue', previous versions of the compiler did not override when parameters only differed by const/volatile qualifiers (..\css\CSSAllInOne.cpp) [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\svg\SVGSVGElement.h(46): warning C4373: 'WebCore::SVGSVGElement::setExternalResourcesRequiredBaseValue': virtual function overrides 'WebCore::SVGExternalResourcesRequired::setExternalResourcesRequiredBaseValue', previous versions of the compiler did not override when parameters only differed by const/volatile qualifiers (..\accessibility\AccessibilityAllInOne.cpp) [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\svg\SVGSVGElement.h(46): warning C4373: 'WebCore::SVGSVGElement::setExternalResourcesRequiredBaseValue': virtual function overrides 'WebCore::SVGExternalResourcesRequired::setExternalResourcesRequiredBaseValue', previous versions of the compiler did not override when parameters only differed by const/volatile qualifiers (..\css\CSSAllInOne.cpp) [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\svg\SVGUseElement.h(46): warning C4373: 'WebCore::SVGUseElement::setExternalResourcesRequiredBaseValue': virtual function overrides 'WebCore::SVGExternalResourcesRequired::setExternalResourcesRequiredBaseValue', previous versions of the compiler did not override when parameters only differed by const/volatile qualifiers (..\dom\DOMAllInOne.cpp) [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\svg\SVGScriptElement.h(82): warning C4373: 'WebCore::SVGScriptElement::setExternalResourcesRequiredBaseValue': virtual function overrides 'WebCore::SVGExternalResourcesRequired::setExternalResourcesRequiredBaseValue', previous versions of the compiler did not override when parameters only differed by const/volatile qualifiers (..\dom\DOMAllInOne.cpp) [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj]
...

Full log at https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/68854/steps/compile-webkit/logs/stdio
Comment 1 Alexey Proskuryakov 2015-04-01 12:03:21 PDT 
This seems to be almost certainly a regression from <http://trac.webkit.org/changeset/182121>.

I'm not sure how to observe this, or even whether there was any change in behavior with either clang or MSVC.
Comment 2 Alexey Proskuryakov 2015-04-01 12:06:00 PDT 
Created attachment 249937 [details]
proposed fix
Comment 3 Darin Adler 2015-04-01 13:40:04 PDT 
Comment on attachment 249937 [details]
proposed fix

Oops! We really need to add the “override” to the overrides of this function. That would have caused this to be a compilation failure rather than a subtle cause of failing tests.
Comment 4 Darin Adler 2015-04-01 13:42:24 PDT 
Wait, no this is a big misunderstanding.

The warning is telling you about a behavior change where the Microsoft tools development team fixed a bug in the old version of Visual Studio. The warning simply indicates that this code would have had different behavior *in an old version of MSVC*. I don’t care about this passionately one way or another, but there’s nothing wrong here at all. We could just turn off this warning instead.
Comment 5 Darin Adler 2015-04-01 13:43:26 PDT 
I think the title of this bug is misleading. I don’t know quite what to title it. It’s more appropriate to say “MSVC started warning, but there is no problem other than the warning”.

Unless there is some symptom here other than the warning that I’m unaware of? Can’t imagine there is given my understanding of the situation.
Comment 6 Alexey Proskuryakov 2015-04-01 15:14:21 PDT 
Committed r182256 with a new title.

If this keeps happening, we should probably just silence the warning.