| Summary: | Fixing some compiler warnings | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Xabier Rodríguez Calvar <calvaris> | ||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Xabier Rodríguez Calvar <calvaris> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | aboxhall, apinheiro, buildbot, cfleizach, cgarcia, commit-queue, dmazzoni, eflews.bot, gustavo, gyuyoung.kim, gyuyoung.kim, jdiggs, mario, mrobinson, rakuco, rniwa, webkit-ews, zan | ||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Xabier Rodríguez Calvar
2013-06-19 04:39:24 PDT
Created attachment 204989 [details]
Patch
Fixed some warnings.
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Comment on attachment 204989 [details] Patch Attachment 204989 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/945263 Created attachment 205032 [details]
Patch
Fixed windows problems, hopefully.
Comment on attachment 205032 [details] Patch Attachment 205032 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/904623 Created attachment 205065 [details]
Patch
Fixed windows problems, again, hopefully.
Comment on attachment 205065 [details] Patch Attachment 205065 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/923781 Comment on attachment 205065 [details] Patch Attachment 205065 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/880478 Comment on attachment 205065 [details] Patch Attachment 205065 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/951479 Created attachment 205067 [details]
Patch
Changelog issue.
Comment on attachment 205067 [details] Patch Attachment 205067 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/880480 Comment on attachment 205067 [details] Patch Attachment 205067 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/880483 Comment on attachment 205067 [details] Patch Attachment 205067 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/943476 Comment on attachment 205067 [details] Patch Attachment 205067 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/924685 Created attachment 205070 [details]
Patch
Added WTF include dir to cmake in netscape plugin to build efl and qt properly.
Comment on attachment 205070 [details] Patch Attachment 205070 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/924692 Comment on attachment 205070 [details] Patch Attachment 205070 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/951485 Created attachment 205084 [details]
Patch
Added WTF include dir to cmake in netscape plugin to build efl and qt properly.
Comment on attachment 205084 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205084&action=review > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:637 > + offsetType = GAIL_AT_OFFSET; // to prevent a compiler warning I'd prefer this to be defined when the variable is declared. Comments should be complete sentences with a capital letter and a period, but it's okay if you just omit these kind of comments completely. > Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.h:31 > +#ifndef WTF_EXPORT_PRIVATE > +#define WTF_EXPORT_PRIVATE > +#endif Is this usually defined in config.h? > Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:47 > +#ifdef CRASH > +#undef CRASH > +#endif > + Maybe it would be better to simply pick a non-conflicting macro name? > Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionNoCache_Bundle.cpp:223 > + index = 0; // to avoid compiler warnings I think it's better to set the value of 'index' when you declare it and omit the comment. (In reply to comment #19) > (From update of attachment 205084 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205084&action=review > > > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:637 > > + offsetType = GAIL_AT_OFFSET; // to prevent a compiler warning > > I'd prefer this to be defined when the variable is declared. Comments should be complete sentences with a capital letter and a period, but it's okay if you just omit these kind of comments completely. Roger that. I did it like this because initialization is supposed to happen always and the default only when something wrong happens, which would be rare. Anyway, compiler can optimize it in a different way and my assumption might be irrelevant. > > Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.h:31 > > +#ifndef WTF_EXPORT_PRIVATE > > +#define WTF_EXPORT_PRIVATE > > +#endif > > Is this usually defined in config.h? > > > Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:47 > > +#ifdef CRASH > > +#undef CRASH > > +#endif > > + > > Maybe it would be better to simply pick a non-conflicting macro name? These two cases come from including <wtf/Assertions.h>. When I add it, I get compilation errors because of those two cases, the first is not defined and the second is redefined. I could define the export in config.h and maybe change the CRASH macro at main.cpp so that it doesn't conflict with the one in the <wtf/Assertions.h> as you suggest and if I understood you correctly. Other solution would be defining the WTF_ATTRIBUTE_PRINTF macro for those cases instead of just including the header that also causes the other problems. > > Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionNoCache_Bundle.cpp:223 > > + index = 0; // to avoid compiler warnings > > I think it's better to set the value of 'index' when you declare it and omit the comment. Same as in the first case. Created attachment 205382 [details]
Patch
Changed according Martin's comments.
Comment on attachment 205382 [details] Patch Attachment 205382 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/919110 Comment on attachment 205382 [details] Patch Attachment 205382 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/849378 Comment on attachment 205382 [details] Patch Attachment 205382 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/912669 Comment on attachment 205382 [details] Patch Attachment 205382 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/849377 Comment on attachment 205382 [details] Patch Attachment 205382 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/850837 Comment on attachment 205382 [details] Patch Attachment 205382 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/930294 Comment on attachment 205382 [details] Patch Attachment 205382 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/894248 Created attachment 205480 [details]
Patch
Fixed Mac compilation and trying fixes for Qt and EFL
Comment on attachment 205480 [details] Patch Attachment 205480 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/919205 Comment on attachment 205480 [details] Patch Attachment 205480 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/946330 Comment on attachment 205480 [details] Patch Attachment 205480 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/919207 Comment on attachment 205480 [details] Patch Attachment 205480 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/930442 Created attachment 205504 [details]
Patch
Including ExportMacros.h instead of the config.h mess.
Comment on attachment 205504 [details]
Patch
I think to fix a warning the changes to the plugin are too big and risky (for instance, linking against wtf). Let's just silence this warning on GTK+.
Created attachment 205529 [details]
Patch
Silenced format warning for Gtk+ as required by Martin.
Comment on attachment 205529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205529&action=review > Source/WebCore/ChangeLog:8 > + No new tests needed. You don't need to include this line when it's obvious, like in this case that you are fixing compile warnings. > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:315 > + 0, // willLoadURLRequest; > + 0, // willLoadDataRequest; Those trailing ; are weird, but harmless anyway Comment on attachment 205529 [details] Patch Clearing flags on attachment: 205529 Committed r152095: <http://trac.webkit.org/changeset/152095> All reviewed patches have been landed. Closing bug. |