Bug 61915

Summary: [EFL][WK2] Add efl port's ewk_view files
Product: WebKit Reporter: Eunmi Lee <enmi.lee>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: antognolli+webkit, eric, gyuyoung.kim, kenneth, leandro, ryuan.choi, sangseok.lim, tonikitoo, webkit.review.bot, youngtaeck.song
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61838    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for ewk_view.
none
patch for ewk_view.
none
patch for ewk_view.
none
patch for ewk_view. none

Eunmi Lee
Reported 2011-06-02 02:55:52 PDT
I have added Source/WebKit2/UIProcess/API/efl directory and created Source/WebKit2/UIProcess/API/efl/ewk_view.cpp and Source/WebKit2/UIProcess/API/efl/ewk_view.h.
Attachments
Patch (19.50 KB, patch)
2011-06-02 03:09 PDT, Eunmi Lee
no flags
Patch (22.39 KB, patch)
2011-06-02 19:26 PDT, Eunmi Lee
no flags
Patch (22.47 KB, patch)
2011-06-02 19:59 PDT, Eunmi Lee
no flags
Patch (22.83 KB, patch)
2011-06-06 21:30 PDT, Eunmi Lee
no flags
Patch (22.92 KB, patch)
2011-06-09 01:50 PDT, Eunmi Lee
no flags
Patch (23.98 KB, patch)
2011-06-09 06:26 PDT, Eunmi Lee
no flags
Patch (23.79 KB, patch)
2011-06-09 18:31 PDT, Eunmi Lee
no flags
Patch (23.55 KB, patch)
2011-06-10 01:31 PDT, Eunmi Lee
no flags
Patch (23.58 KB, patch)
2011-06-10 04:52 PDT, Eunmi Lee
no flags
Patch (23.61 KB, patch)
2011-06-14 05:34 PDT, Eunmi Lee
no flags
Patch (23.62 KB, patch)
2011-06-14 18:32 PDT, Eunmi Lee
no flags
Patch (23.54 KB, patch)
2011-06-15 22:24 PDT, Eunmi Lee
no flags
Patch (23.54 KB, patch)
2011-06-16 17:45 PDT, Eunmi Lee
no flags
Patch for ewk_view. (24.86 KB, patch)
2011-12-15 01:01 PST, Eunmi Lee
no flags
patch for ewk_view. (25.29 KB, patch)
2011-12-18 21:05 PST, Eunmi Lee
no flags
patch for ewk_view. (25.29 KB, patch)
2011-12-18 21:10 PST, Eunmi Lee
no flags
patch for ewk_view. (25.49 KB, patch)
2011-12-18 22:31 PST, Eunmi Lee
no flags
Eunmi Lee
Comment 1 2011-06-02 03:09:15 PDT
WebKit Review Bot
Comment 2 2011-06-02 03:11:53 PDT
Attachment 95745 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Last 3072 characters of output: dentifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:178: _ewk_view_on_mouse_move is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:178: event_info is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:187: _ewk_view_on_key_down is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:187: event_info is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:196: _ewk_view_on_key_up is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:196: event_info is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:205: _parent_sc is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:217: _ewk_view_priv_del is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:225: _ewk_view_smart_add is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:266: _ewk_view_smart_del is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:275: _ewk_view_smart_resize is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:287: _ewk_view_smart_move is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:294: _ewk_view_smart_calculate is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:319: _ewk_view_smart_show is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:327: _ewk_view_smart_hide is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:334: ewk_view_smart_set is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:386: ewk_view_add is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:404: ewk_view_page_get is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 41 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Eric Seidel (no email)
Comment 3 2011-06-02 08:24:10 PDT
I'm not sure who would review this. I can rubber stamp it... but I don't really know anything about EFL.
Leandro Pereira
Comment 4 2011-06-02 09:32:16 PDT
Comment on attachment 95745 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95745&action=review I don't know much about WebKit2, but I've found some small issues, after a quick glance over the code. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:33 > +static const char EWK_VIEW_TYPE_STR[] = "EWK_View"; EWK_View is also used by EWK1 -- at least while the two APIs are coexisting here, I'd change that to "EWK2_View" or something similar. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:58 > +static void _ewk_view_smart_focus_in(Ewk_View_Smart_Data* sd) > +{ > + if (!sd) > + return; > + EWK_VIEW_PRIV_GET(sd, priv); This pattern repeats all over -- wouldn't it be nicer you had a macro like EWK_VIEW_PRIV_GET_OR_RETURN, like you have on current WebKitEFL? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:222 > +{ > + if (!priv) > + return; > + > + free(priv); This will leak the RefPtr on the Ewk_View_Private_Data structure -- attribute 0 to it before freeing the structure. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:246 > + if (!sd->priv) > + return; sd leaks here. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:102 > +struct _Ewk_View_Smart_Data { > + Evas_Object_Smart_Clipped_Data base; > + const Ewk_View_Smart_Class* api; /**< reference to casted class instance */ > + Evas_Object* self; /**< reference to owner object */ > + Evas_Object *backing_store; /**< reference to backing store */ Small style issue: * aligned to the type.
Eunmi Lee
Comment 5 2011-06-02 19:26:41 PDT
Eunmi Lee
Comment 6 2011-06-02 19:35:31 PDT
Leandro, Thank you very much for you comments :) This patch is significant to me because it is my first patch for webkit! I have upload new patch which is fixed for your comments. I'd appreciate it if you would review it again :)
Ryuan Choi
Comment 7 2011-06-02 19:49:08 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=95846&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.h:41 > + // - if overridden, have to call parent method if desired If possible, I like to use /* */ instead of // for c header. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:102 > + Evas_Object *backing_store; /**< reference to backing store */ it's missed. > Source/WebKit2/UIProcess/API/efl/ewk_view.h:115 > +EAPI WKPageRef ewk_view_page_get(Evas_Object*); Isn't it better to keep a parameter name like WebKit/EFL? > Source/WebKit2/UIProcess/API/efl/ewk_view.h:120 > +#endif // ewk_view_h Ditto.
Eunmi Lee
Comment 8 2011-06-02 19:59:40 PDT
Eunmi Lee
Comment 9 2011-06-02 21:54:32 PDT
(In reply to comment #4) > (From update of attachment 95745 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95745&action=review > > I don't know much about WebKit2, but I've found some small issues, after a quick glance over the code. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:33 > > +static const char EWK_VIEW_TYPE_STR[] = "EWK_View"; > > EWK_View is also used by EWK1 -- at least while the two APIs are coexisting here, I'd change that to "EWK2_View" or something similar. Done. I changed name to "EWK2_View" > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:58 > > +static void _ewk_view_smart_focus_in(Ewk_View_Smart_Data* sd) > > +{ > > + if (!sd) > > + return; > > + EWK_VIEW_PRIV_GET(sd, priv); > > This pattern repeats all over -- wouldn't it be nicer you had a macro like EWK_VIEW_PRIV_GET_OR_RETURN, like you have on current WebKitEFL? Done. I added EWK_VIEW_PRIV_GET_OR_RETURN macro > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:222 > > +{ > > + if (!priv) > > + return; > > + > > + free(priv); > > This will leak the RefPtr on the Ewk_View_Private_Data structure -- attribute 0 to it before freeing the structure. Done. I added priv->view=0 > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:246 > > + if (!sd->priv) > > + return; > > sd leaks here. Done. I added free(sd) > > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:102 > > +struct _Ewk_View_Smart_Data { > > + Evas_Object_Smart_Clipped_Data base; > > + const Ewk_View_Smart_Class* api; /**< reference to casted class instance */ > > + Evas_Object* self; /**< reference to owner object */ > > + Evas_Object *backing_store; /**< reference to backing store */ > > Small style issue: * aligned to the type. Done.
Eunmi Lee
Comment 10 2011-06-02 21:55:12 PDT
(In reply to comment #7) > View in context: https://bugs.webkit.org/attachment.cgi?id=95846&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:41 > > + // - if overridden, have to call parent method if desired > > If possible, I like to use /* */ instead of // for c header. > Done. I change // to /**/ > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:102 > > + Evas_Object *backing_store; /**< reference to backing store */ > > it's missed. Done. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:115 > > +EAPI WKPageRef ewk_view_page_get(Evas_Object*); > > Isn't it better to keep a parameter name like WebKit/EFL? > > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:120 > > +#endif // ewk_view_h > > Ditto. Done.
Leandro Pereira
Comment 11 2011-06-03 10:23:31 PDT
Comment on attachment 95847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95847&action=review I know nothing about WebKit2. I'm reviewing only style and other possibly minor issues. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:286 > + if (!sd->priv) { > + EINA_LOG_CRIT("could not allocate _Ewk_View_Private_Data"); > + free(sd); > + return; Dangling reference. evas_object_smart_data_set(o, 0); > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:320 > + EWK_VIEW_SD_GET(o, sd); EWK_VIEW_SD_GET_OR_RETURN(...) > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:332 > + EWK_VIEW_SD_GET(o, sd); Ditto. Please review all EWK_VIEW_SD_GET() usage on the following functions.
Eunmi Lee
Comment 12 2011-06-06 21:30:53 PDT
Eunmi Lee
Comment 13 2011-06-06 21:33:46 PDT
(In reply to comment #11) > (From update of attachment 95847 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95847&action=review > > I know nothing about WebKit2. I'm reviewing only style and other possibly minor issues. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:286 > > + if (!sd->priv) { > > + EINA_LOG_CRIT("could not allocate _Ewk_View_Private_Data"); > > + free(sd); > > + return; > > Dangling reference. evas_object_smart_data_set(o, 0); > I've added evas_object_smart_data_set(o, 0) before free(sd) > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:320 > > + EWK_VIEW_SD_GET(o, sd); > > EWK_VIEW_SD_GET_OR_RETURN(...) > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:332 > > + EWK_VIEW_SD_GET(o, sd); > > Ditto. Please review all EWK_VIEW_SD_GET() usage on the following functions. I've checked all EWK_VIEW_SD_GET and EWK_VIEW_PRIV_GET and replace some with *_GET_OR_RETURN
Eunmi Lee
Comment 14 2011-06-09 01:50:11 PDT
Eunmi Lee
Comment 15 2011-06-09 01:52:28 PDT
(In reply to comment #14) > Created an attachment (id=96561) [details] > Patch I replaced WebView to PageClientImpl.
Kenneth Rohde Christiansen
Comment 16 2011-06-09 02:44:26 PDT
Comment on attachment 96561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96561&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:43 > +#ifndef EWK_TYPE_CHECK > +#define EWK_VIEW_TYPE_CHECK(o, ...) do { } while (0) > +#else > +#define EWK_VIEW_TYPE_CHECK(o, ...) \ Shouldnt this be in some common header file?
Eunmi Lee
Comment 17 2011-06-09 06:26:24 PDT
Eunmi Lee
Comment 18 2011-06-09 06:38:46 PDT
(In reply to comment #16) > (From update of attachment 96561 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96561&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:43 > > +#ifndef EWK_TYPE_CHECK > > +#define EWK_VIEW_TYPE_CHECK(o, ...) do { } while (0) > > +#else > > +#define EWK_VIEW_TYPE_CHECK(o, ...) \ > > Shouldnt this be in some common header file? The EWK_VIEW_TYPE_CHECK is used before getting evas_object_smart_data, and evas_object_smart_data is used only in the ewk_view. So, I think that we can leave that macro in the cpp file. Anyway, I modified some codes in the new patch. 1. remove EWK_TYPE_CHECK +#ifndef EWK_TYPE_CHECK Adding above ifndef is my mistake. I copied that code from WebKit1's ewk_view but it is not used in the WebKit2's ewk_view. So, I have removed above ifndef from the code. 2. add EWK_TYPE_CHECK for EWK_VIEW_SD_GET EWK_TYPE_CHECK was done only in the EWK_VIEW_SD_GET_OR_RETURN. So, I have added EWK_TYPE_CHECK for EWK_VIEW_SD_GET too. 3. add eina_log_domain I have added variable _ewk2_log_dom for eina_log_domain.
Leandro Pereira
Comment 19 2011-06-09 07:35:04 PDT
Comment on attachment 96580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96580&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:126 > + NativeWebWheelEvent wheelEvent = NativeWebWheelEvent(ev, &position); NativeWebWheelEvent wheelEvent(ev, &position); Please review other constructors as well.
Eunmi Lee
Comment 20 2011-06-09 18:31:46 PDT
Eunmi Lee
Comment 21 2011-06-09 18:32:59 PDT
(In reply to comment #19) > (From update of attachment 96580 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96580&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:126 > > + NativeWebWheelEvent wheelEvent = NativeWebWheelEvent(ev, &position); > > NativeWebWheelEvent wheelEvent(ev, &position); > > Please review other constructors as well. Done. I've modified that as follows: priv->pageClient->page()->handleMouseEvent(NativeWebMouseEvent(ev, &position));
Eunmi Lee
Comment 22 2011-06-10 01:31:02 PDT
Eunmi Lee
Comment 23 2011-06-10 01:32:21 PDT
(In reply to comment #22) > Created an attachment (id=96712) [details] > Patch I removed free(sd) code from _ewk_view_smart_del() as follows: static void _ewk_view_smart_del(Evas_Object* o) { EWK_VIEW_SD_GET(o, sd); - if (sd) { - if (sd->priv) - _ewk_view_priv_del(sd->priv); - evas_object_smart_data_set(o, 0); - free(sd); - } + if (sd && sd->priv) + _ewk_view_priv_del(sd->priv); _parent_sc.del(o); } Because sd will be freed in the _parent_sc.del(o).
Eunmi Lee
Comment 24 2011-06-10 04:52:05 PDT
Eunmi Lee
Comment 25 2011-06-10 04:53:01 PDT
(In reply to comment #24) > Created an attachment (id=96736) [details] > Patch I've added #include "NativeWebKeyboardEvent.h".
Leandro Pereira
Comment 26 2011-06-10 09:54:58 PDT
Comment on attachment 96736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96736&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:380 > +Eina_Bool ewk_view_smart_set(Ewk_View_Smart_Class* api) Shouldn't this be called ewk_view_smart_class_init() or something like this?
Eunmi Lee
Comment 27 2011-06-12 18:55:46 PDT
(In reply to comment #26) > (From update of attachment 96736 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96736&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:380 > > +Eina_Bool ewk_view_smart_set(Ewk_View_Smart_Class* api) > > Shouldn't this be called ewk_view_smart_class_init() or something like this? ewk_view_smart_set() is called in the _ewk_view_smart_class_new(). We have to fill the Smart_Class functions and Ewk APIs before we create Evas_Smart using evas_smart_class_new(). ewk_view_smart_set()'s codes can be resident in the _ewk_view_smart_class_new() function, but I made that as a separate function for readability.
Leandro Pereira
Comment 28 2011-06-13 07:40:06 PDT
(In reply to comment #27) > (...) > ewk_view_smart_set()'s codes can be resident in the _ewk_view_smart_class_new() > function, but I made that as a separate function for readability. But if you call it "ewk_view_smart_set", you're basically telling me: "set the smart property of ewk_view". Which doesn't make much sense, does it?
Eunmi Lee
Comment 29 2011-06-14 05:34:16 PDT
Eunmi Lee
Comment 30 2011-06-14 05:37:35 PDT
(In reply to comment #28) > (In reply to comment #27) > > (...) > > ewk_view_smart_set()'s codes can be resident in the _ewk_view_smart_class_new() > > function, but I made that as a separate function for readability. > > But if you call it "ewk_view_smart_set", you're basically telling me: "set the smart property of ewk_view". Which doesn't make much sense, does it? Oops, I did not catch your point. I thought your comments' "called" is function call :( You mean rename! yes, I agree that ewk_view_smart_class_init() is better name than ewk_view_smart_set() And I've uploaded new patch for that :)
Leandro Pereira
Comment 31 2011-06-14 08:57:28 PDT
Comment on attachment 97100 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97100&action=review Looks almost OK -- just a couple of questions. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:348 > + if (priv->pageClient && sd->changed.size && ((w != sd->view.w) || (h != sd->view.h))) { Couldn't you just use sd->changed.size here? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:355 > + if (sd->changed.position && ((x != sd->view.x) || (y != sd->view.y))) { Ditto, for sd->changed.position.
Eunmi Lee
Comment 32 2011-06-14 18:32:33 PDT
Eunmi Lee
Comment 33 2011-06-14 18:36:58 PDT
(In reply to comment #31) > (From update of attachment 97100 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97100&action=review > > Looks almost OK -- just a couple of questions. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:348 > > + if (priv->pageClient && sd->changed.size && ((w != sd->view.w) || (h != sd->view.h))) { > > Couldn't you just use sd->changed.size here? > I've modified that code as follows: - if (priv->pageClient && sd->changed.size && ((w != sd->view.w) || (h != sd->view.h))) { - priv->pageClient->page()->drawingArea()->setSize(IntSize(w, h), IntSize()); + if (sd->changed.size && ((w != sd->view.w) || (h != sd->view.h))) { + if (priv->pageClient) + priv->pageClient->page()->drawingArea()->setSize(IntSize(w, h), IntSize()); I've just move priv->pageClient. > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:355 > > + if (sd->changed.position && ((x != sd->view.x) || (y != sd->view.y))) { > > Ditto, for sd->changed.position. I don't know what can I do more. Actually, I could not catch your point. Would you explain more about what you want?
Leandro Pereira
Comment 34 2011-06-15 09:12:04 PDT
(In reply to comment #33) > I don't know what can I do more. > Actually, I could not catch your point. > Would you explain more about what you want? Flags in sd->changed, AFAICT from these snippets, determine if something has changed values; so, if you keep these flags updated whenever something changed, there's no need to check the values again, only the respective flag.
Eunmi Lee
Comment 35 2011-06-15 22:24:44 PDT
Eunmi Lee
Comment 36 2011-06-15 22:26:31 PDT
(In reply to comment #34) > (In reply to comment #33) > > > I don't know what can I do more. > > Actually, I could not catch your point. > > Would you explain more about what you want? > > Flags in sd->changed, AFAICT from these snippets, determine if something has changed values; so, if you keep these flags updated whenever something changed, there's no need to check the values again, only the respective flag. Now, I understand what you mean. Yes, I agree that the size and position variable checking codes are not necessary. So, I've removed that.
Leandro Pereira
Comment 37 2011-06-16 07:28:12 PDT
Comment on attachment 97399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97399&action=review LGTM, just some small nits. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:354 > + if (sd->changed.size) { > + if (priv->pageClient) > + priv->pageClient->page()->drawingArea()->setSize(IntSize(w, h), IntSize()); > + sd->view.w = w; > + sd->view.h = h; > + } > + sd->changed.size = EINA_FALSE; You could move the attribution to sd->changed.size inside the if block. Same below.
Eunmi Lee
Comment 38 2011-06-16 17:45:07 PDT
Eunmi Lee
Comment 39 2011-06-16 17:45:58 PDT
(In reply to comment #37) > (From update of attachment 97399 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97399&action=review > > LGTM, just some small nits. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:354 > > + if (sd->changed.size) { > > + if (priv->pageClient) > > + priv->pageClient->page()->drawingArea()->setSize(IntSize(w, h), IntSize()); > > + sd->view.w = w; > > + sd->view.h = h; > > + } > > + sd->changed.size = EINA_FALSE; > > You could move the attribution to sd->changed.size inside the if block. Same below. Yes, I've moved that inside the if block.
Leandro Pereira
Comment 40 2011-06-17 08:16:15 PDT
(In reply to comment #39) > > Yes, I've moved that inside the if block. > LGTM.
Gyuyoung Kim
Comment 41 2011-06-23 00:50:04 PDT
Comment on attachment 97529 [details] Patch Internal r+ on my side. However, reviewer should review this patch.
Eunmi Lee
Comment 42 2011-12-15 01:01:52 PST
Created attachment 119394 [details] Patch for ewk_view. I've modified codes to follow changed ewk_*'s coding style rule by referencing https://bugs.webkit.org/show_bug.cgi?id=69073.
Eunmi Lee
Comment 43 2011-12-18 21:05:30 PST
Created attachment 119807 [details] patch for ewk_view.
Eunmi Lee
Comment 44 2011-12-18 21:10:25 PST
Created attachment 119808 [details] patch for ewk_view. I changed Ewk_View_Smart_Class functions' return-type from void to Eina_Bool, and modified ewk_view.h's style to follow the efl style. (ewk_view.cpp follows WebKit style.)
Gyuyoung Kim
Comment 45 2011-12-18 21:56:52 PST
Comment on attachment 119808 [details] patch for ewk_view. View in context: https://bugs.webkit.org/attachment.cgi?id=119808&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:175 > + Ewk_View_Smart_Data* smartData = (Ewk_View_Smart_Data*)data; Do not use c type casting. Please use C++ casting. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:183 > + Ewk_View_Smart_Data* smartData = (Ewk_View_Smart_Data*)data; ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:191 > + Evas_Event_Mouse_Wheel* wheelEvent = (Evas_Event_Mouse_Wheel*)eventInfo; ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:192 > + Ewk_View_Smart_Data* smartData = (Ewk_View_Smart_Data*)data; ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:200 > + Evas_Event_Mouse_Down* downEvent = (Evas_Event_Mouse_Down*)eventInfo; ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:201 > + Ewk_View_Smart_Data* smartData = (Ewk_View_Smart_Data*)data; ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:209 > + Evas_Event_Mouse_Up* upEvent = (Evas_Event_Mouse_Up*)eventInfo; ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:218 > + Evas_Event_Mouse_Move* moveEvent = (Evas_Event_Mouse_Move*)eventInfo; ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:219 > + Ewk_View_Smart_Data* smartData = (Ewk_View_Smart_Data*)data; ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:227 > + Evas_Event_Key_Down* downEvent = (Evas_Event_Key_Down*)eventInfo; ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:228 > + Ewk_View_Smart_Data* smartData = (Ewk_View_Smart_Data*)data; ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:236 > + Evas_Event_Key_Up* upEvent = (Evas_Event_Key_Up*)eventInfo; ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:237 > + Ewk_View_Smart_Data* smartData = (Ewk_View_Smart_Data*)data; ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:248 > + (Ewk_View_Private_Data*)calloc(1, sizeof(Ewk_View_Private_Data)); ditto.
Eunmi Lee
Comment 46 2011-12-18 22:30:28 PST
Thank you for your comment, Gyuyoung. I will update patch after changing all c style type casting to c++ style. (In reply to comment #45) > (From update of attachment 119808 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119808&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:175 > > + Ewk_View_Smart_Data* smartData = (Ewk_View_Smart_Data*)data; > > Do not use c type casting. Please use C++ casting. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:183 > > + Ewk_View_Smart_Data* smartData = (Ewk_View_Smart_Data*)data; > > ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:191 > > + Evas_Event_Mouse_Wheel* wheelEvent = (Evas_Event_Mouse_Wheel*)eventInfo; > > ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:192 > > + Ewk_View_Smart_Data* smartData = (Ewk_View_Smart_Data*)data; > > ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:200 > > + Evas_Event_Mouse_Down* downEvent = (Evas_Event_Mouse_Down*)eventInfo; > > ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:201 > > + Ewk_View_Smart_Data* smartData = (Ewk_View_Smart_Data*)data; > > ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:209 > > + Evas_Event_Mouse_Up* upEvent = (Evas_Event_Mouse_Up*)eventInfo; > > ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:218 > > + Evas_Event_Mouse_Move* moveEvent = (Evas_Event_Mouse_Move*)eventInfo; > > ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:219 > > + Ewk_View_Smart_Data* smartData = (Ewk_View_Smart_Data*)data; > > ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:227 > > + Evas_Event_Key_Down* downEvent = (Evas_Event_Key_Down*)eventInfo; > > ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:228 > > + Ewk_View_Smart_Data* smartData = (Ewk_View_Smart_Data*)data; > > ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:236 > > + Evas_Event_Key_Up* upEvent = (Evas_Event_Key_Up*)eventInfo; > > ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:237 > > + Ewk_View_Smart_Data* smartData = (Ewk_View_Smart_Data*)data; > > ditto. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:248 > > + (Ewk_View_Private_Data*)calloc(1, sizeof(Ewk_View_Private_Data)); > > ditto.
Eunmi Lee
Comment 47 2011-12-18 22:31:13 PST
Created attachment 119816 [details] patch for ewk_view. Change c style type casting to c++ style type casting.
Gyuyoung Kim
Comment 48 2011-12-21 20:23:04 PST
Comment on attachment 119816 [details] patch for ewk_view. LGTM.
Eric Seidel (no email)
Comment 49 2011-12-21 20:47:30 PST
Comment on attachment 119816 [details] patch for ewk_view. OK. rs=me.
WebKit Review Bot
Comment 50 2011-12-21 21:12:16 PST
Comment on attachment 119816 [details] patch for ewk_view. Clearing flags on attachment: 119816 Committed r103492: <http://trac.webkit.org/changeset/103492>
WebKit Review Bot
Comment 51 2011-12-21 21:12:24 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.