WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
proposed patch
(3.67 KB, patch)
2012-01-16 04:06 PST
,
Eunsol Park
no flags
Details
Formatted Diff
Diff
proposed patch
(3.67 KB, patch)
2012-01-16 17:59 PST
,
Eunsol Park
no flags
Details
Formatted Diff
Diff
proposed patch
(3.65 KB, patch)
2012-01-18 01:31 PST
,
Eunsol Park
no flags
Details
Formatted Diff
Diff
proposed patch
(3.63 KB, patch)
2012-01-29 17:45 PST
,
Eunsol Park
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug