Bug 125064 – Fix EFL build with INSPECTOR disabled

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Peter Molnar]

EFL build currently fails when INSPECTOR is turned off.

[Peter Molnar]

Created attachment 218149 [details]
Patch

[Gyuyoung Kim]

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.

[Peter Molnar]

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

[Gyuyoung Kim]

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.

[Michal Pakula vel Rutka]

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

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

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?

[Peter Molnar]

Created attachment 223329 [details]
updated patch

[Gyuyoung Kim]

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.

[Peter Molnar]

Created attachment 223684 [details]
updated patch

[Csaba Osztrogonác]

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.

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

Peter, could you request review with previous patch ?

[Peter Molnar]

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

[Gyuyoung Kim]

Comment on attachment 223684 [details]
updated patch

LGTM. Ossy ?

[Csaba Osztrogonác]

Comment on attachment 223684 [details]
updated patch

LGTM, r=me

[WebKit Commit Bot]

Comment on attachment 223684 [details]
updated patch

Clearing flags on attachment: 223684

Committed r163777: <http://trac.webkit.org/changeset/163777>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.