[webkit-reviews] review denied: [Bug 104215] [WK2][EFL] Modify a zoom level setting on MiniBrowser : [Attachment 177945] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied KyungTae Kim
<ktf.kim at samsung.com>'s request for review:
Bug 104215: [WK2][EFL] Modify a zoom level setting on MiniBrowser
https://bugs.webkit.org/show_bug.cgi?id=104215

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=177945&action=review


> Tools/MiniBrowser/efl/main.c:67
> +#if USE_ZOOM_LEVEL
>  #define DEFAULT_ZOOM_LEVEL 5 // Set default zoom level to 1.0 (index 5 on
zoomLevels).
>  // The zoom values are chosen to be like in Mozilla Firefox 3.
>  const static float zoomLevels[] = {0.3, 0.5, 0.67, 0.8, 0.9, 1.0, 1.1, 1.2,
1.33, 1.5, 1.7, 2.0, 2.4, 3.0};
>  
> +static float
> +get_scale_factor_from_zoom_levels(float currentScaleFactor, Zoom_Set_Mode
mode)
> +{
> +    int i = 0;

Why not just use percentage increase/decrease. What is the point of using those
firefox values? I don't get it

> Tools/MiniBrowser/efl/main.c:98
> +    else
> +	   currentScaleFactor = ewk_view_device_pixel_ratio_get(webview);

This is wrong. You need the current scale. If the ewk_view_scale_set is setting
effective scale and not css scale, it should include the pixel ratio.

> Tools/MiniBrowser/efl/main.c:102
> +#if USE_ZOOM_LEVEL // Use pre-defined zoom level list
> +    scaleFactor = get_scale_factor_from_zoom_levels(currentScaleFactor,
mode);
> +#else // Just calculate from current scale factor

Why have this at all?

> Tools/MiniBrowser/efl/main.c:114
> +    // Adjust scaleFactor to boundary.
> +    if (scaleFactor < 0.25)
> +	   scaleFactor = 0.25;
> +    else if (scaleFactor > 4.0)
> +	   scaleFactor = 4.0;

What if the viewport meta tag says that you cannot scale at all? This code
needs to cooperate with the PageViewportController.

Maybe the set_scale method should take care of this internally and return bool
if it succeeded or not

> Tools/MiniBrowser/efl/main.c:120
> +    if (legacy_behavior_enabled) {
> +	   Evas_Coord ox, oy, mx, my, cx, cy;
> +	   evas_pointer_canvas_xy_get(evas_object_evas_get(webview), &mx, &my);
// Get current mouse position on window.
> +	   evas_object_geometry_get(webview, &ox, &oy, NULL, NULL); // Get
webview's position on window.

Such code is ugly and that is why we can separate it out in functions

> Tools/MiniBrowser/efl/main.c:122
> +	   cy = my - oy; // current y position = mouse y position - webview y
position7

7 ?