[EFL] Set content hint information for ewk_view_single.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[KwangHyuk]

The content hint information corresponding to opengl_x11 engine is set for the image object which ewk_view_single owns when evas is based on opengl_x11 engine.

[KwangHyuk]

Created attachment 124499 [details]
Patch.

[Ryuan Choi]

Comment on attachment 124499 [details]
Patch.

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

Patch itself looks good to me.

> Source/WebKit/efl/ewk/ewk_view_single.cpp:64
> +    const std::string engine(ecore_evas_engine_name_get(ecoreEvas));
> +    if (!engine.compare("opengl_x11"))

How do you think about using WebCore::String ?Why do

[Gyuyoung Kim]

Comment on attachment 124499 [details]
Patch.

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

> Source/WebKit/efl/ewk/ewk_view_single.cpp:60
>      Evas_Object* bs = evas_object_image_add(smartData->base.evas);

I think it is better to change use full name instead of abbreviation for *bs*.

[KwangHyuk]

(In reply to comment #3)
> (From update of attachment 124499 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124499&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view_single.cpp:60
> >      Evas_Object* bs = evas_object_image_add(smartData->base.evas);
> 
> I think it is better to change use full name instead of abbreviation for *bs*.

Yepp, I noticed it, but https://bugs.webkit.org/show_bug.cgi?id=76378 already did it. :)

[KwangHyuk]

(In reply to comment #2)
> (From update of attachment 124499 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124499&action=review
> 
> Patch itself looks good to me.
> 
> > Source/WebKit/efl/ewk/ewk_view_single.cpp:64
> > +    const std::string engine(ecore_evas_engine_name_get(ecoreEvas));
> > +    if (!engine.compare("opengl_x11"))
> 
> How do you think about using WebCore::String ?Why do

Do you mean WTF::String ?

In fact, I couldn't find any restriction for data type on port specific module. :)
So, I would like to know the policy related with it.

[KwangHyuk]

Created attachment 124515 [details]
Patch updated.

Replace std::string with WTF::String.

[KwangHyuk]

(In reply to comment #2)
> (From update of attachment 124499 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124499&action=review
> 
> Patch itself looks good to me.
> 
> > Source/WebKit/efl/ewk/ewk_view_single.cpp:64
> > +    const std::string engine(ecore_evas_engine_name_get(ecoreEvas));
> > +    if (!engine.compare("opengl_x11"))
> 
> How do you think about using WebCore::String ?Why do

I agree that using WTF::String is more readable than std::string. :)

[Gyuyoung Kim]

Comment on attachment 124515 [details]
Patch updated.

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

> Source/WebKit/efl/ewk/ewk_view_single.cpp:63
> +    if (engine == "opengl_x11")

If engine name may become upper case, why don't you compare both via equalIgnoringCase() ?

[KwangHyuk]

(In reply to comment #8)
> (From update of attachment 124515 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124515&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view_single.cpp:63
> > +    if (engine == "opengl_x11")
> 
> If engine name may become upper case, why don't you compare both via equalIgnoringCase() ?

Good point.
In fact I referred the efl code in /ecore_evas/ecore_evas.c.
And then I could know that efl is just using lower case letters. :)

static const struct ecore_evas_engine _engines[] = {
  /* unix */
#ifdef BUILD_ECORE_EVAS_SOFTWARE_X11
  {"software_x11", _ecore_evas_constructor_software_x11},
#endif
#ifdef BUILD_ECORE_EVAS_OPENGL_X11
  {"opengl_x11", _ecore_evas_constructor_opengl_x11},
#endif
#ifdef BUILD_ECORE_EVAS_SOFTWARE_8_X11
  {"software_8_x11", _ecore_evas_constructor_software_8_x11},
#endif
#ifdef BUILD_ECORE_EVAS_SOFTWARE_16_X11
  {"software_16_x11", _ecore_evas_constructor_software_16_x11},
#endif
#ifdef BUILD_ECORE_EVAS_DIRECTFB
  {"directfb", _ecore_evas_constructor_directfb},
#endif
#ifdef BUILD_ECORE_EVAS_FB
  {"fb", _ecore_evas_constructor_fb},
#endif

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 124515 [details]
Patch updated.

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

> Source/WebKit/efl/ChangeLog:28
> [phone]  KwangHyuk Kim  <[e-mail]>
> +
> +        [EFL] Set content hint information for ewk_view_single.
> +        https://bugs.webkit.org/show_bug.cgi?id=77319
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        The content hint information corresponding to opengl_x11 engine is set
> +        for the image object which ewk_view_single owns when evas is based on
> +        opengl_x11 engine.
> +
> +        * ewk/ewk_view_single.cpp:
> +        (_ewk_view_single_smart_backing_store_add):
> +

Duplicate ChangeLog.

>>> Source/WebKit/efl/ewk/ewk_view_single.cpp:63
>>> +    const WTF::String engine(ecore_evas_engine_name_get(ecoreEvas));
>>> +    if (engine == "opengl_x11")
>> 
>> If engine name may become upper case, why don't you compare both via equalIgnoringCase() ?
> 
> Good point.
> In fact I referred the efl code in /ecore_evas/ecore_evas.c.
> And then I could know that efl is just using lower case letters. :)
> 
> static const struct ecore_evas_engine _engines[] = {
>   /* unix */
> #ifdef BUILD_ECORE_EVAS_SOFTWARE_X11
>   {"software_x11", _ecore_evas_constructor_software_x11},
> #endif
> #ifdef BUILD_ECORE_EVAS_OPENGL_X11
>   {"opengl_x11", _ecore_evas_constructor_opengl_x11},
> #endif
> #ifdef BUILD_ECORE_EVAS_SOFTWARE_8_X11
>   {"software_8_x11", _ecore_evas_constructor_software_8_x11},
> #endif
> #ifdef BUILD_ECORE_EVAS_SOFTWARE_16_X11
>   {"software_16_x11", _ecore_evas_constructor_software_16_x11},
> #endif
> #ifdef BUILD_ECORE_EVAS_DIRECTFB
>   {"directfb", _ecore_evas_constructor_directfb},
> #endif
> #ifdef BUILD_ECORE_EVAS_FB
>   {"fb", _ecore_evas_constructor_fb},
> #endif

I don't see the need for the const char* -> WTF::String conversions at all.

const Ecore_Evas* ecoreEvas = ecore_evas_ecore_evas_get(smartData->base.evas);
const char* engine = ecore_evas_engine_name_get(ecoreEvas);
if (!strncmp(engine, "opengl_x11", strlen("opengl_x11")))
    evas_object_image_content_hint_set(...);

[Tomasz Morawski]

(In reply to comment #10)
> (From update of attachment 124515 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124515&action=review
> 
> > Source/WebKit/efl/ChangeLog:28
> > [phone]  KwangHyuk Kim  <[e-mail]>
> > +
> > +        [EFL] Set content hint information for ewk_view_single.
> > +        https://bugs.webkit.org/show_bug.cgi?id=77319
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        The content hint information corresponding to opengl_x11 engine is set
> > +        for the image object which ewk_view_single owns when evas is based on
> > +        opengl_x11 engine.
> > +
> > +        * ewk/ewk_view_single.cpp:
> > +        (_ewk_view_single_smart_backing_store_add):
> > +
> 
> Duplicate ChangeLog.
> 
> >>> Source/WebKit/efl/ewk/ewk_view_single.cpp:63
> >>> +    const WTF::String engine(ecore_evas_engine_name_get(ecoreEvas));
> >>> +    if (engine == "opengl_x11")
> >> 
> >> If engine name may become upper case, why don't you compare both via equalIgnoringCase() ?
> > 
> > Good point.
> > In fact I referred the efl code in /ecore_evas/ecore_evas.c.
> > And then I could know that efl is just using lower case letters. :)
> > 
> > static const struct ecore_evas_engine _engines[] = {
> >   /* unix */
> > #ifdef BUILD_ECORE_EVAS_SOFTWARE_X11
> >   {"software_x11", _ecore_evas_constructor_software_x11},
> > #endif
> > #ifdef BUILD_ECORE_EVAS_OPENGL_X11
> >   {"opengl_x11", _ecore_evas_constructor_opengl_x11},
> > #endif
> > #ifdef BUILD_ECORE_EVAS_SOFTWARE_8_X11
> >   {"software_8_x11", _ecore_evas_constructor_software_8_x11},
> > #endif
> > #ifdef BUILD_ECORE_EVAS_SOFTWARE_16_X11
> >   {"software_16_x11", _ecore_evas_constructor_software_16_x11},
> > #endif
> > #ifdef BUILD_ECORE_EVAS_DIRECTFB
> >   {"directfb", _ecore_evas_constructor_directfb},
> > #endif
> > #ifdef BUILD_ECORE_EVAS_FB
> >   {"fb", _ecore_evas_constructor_fb},
> > #endif
> 
> I don't see the need for the const char* -> WTF::String conversions at all.
> 
> const Ecore_Evas* ecoreEvas = ecore_evas_ecore_evas_get(smartData->base.evas);
> const char* engine = ecore_evas_engine_name_get(ecoreEvas);
> if (!strncmp(engine, "opengl_x11", strlen("opengl_x11")))
>     evas_object_image_content_hint_set(...);
Yes, I almost agree with you. This is extremely useless conversion here. I think we should decide whether to use WTF::String, when to use or not use at all inside ewk. I think that it could be a good point to start discussion about it, we need a rule for that to avoid some situation in the future.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #11)
> I think that it could be a good point to start discussion about it, we need a rule for that to avoid some situation in the future.

Do we really need so many rules? Working out all cases and writing everything down takes more time than using some common sense ;) In this case, for example, one wants to compare a const char* with another const char*. There shouldn't be any doubt that creating WTF::Strings here just for that is pointless.

[Tomasz Morawski]

(In reply to comment #12)
> (In reply to comment #11)
> > I think that it could be a good point to start discussion about it, we need a rule for that to avoid some situation in the future.
> 
> Do we really need so many rules? Working out all cases and writing everything down takes more time than using some common sense ;) In this case, for example, one wants to compare a const char* with another const char*. There shouldn't be any doubt that creating WTF::Strings here just for that is pointless.

There is not need to consider a lot of cases. There is only one that is need to be finaly solve. How much c++ we could use inside ewk? :) 

Yes... I realy don't know why it is not allowed to use classes or even structures that have constructor/destructor inside cpp files for private object inside ewk. Is it allowed to use WebKit's containers inside cpp files inside ewk? If not why? Currently we have a situation that we use: smart pointers, new/delete operators, WTF::String, WebKit's classes or own classes inside WebKitSupport etc. EFL WebKit port is very mixed in languages that uses.

[KwangHyuk]

Created attachment 124546 [details]
Patch updated according to Kubo's comment.

Fix duplicated ChangeLog and just use strncmp for the engine name comparison.

[KwangHyuk]

(In reply to comment #10)
> (From update of attachment 124515 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124515&action=review
> 
> > Source/WebKit/efl/ChangeLog:28
> > [phone]  KwangHyuk Kim  <[e-mail]>
> > +
> > +        [EFL] Set content hint information for ewk_view_single.
> > +        https://bugs.webkit.org/show_bug.cgi?id=77319
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        The content hint information corresponding to opengl_x11 engine is set
> > +        for the image object which ewk_view_single owns when evas is based on
> > +        opengl_x11 engine.
> > +
> > +        * ewk/ewk_view_single.cpp:
> > +        (_ewk_view_single_smart_backing_store_add):
> > +
> 
> Duplicate ChangeLog.

Thx,fixed. :)

> 
> >>> Source/WebKit/efl/ewk/ewk_view_single.cpp:63
> >>> +    const WTF::String engine(ecore_evas_engine_name_get(ecoreEvas));
> >>> +    if (engine == "opengl_x11")
> >> 
> >> If engine name may become upper case, why don't you compare both via equalIgnoringCase() ?
> > 
> > Good point.
> > In fact I referred the efl code in /ecore_evas/ecore_evas.c.
> > And then I could know that efl is just using lower case letters. :)
> > 
> > static const struct ecore_evas_engine _engines[] = {
> >   /* unix */
> > #ifdef BUILD_ECORE_EVAS_SOFTWARE_X11
> >   {"software_x11", _ecore_evas_constructor_software_x11},
> > #endif
> > #ifdef BUILD_ECORE_EVAS_OPENGL_X11
> >   {"opengl_x11", _ecore_evas_constructor_opengl_x11},
> > #endif
> > #ifdef BUILD_ECORE_EVAS_SOFTWARE_8_X11
> >   {"software_8_x11", _ecore_evas_constructor_software_8_x11},
> > #endif
> > #ifdef BUILD_ECORE_EVAS_SOFTWARE_16_X11
> >   {"software_16_x11", _ecore_evas_constructor_software_16_x11},
> > #endif
> > #ifdef BUILD_ECORE_EVAS_DIRECTFB
> >   {"directfb", _ecore_evas_constructor_directfb},
> > #endif
> > #ifdef BUILD_ECORE_EVAS_FB
> >   {"fb", _ecore_evas_constructor_fb},
> > #endif
> 
> I don't see the need for the const char* -> WTF::String conversions at all.
> 
> const Ecore_Evas* ecoreEvas = ecore_evas_ecore_evas_get(smartData->base.evas);
> const char* engine = ecore_evas_engine_name_get(ecoreEvas);
> if (!strncmp(engine, "opengl_x11", strlen("opengl_x11")))
>     evas_object_image_content_hint_set(...);

I agree with you. :)

[KwangHyuk]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > I think that it could be a good point to start discussion about it, we need a rule for that to avoid some situation in the future.
> > 
> > Do we really need so many rules? Working out all cases and writing everything down takes more time than using some common sense ;) In this case, for example, one wants to compare a const char* with another const char*. There shouldn't be any doubt that creating WTF::Strings here just for that is pointless.
> 
> There is not need to consider a lot of cases. There is only one that is need to be finaly solve. How much c++ we could use inside ewk? :) 
> 
> Yes... I realy don't know why it is not allowed to use classes or even structures that have constructor/destructor inside cpp files for private object inside ewk. Is it allowed to use WebKit's containers inside cpp files inside ewk? If not why? Currently we have a situation that we use: smart pointers, new/delete operators, WTF::String, WebKit's classes or own classes inside WebKitSupport etc. EFL WebKit port is very mixed in languages that uses.

In fact, the reason why I choose std::string or WTF::String was to make it as c++ style.
So, I partially agree with your idea too. :)

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 124546 [details]
Patch updated according to Kubo's comment.

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

> Source/WebKit/efl/ewk/ewk_view_single.cpp:62
> +    Ecore_Evas* ecoreEvas = ecore_evas_ecore_evas_get(smartData->base.evas);

I'd make this variable const as well.

[KwangHyuk]

Created attachment 124549 [details]
Patch.

[KwangHyuk]

(In reply to comment #17)
> (From update of attachment 124546 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124546&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view_single.cpp:62
> > +    Ecore_Evas* ecoreEvas = ecore_evas_ecore_evas_get(smartData->base.evas);
> 
> I'd make this variable const as well.

Done. :)

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #13)
> There is not need to consider a lot of cases. There is only one that is need to be finaly solve. How much c++ we could use inside ewk? :) 

This question is orthogonal to the const char*/WTF::String issue we were discussing ;)

> Yes... I realy don't know why it is not allowed to use classes or even structures that have constructor/destructor inside cpp files for private object inside ewk. Is it allowed to use WebKit's containers inside cpp files inside ewk? If not why? Currently we have a situation that we use: smart pointers, new/delete operators, WTF::String, WebKit's classes or own classes inside WebKitSupport etc. EFL WebKit port is very mixed in languages that uses.

I had never heard of certain idioms or features being "forbidden" in the port. It just happened that it was rewritten and upstreamed with the API somewhat based on the GTK+ port (which was the only other port providing a C API and which relied heavily on C dependencies), and it doesn't (or at least didn't) use some C++ features. I'm all open to suggestions (which are probably more suited in the mailing list, anyway).

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 124549 [details]
Patch.

LGTM.

[Tomasz Morawski]

(In reply to comment #20)
> (In reply to comment #13)
> > There is not need to consider a lot of cases. There is only one that is need to be finaly solve. How much c++ we could use inside ewk? :) 
> 
> This question is orthogonal to the const char*/WTF::String issue we were discussing ;)
> 
> > Yes... I realy don't know why it is not allowed to use classes or even structures that have constructor/destructor inside cpp files for private object inside ewk. Is it allowed to use WebKit's containers inside cpp files inside ewk? If not why? Currently we have a situation that we use: smart pointers, new/delete operators, WTF::String, WebKit's classes or own classes inside WebKitSupport etc. EFL WebKit port is very mixed in languages that uses.
> 
> I had never heard of certain idioms or features being "forbidden" in the port. It just happened that it was rewritten and upstreamed with the API somewhat based on the GTK+ port (which was the only other port providing a C API and which relied heavily on C dependencies), and it doesn't (or at least didn't) use some C++ features. I'm all open to suggestions (which are probably more suited in the mailing list, anyway).

Thanks for your replay. I will try to write few on mailing list in the future.

[KwangHyuk]

Created attachment 124859 [details]
Patch rebased.

[Ryuan Choi]

(In reply to comment #23)
> Created an attachment (id=124859) [details]
> Patch rebased.

LGTM.

For me, const char* is reasonable enough.

[WebKit Review Bot]

Comment on attachment 124859 [details]
Patch rebased.

Clearing flags on attachment: 124859

Committed r106424: <http://trac.webkit.org/changeset/106424>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.