Summary: | [EFL] Define the names of view smart class | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eunsol Park <eunsol47.park> | ||||||||||||
Component: | WebKit EFL | Assignee: | 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
Eunsol Park
2012-01-16 00:48:18 PST
Created attachment 122602 [details]
proposed patch
To make them more clear , Single view was also changed.
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*. > 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 ?
Created attachment 122612 [details]
proposed patch
applied comments.
1. *#define* changed to *const*.
2. modifined -> modified
3. is not a -> isn't an
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 on attachment 122602 [details]
proposed patch
Flagging the first patch as obsolete and clearing its flags.
Created attachment 122700 [details]
proposed patch
(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 on attachment 122700 [details]
proposed patch
LGTM.
(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. > 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."
Created attachment 122889 [details]
proposed patch
(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 (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 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. Created attachment 124479 [details]
proposed patch
(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 on attachment 124479 [details] proposed patch Clearing flags on attachment: 124479 Committed r106330: <http://trac.webkit.org/changeset/106330> All reviewed patches have been landed. Closing bug. |