EFL build currently fails when INSPECTOR is turned off.
Created attachment 218149 [details] Patch
Comment on attachment 218149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218149&action=review > Source/WebKit/efl/ewk/ewk_contextmenu.h:112 > +#if defined(ENABLE_INSPECTOR) && ENABLE_INSPECTOR I think this isn't meaningful. ewk_contentmenu.h is public header. When application includes this, they can't know ENABLE_INSPECTOR is enabled or not.
(In reply to comment #2) > (From update of attachment 218149 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=218149&action=review > > > Source/WebKit/efl/ewk/ewk_contextmenu.h:112 > > +#if defined(ENABLE_INSPECTOR) && ENABLE_INSPECTOR > > I think this isn't meaningful. ewk_contentmenu.h is public header. When application includes this, they can't know ENABLE_INSPECTOR is enabled or not. This context menu item is correctly guarded in ContextMenuItem.h:134, but not here. These two must be in sync, as stated at the beginning of this file. They are not in sync now, that causes a lot of enum assertions, therefore WebKit currenty doesn't build.
Comment on attachment 218149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218149&action=review >>> Source/WebKit/efl/ewk/ewk_contextmenu.h:112 >>> +#if defined(ENABLE_INSPECTOR) && ENABLE_INSPECTOR >> >> I think this isn't meaningful. ewk_contentmenu.h is public header. When application includes this, they can't know ENABLE_INSPECTOR is enabled or not. > > This context menu item is correctly guarded in ContextMenuItem.h:134, but not here. These two must be in sync, as stated at the beginning of this file. > > They are not in sync now, that causes a lot of enum assertions, therefore WebKit currenty doesn't build. This file will be included by EFL application. If EFL application doesn't define ENABLE_INSPECTOR, it can't use EWK_CONTEXT_MENU_ITEM_TAG_INSPECT_ELEMENT when INSPECTOR is enabled. Now you only think when INSPECTOR is disabled.
Maybe we should remove context menu's COMPILE_ASSERT_MATCHING_ENUM, and replace it with methods similar to those used in WK API: https://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/API/c/WKSharedAPICast.h#L328 and https://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/API/c/WKSharedAPICast.h#L522
(In reply to comment #5) > Maybe we should remove context menu's COMPILE_ASSERT_MATCHING_ENUM, and replace it with methods similar to those used in WK API: https://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/API/c/WKSharedAPICast.h#L328 and https://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/API/c/WKSharedAPICast.h#L522 This one was done by https://trac.webkit.org/changeset/163033
Comment on attachment 218149 [details] Patch r- now, because COMPILE_ASSERT_MATCHING_ENUM was already removed. Péter, could you update the patch and check if we need additional fixes too to make !INSPECTOR build happy?
Created attachment 223329 [details] updated patch
Comment on attachment 223329 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=223329&action=review > Source/WebCore/inspector/ScriptCallFrame.cpp:38 > #include <inspector/InspectorValues.h> Isn't it also more clear to contain this inclusion by ENABLE(INSPECTOR) ? > Source/WebCore/inspector/ScriptCallStack.cpp:38 > #include <inspector/InspectorValues.h> ditto.
Created attachment 223684 [details] updated patch
Comment on attachment 223329 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=223329&action=review >> Source/WebCore/inspector/ScriptCallFrame.cpp:38 >> #include <inspector/InspectorValues.h> > > Isn't it also more clear to contain this inclusion by ENABLE(INSPECTOR) ? Are you sure if it is good? As far as I remember InspectorValues.h doesn't have ENABLE(INSPECTOR) guard intentionally, because it is used on many places regardless of enabled/disabled inspector.
Comment on attachment 223329 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=223329&action=review >>> Source/WebCore/inspector/ScriptCallFrame.cpp:38 >>> #include <inspector/InspectorValues.h> >> >> Isn't it also more clear to contain this inclusion by ENABLE(INSPECTOR) ? > > Are you sure if it is good? As far as I remember InspectorValues.h > doesn't have ENABLE(INSPECTOR) guard intentionally, because it > is used on many places regardless of enabled/disabled inspector. Ah, right. InspectorValues.h is being used by many places regardless of enabled/disabled inspector. I only saw where it is used on ENABLE(INSPECTOR) guard. Sorry for noise.
Peter, could you request review with previous patch ?
(In reply to comment #13) > Peter, could you request review with previous patch ? The previous patch wouldn't apply because ScriptCallFrame.cpp and ScriptCallStack.cpp have been renamed since I uploaded that patch. Also, there were other changes since that, so, that patch doesn't even build now. The most recent patch builds on the current ToT, so I think it is the one that should be reviewed.
Comment on attachment 223684 [details] updated patch LGTM. Ossy ?
Comment on attachment 223684 [details] updated patch LGTM, r=me
Comment on attachment 223684 [details] updated patch Clearing flags on attachment: 223684 Committed r163777: <http://trac.webkit.org/changeset/163777>
All reviewed patches have been landed. Closing bug.