Bug 61915 - [EFL][WK2] Add efl port's ewk_view files
Summary: [EFL][WK2] Add efl port's ewk_view files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 61838
  Show dependency treegraph
 
Reported: 2011-06-02 02:55 PDT by Eunmi Lee
Modified: 2011-12-21 21:12 PST (History)
10 users (show)

See Also:


Attachments
Patch (19.50 KB, patch)
2011-06-02 03:09 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (22.39 KB, patch)
2011-06-02 19:26 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (22.47 KB, patch)
2011-06-02 19:59 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (22.83 KB, patch)
2011-06-06 21:30 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (22.92 KB, patch)
2011-06-09 01:50 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (23.98 KB, patch)
2011-06-09 06:26 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (23.79 KB, patch)
2011-06-09 18:31 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (23.55 KB, patch)
2011-06-10 01:31 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (23.58 KB, patch)
2011-06-10 04:52 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (23.61 KB, patch)
2011-06-14 05:34 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (23.62 KB, patch)
2011-06-14 18:32 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (23.54 KB, patch)
2011-06-15 22:24 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (23.54 KB, patch)
2011-06-16 17:45 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch for ewk_view. (24.86 KB, patch)
2011-12-15 01:01 PST, Eunmi Lee
no flags Details | Formatted Diff | Diff
patch for ewk_view. (25.29 KB, patch)
2011-12-18 21:05 PST, Eunmi Lee
no flags Details | Formatted Diff | Diff
patch for ewk_view. (25.29 KB, patch)
2011-12-18 21:10 PST, Eunmi Lee
no flags Details | Formatted Diff | Diff
patch for ewk_view. (25.49 KB, patch)
2011-12-18 22:31 PST, Eunmi Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.