Bug 76370 – [EFL] Define the names of view smart class

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Eunsol Park]

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.

[Eunsol Park]

Created attachment 122602 [details]
proposed patch

To make them more clear , Single view was also changed.

[Gyuyoung Kim]

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]

> 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]

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]

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 on attachment 122602 [details]
proposed patch

Flagging the first patch as obsolete and clearing its flags.

[Eunsol Park]

Created attachment 122700 [details]
proposed patch

[Eunsol Park]

(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 on attachment 122700 [details]
proposed patch

LGTM.

[Grzegorz Czajkowski]

(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]

> 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]

Created attachment 122889 [details]
proposed patch

[Eunsol Park]

(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]

(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 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]

Created attachment 124479 [details]
proposed patch

[Eunsol Park]

(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 on attachment 124479 [details]
proposed patch

Clearing flags on attachment: 124479

Committed r106330: <http://trac.webkit.org/changeset/106330>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.