Building WebCore for Chromium on Mac produces two warnings: /Volumes/Yttrium/src/third_party/WebKit/WebCore/WebCore.gyp/../accessibility/AccessibilityRenderObject.cpp:2229: warning: 'WebCore::createARIARoleMap()::RoleEntry' declared with greater visibility than the type of its field 'WebCore::createARIARoleMap()::RoleEntry::ariaRole' /Volumes/Yttrium/src/third_party/WebKit/WebCore/WebCore.gyp/../editing/EditorCommand.cpp:1296: warning: 'WebCore::createCommandMap()::CommandEntry' declared with greater visibility than the type of its field 'WebCore::createCommandMap()::CommandEntry::command' If these were fixed we could turn on 'treat warnings as errors' in WebCore, which would be a Good Thing. Additionally, it would be an Even Better Thing to enable all warnings using "-Wall", similarly to the Chrome-only projects. This requires fixing a handful of other warnings.
(Previously filed against Chromium as http://code.google.com/p/chromium/issues/detail?id=25436 )
Created attachment 41728 [details] patch 1
The CommandEntry and RoleEntry warnings seem bogus. Can you explain why moving those declarations is the correct fix?
Comment on attachment 41728 [details] patch 1 Nice. Just one thing to be addressed. > Index: WebCore/rendering/RenderMediaControlsChromium.cpp > case MediaCurrentTimePart: > case MediaTimeRemainingPart: > return true; > + default: > + ; The missing enum value(s) should be added to the switch (not a default: which hides the fact that values aren't being handled). > @@ -272,6 +274,8 @@ bool RenderMediaControlsChromium::paintM > case MediaControlsPanel: > ASSERT_NOT_REACHED(); > break; > + default: > + ; The missing enum value(s) should be added to the switch (not a default: which hides the fact that values aren't being handled).
One other comment: WebCore/xml/XPathFunctions.cpp 670 struct FunctionMapping { 671 const char *name; Since you're touching this line anyway, please fix the * placement.
> The CommandEntry and RoleEntry warnings seem bogus. To be honest I am not sure why moving the declaration fixed them. But it's a no-op change in all other respects, so it didn't seem time-effective to research exactly what was going on with symbol visibility in those places. >The missing enum value(s) should be added to the switch (not a default: which >hides the fact that values aren't being handled). But the existing code was clearly intended to allow unlisted cases, since there's a 'return false' after the end of the switch. It was just doing it in a way that didn't signal that intention to GCC.
Created attachment 41873 [details] patch 2 Updated patch: - Fixed 'char *' in XPathFunctions.cpp - A recent commit to ClipboardChromium.cpp made my change to it unnecessary.
(In reply to comment #6) > > The CommandEntry and RoleEntry warnings seem bogus. > > To be honest I am not sure why moving the declaration fixed them. But it's a > no-op change in all other respects, so it didn't seem time-effective to > research exactly what was going on with symbol visibility in those places. I just don't understand the warning, or the fix, so it's difficult to judge the quality of the fix or adjust my future behavior when writing code like that. It seems a small amount of googling or similar could document this change. > >The missing enum value(s) should be added to the switch (not a default: which > >hides the fact that values aren't being handled). > > But the existing code was clearly intended to allow unlisted cases, since > there's a 'return false' after the end of the switch. It was just doing it in a > way that didn't signal that intention to GCC. I think better would be to add all the missing cases. We try and avoid default so that the compiler displays that warning when we forget cases.
Created attachment 41961 [details] patch 3 Revised to add the three missing cases to RenderMediaControlsChromium::paintMediaControlsPart instead of using a 'default' label. I looked at doing the same to shouldRenderMediaControlPart, but there are **33** missing cases that would have to be added! If you look at the purpose of the function, it's clearly to identify the Media-related control parts and return false for the rest, so the 'default' case is valid. Otherwise this function would produce a warning whenever anyone added a new ControlPart, whether or not it was related to media controls, which would be a PITA. Here's the story about the weird struct-visibility warning. Chrome's WebCore.xcodeproj builds with -fvisibility=hidden, aka "Symbols Hidden By Default". Regular WebKit's WebCore.xcodeproj doesn't. I don't know why they use different settings, but this explains why regular WebKit builds won't produce this warning: the symbol EditorInternalCommand isn't hidden in their build. As far as I can tell, the warning is mistaken. I think the compiler is confused about the additional invisibility that the CommandEntry struct has by virtue of being declared inside function scope. Although this is even more hidden than a private symbol at file scope, it seems to be thinking that it's _less_ hidden. This explains why moving the declaration out to file scope fixes the problem.
Comment on attachment 41961 [details] patch 3 This looks OK. We should strongly consider reporting the +struct FunctionMapping { bug to GCC.
Comment on attachment 41961 [details] patch 3 Clearing flags on attachment: 41961 Committed r50253: <http://trac.webkit.org/changeset/50253>
All reviewed patches have been landed. Closing bug.
(In reply to comment #10) > (From update of attachment 41961 [details]) > This looks OK. We should strongly consider reporting the +struct > FunctionMapping { > bug to GCC. See bug 26756.
*** Bug 26756 has been marked as a duplicate of this bug. ***