[Webkit-unassigned] [Bug 40278] [EFL] EFLWebKit doesn't support viewport meta tag

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=40278





--- Comment #28 from Lucas De Marchi <lucas.demarchi at profusion.mobi>  2010-06-16 06:01:26 PST ---
(From update of attachment 58867)
Hi,  Gyuyoung

Now the patch is indeed better than before. I have a few comments yet.

>Index: WebKit/ChangeLog
Thanks for explaining in the ChangeLog the changes you made. Now it's ok.

>+/**
>+ * "viewport,changed" signal will be always emitted irregardless of the viewport existence. 
typo: s/irregardless/regardless/

>+ *
>+ * If you don't want to process the viewport tag, you can either do nothing in this callback
>+ * or simply ignore the signal in your application.
Good.
>+ */
>+static void
>+on_viewport_changed(void* user_data, Evas_Object* webview, void* event_info)
>+{
>+    int w, h;
>+    float initScale, minScale, maxScale, userScalable;
>+
>+    ewk_view_viewport_get(webview, &w, &h, &initScale, &maxScale, &minScale, &userScalable);
>+
>+    /**
>+     * If there is no argument in viewport tag, argument's value is -1.
>+     */
>+    w = (w == -1) ? DEFAULT_WIDTH : w;
>+    h = (h == -1) ? DEFAULT_HEIGHT : h;
>+    initScale = ((int)initScale == -1) ? ZOOM_INIT : initScale;
>+    minScale = ((int)minScale == -1) ? ZOOM_MIN : minScale; 
>+    maxScale = ((int)maxScale == -1) ? ZOOM_MAX : maxScale;
>+    userScalable = ((int)userScalable == -1) ? EINA_TRUE : userScalable;

Why are you using the "?" operator if the else part is the same value as before? Isn't it better to do like this?
if (w == -1)
    w = DEFAULT_WIDTH;

>+void ChromeClientEfl::didReceiveViewportArguments(Frame* frame, const ViewportArguments& arguments) const
>+{
>+    ewk_view_viewport_set(m_view, (int)arguments.width, (int)arguments.height, arguments.initialScale, arguments.minimumScale, arguments.maximumScale, arguments.userScalable);
>+
>+    FrameLoaderClientEfl* client = static_cast<FrameLoaderClientEfl*>(frame->loader()->client());
>+    client->setInitLayoutCompleted(true);

What the viewport arguments has to do with "InitLayoutCompleted"? I know you are calling this function to not send twice the signal to the application, but the name is meaningless. I suggest something like "setViewportArgumentsReceived(true);"


>Index: WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp
>===================================================================
>--- WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp	(revision 61229)
>+++ WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp	(working copy)

> <...>
> void FrameLoaderClientEfl::dispatchDidFinishDocumentLoad()
>@@ -595,7 +598,10 @@ void FrameLoaderClientEfl::dispatchDidFi
> 
> void FrameLoaderClientEfl::dispatchDidFirstVisuallyNonEmptyLayout()

I understood your point about using this function. However if you set the viewport on this function it will trigger a re-layout. I think the double signal you got on the other function is related to something else, since QT guys are using it with no problem (afaik).


>Index: WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.h
>===================================================================
>--- WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.h	(revision 61229)
>+++ WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.h	(working copy)
>@@ -55,6 +55,9 @@ class FrameLoaderClientEfl : public Fram
>     void setCustomUserAgent(const String &agent);
>     const String& customUserAgent() const;
> 
>+    void setInitLayoutCompleted(bool completed) { m_initLayoutCompleted = completed; }
>+    bool getInitLayoutCompleted() { return m_initLayoutCompleted; }
As I told, please re-think these names.


>Index: WebKit/efl/ewk/ewk_private.h
>===================================================================
>--- WebKit/efl/ewk/ewk_private.h	(revision 61229)
>+++ WebKit/efl/ewk/ewk_private.h	(working copy)
>@@ -93,6 +93,8 @@ WTF::PassRefPtr<WebCore::Widget> ewk_vie
> 
> void             ewk_view_popup_new(Evas_Object *o, WebCore::PopupMenuClient* client, int selected, const WebCore::IntRect& rect);
> 
>+void             ewk_view_viewport_set(Evas_Object *o, int w, int h, float init_scale, float max_scale, float min_scale, float user_scalable);

w and h are int, requiring casts when you call this function. However, user_scalable is float, as in WebCore. In order to avoid the casts I'd let w and h as floats too.

>Index: WebKit/efl/ewk/ewk_view.cpp
>===================================================================
>--- WebKit/efl/ewk/ewk_view.cpp	(revision 61229)
>+++ WebKit/efl/ewk/ewk_view.cpp	(working copy)
>@@ -3692,3 +3729,135 @@ void ewk_view_download_request(Evas_Obje
>     DBG("view=%p", o);
>     evas_object_smart_callback_call(o, "download,request", download);
> }
>+
>+/**
>+ * @internal
>+ * Reports the viewport was changed.
s/was/has/


> <...>
>+/**
>+ * Get if zoom is enabled.
>+ *
>+ * @param o view.
>+ * @param user_scalable where to return the current user scalable value.
>+ */
>+void ewk_view_user_scalable_get(Evas_Object* o, Eina_Bool* user_scalable)
Eina_Bool ewk_view_user_scalable_get(Evas_Object* o)

>+{
>+    EWK_VIEW_SD_GET(o, sd);
>+    EWK_VIEW_PRIV_GET(sd, priv);
>+
>+    if (user_scalable)
>+        *user_scalable = priv->settings.zoom_range.user_scalable;

There's only 1 parameter to get. It's meaningless to make it optional in this case. Just change the function signature and return priv->settings.zoom_range.user_scalable

>Index: WebKit/efl/ewk/ewk_view.h
>===================================================================
>--- WebKit/efl/ewk/ewk_view.h	(revision 61229)
>+++ WebKit/efl/ewk/ewk_view.h	(working copy)
>@@ -83,6 +83,7 @@ extern "C" {
>  *  - "download,request", Ewk_Download: reports a download is being requested
>  *    and as arguments gives its details.
>  *  - "icon,received", void: main frame received an icon.
>+ *  - "viewport,changed", void: Report that viewport was changed. 
s/was/has/

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.