Bug 105267

Summary:

[EFL] Add guard around ContextMenuItemTagInspectElement

Product:

WebKit

Reporter:

Seokju Kwon <seokju>

Component:

WebKit EFL

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

Linux

Attachments:

Description	Flags
Patch	none
Patch	none

Seokju Kwon

Reported 2012-12-18 01:58:34 PST

Need INSPECTOR guard for using ContextMenuItemTagInspectElement.

Attachments
Patch (5.69 KB, patch) 2012-12-18 06:22 PST, Seokju Kwon	no flags	Details Formatted Diff Diff
Patch (4.09 KB, patch) 2012-12-21 06:28 PST, Seokju Kwon	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Seokju Kwon

Comment 1 2012-12-18 06:22:51 PST

Created attachment 179930 [details] Patch

Thiago Marcos P. Santos

Comment 2 2012-12-19 01:53:33 PST

Comment on attachment 179930 [details] Patch LGTM.

Ryosuke Niwa

Comment 3 2012-12-19 21:59:56 PST

Comment on attachment 179930 [details] Patch rs=me.

Gyuyoung Kim

Comment 4 2012-12-19 22:06:56 PST

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 5 2012-12-19 22:28:55 PST

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 6 2012-12-20 01:14:59 PST

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

Comment 7 2012-12-20 01:22:03 PST

(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

Comment 8 2012-12-21 06:28:24 PST

Created attachment 180512 [details] Patch

Seokju Kwon

Comment 9 2012-12-21 06:30:33 PST

Comment on attachment 180512 [details] Patch No #ifdefs in public header. I have removed them. Thank you for review :)

WebKit Review Bot

Comment 10 2012-12-21 06:52:39 PST

Comment on attachment 180512 [details] Patch Clearing flags on attachment: 180512 Committed r138370: <http://trac.webkit.org/changeset/138370>

WebKit Review Bot

Comment 11 2012-12-21 06:52:43 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.