[EFL] EFLWebKit doesn't support viewport meta tag

• Component: WebKit API
• Status: RESOLVED FIXED

[Gyuyoung Kim]

EFLWebKit dones't support viewport meta tag. I think EFLWebkit also should support viewport meta tag.

[Gyuyoung Kim]

Created attachment 58104 [details]
viewport-patch-for-eflwebkit

I implemented efl port of webkit sends the arguments of viewport tag to application.
So, application can set layout according to the data regarding viewport tag.

3 APIs are added in this patch.
  - ewk_view_set_viewport() : To send arguments of viewport to application.
  - ewk_view_set_zoom_range() : To set zoom range according to minimumScale / maximumScale.
  - ewk_view_set_user_scalable() : To set if user can zoom page or not.

I would like to get comments related to this patch.

[Leandro Pereira]

I'm not a reviewer, so consider this as an informal review.

> Index: WebKit/ChangeLog
> +        * efl/EWebLauncher/main.c:
> +        (on_viewport_changed):
> +        (browserCreate):
> +        * efl/WebCoreSupport/ChromeClientEfl.cpp:
> +        (WebCore::ChromeClientEfl::didReceiveViewportArguments):
> +        * efl/ewk/ewk_view.cpp:
> +        (_ewk_view_priv_new):
> +        (ewk_view_zoom_set):
> +        (ewk_view_zoom_weak_set):
> +        (ewk_view_zoom_animated_set):
> +        (ewk_view_load_started):
> +        (ewk_view_set_viewport):
> +        (ewk_view_set_zoom_range):
> +        (ewk_view_set_user_scalable):
> +        * efl/ewk/ewk_view.h:

Try to write small, one-line summaries, explaining the changes.

> Index: WebKit/efl/ewk/ewk_view.cpp
> ===================================================================
> --- WebKit/efl/ewk/ewk_view.cpp	(revision 60742)
> +++ WebKit/efl/ewk/ewk_view.cpp	(working copy)
> @@ -100,6 +100,7 @@ struct _Ewk_View_Private_Data {
>          Eina_Bool resizable_textareas:1;
>          Eina_Bool private_browsing:1;
>          Eina_Bool caret_browsing:1;
> +        Eina_Bool isUserScalable:1;
>      } settings;
>      struct {
>          struct {
> @@ -121,6 +122,10 @@ struct _Ewk_View_Private_Data {
>          Evas_Coord w, h;
>          Eina_Bool use:1;
>      } fixed_layout;
> +    struct {
> +        float minimumScale;
> +        float maximumScale;
> +    } zoom_range;
>  };

Naming isn't consistent with the rest of EWK files. camelCase is used inside WebKit, but EWK prefers unix_hacker_style.


Patch looks fine otherwise.

[Gustavo Sverzut Barbieri]

Hi Gyuyoung Kim,

First part of the review is coding style of EFL and is important in order to let it consistent with other bits like Evas, Edje and Elementary:

 * the place of "set" and "get" keywords are at the end, not at the beginning as the rest of EFL (ie: evas_object_color_get())

 * The struct members should not be in camelCase, rather all in lower case and underline "_" separated. So it's not "isUserScalable" but "user_scalable".  Width and Height are always used as "w" and "h", minimum and maximum as "min" and "max" (in the case of minimumScale and maximumScale inside zoom_range, just "min" and "max" are more than enough)

 * add a call ewk_view_viewport_get(), instead of just rely on the signal being emitted.

 * last but not least, avoid a new structure like Ewk_Viewport. Rasterman says he rather use individual and optional variables for these members, so your code would be (and likewise for ewk_view_viewport_set()):

    void ewk_view_viewport_get(const Evas_Object *view, int *w, int *h, float *init_scale, float *max_scale, float *min_scale, Eina_Bool *user_scalable)
    {
        // regular validity checks on view...
        if (w) *w = ...sd->settings.viewport.w;
        if (h) *h = ...sd->settings.viewport.h;
        ...
    }

    This is like other pieces of EFL are. You can also do more simpler methods like ewk_view_viewport_size_get(view, w, h) and ewk_view_viewport_scale_get(view, init, min, max).
    You should just use the Ewk_Viewport if you want to handle this as callback data, but it is not necessary. You can just provide ewk_view_viewport_get() and then emit the callback with NULL data, the user would just call the method to retrieve data if he is interested.

The code looks fine, just be sure to fix these coding style otherwise EFL developer will not get crazy or confused. :-)

[Gyuyoung Kim]

Created attachment 58325 [details]
viewport-patch-for-eflwebkit-2

Thank you for your advice. I modified this patch according to your guidance.
If this patch still has any nits, please let me know.

[WebKit Review Bot]

Attachment 58325 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:458:  user_scalable is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/efl/EWebLauncher/main.c:353:  init_scale is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/efl/EWebLauncher/main.c:353:  min_scale is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/efl/EWebLauncher/main.c:353:  max_scale is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/efl/EWebLauncher/main.c:354:  user_scalable is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 6 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Created attachment 58326 [details]
viewport-patch-for-eflwebkit-3

I fix the errors regarding coading style. Is this right?

[WebKit Review Bot]

Attachment 58326 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Created attachment 58327 [details]
viewport-patch-for-eflwebkit-4

Oops, style nit is still exist. I want that there are not nits anymore.

[Gyuyoung Kim]

Created attachment 58330 [details]
viewport-patch-for-eflwebkit-5

Sorry, there are some problems in previous patch. I fix them.

[Gustavo Sverzut Barbieri]

Hi Gyuyoung Kim, patch is highly improved! Thanks. Very minor nitpickings to make it perfect:

 * The comment of ewk_view_viewport_set() is now incorrect:
       Emits signal: "viewport,changed" with arguments of viewport
   You now have no arguments (NULL/0 is used).

 * As ewk_view_viewport_set() is internal/private just to WebCoreSupport usage, it should be in ewk_private.h, not ewk_view.h

 * Missing signal "viewport,changed" documentation in ewk_view.h

 * ewk_view_viewport_get() is not @internal, rather for public usage. Maybe copy & paste from the _set()? (The setter should be considered internal, as it is just to be provided from WebCoreSupport)

 * As other EFL, ewk_view_viewport_get() should get optional arguments. So you should check if any of the given return pointers are not NULL:
       - *w = priv->settings.viewport.w;
       + if (w) *w = priv->settings.viewport.w;


Now some important questions, as ViewportArguments.h is not in my knowledge domain but may result in bugs if not handled properly:

  1. Should the meta tag ALWAYS be enforced in the user? Should it always prohibit users from changing zoom level or limiting the available zoom range? I don't think so. It should be only enforced by Browsers and thus ewk_view users.   If this is the case, then you must have a different structure to hold WebCore/dom reported values, and another one that is user-settable with the values to enforce with ewk_view_zoom_range_set() and ewk_view_user_scalable_set(). With this approach you will also need ewk_view_zoom_range_get() and ewk_view_user_scalable_get() as ewk_view_viewport_get() will always report the original WebCore/dom values.   In any case, you must document this behavior and side-effects in the public API in ewk_view.cpp

  2. How to reset these values on new page load? Are you sure WebCore/dom will always report new value for each visited page, even if the page does not contain the mentioned meta tag? If does and allows you to reset the values, good, otherwise you'd have to reset these values yourself. Failing to do so will result in bugs when user visits one page that applies the tag and then goes to another that does not and is not prepared to handle those constraints.

As I'm not an official reviewer, I'll try to find someone that knows about it to comment on these questions.

[Jesus Sanchez-Palencia]

(In reply to comment #10)
I've been working on this recently for the Qt WebKit port. This is the bug which has a patch pending review: https://bugs.webkit.org/show_bug.cgi?id=39902

>   1. Should the meta tag ALWAYS be enforced in the user? Should it always prohibit users from changing zoom level or limiting the available zoom range? I don't think so. It should be only enforced by Browsers and thus ewk_view users.   If this is the case, then you must have a different structure to hold WebCore/dom reported values, and another one that is user-settable with the values to enforce with ewk_view_zoom_range_set() and ewk_view_user_scalable_set(). With this approach you will also need ewk_view_zoom_range_get() and ewk_view_user_scalable_get() as ewk_view_viewport_get() will always report the original WebCore/dom values.   In any case, you must document this behavior and side-effects in the public API in ewk_view.cpp

IMHO the values shouldn't be enforced always. This meta tag was created for the iPhone, so targeting mobile browsers is the best idea. So we have two code paths here: one when the webpage does have the viewport meta tag and one when it does not.
In order to cover these two cases I added the signal QWebPage::viewportChangeRequested(ViewportHints). For the first case, when we have the meta tag, I implemented the ChromeClient::didReceiveViewportArguments  in our ChromeClientQt, which creates our ViewportHints and emits the signal. For the second case we now emits the signal with an empty ViewportHints on FrameLoaderClientQt::dispatchDidCommitLoad. 
In both cases this signal will be emitted before any visual layout of the page so you can adapt your viewport if needed and avoid extra re-layouts after. By doing this we also ignore any viewport meta tag that is not on the <head>, which is correctly by the meta tag specification.
It's expected that the browser developer will handle this signal properly by following its documentation and nothing changes for your application if you don't use this signal at all.

 
>   2. How to reset these values on new page load? Are you sure WebCore/dom will always report new value for each visited page, even if the page does not contain the mentioned meta tag? If does and allows you to reset the values, good, otherwise you'd have to reset these values yourself. Failing to do so will result in bugs when user visits one page that applies the tag and then goes to another that does not and is not prepared to handle those constraints.
> 

IIRC, yes, WebCore/dom is our friend! :)
I think that the explanation above covers the rest of this question, right? Having those two code paths cover this quite well.

I'll have a quick look at the patch now and see if I can help. I'll try but since I'm not an EFL-deep-underground-hacker I'm not sure I can go any further.

[Jesus Sanchez-Palencia]

(In reply to comment #11)
> I'll have a quick look at the patch now and see if I can help. I'll try but since I'm not an EFL-deep-underground-hacker I'm not sure I can go any further.

Well, I think that the major problem of the patch is what concerns Gustavo: always applying the viewport meta tag arguments. You could try to add a callback and handle both cases by using it on the application layer. Hope that this helps you out!

[Gyuyoung Kim]

Hi, Gustavo Sverzut Barbieri and Jesus Sanchez-Palencia, Thank you for your advice.

I also have considered the problems Gustavo mentioned. Temporarily, I set default value regarding the width, height, zoom level and so on when page loading is started. But, I think this is bad method. Thus, I didn't add the codes to this patch so far. 

As mentioned in Jesus Sanchez-Palencia's comment, it seems to me that we can emit signals both with viewport and without viewport. If website doens't contain viewport tag, we can send empty viewport data to application. Then, application can decide if data of viewport is used or not. In this case, application should know default value regarding width, height, userScalable and so on. Or, we can send default values from WebCore/dom when there are no viewport tag.

[Gyuyoung Kim]

(In reply to comment #12)
> (In reply to comment #11)
> > I'll have a quick look at the patch now and see if I can help. I'll try but since I'm not an EFL-deep-underground-hacker I'm not sure I can go any further.
> 
> Well, I think that the major problem of the patch is what concerns Gustavo: always applying the viewport meta tag arguments. You could try to add a callback and handle both cases by using it on the application layer. Hope that this helps you out!

Ok, I will report new patch accroding to Gustavo and you's guidance.

[Gyuyoung Kim]

(In reply to comment #12)
> (In reply to comment #11)
> > I'll have a quick look at the patch now and see if I can help. I'll try but since I'm not an EFL-deep-underground-hacker I'm not sure I can go any further.
> 
> Well, I think that the major problem of the patch is what concerns Gustavo: always applying the viewport meta tag arguments.

One more thing, I think that viewport meta tag arguments is not always adjusted.
The viewport only can be adjusted when webpage contains viewport. As far as I know, didReceiveViewportArguments() function only can be invoked when webpage contains the viewport tag. 

In my opinion, major problem is that application goes website which has viewport, and then it goes other websites which doesn't contain viewport. In latest patch, there are no solutions for this case. 

I think we are already enough to know this problem. I will try to consider this problem.

[Gyuyoung Kim]

Created attachment 58643 [details]
 viewport-patch-for-eflwebkit-6

I tried to make a patch accroding to Gustavo and Jesus Sanchez-Palencia's advice. 

First, I distinguish zoom_range variables from viewport struct in order to set zoom_range by application or users.

+        struct {
+            float min_scale;
+            float max_scale;
+        } viewport;
+        struct {
+            float min_scale;
+            float max_scale;
+        } zoom_range;

So, zoom range can be changed by ewk_view_zoom_range_set() function.

Second, I modify this patch to send signal(viewport,changed) both there is viewport and no viewport. So, if website contains "viewport" meta tag, viewport signal is sent by didReceiveViewportArguments() in ChromeClientEfl.cpp. However, if there no viewport, the signal(viewport,changed) is sent by dispatchDidFirstVisuallyNonEmptyLayout() in FrameLoaderClientEfl.cpp. As far as i know, the dispatchDidFirstVisuallyNonEmptyLayout() will be invoked when layout is just started(). Thus, I think viewport signal need to be sent by the function when there are no viewport meta tag.

If the signal already was sent by viewport meta tag, we should not send signal duplicately. So, I add a condition to check if layout is done or not as below,

void FrameLoaderClientEfl::dispatchDidFirstVisuallyNonEmptyLayout()
{
-    notImplemented();
+    if (!ewk_view_init_layout_completed_get(m_view)) {
+        ViewportArguments arguments;
+        ewk_view_viewport_set(m_view, (int)arguments.width, (int)arguments.height, arguments.initialScale, arguments.minimumScale, arguments.maximumScale, arguments.userScalable);
+    }
}

If website doesn't contain viewport meta tag, all data except for userScalable is set as "-1". So, application need to process them when viewport's datas are "-1". I mentioned this in EWebLauncher/main.c,

+on_viewport_changed(void* user_data, Evas_Object* webview, void* event_info)
+{
+    w = (w == -1) ? DEFAULT_WIDTH : w;
+    h = (h == -1) ? DEFAULT_HEIGHT : h;
+    initScale = ((int)initScale == -1) ? DEFAULT_SCALE : initScale;
+    minScale = ((int)minScale == -1) ? DEFAULT_SCALE : minScale; 
+    maxScale = ((int)maxScale == -1) ? DEFAULT_SCALE : maxScale;
...


Gustavo,
I would like to know how do you think about new patch.

[WebKit Review Bot]

Attachment 58643 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/efl/ewk/ewk_view.cpp:3810:  More than one command on the same line in if  [whitespace/parens] [4]
WebKit/efl/ewk/ewk_view.cpp:3811:  More than one command on the same line in if  [whitespace/parens] [4]
WebKit/efl/ewk/ewk_view.cpp:3812:  More than one command on the same line in if  [whitespace/parens] [4]
WebKit/efl/ewk/ewk_view.cpp:3813:  More than one command on the same line in if  [whitespace/parens] [4]
WebKit/efl/ewk/ewk_view.cpp:3814:  More than one command on the same line in if  [whitespace/parens] [4]
WebKit/efl/ewk/ewk_view.cpp:3815:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 6 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Created attachment 58645 [details]
viewport-patch-for-eflwebkit-6

Opps, there was style errors. I fix them.

[Lucas De Marchi]

(In reply to comment #8)
> Created an attachment (id=58327) [details]
> viewport-patch-for-eflwebkit-4
> 
> Oops, style nit is still exist. I want that there are not nits anymore.

Just a suggestion on this: you can always run 'WebKitTools/Scripts/check-webkit-style' manually in order to avoid sending twice the same patch with style fixes.

[Lucas De Marchi]

Comment on attachment 58645 [details]
viewport-patch-for-eflwebkit-6

>Index: WebKit/ChangeLog
>===================================================================
>--- WebKit/ChangeLog	(revision 61120)
>+++ WebKit/ChangeLog	(working copy)
>@@ -1,3 +1,35 @@
>[phone]  Gyuyoung Kim  <[e-mail]>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        [EFL] EFLWebKit doesn't support viewport meta tag.
Please describe the patch here, not the but title.

>+        https://bugs.webkit.org/show_bug.cgi?id=40278
>+
>+        Support viewport meta tag on EFL Port.
>+
>+        * efl/EWebLauncher/main.c:
>+        (on_viewport_changed):
>+        (browserCreate):
>+        * efl/WebCoreSupport/ChromeClientEfl.cpp:
>+        (WebCore::ChromeClientEfl::didReceiveViewportArguments):
>+        * efl/WebCoreSupport/ChromeClientEfl.h:
>+        * efl/WebCoreSupport/FrameLoaderClientEfl.cpp:
>+        (WebCore::FrameLoaderClientEfl::dispatchDidCommitLoad):
>+        (WebCore::FrameLoaderClientEfl::dispatchDidFirstVisuallyNonEmptyLayout):
>+        * efl/ewk/ewk_private.h:
>+        * efl/ewk/ewk_view.cpp:
>+        (_ewk_view_priv_new):
>+        (ewk_view_zoom_set):
>+        (ewk_view_zoom_weak_set):
>+        (ewk_view_zoom_animated_set):
>+        (ewk_view_init_layout_completed_set):
>+        (ewk_view_init_layout_completed_get):
>+        (ewk_view_viewport_set):
>+        (ewk_view_viewport_get):
>+        (ewk_view_zoom_range_set):
>+        (ewk_view_user_scalable_set):
>+        * efl/ewk/ewk_view.h:
>+
Comments about the changed functions are still missing.

> 2010-06-14  Ilya Tikhonovsky  <[e-mail]>
> 
>         Reviewed by Pavel Feldman.
>Index: WebKit/efl/EWebLauncher/main.c
>===================================================================
>--- WebKit/efl/EWebLauncher/main.c	(revision 61116)
>+++ WebKit/efl/EWebLauncher/main.c	(working copy)
>@@ -47,6 +47,7 @@
> 
> #define DEFAULT_WIDTH      800
> #define DEFAULT_HEIGHT     600
>+#define DEFAULT_SCALE      1.0
> 
> #define info(format, args...)       \
>     do {                            \
>@@ -346,6 +347,27 @@ on_tooltip_text_set(void* user_data, Eva
> }
> 
> static void
>+on_viewport_changed(void* user_data, Evas_Object* webview, void* event_info)
>+{
>+    int w, h;
>+    float initScale, minScale, maxScale;
>+    Eina_Bool userScalable;
>+
>+    ewk_view_viewport_get(webview, &w, &h, &initScale, &maxScale, &minScale, &userScalable);
>+
>+    w = (w == -1) ? DEFAULT_WIDTH : w;
>+    h = (h == -1) ? DEFAULT_HEIGHT : h;
>+    initScale = ((int)initScale == -1) ? DEFAULT_SCALE : initScale;
>+    minScale = ((int)minScale == -1) ? DEFAULT_SCALE : minScale; 
>+    maxScale = ((int)maxScale == -1) ? DEFAULT_SCALE : maxScale;
imo these two are wrong. The default should be ZOOM_MIN/ZOOM_MAX respectively. Otherwise, if the page does not have a viewport meta tag it will not possible to set the zoom because of checks made in ewk_view_zoom_set() and functions alike.

>+
>+    ewk_view_fixed_layout_size_set(webview, w, h);
>+    ewk_view_zoom_set(webview, initScale, 0, 0);
>+    ewk_view_zoom_range_set(webview, minScale, maxScale);
>+    ewk_view_user_scalable_set(webview, userScalable);
>+}

What if I don't want to be forced to apply the new viewport arguments? There's no way as of now to ignore the meta tag.


>+void ChromeClientEfl::didReceiveViewportArguments(Frame* frame, const ViewportArguments& arguments) const
>+{
>+    Eina_Bool userScalable;
>+
>+    userScalable = (int)arguments.userScalable == 1 ? EINA_TRUE : EINA_FALSE;
You should check for arguments.userScalable == -1, not just == 1.

>+    ewk_view_viewport_set(m_view, (int)arguments.width, (int)arguments.height, arguments.initialScale, arguments.minimumScale, arguments.maximumScale, userScalable);
>+    ewk_view_init_layout_completed_set(m_view, EINA_TRUE);

Since the only place where you need this is inside ChromeClientEfl/FrameLoaderClientEfl, it's not necessary (neither desired) to have this function as an ewk_view_*. Just add a boolean field to FrameLoaderClientEfl to check/set if the signal has already been sent.


>Index: WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp
>===================================================================
>@@ -580,6 +581,8 @@ void FrameLoaderClientEfl::dispatchDidCo
>         return;
>     ewk_view_title_set(m_view, 0);
>     ewk_view_uri_changed(m_view);
>+
>+    ewk_view_init_layout_completed_set(m_view, EINA_FALSE);
Same here, use a field in this class.

>Index: WebKit/efl/ewk/ewk_private.h
>===================================================================
>--- WebKit/efl/ewk/ewk_private.h	(revision 61116)
>+++ WebKit/efl/ewk/ewk_private.h	(working copy)
>@@ -93,6 +93,10 @@ 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, Eina_Bool user_scalable);
>+void             ewk_view_init_layout_completed_set(Evas_Object *o, Eina_Bool init_layout);
>+Eina_Bool        ewk_view_init_layout_completed_get(Evas_Object *o);
As mentioned above, I think it's better if we don't have these two .

>Index: WebKit/efl/ewk/ewk_view.cpp
>===================================================================
>--- WebKit/efl/ewk/ewk_view.cpp	(revision 61116)
>+++ WebKit/efl/ewk/ewk_view.cpp	(working copy)
>@@ -100,6 +100,20 @@ struct _Ewk_View_Private_Data {
>         Eina_Bool resizable_textareas:1;
>         Eina_Bool private_browsing:1;
>         Eina_Bool caret_browsing:1;
>+        Eina_Bool init_layout_completed:1;
>+        Eina_Bool user_scalable:1;
>+        struct {
>+            int w;
>+            int h;
>+            float init_scale;
>+            float min_scale;
>+            float max_scale;
>+            Eina_Bool user_scalable:1;
You have 2 user_scalable... one inside and one outside viewport.

>+        } viewport;
>+        struct {
>+            float min_scale;
>+            float max_scale;
>+        } zoom_range;
>     } settings;
>     struct {
>         struct {
>@@ -574,6 +588,9 @@ static Ewk_View_Private_Data* _ewk_view_
>     priv->settings.private_browsing = priv->page_settings->privateBrowsingEnabled();
>     priv->settings.caret_browsing = priv->page_settings->caretBrowsingEnabled();
> 
>+    priv->settings.zoom_range.min_scale = ZOOM_MIN;
>+    priv->settings.zoom_range.max_scale = ZOOM_MAX;
>+
Why did you set these parameters and didn't set the others? This would be set anyway when the first page is loaded, even when it does not have a viewport tag, doesn't it?

>     priv->main_frame = _ewk_view_core_frame_new(sd, priv, 0).get();
>     if (!priv->main_frame) {
>         CRITICAL("Could not create main frame.");
>@@ -1754,14 +1771,22 @@ float ewk_view_zoom_get(const Evas_Objec
> Eina_Bool ewk_view_zoom_set(Evas_Object* o, float zoom, Evas_Coord cx, Evas_Coord cy)
> {
>     EWK_VIEW_SD_GET_OR_RETURN(o, sd, EINA_FALSE);
>+    EWK_VIEW_PRIV_GET(sd, priv);
>+
>     EINA_SAFETY_ON_NULL_RETURN_VAL(sd->api, EINA_FALSE);
>     EINA_SAFETY_ON_NULL_RETURN_VAL(sd->api->zoom_set, EINA_FALSE);
>-    if (zoom < ZOOM_MIN) {
>-        WRN("zoom level is < %f : %f", (double)ZOOM_MIN, (double)zoom);
>+
>+    if (!priv->settings.user_scalable) {
Again, there must be a way to ignore the tag.

>+        WRN("userScalable is false");
>         return EINA_FALSE;
>     }
>-    if (zoom > ZOOM_MAX) {
>-        WRN("zoom level is > %f : %f", (double)ZOOM_MAX, (double)zoom);
>+
>+    if (zoom < priv->settings.zoom_range.min_scale) {
priv->settings.zoom_range.min_scale can be == -1 if page didn't add the tag. Therefore, You must check if priv->settings.zoom_range.min_scale != -1 before comparing to zoom. something like this:

    if (priv->settings.zoom_range.min_scale >= 0.0 && zoom < priv->settings.zoom_range.min_scale) {


>+        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
>+        return EINA_FALSE;
>+    }
>+    if (zoom > priv->settings.zoom_range.max_scale) {
Here you have to put the same check as above.

>+        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
>         return EINA_FALSE;
>     }
> 
>@@ -1824,14 +1849,22 @@ void ewk_view_zoom_weak_smooth_scale_set
> Eina_Bool ewk_view_zoom_weak_set(Evas_Object* o, float zoom, Evas_Coord cx, Evas_Coord cy)
> {
>     EWK_VIEW_SD_GET_OR_RETURN(o, sd, EINA_FALSE);
>+    EWK_VIEW_PRIV_GET(sd, priv);
>+
>     EINA_SAFETY_ON_NULL_RETURN_VAL(sd->api, EINA_FALSE);
>     EINA_SAFETY_ON_NULL_RETURN_VAL(sd->api->zoom_weak_set, EINA_FALSE);
>-    if (zoom < ZOOM_MIN) {
>-        WRN("zoom level is < %f : %f", (double)ZOOM_MIN, (double)zoom);
>+
>+    if (!priv->settings.user_scalable) {
>+        WRN("userScalable is false");
>+        return EINA_FALSE;
>+    }
>+
>+    if (zoom < priv->settings.zoom_range.min_scale) {
check here as above.

>+        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
>         return EINA_FALSE;
>     }
>-    if (zoom > ZOOM_MAX) {
>-        WRN("zoom level is > %f : %f", (double)ZOOM_MAX, (double)zoom);
>+    if (zoom > priv->settings.zoom_range.max_scale) {
check here as above.

>@@ -1974,12 +2007,17 @@ Eina_Bool ewk_view_zoom_animated_set(Eva
>     EINA_SAFETY_ON_NULL_RETURN_VAL(sd->api, EINA_FALSE);
>     EINA_SAFETY_ON_NULL_RETURN_VAL(sd->api->zoom_weak_set, EINA_FALSE);
> 
>-    if (zoom < ZOOM_MIN) {
>-        WRN("zoom level is < %f : %f", (double)ZOOM_MIN, (double)zoom);
>+    if (!priv->settings.user_scalable) {
>+        WRN("userScalable is false");
>         return EINA_FALSE;
>     }
>-    if (zoom > ZOOM_MAX) {
>-        WRN("zoom level is > %f : %f", (double)ZOOM_MAX, (double)zoom);
>+
>+    if (zoom < priv->settings.zoom_range.min_scale) {
check here as above.

>+        WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom);
>+        return EINA_FALSE;
>+    }
>+    if (zoom > priv->settings.zoom_range.max_scale) {
check here as above.

<...>

>Index: WebKit/efl/ewk/ewk_view.h
>===================================================================
>--- WebKit/efl/ewk/ewk_view.h	(revision 61116)
>+++ 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. 
>  */
> 
> typedef struct _Ewk_View_Smart_Data Ewk_View_Smart_Data;
>@@ -451,6 +452,10 @@ EAPI void ewk_view_paint_context_transla
> EAPI Eina_Bool ewk_view_paint(Ewk_View_Private_Data *priv, cairo_t *cr, const Eina_Rectangle *area);
> EAPI Eina_Bool ewk_view_paint_contents(Ewk_View_Private_Data *priv, cairo_t *cr, const Eina_Rectangle *area);
> 
>+EAPI void ewk_view_viewport_get(Evas_Object *o, int* w, int* h, float* init_scale, float* max_scale, float* min_scale, Eina_Bool* user_scalable);
>+EAPI void ewk_view_zoom_range_set(Evas_Object* o, float min_scale, float max_scale);
>+EAPI void ewk_view_user_scalable_set(Evas_Object* o, Eina_Bool user_scalable);
Missing the getters for these functions.

[Lucas De Marchi]

(In reply to comment #16)
> Second, I modify this patch to send signal(viewport,changed) both there is viewport and no viewport. So, if website contains "viewport" meta tag, viewport signal is sent by didReceiveViewportArguments() in ChromeClientEfl.cpp. However, if there no viewport, the signal(viewport,changed) is sent by dispatchDidFirstVisuallyNonEmptyLayout() in FrameLoaderClientEfl.cpp. As far as 

Please, consider the comment #11 about the place to put this signal in order to ignore the meta tag outside of <head></head>.

[Lucas De Marchi]

(In reply to comment #20)
> >+        Eina_Bool init_layout_completed:1;
> >+        Eina_Bool user_scalable:1;
Ahn... I got why you have 2 user_scalable fields. But please, move this one inside zoom_range to make it clearer.

> >+        struct {
> >+            int w;
> >+            int h;
> >+            float init_scale;
> >+            float min_scale;
> >+            float max_scale;
> >+            Eina_Bool user_scalable:1;
> You have 2 user_scalable... one inside and one outside viewport.
this on is ok.

> 
> >+        } viewport;
> >+        struct {
> >+            float min_scale;
> >+            float max_scale;
put the first one here.

> >+        } zoom_range;

[Gyuyoung Kim]

Hello Lukasz, 
Thank you for your review. It seems I need to explain this patch more detail.

First of all, this patch implemented that viewport tag should be processed by application(EWebLauncher) or user.

When the viewport is found in WebCore, viewport arguments are sent to ChromeClientEfl::didReceiveViewportArguments(). Then, the function sends a signal "viewport,changed" to application(EWebLauncher). When application receives the signal, application can decide if viewport tag is adjusted or not as below,

+on_viewport_changed(void* user_data, Evas_Object* webview, void* event_info)
+{
+    int w, h;
+    float initScale, minScale, maxScale;
+    Eina_Bool userScalable;
+
+    ewk_view_viewport_get(webview, &w, &h, &initScale, &maxScale, &minScale, &userScalable);
+
+    w = (w == -1) ? DEFAULT_WIDTH : w;
+    h = (h == -1) ? DEFAULT_HEIGHT : h;
+    initScale = ((int)initScale == -1) ? DEFAULT_SCALE : initScale;
+    minScale = ((int)minScale == -1) ? DEFAULT_SCALE : minScale; 
+    maxScale = ((int)maxScale == -1) ? DEFAULT_SCALE : maxScale;

I think, if application doens't want to adjust viewport, they just doesn't process nothing in the callback function(on_viewport_changed).

But, if user want to process viewport tag, they can process viewport via APIs below,

+    ewk_view_fixed_layout_size_set(webview, w, h);
+    ewk_view_zoom_set(webview, initScale, 0, 0);
+    ewk_view_zoom_range_set(webview, minScale, maxScale);
+    ewk_view_user_scalable_set(webview, userScalable);

As you know, the ewk_view_zoom_range_set(), ewk_view_zoom_scalable_set() were made by this patch. 

Zoom range can be set via the ewk_view_zoom_range_set() as below,
+void ewk_view_zoom_range_set(...)
+{
+    priv->settings.zoom_range.min_scale = min_scale;
+    priv->settings.zoom_range.max_scale = max_scale;
+}
 
I needed to have min_scale, max_scale, init_scale and user_scalable respectively.

+        Eina_Bool user_scalable:1;
+        struct {
+            int w;
+            int h;
+            float init_scale;
+            float min_scale;
+            float max_scale;
+            Eina_Bool user_scalable:1;
+        } viewport;
+        struct {
+            float min_scale;
+            float max_scale;
+        } zoom_range;


Because there are two reasons. First. to store argument of viewport tag in order to send the data of viewport via ewk_viewport_get() to application(EWebLauncher). Second, do not adjust the data of viewport directly. If we don't have two vairaiables, viewport can be set via ewk_view_viewport_set() directly, Or, there are no way to adjust viewport by application(EWebLauncher). So, I made two variables respectly.

The min_scale and max_scale in zoom_range were just replaced with ZOOM_MIN / ZOOM_MAX in order to change ZOOM_MIN, ZOOM_MAX with application's request(I mean ewk_view_zoom_range_set())

+        struct {
+            float min_scale;
+            float max_scale;
+        } zoom_range;

Thus, I need to initialize the min_scale and max_scale with ZOOM_MIN, ZOOM_MAX

>+    priv->settings.zoom_range.min_scale = ZOOM_MIN;
>+    priv->settings.zoom_range.max_scale = ZOOM_MAX;

As mentioned in above, the zoom_range.min_scale/max_scale only can be changed by the ewk_view_zoom_set()(from application)


>+void ChromeClientEfl::didReceiveViewportArguments(Frame* frame, const ViewportArguments& arguments) const
>+{
>+    Eina_Bool userScalable;
>+

>+
>+    userScalable = (int)arguments.userScalable == 1 ? EINA_TRUE : EINA_FALSE;
>>You should check for arguments.userScalable == -1, not just == 1.

As you know, userScalable is boolean(yes or no) check if user can scale or not. So, I process it in the didReceiveViewportArguments directly. If userScalable is "1", user can scalable, If not so, user can't scalable. The user-scalable is defined as below,

"Determines whether or not the user can zoom in and out—whether or not the user can change the scale of the viewport. Set to yes to allow scaling and no to disallow scaling. The default is yes."
(http://developer.apple.com/safari/library/documentation/appleapplications/reference/safarihtmlref/articles/metatags.html#//apple_ref/html/const/viewport)

However, if you think it is better userScalalble also is processed by application, I will modify it.

>> Please, consider the comment #11 about the place to put this signal in order to ignore the meta tag outside of <head></head>.

According to the comment #11, I put this signal on "dispatchDidCommitLoad" and ChromeClientEfl::didReceiveViewportArguments. However, To my debugging, dispatchDidCommitLoad() is invoked more faster than didReceiveViewportArguments(). Thus, if webpage contains viewport tag, the viewport,changed signal are sent to application twice by dispatchDidCommitLoad() and ChromeClientEfl::didReceiveViewportArguments(). I don't want to send twice signal to application. It can make confusion for application. Because application processes viewport tag twice.


If didReceiveViewportArguments() is invoked, I don't want to send a signal for viewport to applicaiton. So, I needed to set flag (initialLayoutComplete), But, there was no way to notify the flog from ChoromeClientEfl to FrameLoaderClientEfl. So, I made internal APIs for the flag as below,

+void   ewk_view_init_layout_completed_set(Evas_Object *o, Eina_Bool init_layout);
+Eina_Bool ewk_view_init_layout_completed_get(Evas_Object *o);
  
Do you think should we send viewport signal twice ?

Thank you.

[Gyuyoung Kim]

> 
> typedef struct _Ewk_View_Smart_Data Ewk_View_Smart_Data;
>@@ -451,6 +452,10 @@ EAPI void ewk_view_paint_context_transla
> EAPI Eina_Bool ewk_view_paint(Ewk_View_Private_Data *priv, cairo_t *cr, const Eina_Rectangle *area);
> EAPI Eina_Bool ewk_view_paint_contents(Ewk_View_Private_Data *priv, cairo_t *cr, const Eina_Rectangle *area);
> 
>+EAPI void ewk_view_viewport_get(Evas_Object *o, int* w, int* h, float* init_scale, float* max_scale, float* min_scale, Eina_Bool* user_scalable);
>+EAPI void ewk_view_zoom_range_set(Evas_Object* o, float min_scale, float max_scale);
>+EAPI void ewk_view_user_scalable_set(Evas_Object* o, Eina_Bool user_scalable);

>> Missing the getters for these functions.

One more thing,
Appliaction can get data regarding zoom_range and user_scalable by ewk_view_viewport_get().

[Gustavo Sverzut Barbieri]

(In reply to comment #23)
> Zoom range can be set via the ewk_view_zoom_range_set() as below,
> +void ewk_view_zoom_range_set(...)
> +{
> +    priv->settings.zoom_range.min_scale = min_scale;
> +    priv->settings.zoom_range.max_scale = max_scale;
> +}
> 
> I needed to have min_scale, max_scale, init_scale and user_scalable respectively.

Missing sanity checks here, min_scale <= max_scale.


> >+void ChromeClientEfl::didReceiveViewportArguments(Frame* frame, const ViewportArguments& arguments) const
> >+{
> >+    Eina_Bool userScalable;
> >+
> 
> >+
> >+    userScalable = (int)arguments.userScalable == 1 ? EINA_TRUE : EINA_FALSE;
> >>You should check for arguments.userScalable == -1, not just == 1.
> 
> As you know, userScalable is boolean(yes or no) check if user can scale or not. So, I process it in the didReceiveViewportArguments directly. If userScalable is "1", user can scalable, If not so, user can't scalable. The user-scalable is defined as below,

WebCore/dom/ViewportArguments.h: 

    enum { ValueUndefined = -1 };
    ...
    userScalable(ValueUndefined)

so Lucas is right, must check for it being -1. The type of this variable is float (no idea why :-P).


> +void   ewk_view_init_layout_completed_set(Evas_Object *o, Eina_Bool init_layout);
> +Eina_Bool ewk_view_init_layout_completed_get(Evas_Object *o);
> 
> Do you think should we send viewport signal twice ?

No, but as he said this should be controlled in FrameLoaderClientEfl itself. No need to have these C-like functions in "ewk", just have them in WebCoreSupport/FrameLoaderClientEfl and use them from ChromeClientEfl.


(In reply to comment #24)
>>+EAPI void ewk_view_viewport_get(Evas_Object *o, int* w, int* h, float* init_scale, float* max_scale, float* min_scale, Eina_Bool* user_scalable);
>>+EAPI void ewk_view_zoom_range_set(Evas_Object* o, float min_scale, float max_scale);
>>+EAPI void ewk_view_user_scalable_set(Evas_Object* o, Eina_Bool user_scalable);
>>> Missing the getters for these functions.
>
>One more thing,
>Appliaction can get data regarding zoom_range and user_scalable by >ewk_view_viewport_get().

This is not correct. The function you said is about document/tag provided values, the one Lucas said is about user-set values (values one previously set before, or may be used to query the initial/default values). Will be of use particularly for debug. Make sure you follow the other functions and make the return pointers optional.

Patch is coming along nicely, most ready to be merged in my opinion (need some official reviewer to grant it).

[Gyuyoung Kim]

(In reply to comment #25)
> (In reply to comment #23)
> > Zoom range can be set via the ewk_view_zoom_range_set() as below,
> > +void ewk_view_zoom_range_set(...)
> > +{
> > +    priv->settings.zoom_range.min_scale = min_scale;
> > +    priv->settings.zoom_range.max_scale = max_scale;
> > +}
> > 
> > I needed to have min_scale, max_scale, init_scale and user_scalable respectively.
> 
> Missing sanity checks here, min_scale <= max_scale.
> 
> 
> > >+void ChromeClientEfl::didReceiveViewportArguments(Frame* frame, const ViewportArguments& arguments) const
> > >+{
> > >+    Eina_Bool userScalable;
> > >+
> > 
> > >+
> > >+    userScalable = (int)arguments.userScalable == 1 ? EINA_TRUE : EINA_FALSE;
> > >>You should check for arguments.userScalable == -1, not just == 1.
> > 
> > As you know, userScalable is boolean(yes or no) check if user can scale or not. So, I process it in the didReceiveViewportArguments directly. If userScalable is "1", user can scalable, If not so, user can't scalable. The user-scalable is defined as below,
> 
> WebCore/dom/ViewportArguments.h: 
> 
>     enum { ValueUndefined = -1 };
>     ...
>     userScalable(ValueUndefined)
> 
> so Lucas is right, must check for it being -1. The type of this variable is float (no idea why :-P).
> 
> 
> > +void   ewk_view_init_layout_completed_set(Evas_Object *o, Eina_Bool init_layout);
> > +Eina_Bool ewk_view_init_layout_completed_get(Evas_Object *o);
> > 
> > Do you think should we send viewport signal twice ?
> 
> No, but as he said this should be controlled in FrameLoaderClientEfl itself. No need to have these C-like functions in "ewk", just have them in WebCoreSupport/FrameLoaderClientEfl and use them from ChromeClientEfl.
> 
> 
> (In reply to comment #24)
> >>+EAPI void ewk_view_viewport_get(Evas_Object *o, int* w, int* h, float* init_scale, float* max_scale, float* min_scale, Eina_Bool* user_scalable);
> >>+EAPI void ewk_view_zoom_range_set(Evas_Object* o, float min_scale, float max_scale);
> >>+EAPI void ewk_view_user_scalable_set(Evas_Object* o, Eina_Bool user_scalable);
> >>> Missing the getters for these functions.
> >
> >One more thing,
> >Appliaction can get data regarding zoom_range and user_scalable by >ewk_view_viewport_get().
> 
> This is not correct. The function you said is about document/tag provided values, the one Lucas said is about user-set values (values one previously set before, or may be used to query the initial/default values). Will be of use particularly for debug. Make sure you follow the other functions and make the return pointers optional.
> 
> Patch is coming along nicely, most ready to be merged in my opinion (need some official reviewer to grant it).

Thank you your help, I will change this patch according to your guys soon.

[Gyuyoung Kim]

Created attachment 58867 [details]
viewport-patch-for-eflwebkit-7

I tried to modify this patch according to profusion's guys. I remove the ewk functions regarding init_layout.
It is just processed by FrameLoaderClientEfl. I add two APIs to get viewport data as below,

    void ewk_view_zoom_range_get(Evas_Object* o, float* min_scale, float* max_scale);
    void ewk_view_user_scalable_get(Evas_Object* o, Eina_Bool* user_scalable);

Lastly, I modify WebKit's ChangeLog and try to add comment to EWebLauncher/main.c file.

[Lucas De Marchi]

Comment on attachment 58867 [details]
viewport-patch-for-eflwebkit-7

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/

[Gyuyoung Kim]

Created attachment 58966 [details]
viewport-patch-for-eflwebkit-8

Hi Lukas, 

Thank you for your detail review. I modified this patch according to your points again.

>>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).

I tried to understand the viewport patch for QT port. As far as I understand, the QT's patch sends signal regarding viewport in both cases there is viewport and no viewport before any visual layout of the page. And, QT's patch prohibits to set layout before finishing first layout. Because, to prevent re-layout.

void QWebPage::setPreferredContentsSize(const QSize& size) const 
{
    if (frame->d->initialLayoutComplete) // the initialLayoutComplete can be set on FrameLoaderClientQt::dispatchDidFirstLayout()
        view->layout();

It means that application only can received data of viewport tag on callback function for viewport. Application can't set layout with viewport data as soon as viewport signal is received. Because, viewport tag should be located at <head>...</head>. The first layout is not finished on <head>...</head>. Thus, user needs to find good position in order to adjust viewport tag. It seems this can prevent re-layout because application only can decide if viewport is adjust or not. In other words, application is responsible for layout.

I modified this patch similar to QT port's patch and add an example code to adjust viewport tag as below,

/**
 * This is en example function to adjust viewport via viewport tag's arguments.
 * Application can invoke this function in order to adjust viewport tag when it is required.
 */
static void
viewport_set()
{
    ELauncher *app;
    app = (ELauncher*) eina_list_data_get(windows);

    ewk_view_fixed_layout_size_set(app->browser, app->viewport.w, app->viewport.h);
    ewk_view_zoom_set(app->browser, app->viewport.initScale, 0, 0);
    if (!ewk_view_zoom_range_set(app->browser, app->viewport.minScale, app->viewport.maxScale))
       info(" Fail to set zoom range. minScale = %f, maxScale = %f\n", app->viewport.minScale, app->viewport.maxScale);
    ewk_view_user_scalable_set(app->browser, app->viewport.userScalable);
}

It seems to me that callback function regarding dispatchDidFirstVisuallyNonEmptyLayout() is best. Of course, user can decide to put the viewport_set().


I also prohibit to set layout before finishing first layout.

void ewk_view_fixed_layout_size_set(Evas_Object* o, Evas_Coord w, Evas_Coord h)
{
    EWK_VIEW_SD_GET_OR_RETURN(o, sd);
    EWK_VIEW_PRIV_GET_OR_RETURN(sd, priv);

    WebCore::FrameLoaderClientEfl* client = static_cast<WebCore::FrameLoaderClientEfl*>(priv->main_frame->loader()->client());
    if (!client->getInitLayoutCompleted())
        return;
...


Thanks,
Gyuyoung Kim

[Gyuyoung Kim]

Created attachment 58967 [details]
viewport-patch-for-eflwebkit-9

There was a problem regarding float variable and initialize.

[Lucas De Marchi]

(In reply to comment #29)
> Created an attachment (id=58966) [details]
> viewport-patch-for-eflwebkit-8
> 
> Hi Lukas, 
> 
> Thank you for your detail review. I modified this patch according to your points again.

Looks awesome! Thanks for your patience updating this patch to meet the requirements.

Now I'll try to find a reviewer that can give you r+/cq+.

[Lucas De Marchi]

Dimitri Glazkov / Eric Seidel,


We made the informal reviews from our port side.

Can you check this patch and r+/cq+ in case there are no other problems?

Thanks.

[Gyuyoung Kim]

Created attachment 59221 [details]
viewport-patch-for-eflwebkit-10

Lukas,Thank you for your review. 
I upload new patch modified in some comments. There are no code changes.

Dimitri Glazkov / Eric Seidel,
Please review this patch. I am waiting for your review as well.

Thanks,
Gyuyoung Kim

[Kenneth Rohde Christiansen]

Comment on attachment 59221 [details]
viewport-patch-for-eflwebkit-10

WebKit/efl/EWebLauncher/main.c:52
 +  #define ZOOM_MAX           1.0
Is it really good with default that does not allow zooming in the example app?
WebKit/efl/EWebLauncher/main.c:379
 +   * "viewport,changed" signal will be always emitted regardless of the viewport existence. 
viewport existence? Existence of the viewport meta tag?

WebKit/efl/ewk/ewk_view.cpp:115
 +          } zoom_range;
zoom_range contains min_scale etc? Shouldnt it be called scale_range instead?

WebKit/efl/ewk/ewk_view.cpp:591
 +      priv->settings.zoom_range.max_scale = ZOOM_MAX;
Same here... confusing that you mix scale and zoom.

[Lucas De Marchi]

Comment on attachment 59221 [details]
viewport-patch-for-eflwebkit-10

Kenneth, thank you for spotting this bug. You are right.

Gyuyoung, I'm removing cq+ because this must be fixed before getting in.

[Lucas De Marchi]

>Index: WebKit/efl/EWebLauncher/main.c
>===================================================================
>--- WebKit/efl/EWebLauncher/main.c	(revision 61229)
>+++ WebKit/efl/EWebLauncher/main.c	(working copy)
>@@ -47,6 +47,9 @@
> 
> #define DEFAULT_WIDTH      800
> #define DEFAULT_HEIGHT     600
>+#define ZOOM_INIT          1.0
>+#define ZOOM_MIN           1.0
>+#define ZOOM_MAX           1.0

You should not define those here. Instead, they have to come from WebKit.

>+    if (!ewk_view_zoom_range_set(app->browser, app->viewport.minScale, app->viewport.maxScale))
>+       info(" Fail to set zoom range. minScale = %f, maxScale = %f\n", app->viewport.minScale, app->viewport.maxScale);

Problem with style. Use 4 spaces.

>@@ -345,6 +375,48 @@ on_tooltip_text_set(void* user_data, Eva
>         info("%s\n", text);
> }
> 
>+/**
>+ * "viewport,changed" signal will be always emitted regardless of the viewport existence. 
>+ *
>+ * 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.
>+ * 
>+ * More information about this can be found at http://developer.apple.com/safari/library/docum
>+ * entation/appleapplications/reference/safariwebcontent/usingtheviewport/usingtheviewport.html
>+ */
>+static void
>+on_viewport_changed(void* user_data, Evas_Object* webview, void* event_info)
>+{
>+    ELauncher *app = (ELauncher *)user_data;
>+   

There are several trailing white spaces. Although WebKit does not prohibit
them, we try to avoid them. Please, remove.

>+    float w, h, 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.
>+     */
>+    if ((int)w == -1)
>+        w = DEFAULT_WIDTH;
>+    if ((int)h == -1)
>+        h = DEFAULT_HEIGHT;
>+    if ((int)initScale == -1)
>+        initScale = ZOOM_INIT;
>+    if ((int)minScale == -1)
>+        minScale = ZOOM_MIN;
>+    if ((int)maxScale == -1)
>+        maxScale = ZOOM_MAX;
>+    if ((int)userScalable == -1)
>+         userScalable = EINA_TRUE;

Here you have 5 spaces. Use 4, instead. This should be caught by
check-webkit-style. Don't know why it didn't get.

Worse than style issues, as Kenneth noticed, this would not allow to set zoom
if there isn't any meta tag. Either define the constants the same of webkit or,
better, add the proper API to get/set max and min zoom. Although the second is
the desired approach, I understand it would overly complicate this patch you
are submitting. Therefore, it would suffice if you submit the first one.



Please, consider also the other comments from Kenneth.

[Lucas De Marchi]

Comment on attachment 59221 [details]
viewport-patch-for-eflwebkit-10

It was a misunderstanding between me and Eric. Please, fix these minor issues and it will be rq+ again.

[Gyuyoung Kim]

Created attachment 59719 [details]
 viewport-patch-for-eflwebkit-11

Kenneth and Lukas, thank you for your review. :)
I try to fix the style errors. I hope that there are no style erros anymore.

I add "DEFAULT_" to ZOOM_INIT, ZOOM_MIN and ZOOM_MAX as prefix. The macro meaning needs to be clear.

The DEFAULT_ZOOM_INIT, DEFAULT_ZOOM_MIN and DEFAULT_ZOOM_MAX are used when there are no arguments of viewport tag in webpage. Thus, application needs to set default zoom_init, zoom_max and zoom_min fot that situation.

So, I think that 1.0 value is appropriate. Because, currently, viewport tag is used by mobile webpage. Almost mobile webpages do not permit to zoom.

And, as you know, we can set / get zoom range via ewk_view_zoom_range_set(), ewk_view_zoom_range_get(). User(application) can decide if the zoom range is chagned via viewport information.

By the way, Should I request review again for new patch ?

[Lucas De Marchi]

Comment on attachment 59719 [details]
 viewport-patch-for-eflwebkit-11

>Index: WebKit/efl/EWebLauncher/main.c
>===================================================================
>--- WebKit/efl/EWebLauncher/main.c	(revision 61229)
>+++ WebKit/efl/EWebLauncher/main.c	(working copy)
>@@ -47,6 +47,9 @@
> 
> #define DEFAULT_WIDTH      800
> #define DEFAULT_HEIGHT     600
>+#define DEFAULT_ZOOM_INIT  1.0
>+#define DEFAULT_ZOOM_MIN   1.0
>+#define DEFAULT_ZOOM_MAX   1.0

Suppose the following scenario:
1. The function on_viewport_changed() is implemented by browser to cover the case in which there's a viewport meta tag. 
2. You go to a website that doesn't use the tag;
3. As WebKit always sends the notification about viewport (even if the meta tag doesn't exist), a notification will arrive with all fields == -1.0.
4. At that time the browser will call on_viewport_changed() and you will set 
+    if ((int)initScale == -1)
+        initScale = DEFAULT_ZOOM_INIT;
+    if ((int)minScale == -1)
+        minScale = DEFAULT_ZOOM_MIN;
+    if ((int)maxScale == -1)
+        maxScale = DEFAULT_ZOOM_MAX

5. What happens now if browser tries to zoom in and call ewk_view_zoom_set()?

+    if (zoom > priv->settings.zoom_range.max_scale) {
+        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
         return EINA_FALSE;
     }

But:
maxScale == DEFAULT_ZOOM_MAX == 1.0

Bang! Browser can't zoom in anymore. With the same reasoning, it can't zoom out neither. This is why you have to set ZOOM_MIN and ZOOM_MAX to the same values of webkit or add the proper APIs as I've suggested.

[Gyuyoung Kim]

(In reply to comment #39)
> (From update of attachment 59719 [details])
> >Index: WebKit/efl/EWebLauncher/main.c
> >===================================================================
> >--- WebKit/efl/EWebLauncher/main.c	(revision 61229)
> >+++ WebKit/efl/EWebLauncher/main.c	(working copy)
> >@@ -47,6 +47,9 @@
> > 
> > #define DEFAULT_WIDTH      800
> > #define DEFAULT_HEIGHT     600
> >+#define DEFAULT_ZOOM_INIT  1.0
> >+#define DEFAULT_ZOOM_MIN   1.0
> >+#define DEFAULT_ZOOM_MAX   1.0
> 
> Suppose the following scenario:
> 1. The function on_viewport_changed() is implemented by browser to cover the case in which there's a viewport meta tag. 
> 2. You go to a website that doesn't use the tag;
> 3. As WebKit always sends the notification about viewport (even if the meta tag doesn't exist), a notification will arrive with all fields == -1.0.
> 4. At that time the browser will call on_viewport_changed() and you will set 
> +    if ((int)initScale == -1)
> +        initScale = DEFAULT_ZOOM_INIT;
> +    if ((int)minScale == -1)
> +        minScale = DEFAULT_ZOOM_MIN;
> +    if ((int)maxScale == -1)
> +        maxScale = DEFAULT_ZOOM_MAX
> 
> 5. What happens now if browser tries to zoom in and call ewk_view_zoom_set()?
> 
> +    if (zoom > priv->settings.zoom_range.max_scale) {
> +        WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom);
>          return EINA_FALSE;
>      }
> 
> But:
> maxScale == DEFAULT_ZOOM_MAX == 1.0
> 
> Bang! Browser can't zoom in anymore. With the same reasoning, it can't zoom out neither. This is why you have to set ZOOM_MIN and ZOOM_MAX to the same values of webkit or add the proper APIs as I've suggested.

Browser can zoom in / out in that case. the on_viewport_changed() just receives arguments of viewport. Browser should call the functions below in order to adjust viewport tag.

 87 +/**
 88 + * This is en example function to adjust viewport via viewport tag's arguments.
 89 + * Application can invoke this function in order to adjust viewport tag when it is required.
 90 + */
 1 +static void
 92 +viewport_set()
 93 +{
 94 +    ELauncher *app;
 95 +    app = (ELauncher*) eina_list_data_get(windows);
 96 +
 97 +    ewk_view_fixed_layout_size_set(app->browser, app->viewport.w, app->viewport.h);
 98 +    ewk_view_zoom_set(app->browser, app->viewport.initScale, 0, 0);
 99 +    if (!ewk_view_zoom_range_set(app->browser, app->viewport.minScale, app->viewport.maxScale))
100 +        info(" Fail to set zoom range. minScale = %f, maxScale = %f\n", app->viewport.minScale, app->viewport.maxScale);
101 +    ewk_view_user_scalable_set(app->browser, app->viewport.userScalable);
102 +}
 
If browser doesn't invoke ewk_view_zoom_range_set(), ewk_view_user_scalable_set(), viewport is not adjusted. So, browser can zoom in / out. I made an example function (viewport_set()) to show how to adjust viewport meta tag. If browser doesn't want to adjust viewport, it doesn't invoke ewk_view_zoom_range_set(), ewk_view_user_scalable_set().


> +    if (zoom > priv->settings.zoom_range.max_scale) {
> +        WRN("zoom level is > %f : %f", 

By default, the zoom_range.max_scale / min_scale is set as ZOOM_MIN(0.5) / ZOOM_MAX(4.0) defined in ewk_view.cpp

313 +    priv->settings.zoom_range.min_scale = ZOOM_MIN;
314 +    priv->settings.zoom_range.max_scale = ZOOM_MAX;

Do you mean we should set the DEFAULT_ZOOM_MAX as 4.0 in EWebLauncher/main.c ?

[Gyuyoung Kim]

Lukas, do you mean we should set the zoom min / max value from webkit as below when website doesn't define zoom max / min ?

7 static void
388 on_viewport_changed(void* user_data, Evas_Object* webview, void* event_info)
389 {
...
406     if ((int)minScale == -1) 
407         minScale = ewk_view_zoom_min_get();
408     if ((int)maxScale == -1) 
409         maxScale = ewk_view_zoom_max_get();

[Lucas De Marchi]

(In reply to comment #41)
> Lukas, do you mean we should set the zoom min / max value from webkit as below when website doesn't define zoom max / min ?
> 
> 7 static void
> 388 on_viewport_changed(void* user_data, Evas_Object* webview, void* event_info)
> 389 {
> ...
> 406     if ((int)minScale == -1) 
> 407         minScale = ewk_view_zoom_min_get();
> 408     if ((int)maxScale == -1) 
> 409         maxScale = ewk_view_zoom_max_get();

Yes, you got it.

[Gyuyoung Kim]

Created attachment 59773 [details]
viewport-patch-for-eflwebkit-12

Lukas, you are right. The default value of zoom range should be set as value of webkit.

Thanks.

[Gyuyoung Kim]

Created attachment 60015 [details]
viewport-patch-for-eflwebkit-13

Hello Keneeth,

I told that you rejected this patch. Because, there are mixing zoom with scale.

   priv->settings.zoom_range.min_scale = ZOOM_MIN;
   priv->settings.zoom_range.max_scale = ZOOM_MAX;

However, there is no scale seprated from zooming in webkit-efl. As far as I know, webkit-efl can scale based on zoom.
Thus, this patch implements functionality of viewport meta tag using zoom.

In order to avoid this patch can make a confusion, I add comments for this issue as below,

   // Since there's no scale separated from zooming in webkit-efl, this functionality of
   // viewport meta tag is implemented using zoom.
   priv->settings.zoom_range.min_scale = ZOOM_MIN;
   priv->settings.zoom_range.max_scale = ZOOM_MAX;

If webkit-efl separate scale from zoom, I will also modify this patch based on the change.


Hello barbieri and lucas,

I would like to know your opinions as well.


Thank you,
Gyuyoung Kim

[Eric Seidel (no email)]

Comment on attachment 59221 [details]
viewport-patch-for-eflwebkit-10

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 59221 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Lucas De Marchi]

Comment on attachment 60015 [details]
viewport-patch-for-eflwebkit-13


> @@ -3449,7 +3493,7 @@ uint64_t ewk_view_exceeded_database_quot
>      if (!sd->api->exceeded_database_quota)
>          return 0;
>  
> -    ERR("##### %lu %lu", current_size, expected_size);
> +    ERR("##### %u %u", current_size, expected_size);
>      return sd->api->exceeded_database_quota(sd, frame, databaseName, current_size, expected_size);
>  }

It seems that your last patch came with more pieces than needed. Also, this was a problem with warnings and was already fixed.

[Gyuyoung Kim]

(In reply to comment #46)
> (From update of attachment 60015 [details])
> 
> > @@ -3449,7 +3493,7 @@ uint64_t ewk_view_exceeded_database_quot
> >      if (!sd->api->exceeded_database_quota)
> >          return 0;
> >  
> > -    ERR("##### %lu %lu", current_size, expected_size);
> > +    ERR("##### %u %u", current_size, expected_size);
> >      return sd->api->exceeded_database_quota(sd, frame, databaseName, current_size, expected_size);
> >  }
> 
> It seems that your last patch came with more pieces than needed. Also, this was a problem with warnings and was already fixed.

Ok, I will remove the code.

[Gyuyoung Kim]

Hello Gustavo and Lucas,

It seems kenneth wants to add scale zoomMode to this patch. For example, WebCore has two zoom modes as below,

enum ZoomMode {
    ZoomPage,
    ZoomTextOnly
};

He wants to add scale zoom for EFL port.

enum ZoomMode {
    ZoomPage,
    ZoomTextOnly,
    ZoomScale
};

So, this viewport patch works on Scale zoom. Is this easy ?

However, as you know, this work takes more time. I heard that you will add cairo scaling patch to WebKitEFL in future.

As mentioned in comment #44, this patch is landed first, then I would like to modify this patch after pushing cairo scaling patch.

Gustavo and Lucas, I wants to know your opinions.

[Gustavo Sverzut Barbieri]

I'm fine with merge this one first as we don't have the scale zoom type yet. When we integrate it we can sure add the suggested way.

[Gyuyoung Kim]

Created attachment 60684 [details]
viewport-patch-for-eflwebkit-14

Hello Kenneth,

As mentioned in Gustavo's comment above, EFL port doesn't have scale zoom type yet. I heard that Gustavo will contribute scale zoom patch based on cairo. So, in my opinion, I will modify this patch when the scale zoom patch is contributed to WebKitEFL.

In ewk_view.cpp, I add a comment for this issue.

313 +    // Since there's no scale separated from zooming in webkit-efl, this functionality of
314 +    // viewport meta tag is implemented using zoom. When scale zoom is supported by webkit-efl,
315 +    // this functionality will be modified by the scale zoom patch.
316 +    priv->settings.zoom_range.min_scale = ZOOM_MIN;
317 +    priv->settings.zoom_range.max_scale = ZOOM_MAX;


For now, in my opinion, this patch need to be landed first.

[WebKit Review Bot]

Attachment 60684 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/efl/ewk/ewk_private.h:98:  Extra space between void and ewk_view_viewport_set  [whitespace/declaration] [3]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Created attachment 60686 [details]
viewport-patch-for-eflwebkit-15

Sorry for my frequent patch uploading. It seems to me that ewk_private.h has style errors as below,

WebKit/efl/ewk/ewk_private.h:51:  Extra space between void and ewk_view_ready  [whitespace/declaration] [3]
WebKit/efl/ewk/ewk_private.h:52:  Extra space between void and ewk_view_title_set  [whitespace/declaration] [3]
WebKit/efl/ewk/ewk_private.h:53:  Extra space between void and ewk_view_uri_changed  [whitespace/declaration] [3]
WebKit/efl/ewk/ewk_private.h:54:  Extra space between void and ewk_view_load_started  [whitespace/declaration] [3]

I think this style errors in ewk_private.h should be fixed as well.

[Kenneth Rohde Christiansen]

Comment on attachment 60686 [details]
viewport-patch-for-eflwebkit-15

I'm fine with it, you can fix the rest in follow-up patches.

[WebKit Commit Bot]

Comment on attachment 60686 [details]
viewport-patch-for-eflwebkit-15

Clearing flags on attachment: 60686

Committed r62666: <http://trac.webkit.org/changeset/62666>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.