WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
77319
[EFL] Set content hint information for ewk_view_single.
https://bugs.webkit.org/show_bug.cgi?id=77319
Summary
[EFL] Set content hint information for ewk_view_single.
KwangHyuk
Reported
2012-01-29 23:50:04 PST
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.
Attachments
Patch.
(1.93 KB, patch)
2012-01-29 23:54 PST
,
KwangHyuk
no flags
Details
Formatted Diff
Diff
Patch updated.
(2.39 KB, patch)
2012-01-30 02:31 PST
,
KwangHyuk
no flags
Details
Formatted Diff
Diff
Patch updated according to Kubo's comment.
(1.89 KB, patch)
2012-01-30 07:20 PST
,
KwangHyuk
no flags
Details
Formatted Diff
Diff
Patch.
(1.86 KB, patch)
2012-01-30 07:39 PST
,
KwangHyuk
no flags
Details
Formatted Diff
Diff
Patch rebased.
(1.85 KB, patch)
2012-01-31 17:28 PST
,
KwangHyuk
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
KwangHyuk
Comment 1
2012-01-29 23:54:24 PST
Created
attachment 124499
[details]
Patch.
Ryuan Choi
Comment 2
2012-01-29 23:58:22 PST
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 3
2012-01-30 00:03:33 PST
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
Comment 4
2012-01-30 00:15:43 PST
(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
Comment 5
2012-01-30 00:34:14 PST
(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
Comment 6
2012-01-30 02:31:44 PST
Created
attachment 124515
[details]
Patch updated. Replace std::string with WTF::String.
KwangHyuk
Comment 7
2012-01-30 02:33:15 PST
(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 8
2012-01-30 02:38:36 PST
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
Comment 9
2012-01-30 02:54:00 PST
(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 10
2012-01-30 05:36:51 PST
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 > +2012-01-29 KwangHyuk Kim <
hyuki.kim@samsung.com
> > + > + [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
Comment 11
2012-01-30 05:51:22 PST
(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 > > +2012-01-29 KwangHyuk Kim <
hyuki.kim@samsung.com
> > > + > > + [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)
Comment 12
2012-01-30 05:58:12 PST
(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
Comment 13
2012-01-30 06:41:53 PST
(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
Comment 14
2012-01-30 07:20:41 PST
Created
attachment 124546
[details]
Patch updated according to Kubo's comment. Fix duplicated ChangeLog and just use strncmp for the engine name comparison.
KwangHyuk
Comment 15
2012-01-30 07:22:27 PST
(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 > > +2012-01-29 KwangHyuk Kim <
hyuki.kim@samsung.com
> > > + > > + [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
Comment 16
2012-01-30 07:29:15 PST
(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 17
2012-01-30 07:32:20 PST
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
Comment 18
2012-01-30 07:39:53 PST
Created
attachment 124549
[details]
Patch.
KwangHyuk
Comment 19
2012-01-30 07:40:55 PST
(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)
Comment 20
2012-01-30 07:50:20 PST
(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 21
2012-01-30 07:50:43 PST
Comment on
attachment 124549
[details]
Patch. LGTM.
Tomasz Morawski
Comment 22
2012-01-30 08:11:09 PST
(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
Comment 23
2012-01-31 17:28:39 PST
Created
attachment 124859
[details]
Patch rebased.
Ryuan Choi
Comment 24
2012-01-31 17:34:11 PST
(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 25
2012-01-31 19:38:42 PST
Comment on
attachment 124859
[details]
Patch rebased. Clearing flags on attachment: 124859 Committed
r106424
: <
http://trac.webkit.org/changeset/106424
>
WebKit Review Bot
Comment 26
2012-01-31 19:38:48 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.
Top of Page
Format For Printing
XML
Clone This Bug