[webkit-reviews] review requested: [Bug 30716] Fix Chromium build warnings in WebCore : [Attachment 41961] patch 3

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 27 10:24:32 PDT 2009


Jens Alfke <snej at chromium.org> has asked  for review:
Bug 30716: Fix Chromium build warnings in WebCore
https://bugs.webkit.org/show_bug.cgi?id=30716

Attachment 41961: patch 3
https://bugs.webkit.org/attachment.cgi?id=41961&action=review

------- Additional Comments from Jens Alfke <snej at chromium.org>
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.


More information about the webkit-reviews mailing list