Bug 30716 – Fix Chromium build warnings in WebCore

• Component: Platform
• Status: RESOLVED FIXED

[Jens Alfke]

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.

[Jens Alfke]

(Previously filed against Chromium as http://code.google.com/p/chromium/issues/detail?id=25436 )

[Jens Alfke]

Created attachment 41728 [details]
patch 1

[Eric Seidel (no email)]

The CommandEntry and RoleEntry warnings seem bogus.  Can you explain why moving those declarations is the correct fix?

[David Levin]

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

[David Levin]

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.

[Jens Alfke]

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

[Jens Alfke]

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.

[Eric Seidel (no email)]

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

[Jens Alfke]

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.

[Eric Seidel (no email)]

Comment on attachment 41961 [details]
patch 3

This looks OK.  We should strongly consider reporting the +struct FunctionMapping {
bug to GCC.

[WebKit Commit Bot]

Comment on attachment 41961 [details]
patch 3

Clearing flags on attachment: 41961

Committed r50253: <http://trac.webkit.org/changeset/50253>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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

[Darin Adler]

*** Bug 26756 has been marked as a duplicate of this bug. ***