Bug 105267 – [EFL] Add guard around ContextMenuItemTagInspectElement

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Seokju Kwon]

Need INSPECTOR guard for using ContextMenuItemTagInspectElement.

[Seokju Kwon]

Created attachment 179930 [details]
Patch

[Thiago Marcos P. Santos]

Comment on attachment 179930 [details]
Patch

LGTM.

[Ryosuke Niwa]

Comment on attachment 179930 [details]
Patch

rs=me.

[Gyuyoung Kim]

Comment on attachment 179930 [details]
Patch

Sorry for free cq+. I would like to check if we can use ENABLE_XXX macro in public header a little further.

[Gyuyoung Kim]

Comment on attachment 179930 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179930&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context_menu_item.h:118
> +#if defined ENABLE_INSPECTOR && ENABLE_INSPECTOR

If app want to use this INSPECT_ELEMENT type, app should define ENABLE_INSPECTOR macro, isn't it ? It looks app should do unnecessary job. I would suggest not to use ENABLE_INSPECTOR macro in public header as chromium port. Chromium port only uses macro in internal implementation. Any other comments ?

[Vivek Galatage]

Comment on attachment 179930 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179930&action=review

> Source/WebKit/efl/ewk/ewk_contextmenu.h:112
> +#if defined ENABLE_INSPECTOR && ENABLE_INSPECTOR

Wouldn't the use of #if ENABLE(INSPECTOR) do the same thing as above? I think its better to use the ENABLE(INSPECTOR) macro for consistency.

[Gyuyoung Kim]

(In reply to comment #6)
> (From update of attachment 179930 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179930&action=review
> 
> > Source/WebKit/efl/ewk/ewk_contextmenu.h:112
> > +#if defined ENABLE_INSPECTOR && ENABLE_INSPECTOR
> 
> Wouldn't the use of #if ENABLE(INSPECTOR) do the same thing as above? I think its better to use the ENABLE(INSPECTOR) macro for consistency.

ENABLE(XXX) is defined to WTF/wtf/Platform.h. So, application which uses ewk_contextmenu.h should include the Platform.h to use ENABLE(XXX) macro. But, we don't install Platform.h file. We can't use ENABLE() in public header. Beside, as we already talked, to use macro in public header will make unneeded overhead when using public header.

[Seokju Kwon]

Created attachment 180512 [details]
Patch

[Seokju Kwon]

Comment on attachment 180512 [details]
Patch

No #ifdefs in public header. I have removed them.
Thank you for review :)

[WebKit Review Bot]

Comment on attachment 180512 [details]
Patch

Clearing flags on attachment: 180512

Committed r138370: <http://trac.webkit.org/changeset/138370>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.