Bug 64683 – [EFL] Method ewk_context_menu_destroy returns EINA_FALSE if CONTEXT_MENUS is disabled

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Grzegorz Czajkowski]

If CONTEXT_MENUS macro is disabled ewk_context_menu_destroy does nothing so it returns EINA_FALSE and omits NULL checking.

[Grzegorz Czajkowski]

Created attachment 101139 [details]
proposed patch

[Raphael Kubo da Costa (:rakuco)]

The ChangeLog entry sounds a bit confusing to me -- the one-line description looks like the method _currently_ does what is being described, not that this is what is being changed.

I see that you are submitting a few separate patches related to context menus. Do you plan on adding these #if checks to the other functions in ewk_contextmenu.cpp?

[Grzegorz Czajkowski]

(In reply to comment #2)
> The ChangeLog entry sounds a bit confusing to me -- the one-line description looks like the method _currently_ does what is being described, not that this is what is being changed.
> 

Ok, I will fix ChangeLog.

> I see that you are submitting a few separate patches related to context menus. Do you plan on adding these #if checks to the other functions in ewk_contextmenu.cpp?

I've sent two patches related to the context menu. First fixes build break, second one changes returned value. 
There aren't more references to WebCore's context menu object (controller) by API that are not surrounded by CONTEXT_MENUS macro. I don't think so others methods should check this macro. What is your opinion on that?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #3)
> > I see that you are submitting a few separate patches related to context menus. Do you plan on adding these #if checks to the other functions in ewk_contextmenu.cpp?
> 
> I've sent two patches related to the context menu. First fixes build break, second one changes returned value. 
> There aren't more references to WebCore's context menu object (controller) by API that are not surrounded by CONTEXT_MENUS macro. I don't think so others methods should check this macro. What is your opinion on that?

Let's not fix what's not broken ;) If the rest works, I think it's OK to leave it as it is.

[Grzegorz Czajkowski]

Created attachment 101277 [details]
ChangeLog fix

[WebKit Review Bot]

Attachment 101277 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/efl/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Grzegorz Czajkowski]

Created attachment 101278 [details]
ChangeLog fix

Incorrect URL in the previous patch.

[Gyuyoung Kim]

LGTM.

[Raphael Kubo da Costa (:rakuco)]

r+ from my side too.

[WebKit Review Bot]

Comment on attachment 101278 [details]
ChangeLog fix

Clearing flags on attachment: 101278

Committed r91658: <http://trac.webkit.org/changeset/91658>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.