RESOLVED FIXED 76370
[EFL] Define the names of view smart class
https://bugs.webkit.org/show_bug.cgi?id=76370
Summary [EFL] Define the names of view smart class
Eunsol Park
Reported 2012-01-16 00:48:18 PST
Tiled view names were created as Ewk_View_Tiled and EWK_View_Tiled. It should be modifed to be same to check the view's type.
Attachments
proposed patch (3.66 KB, patch)
2012-01-16 01:00 PST, Eunsol Park
no flags
proposed patch (3.67 KB, patch)
2012-01-16 04:06 PST, Eunsol Park
no flags
proposed patch (3.67 KB, patch)
2012-01-16 17:59 PST, Eunsol Park
no flags
proposed patch (3.65 KB, patch)
2012-01-18 01:31 PST, Eunsol Park
no flags
proposed patch (3.63 KB, patch)
2012-01-29 17:45 PST, Eunsol Park
no flags
Eunsol Park
Comment 1 2012-01-16 01:00:23 PST
Created attachment 122602 [details] proposed patch To make them more clear , Single view was also changed.
Gyuyoung Kim
Comment 2 2012-01-16 01:08:42 PST
Comment on attachment 122602 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=122602&action=review > Source/WebKit/efl/ChangeLog:9 > + so the definitions were modifined to make users not confused. modifined - > modified. > Source/WebKit/efl/ewk/ewk_private.h:47 > +#define EWK_VIEW_TILED_NAME "Ewk_View_Tiled" WebKit prefers *const* to *#define*.
KwangHyuk
Comment 3 2012-01-16 01:58:38 PST
> Source/WebKit/efl/ewk/ewk_view.cpp:270 > + INF("object is not a instance of EWK_VIEW_TILED_NAME"); \ is not a => isn't an ?
Eunsol Park
Comment 4 2012-01-16 04:06:14 PST
Created attachment 122612 [details] proposed patch applied comments. 1. *#define* changed to *const*. 2. modifined -> modified 3. is not a -> isn't an
Grzegorz Czajkowski
Comment 5 2012-01-16 05:10:52 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=122612&action=review > Source/WebKit/efl/ewk/ewk_private.h:62 > +static const char* ewkViewTiledName = "Ewk_View_Tiled"; static const char ewkViewTiledName[] = "Ewk_View_Tiled"; > Source/WebKit/efl/ewk/ewk_private.h:63 > +static const char* ewkViewSingleName = "Ewk_View_Single"; Ditto.
Raphael Kubo da Costa (:rakuco)
Comment 6 2012-01-16 05:13:40 PST
Comment on attachment 122602 [details] proposed patch Flagging the first patch as obsolete and clearing its flags.
Eunsol Park
Comment 7 2012-01-16 17:59:06 PST
Created attachment 122700 [details] proposed patch
Eunsol Park
Comment 8 2012-01-16 18:00:12 PST
(In reply to comment #5) > View in context: https://bugs.webkit.org/attachment.cgi?id=122612&action=review > > > Source/WebKit/efl/ewk/ewk_private.h:62 > > +static const char* ewkViewTiledName = "Ewk_View_Tiled"; > > static const char ewkViewTiledName[] = "Ewk_View_Tiled"; > > > Source/WebKit/efl/ewk/ewk_private.h:63 > > +static const char* ewkViewSingleName = "Ewk_View_Single"; > > Ditto. updated patch
Gyuyoung Kim
Comment 9 2012-01-16 18:14:18 PST
Comment on attachment 122700 [details] proposed patch LGTM.
Grzegorz Czajkowski
Comment 10 2012-01-16 23:37:38 PST
(In reply to comment #8) > (In reply to comment #5) > > View in context: https://bugs.webkit.org/attachment.cgi?id=122612&action=review > > > > > Source/WebKit/efl/ewk/ewk_private.h:62 > > > +static const char* ewkViewTiledName = "Ewk_View_Tiled"; > > > > static const char ewkViewTiledName[] = "Ewk_View_Tiled"; > > > > > Source/WebKit/efl/ewk/ewk_private.h:63 > > > +static const char* ewkViewSingleName = "Ewk_View_Single"; > > > > Ditto. > updated patch LGTM.
KwangHyuk
Comment 11 2012-01-17 19:20:31 PST
> Source/WebKit/efl/ewk/ewk_private.h:63 > +static const char ewkViewSingleName[] = "Ewk_View_Single"; I have got this idea from reviewer morrita's comment for other patch. :) "It looks this should be just const int, without static. Static is for static variables, which are usually staying inside c/cpp file."
Eunsol Park
Comment 12 2012-01-18 01:31:45 PST
Created attachment 122889 [details] proposed patch
Eunsol Park
Comment 13 2012-01-18 01:32:27 PST
(In reply to comment #11) > > Source/WebKit/efl/ewk/ewk_private.h:63 > > +static const char ewkViewSingleName[] = "Ewk_View_Single"; > > I have got this idea from reviewer morrita's comment for other patch. :) > "It looks this should be just const int, without static. Static is for static variables, which are usually staying inside c/cpp file." patch is updated
KwangHyuk
Comment 14 2012-01-18 01:35:23 PST
(In reply to comment #13) > (In reply to comment #11) > > > Source/WebKit/efl/ewk/ewk_private.h:63 > > > +static const char ewkViewSingleName[] = "Ewk_View_Single"; > > > > I have got this idea from reviewer morrita's comment for other patch. :) > > "It looks this should be just const int, without static. Static is for static variables, which are usually staying inside c/cpp file." > > patch is updated Looks good. :)
Andreas Kling
Comment 15 2012-01-27 03:48:46 PST
Comment on attachment 122889 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=122889&action=review > Source/WebKit/efl/ewk/ewk_view.cpp:270 > - if (!evas_object_smart_type_check(ewkView, "Ewk_View_Tiled")) { \ > - INF("object is not a instance of Ewk_View_Tiled"); \ > + if (!evas_object_smart_type_check(ewkView, ewkViewTiledName)) { \ > + INF("object isn't an instance of ewkViewTiledName"); \ Seems like the second line should be: INF("object isn't an instance of " ewkViewTiledName); \ As you don't want the variable name to leak into error messages.
Eunsol Park
Comment 16 2012-01-29 17:45:15 PST
Created attachment 124479 [details] proposed patch
Eunsol Park
Comment 17 2012-01-29 17:52:02 PST
(In reply to comment #15) > (From update of attachment 122889 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122889&action=review > > > Source/WebKit/efl/ewk/ewk_view.cpp:270 > > - if (!evas_object_smart_type_check(ewkView, "Ewk_View_Tiled")) { \ > > - INF("object is not a instance of Ewk_View_Tiled"); \ > > + if (!evas_object_smart_type_check(ewkView, ewkViewTiledName)) { \ > > + INF("object isn't an instance of ewkViewTiledName"); \ > > Seems like the second line should be: > INF("object isn't an instance of " ewkViewTiledName); \ > > As you don't want the variable name to leak into error messages. I've done it.
WebKit Review Bot
Comment 18 2012-01-30 21:03:17 PST
Comment on attachment 124479 [details] proposed patch Clearing flags on attachment: 124479 Committed r106330: <http://trac.webkit.org/changeset/106330>
WebKit Review Bot
Comment 19 2012-01-30 21:03:25 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.