[webkit-reviews] review denied: [Bug 62842] [EFL] Refactor zoom related APIs. : [Attachment 109727] Patch

[bugzilla-daemon at webkit.org]

Raphael Kubo da Costa <kubo at profusion.mobi> has denied Ryuan Choi
<ryuan.choi at samsung.com>'s request for review:
Bug 62842: [EFL] Refactor zoom related APIs.
https://bugs.webkit.org/show_bug.cgi?id=62842

Attachment 109727: Patch
https://bugs.webkit.org/attachment.cgi?id=109727&action=review

------- Additional Comments from Raphael Kubo da Costa <kubo at profusion.mobi>
View in context: https://bugs.webkit.org/attachment.cgi?id=109727&action=review


Informal r- here.

The ChangeLog is very incomplete -- this patch introduces a lot of changes, and
if I were a reviewer who had not been closely paying attention to the whole
discussion, I wouldn't really understand why these changes were made and what
exactly is being changed by just looking at the ChangeLog.

Besides that, two separate things are being done at once here -- the
refactoring of the APIs and the use of a different zoom method when zooming a
view (this change is not mentioned in the ChangeLog at all). I'd rather do them
separately.

> Source/WebKit/efl/ChangeLog:32
> +	   * ewk/ewk_frame.cpp:
> +	   (ewk_frame_text_zoom_get):
> +	   (ewk_frame_text_zoom_set):
> +	   (ewk_frame_zoom_get):
> +	   (ewk_frame_zoom_set):
> +	   * ewk/ewk_frame.h:
> +	   * ewk/ewk_view.cpp:
> +	   (_ewk_view_smart_scale_set):
> +	   (_ewk_view_zoom_animator_cb):
> +	   (ewk_view_base_smart_set):
> +	   (ewk_view_scale_get):
> +	   (ewk_view_scale_set):
> +	   (ewk_view_text_zoom_get):
> +	   (ewk_view_text_zoom_set):
> +	   (ewk_view_zoom_get):
> +	   (ewk_view_zoom_set):
> +	   (ewk_view_zoom_weak_set):
> +	   (ewk_view_zoom_animated_set):
> +	   (ewk_view_pre_render_region):
> +	   (ewk_view_pre_render_relative_radius):
> +	   * ewk/ewk_view.h:

You've made a lot of changes, please describe them accordingly here.

> Source/WebKit/efl/ewk/ewk_view.cpp:931
> +    x = static_cast<double>(x + cx) / currentScaleFactor * scaleFactor - cx;

> +    y = static_cast<double>(y + cy) / currentScaleFactor * scaleFactor - cy;


Use parentheses in the expressions so we don't get confused trying to parse the
precedence of the operators here.

> Source/WebKit/efl/ewk/ewk_view.cpp:933
> +    priv->page->setPageScaleFactor(scaleFactor, WebCore::LayoutPoint(x, y));


Please add the appropriate include for the LayoutPoint typedef.

> Source/WebKit/efl/ewk/ewk_view.cpp:1563
> +float ewk_view_text_zoom_get(const Evas_Object* o)

o -> ewkView?

> Source/WebKit/efl/ewk/ewk_view.cpp:1569
> +Eina_Bool ewk_view_text_zoom_set(Evas_Object* o, float textZoomFactor)

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1575
> +float ewk_view_zoom_get(const Evas_Object* o)

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1581
> +Eina_Bool ewk_view_zoom_set(Evas_Object* o, float zoomFactor)

ditto.