Bug 71433

Created attachment 113419 [details] Patch

CC'ing kenneth, tonikitoo, kubo, leandro, ryuan.

Comment on attachment 113419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113419&action=review > Source/WebKit/efl/ewk/ewk_contextmenu.cpp:259 > Eina_Bool ewk_context_menu_free(Ewk_Context_Menu* menu) Internal functions such as this one could have their return type changed to bool as well.

Created attachment 113428 [details] Patch

(In reply to comment #3) > (From update of attachment 113419 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113419&action=review > > > Source/WebKit/efl/ewk/ewk_contextmenu.cpp:259 > > Eina_Bool ewk_context_menu_free(Ewk_Context_Menu* menu) > > Internal functions such as this one could have their return type changed to bool as well. Ok, I change return type with bool for internal functions.

Comment on attachment 113428 [details] Patch I think that we also change Eina_Bool in static functions which start to "_ewk_XXX" View in context: https://bugs.webkit.org/attachment.cgi?id=113428&action=review > Source/WebKit/efl/ewk/ewk_frame.cpp:469 > - EINA_SAFETY_ON_NULL_RETURN_VAL(smartData->frame, EINA_FALSE); > + EINA_SAFETY_ON_NULL_RETURN_VAL(smartData->frame, false); Although this bug is for fixing style issue, this should be 0.

Created attachment 113429 [details] Patch

(In reply to comment #6) > (From update of attachment 113428 [details]) > I think that we also change Eina_Bool in static functions which start to "_ewk_XXX" > > View in context: https://bugs.webkit.org/attachment.cgi?id=113428&action=review > > > Source/WebKit/efl/ewk/ewk_frame.cpp:469 > > - EINA_SAFETY_ON_NULL_RETURN_VAL(smartData->frame, EINA_FALSE); > > + EINA_SAFETY_ON_NULL_RETURN_VAL(smartData->frame, false); > > Although this bug is for fixing style issue, this should be 0. Yes, right. it is fixed.

You didn't update any public header files of WebKit-EFL. Present documentation describes that API returns EINA_TRUE or EINA_FALSE not true or false. Please update header files too.

(In reply to comment #9) > You didn't update any public header files of WebKit-EFL. Present documentation describes that API returns EINA_TRUE or EINA_FALSE not true or false. > Please update header files too. I'm not sure if we should change the return type in documentation. Is it better to know return type is *Eina_Bool* type instead of *bool* ?

(In reply to comment #10) > (In reply to comment #9) > > You didn't update any public header files of WebKit-EFL. Present documentation describes that API returns EINA_TRUE or EINA_FALSE not true or false. > > Please update header files too. > > I'm not sure if we should change the return type in documentation. Is it better to know return type is *Eina_Bool* type instead of *bool* ? You've already changed documentation of private functions by replacing EINA_TRUE/EINA_FALSE to true/false. I think we should consistently update API documentation too.

(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > You didn't update any public header files of WebKit-EFL. Present documentation describes that API returns EINA_TRUE or EINA_FALSE not true or false. > > > Please update header files too. > > > > I'm not sure if we should change the return type in documentation. Is it better to know return type is *Eina_Bool* type instead of *bool* ? > > You've already changed documentation of private functions by replacing EINA_TRUE/EINA_FALSE to true/false. I think we should consistently update API documentation too. Hmm, it seems it can make confusion when we add new APIs or internal functions. However, we can't change efl style with webkit style in public headers. Thus, I think it is difficult to change Eina_Bool of documentation with true/false. Because, I think efl application developers refer to public headers basically. But, in internal function case, that is used by webkit efl port developers. So, I think it is better to keep Eina_Bool in documentation of public headers. I'd like to know other guys opinions.

(In reply to comment #12) > Hmm, it seems it can make confusion when we add new APIs or internal functions. However, we can't change efl style with webkit style in public headers. Thus, I think it is difficult to change Eina_Bool of documentation with true/false. Because, I think efl application developers refer to public headers basically. But, in internal function case, that is used by webkit efl port developers. So, I think it is better to keep Eina_Bool in documentation of public headers. I'd like to know other guys opinions. Yes. Just like the public headers still have "Evas_Object *o", I think they should keep things familiar to the EFL users who are going to use the API -- whether we use bools or Eina_Bools, or the coding style we use internally should be transparent.

LGTM.

Looks good to me, too.

Comment on attachment 113429 [details] Patch Nice work! r=me

Comment on attachment 113429 [details] Patch Clearing flags on attachment: 113429 Committed r99268: <http://trac.webkit.org/changeset/99268>

All reviewed patches have been landed. Closing bug.

Created attachment 115132 [details] Patch

I missed to change *Eina_Bool* in internal struct. So, I re-open this bug.

LGTM, just not sure if it's better to reopen this one or open a new bug.

Comment on attachment 115132 [details] Patch This looks fine but you really need to open a new bug for it. CC me and I'll r+ it.

Closing.