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

Description Eunmi Lee 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.
Comment 1 Eunmi Lee 2011-06-02 03:09:15 PDT
Created attachment 95745 [details]
Patch
Comment 2 WebKit Review Bot 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.
Comment 3 Eric Seidel (no email) 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.
Comment 4 Leandro Pereira 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.
Comment 5 Eunmi Lee 2011-06-02 19:26:41 PDT
Created attachment 95846 [details]
Patch
Comment 6 Eunmi Lee 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 :)
Comment 7 Ryuan Choi 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.
Comment 8 Eunmi Lee 2011-06-02 19:59:40 PDT
Created attachment 95847 [details]
Patch
Comment 9 Eunmi Lee 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.
Comment 10 Eunmi Lee 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.
Comment 11 Leandro Pereira 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.
Comment 12 Eunmi Lee 2011-06-06 21:30:53 PDT
Created attachment 96196 [details]
Patch
Comment 13 Eunmi Lee 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
Comment 14 Eunmi Lee 2011-06-09 01:50:11 PDT
Created attachment 96561 [details]
Patch
Comment 15 Eunmi Lee 2011-06-09 01:52:28 PDT
(In reply to comment #14)
> Created an attachment (id=96561) [details]
> Patch

I replaced WebView to PageClientImpl.
Comment 16 Kenneth Rohde Christiansen 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?
Comment 17 Eunmi Lee 2011-06-09 06:26:24 PDT
Created attachment 96580 [details]
Patch
Comment 18 Eunmi Lee 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.
Comment 19 Leandro Pereira 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.
Comment 20 Eunmi Lee 2011-06-09 18:31:46 PDT
Created attachment 96683 [details]
Patch
Comment 21 Eunmi Lee 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));
Comment 22 Eunmi Lee 2011-06-10 01:31:02 PDT
Created attachment 96712 [details]
Patch
Comment 23 Eunmi Lee 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).
Comment 24 Eunmi Lee 2011-06-10 04:52:05 PDT
Created attachment 96736 [details]
Patch
Comment 25 Eunmi Lee 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".
Comment 26 Leandro Pereira 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?
Comment 27 Eunmi Lee 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.
Comment 28 Leandro Pereira 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?
Comment 29 Eunmi Lee 2011-06-14 05:34:16 PDT
Created attachment 97100 [details]
Patch
Comment 30 Eunmi Lee 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 :)
Comment 31 Leandro Pereira 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.
Comment 32 Eunmi Lee 2011-06-14 18:32:33 PDT
Created attachment 97214 [details]
Patch
Comment 33 Eunmi Lee 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?
Comment 34 Leandro Pereira 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.
Comment 35 Eunmi Lee 2011-06-15 22:24:44 PDT
Created attachment 97399 [details]
Patch
Comment 36 Eunmi Lee 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.
Comment 37 Leandro Pereira 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.
Comment 38 Eunmi Lee 2011-06-16 17:45:07 PDT
Created attachment 97529 [details]
Patch
Comment 39 Eunmi Lee 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.
Comment 40 Leandro Pereira 2011-06-17 08:16:15 PDT
(In reply to comment #39)
>
> Yes, I've moved that inside the if block.
>

LGTM.
Comment 41 Gyuyoung Kim 2011-06-23 00:50:04 PDT
Comment on attachment 97529 [details]
Patch

Internal r+ on my side. However, reviewer should review this patch.
Comment 42 Eunmi Lee 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.
Comment 43 Eunmi Lee 2011-12-18 21:05:30 PST
Created attachment 119807 [details]
patch for ewk_view.
Comment 44 Eunmi Lee 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.)
Comment 45 Gyuyoung Kim 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.
Comment 46 Eunmi Lee 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.
Comment 47 Eunmi Lee 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.
Comment 48 Gyuyoung Kim 2011-12-21 20:23:04 PST
Comment on attachment 119816 [details]
patch for ewk_view.

LGTM.
Comment 49 Eric Seidel (no email) 2011-12-21 20:47:30 PST
Comment on attachment 119816 [details]
patch for ewk_view.

OK.  rs=me.
Comment 50 WebKit Review Bot 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>
Comment 51 WebKit Review Bot 2011-12-21 21:12:24 PST
All reviewed patches have been landed.  Closing bug.