Bug 68321 - [EFL] Use C++ type cast instead of C style type cast
Summary: [EFL] Use C++ type cast instead of C style type cast
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks: 68209
  Show dependency treegraph
 
Reported: 2011-09-18 00:37 PDT by Gyuyoung Kim
Modified: 2011-10-12 18:56 PDT (History)
6 users (show)

See Also:


Attachments
Patch (27.26 KB, patch)
2011-09-22 10:22 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (27.95 KB, patch)
2011-09-22 22:13 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (26.36 KB, patch)
2011-09-25 02:35 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (26.06 KB, patch)
2011-09-27 22:27 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (26.11 KB, patch)
2011-09-27 22:53 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (26.26 KB, patch)
2011-09-29 09:29 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (26.17 KB, patch)
2011-09-29 17:31 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (26.16 KB, patch)
2011-09-29 18:04 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2011-09-18 00:37:02 PDT
Some codes are using C type cast though file format is .cpp. A patch for this bug will be submitted after fixing Bug 68231.
Comment 1 Gyuyoung Kim 2011-09-22 10:22:57 PDT
Created attachment 108354 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2011-09-22 10:57:05 PDT
Comment on attachment 108354 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108354&action=review

I was going to comment on all occurrences which changed "Foo* p = ..." to "Foo *p = ..." but gave up about halfway into the patch.

> Source/WebKit/efl/ewk/ewk_auth_soup.cpp:82
> +    Ewk_Auth_Data *auth_data = static_cast<Ewk_Auth_Data*>(data);

Wrong '*' position.

> Source/WebKit/efl/ewk/ewk_contextmenu.cpp:106
> +    Ewk_Context_Menu_Item *item = static_cast<Ewk_Context_Menu_Item*>(malloc(sizeof(*item)));

Ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:689
> +    Ewk_Hit_Test *hit_test = static_cast<Ewk_Hit_Test*>(calloc(1, sizeof(Ewk_Hit_Test)));

Ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:210
> +    Ewk_View_Smart_Data *ptr = static_cast<Ewk_View_Smart_Data*>(evas_object_smart_data_get(o))

The placement of the first '*' could be fixed here.

> Source/WebKit/efl/ewk/ewk_view.cpp:473
> +    Ewk_View_Smart_Data *sd = static_cast<Ewk_View_Smart_Data*>(data);

Wrong position for the first '*'.

> Source/WebKit/efl/ewk/ewk_view.cpp:481
> +    Ewk_View_Smart_Data *sd = static_cast<Ewk_View_Smart_Data*>(data);

Ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:727
> +    const Ewk_View_Smart_Class* api = (Ewk_View_Smart_Class*)(sc);

No C++ cast here.
Comment 3 Lucas De Marchi 2011-09-22 12:49:52 PDT
(In reply to comment #2)
> (From update of attachment 108354 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108354&action=review
> 
> I was going to comment on all occurrences which changed "Foo* p = ..." to "Foo *p = ..." but gave up about halfway into the patch.

Gyuyoung, you might want to apply the uncrustify config on top of this patch and check if it changed back the position of '*'.
Comment 4 Gyuyoung Kim 2011-09-22 22:13:03 PDT
Created attachment 108441 [details]
Patch
Comment 5 Gyuyoung Kim 2011-09-22 22:20:58 PDT
Sorry, I missed to fix '*' operator in my local changes despite running git pull. I run uncrustify.cfg in my files. Fortunately, I found remained wrong pointer operator placement. I fix them as well.

>> Source/WebKit/efl/ewk/ewk_view.cpp:727
>> +    const Ewk_View_Smart_Class* api = (Ewk_View_Smart_Class*)(sc);

> No C++ cast here.

I fix this line using reinterpret_cast because of different pointer casting.
Comment 6 Raphael Kubo da Costa (:rakuco) 2011-09-23 07:52:23 PDT
LGTM.
Comment 7 Lucas De Marchi 2011-09-23 10:24:42 PDT
Comment on attachment 108441 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108441&action=review

> Source/WebKit/efl/ChangeLog:60
> +        * ewk/ewk_auth_soup.cpp:
> +        (_Ewk_Auth_Data::ewk_auth_soup_credentials_set):
> +        (_Ewk_Auth_Data::session_authenticate):
> +        * ewk/ewk_contextmenu.cpp:
> +        (ewk_context_menu_item_new):
> +        * ewk/ewk_frame.cpp:
> +        (_ewk_frame_smart_add):
> +        (ewk_frame_children_iterator_new):
> +        (ewk_frame_hit_test_new):
> +        * ewk/ewk_history.cpp:
> +        (_ewk_history_item_new):
> +        (ewk_history_item_title_get):
> +        (ewk_history_item_title_alternate_get):
> +        (ewk_history_item_uri_get):
> +        (ewk_history_item_uri_original_get):
> +        (ewk_history_new):
> +        * ewk/ewk_view.cpp:
> +        (_ewk_view_repaints_resize):
> +        (_ewk_view_scrolls_resize):
> +        (_ewk_view_on_focus_in):
> +        (_ewk_view_on_focus_out):
> +        (_ewk_view_on_mouse_wheel):
> +        (_ewk_view_on_mouse_down):
> +        (_ewk_view_on_mouse_up):
> +        (_ewk_view_on_mouse_move):
> +        (_ewk_view_on_key_down):
> +        (_ewk_view_on_key_up):
> +        (_ewk_view_priv_new):
> +        (_ewk_view_smart_add):
> +        (_ewk_view_smart_zoom_set):
> +        (_ewk_view_zoom_animator_cb):
> +        (_ewk_view_viewport_attributes_compute):
> +        (ewk_view_zoom_set):
> +        (ewk_view_zoom_weak_set):
> +        (ewk_view_zoom_animated_set):
> +        (ewk_view_paint_context_new):
> +        (ewk_view_popup_new):
> +        (ewk_view_popup_destroy):
> +        * ewk/ewk_view_single.cpp:
> +        (_ewk_view_single_smart_add):
> +        (_ewk_view_single_smart_resize):
> +        (_ewk_view_single_scroll_process_single):
> +        (_ewk_view_single_smart_repaints_process):
> +        * ewk/ewk_view_tiled.cpp:
> +        (_ewk_view_tiled_render_cb):
> +        (_ewk_view_tiled_updates_process_pre):
> +        (_ewk_view_tiled_contents_size_changed_cb):
> +        (_ewk_view_tiled_smart_add):

As mentioned by Antonio Gomes in the other giant patch, please remove this from ChangeLog since it's not adding any useful information.

> Source/WebKit/efl/ewk/ewk_view.cpp:1564
> -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> +        WRN("zoom level is < %f : %f", static_cast<double>(priv->settings.zoom_range.min_scale), static_cast<double>(zoom));

I'm surprised to see we are casting a float to a double. This is a nice time to remove that.

> Source/WebKit/efl/ewk/ewk_view.cpp:1568
> -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> +        WRN("zoom level is > %f : %f", static_cast<double>(priv->settings.zoom_range.max_scale), static_cast<double>(zoom));

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1608
> -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> +        WRN("zoom level is < %f : %f", static_cast<double>(priv->settings.zoom_range.min_scale), static_cast<double>(zoom));

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1612
> -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> +        WRN("zoom level is > %f : %f", static_cast<double>(priv->settings.zoom_range.max_scale), static_cast<double>(zoom));

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1664
> -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> +        WRN("zoom level is < %f : %f", static_cast<double>(priv->settings.zoom_range.min_scale), static_cast<double>(zoom));

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1668
> -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> +        WRN("zoom level is > %f : %f", static_cast<double>(priv->settings.zoom_range.max_scale), static_cast<double>(zoom));

ditto.
Comment 8 Gyuyoung Kim 2011-09-25 02:35:31 PDT
Created attachment 108604 [details]
Patch
Comment 9 Gyuyoung Kim 2011-09-25 02:39:12 PDT
This is nice time to fix wrong casting. I fixed them. And, meaningless comments are removed from ChangeLog as well.
Comment 10 Lucas De Marchi 2011-09-27 03:00:15 PDT
Comment on attachment 108604 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108604&action=review

Please, check the unneeded casts. Especially the float -> double conversions.

> Source/WebKit/efl/ewk/ewk_view.cpp:929
> -        px = (double)(x + cx) / (w + sd->view.w);
> +        px = static_cast<double>((x + cx) / (w + sd->view.w));

Unneeded cast.

> Source/WebKit/efl/ewk/ewk_view.cpp:934
> -        py = (double)(y + cy) / (h + sd->view.h);
> +        py = static_cast<double>((y + cy) / (h + sd->view.h));

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1052
> -    int available_width = (int) priv->page->chrome()->client()->pageRect().width();
> -    int available_height = (int) priv->page->chrome()->client()->pageRect().height();
> +    int available_width = static_cast<int>(priv->page->chrome()->client()->pageRect().width());
> +    int available_height = static_cast<int>(priv->page->chrome()->client()->pageRect().height());

/me wondering why we are going priv->page->chrome()->client() route when we have the ewk_view available. It would be just a matter of calling ewk_view_page_rect_get().

About the casts, we are probably better off using enclosingIntRect() instead.

> Source/WebKit/efl/ewk/ewk_view.cpp:1055
> -    int device_width = (int) priv->page->chrome()->client()->windowRect().width();
> -    int device_height = (int) priv->page->chrome()->client()->windowRect().height();
> +    int device_width = static_cast<int>(priv->page->chrome()->client()->windowRect().width());
> +    int device_height = static_cast<int>(priv->page->chrome()->client()->windowRect().height());

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1564
> -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> +        WRN("zoom level is < %f : %f", static_cast<float>(priv->settings.zoom_range.min_scale), static_cast<float>(zoom));

Unneeded casts.

> Source/WebKit/efl/ewk/ewk_view.cpp:1568
> -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> +        WRN("zoom level is > %f : %f", static_cast<float>(priv->settings.zoom_range.max_scale), static_cast<float>(zoom));

Unneeded casts.

> Source/WebKit/efl/ewk/ewk_view.cpp:1608
> -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> +        WRN("zoom level is < %f : %f", static_cast<float>(priv->settings.zoom_range.min_scale), static_cast<float>(zoom));

Unneeded casts.

> Source/WebKit/efl/ewk/ewk_view.cpp:1612
> -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> +        WRN("zoom level is > %f : %f", static_cast<float>(priv->settings.zoom_range.max_scale), static_cast<float>(zoom));

Unneeded casts.

> Source/WebKit/efl/ewk/ewk_view.cpp:1664
> -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> +        WRN("zoom level is < %f : %f", static_cast<float>(priv->settings.zoom_range.min_scale), static_cast<float>(zoom));

Unneeded casts.

> Source/WebKit/efl/ewk/ewk_view.cpp:1668
> -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> +        WRN("zoom level is > %f : %f", static_cast<float>(priv->settings.zoom_range.max_scale), static_cast<float>(zoom));

Unneeded casts.
Comment 11 Gyuyoung Kim 2011-09-27 20:41:57 PDT
>> /me wondering why we are going priv->page->chrome()->client() route when we have the ewk_view available. It would be just a matter of calling ewk_view_page_rect_get().

>> About the casts, we are probably better off using enclosingIntRect() instead.

I think that is modified by new bug. So, I think it is better to use static_cast<..> for this bug as below,

1051     int available_width = static_cast<int>(priv->page->chrome()->client()->pageRect().width());
1052     int available_height = static_cast<int>(priv->page->chrome()->client()->pageRect().height());
1053 
1054     int device_width = static_cast<int>(priv->page->chrome()->client()->windowRect().width());
1055     int device_height = static_cast<int>(priv->page->chrome()->client()->windowRect().height());

Do you think that is fixed first ?
Comment 12 Lucas De Marchi 2011-09-27 21:43:47 PDT
(In reply to comment #11)
> >> /me wondering why we are going priv->page->chrome()->client() route when we have the ewk_view available. It would be just a matter of calling ewk_view_page_rect_get().
> 
> >> About the casts, we are probably better off using enclosingIntRect() instead.
> 
> I think that is modified by new bug. So, I think it is better to use static_cast<..> for this bug as below,
> 
> 1051     int available_width = static_cast<int>(priv->page->chrome()->client()->pageRect().width());
> 1052     int available_height = static_cast<int>(priv->page->chrome()->client()->pageRect().height());
> 1053 
> 1054     int device_width = static_cast<int>(priv->page->chrome()->client()->windowRect().width());
> 1055     int device_height = static_cast<int>(priv->page->chrome()->client()->windowRect().height());
> 

ok.
Comment 13 Gyuyoung Kim 2011-09-27 22:27:06 PDT
Created attachment 108962 [details]
Patch
Comment 14 Gyuyoung Kim 2011-09-27 22:31:27 PDT
Comment on attachment 108962 [details]
Patch

Attachment 108962 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9881573
Comment 15 Gyuyoung Kim 2011-09-27 22:53:04 PDT
Created attachment 108967 [details]
Patch
Comment 16 Gyuyoung Kim 2011-09-27 23:03:05 PDT
Hello Lucas,

My though is a little changed. It seems it is better to remove static_cast<...> of _ewk_view_viewport_attributes_compute() in this bug.

WebCore::IntRect available_rect = enclosingIntRect(priv->page->chrome()->client()->pageRect());
int available_width = available_rect.width();
int available_height = available_rect.height();

WebCore::IntRect device_rect = enclosingIntRect(priv->page->chrome()->client()->windowRect());
int device_width = device_rect.width();
int device_height = device_rect.height();

Of course, we can file new bug to decide which functions are used instead of pageRect(), windowRect().
Comment 17 Lucas De Marchi 2011-09-28 06:36:14 PDT
Comment on attachment 108967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108967&action=review

Otherwise looks good now.

> Source/WebKit/efl/ewk/ewk_view.cpp:1053
> -    int available_width = (int) priv->page->chrome()->client()->pageRect().width();
> -    int available_height = (int) priv->page->chrome()->client()->pageRect().height();
> +    WebCore::IntRect available_rect = enclosingIntRect(priv->page->chrome()->client()->pageRect());
> +    int available_width = available_rect.width();
> +    int available_height = available_rect.height();

WebCore::IntRect availableRect = enclosingIntRect(priv->page->chrome()->client()->pageRect());

and you don't need available_{width,height}. Just use .width()/.height() below.

> Source/WebKit/efl/ewk/ewk_view.cpp:1057
> -    int device_width = (int) priv->page->chrome()->client()->windowRect().width();
> -    int device_height = (int) priv->page->chrome()->client()->windowRect().height();
> +    WebCore::IntRect device_rect = enclosingIntRect(priv->page->chrome()->client()->windowRect());
> +    int device_width = device_rect.width();
> +    int device_height = device_rect.height();

Same here. deviceRect would be more WebKit-style aligned.
Comment 18 Gyuyoung Kim 2011-09-28 19:03:07 PDT
(In reply to comment #17)
> (From update of attachment 108967 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108967&action=review
> 
> Otherwise looks good now.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1053
> > -    int available_width = (int) priv->page->chrome()->client()->pageRect().width();
> > -    int available_height = (int) priv->page->chrome()->client()->pageRect().height();
> > +    WebCore::IntRect available_rect = enclosingIntRect(priv->page->chrome()->client()->pageRect());
> > +    int available_width = available_rect.width();
> > +    int available_height = available_rect.height();
> 
> WebCore::IntRect availableRect = enclosingIntRect(priv->page->chrome()->client()->pageRect());
> 
> and you don't need available_{width,height}. Just use .width()/.height() below.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1057
> > -    int device_width = (int) priv->page->chrome()->client()->windowRect().width();
> > -    int device_height = (int) priv->page->chrome()->client()->windowRect().height();
> > +    WebCore::IntRect device_rect = enclosingIntRect(priv->page->chrome()->client()->windowRect());
> > +    int device_width = device_rect.width();
> > +    int device_height = device_rect.height();
> 
> Same here. deviceRect would be more WebKit-style aligned.

efl port is using mixed efl-style and webkit-style for local variable. I think we should make a rule for local variable. This is an new item for Bug 68321. Do you want to remove "_" in available_rect and device_rect in this bug ?
Comment 19 Lucas De Marchi 2011-09-29 07:43:59 PDT
Comment on attachment 108967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108967&action=review

>>> Source/WebKit/efl/ewk/ewk_view.cpp:1053
>>> +    int available_height = available_rect.height();
>> 
>> WebCore::IntRect availableRect = enclosingIntRect(priv->page->chrome()->client()->pageRect());
>> 
>> and you don't need available_{width,height}. Just use .width()/.height() below.
> 
> efl port is using mixed efl-style and webkit-style for local variable. I think we should make a rule for local variable. This is an new item for Bug 68321. Do you want to remove "_" in available_rect and device_rect in this bug ?

I don't care too much about the name now (since it can be made in another bug). Just send another patch removing the useless available_width, available_height, device_width and device_height variables. Then you have my informal r+.
Comment 20 Gyuyoung Kim 2011-09-29 09:29:58 PDT
Created attachment 109168 [details]
Patch
Comment 21 Gyuyoung Kim 2011-09-29 09:32:52 PDT
I remove useless local variables in _ewk_view_viewport_attributes_compute() according to your comment.
Comment 22 Lucas De Marchi 2011-09-29 10:01:35 PDT
Comment on attachment 109168 [details]
Patch

Looks good. Informal r+
Comment 23 Martin Robinson 2011-09-29 17:06:48 PDT
Comment on attachment 109168 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109168&action=review

Nice cleanup, but I have some concerns.

> Source/WebKit/efl/ewk/ewk_view.cpp:930
> -        px = (double)(x + cx) / (w + sd->view.w);
> +        px = (x + cx) / (w + sd->view.w);
>      else

Are you sure a cast isn't required here to force floating point arithmetic?

> Source/WebKit/efl/ewk/ewk_view.cpp:934
> -        py = (double)(y + cy) / (h + sd->view.h);
> +        py = (y + cy) / (h + sd->view.h);

Ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1055
> +    WebCore::IntSize available_size = WebCore::IntSize(available_rect.width(), available_rect.height());
> +    WebCore::ViewportAttributes attributes = WebCore::computeViewportAttributes(priv->viewport_arguments, desktop_width, device_rect.width(), device_rect.height(), device_dpi, available_size);

You should just do available_rect.size()

> Source/WebKit/efl/ewk/ewk_view.cpp:1565
> -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> +        WRN("zoom level is < %f : %f", priv->settings.zoom_range.min_scale, zoom);
>          return EINA_FALSE;
>      }
>      if (zoom > priv->settings.zoom_range.max_scale) {
> -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> +        WRN("zoom level is > %f : %f", priv->settings.zoom_range.max_scale, zoom);

Are these values not floats? If so why not fix the actual format specifiers instead of just removing the cast?
Comment 24 Lucas De Marchi 2011-09-29 17:20:18 PDT
(In reply to comment #23)
> (From update of attachment 109168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109168&action=review
> 
> Nice cleanup, but I have some concerns.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:930
> > -        px = (double)(x + cx) / (w + sd->view.w);
> > +        px = (x + cx) / (w + sd->view.w);
> >      else
> 
> Are you sure a cast isn't required here to force floating point arithmetic?

This was my bad. I didn't pay attention w and sd->view.w are of type Evas_Coord and not double.
 
 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1565
> > -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> > +        WRN("zoom level is < %f : %f", priv->settings.zoom_range.min_scale, zoom);
> >          return EINA_FALSE;
> >      }
> >      if (zoom > priv->settings.zoom_range.max_scale) {
> > -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> > +        WRN("zoom level is > %f : %f", priv->settings.zoom_range.max_scale, zoom);
> 
> Are these values not floats? If so why not fix the actual format specifiers instead of just removing the cast?

These values are floats. This is the reason the cast is not needed.
Comment 25 Gyuyoung Kim 2011-09-29 17:22:52 PDT
(In reply to comment #23)
> (From update of attachment 109168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109168&action=review
> 
> Nice cleanup, but I have some concerns.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:930
> > -        px = (double)(x + cx) / (w + sd->view.w);
> > +        px = (x + cx) / (w + sd->view.w);
> >      else
> 
> Are you sure a cast isn't required here to force floating point arithmetic?

I think it is better to use a cast explicitly. Frankly I am not sure.

> > Source/WebKit/efl/ewk/ewk_view.cpp:934
> > -        py = (double)(y + cy) / (h + sd->view.h);
> > +        py = (y + cy) / (h + sd->view.h);
> 
> Ditto.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1055
> > +    WebCore::IntSize available_size = WebCore::IntSize(available_rect.width(), available_rect.height());
> > +    WebCore::ViewportAttributes attributes = WebCore::computeViewportAttributes(priv->viewport_arguments, desktop_width, device_rect.width(), device_rect.height(), device_dpi, available_size);
> 
> You should just do available_rect.size()
I missed to use .size(). I will fix it.

> > Source/WebKit/efl/ewk/ewk_view.cpp:1565
> > -        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
> > +        WRN("zoom level is < %f : %f", priv->settings.zoom_range.min_scale, zoom);
> >          return EINA_FALSE;
> >      }
> >      if (zoom > priv->settings.zoom_range.max_scale) {
> > -        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
> > +        WRN("zoom level is > %f : %f", priv->settings.zoom_range.max_scale, zoom);
> 
> Are these values not floats? If so why not fix the actual format specifiers instead of just removing the cast?

max_scale and zoom variables are float type. So, IMHO, it is better to use "%f" format specifier without casting.
Comment 26 Gyuyoung Kim 2011-09-29 17:31:14 PDT
Created attachment 109226 [details]
Patch
Comment 27 Martin Robinson 2011-09-29 17:42:13 PDT
(In reply to comment #25)
> (In reply to comment #23)
> > (From update of attachment 109168 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=109168&action=review
> > 
> > Nice cleanup, but I have some concerns.
> > 
> > > Source/WebKit/efl/ewk/ewk_view.cpp:930
> > > -        px = (double)(x + cx) / (w + sd->view.w);
> > > +        px = (x + cx) / (w + sd->view.w);
> > >      else
> > 
> > Are you sure a cast isn't required here to force floating point arithmetic?
> 
> I think it is better to use a cast explicitly. Frankly I am not sure.

If Evas_Coord is an integer type then I believe it's necessary.
Comment 28 Martin Robinson 2011-09-29 17:44:22 PDT
Comment on attachment 109226 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109226&action=review

> Source/WebKit/efl/ewk/ewk_view.cpp:929
> +        px = static_cast<double>((x + cx) / (w + sd->view.w));

Casting the entire expression here is semantically identical to the implicit cast. You need to at least cast the first operand of the division.
Comment 29 Gyuyoung Kim 2011-09-29 18:04:16 PDT
Created attachment 109229 [details]
Patch
Comment 30 Martin Robinson 2011-09-29 18:11:16 PDT
Comment on attachment 109229 [details]
Patch

Nice cleanup!
Comment 31 WebKit Review Bot 2011-09-29 19:17:10 PDT
Comment on attachment 109229 [details]
Patch

Clearing flags on attachment: 109229

Committed r96382: <http://trac.webkit.org/changeset/96382>
Comment 32 WebKit Review Bot 2011-09-29 19:17:16 PDT
All reviewed patches have been landed.  Closing bug.