Bug 64683 - [EFL] Method ewk_context_menu_destroy returns EINA_FALSE if CONTEXT_MENUS is disabled
Summary: [EFL] Method ewk_context_menu_destroy returns EINA_FALSE if CONTEXT_MENUS is ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-18 02:16 PDT by Grzegorz Czajkowski
Modified: 2011-07-25 00:09 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (1.45 KB, patch)
2011-07-18 02:17 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
ChangeLog fix (1.44 KB, patch)
2011-07-18 23:49 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
ChangeLog fix (1.44 KB, patch)
2011-07-18 23:55 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Grzegorz Czajkowski 2011-07-18 02:16:33 PDT
If CONTEXT_MENUS macro is disabled ewk_context_menu_destroy does nothing so it returns EINA_FALSE and omits NULL checking.
Comment 1 Grzegorz Czajkowski 2011-07-18 02:17:46 PDT
Created attachment 101139 [details]
proposed patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2011-07-18 05:57:17 PDT
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?
Comment 3 Grzegorz Czajkowski 2011-07-18 07:46:38 PDT
(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?
Comment 4 Raphael Kubo da Costa (:rakuco) 2011-07-18 08:35:51 PDT
(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.
Comment 5 Grzegorz Czajkowski 2011-07-18 23:49:19 PDT
Created attachment 101277 [details]
ChangeLog fix
Comment 6 WebKit Review Bot 2011-07-18 23:52:07 PDT
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.
Comment 7 Grzegorz Czajkowski 2011-07-18 23:55:17 PDT
Created attachment 101278 [details]
ChangeLog fix

Incorrect URL in the previous patch.
Comment 8 Gyuyoung Kim 2011-07-19 00:18:20 PDT
LGTM.
Comment 9 Raphael Kubo da Costa (:rakuco) 2011-07-19 06:02:17 PDT
r+ from my side too.
Comment 10 WebKit Review Bot 2011-07-25 00:09:50 PDT
Comment on attachment 101278 [details]
ChangeLog fix

Clearing flags on attachment: 101278

Committed r91658: <http://trac.webkit.org/changeset/91658>
Comment 11 WebKit Review Bot 2011-07-25 00:09:55 PDT
All reviewed patches have been landed.  Closing bug.