Bug 76370

Summary: [EFL] Define the names of view smart class
Product: WebKit Reporter: Eunsol Park <eunsol47.park>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: g.czajkowski, gyuyoung.kim, hyuki.kim, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: Linux   
Attachments:
Description Flags
proposed patch
none
proposed patch
none
proposed patch
none
proposed patch
none
proposed patch none

Description Eunsol Park 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.
Comment 1 Eunsol Park 2012-01-16 01:00:23 PST
Created attachment 122602 [details]
proposed patch

To make them more clear , Single view was also changed.
Comment 2 Gyuyoung Kim 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*.
Comment 3 KwangHyuk 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 ?
Comment 4 Eunsol Park 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
Comment 5 Grzegorz Czajkowski 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.
Comment 6 Raphael Kubo da Costa (:rakuco) 2012-01-16 05:13:40 PST
Comment on attachment 122602 [details]
proposed patch

Flagging the first patch as obsolete and clearing its flags.
Comment 7 Eunsol Park 2012-01-16 17:59:06 PST
Created attachment 122700 [details]
proposed patch
Comment 8 Eunsol Park 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
Comment 9 Gyuyoung Kim 2012-01-16 18:14:18 PST
Comment on attachment 122700 [details]
proposed patch

LGTM.
Comment 10 Grzegorz Czajkowski 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.
Comment 11 KwangHyuk 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."
Comment 12 Eunsol Park 2012-01-18 01:31:45 PST
Created attachment 122889 [details]
proposed patch
Comment 13 Eunsol Park 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
Comment 14 KwangHyuk 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. :)
Comment 15 Andreas Kling 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.
Comment 16 Eunsol Park 2012-01-29 17:45:15 PST
Created attachment 124479 [details]
proposed patch
Comment 17 Eunsol Park 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.
Comment 18 WebKit Review Bot 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>
Comment 19 WebKit Review Bot 2012-01-30 21:03:25 PST
All reviewed patches have been landed.  Closing bug.