[webkit-reviews] review denied: [Bug 75592] [EFL] Move viewport functions to new files : [Attachment 121250] Patch

[bugzilla-daemon at webkit.org]

Raphael Kubo da Costa <kubo at profusion.mobi> has denied Gyuyoung Kim
<gyuyoung.kim at samsung.com>'s request for review:
Bug 75592: [EFL] Move viewport functions to new files
https://bugs.webkit.org/show_bug.cgi?id=75592

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

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


This new architecture is weird and does not seem to simplify things very much.
We have moved from a situation in which ChromeClientEfl called code in ewk_view
to one in which ChromClientEfl uses a view to get private viewport data that is
then passed to another viewport function, and all these parts have pointers to
the view. Plus, it looks like the user cannot retrieve the viewport data
anymore.

The situation at stake here is quite simple: ChromeClientEfl needs to notify
that the viewport's properties have changed, and ewk needs to provide access to
the viewport's properties (ie. convert the WebCore data types to user-visible
data types). This all sounds like view-specific stuff to me.

I agree that refactoring and breaking up ewk_view and ewk_frame is good (they
are huge), but this specific refactoring does not help much.

> Source/WebKit/efl/ewk/ewk_view.cpp:-3553
> -void ewk_view_viewport_attributes_get(const Evas_Object* ewkView, int*
width, int* height, float* initScale, float* maxScale, float* minScale, float*
devicePixelRatio, Eina_Bool* userScalable)

How does a user get this information now?

> Source/WebKit/efl/ewk/ewk_view.h:-83
> - *  - "viewport,changed", void: reports that viewport was changed.

The signal is still emitted by the view.

> Source/WebKit/efl/ewk/ewk_viewport_attributes.cpp:24
> +#include "ChromeClientEfl.h"

ewk shouldn't access WebCoreSupport.

> Source/WebKit/efl/ewk/ewk_viewport_attributes.cpp:128
> +    if (!priv) {
> +	   CRITICAL("could not allocate Ewk_Viewport_Private_Data");
> +	   return 0;
> +    }

new never returns 0.

> Source/WebKit/efl/ewk/ewk_viewport_attributes.h:24
> +#include <Eina.h>
> +#include <Evas.h>

Not needed.