Bug 188746 - [GTK] Touchscreen pinch to zoom should scale the page like other platforms
Summary: [GTK] Touchscreen pinch to zoom should scale the page like other platforms
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-20 09:18 PDT by Justin Michaud
Modified: 2018-08-30 15:55 PDT (History)
11 users (show)

See Also:


Attachments
Patch (13.43 KB, patch)
2018-08-20 09:24 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews201 for win-future (12.88 MB, application/zip)
2018-08-20 11:57 PDT, Build Bot
no flags Details
Patch (3.70 KB, patch)
2018-08-21 08:18 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (3.70 KB, patch)
2018-08-21 14:39 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (9.87 MB, application/zip)
2018-08-21 17:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-sierra (2.29 MB, application/zip)
2018-08-21 19:36 PDT, Build Bot
no flags Details
Patch (9.27 KB, patch)
2018-08-22 09:31 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (8.78 KB, patch)
2018-08-23 08:01 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (8.79 KB, patch)
2018-08-23 08:34 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (6.01 KB, patch)
2018-08-24 06:47 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2018-08-20 09:18:38 PDT
Currently on gtk, pinching to zoom causes the page to be zoomed in the same way as pressing ctrl+`+`. It should scale and pan the page like on ios or macos.
Comment 1 Justin Michaud 2018-08-20 09:20:20 PDT
Questions for reviewer:
- The performance is not very good (it wasn't before either). Any ideas on how to improve it?
- I have no idea how/where to write a test for this
Comment 2 Michael Catanzaro 2018-08-20 09:23:09 PDT
(In reply to Justin Michaud from comment #1)
> - I have no idea how/where to write a test for this

It would be pretty hard since you'd need to develop some way to emulate pinching a touchscreen. We require tests for everything that can be easily tested, not for absolutely everything. I think manual testing will suffice for this.
Comment 3 Justin Michaud 2018-08-20 09:24:42 PDT
Created attachment 347503 [details]
Patch
Comment 4 Build Bot 2018-08-20 09:28:07 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 5 Jan-Michael Brummer 2018-08-20 09:56:29 PDT
For me it's a question of philosophy. My current development focus is feature parity with Firefox and not iOS apps.

I really like the current situation as it simplifies the usage within the hamburger menu for touch input: One simple common function which does it all. So at the moment i do not want a change here. My two cents...
Comment 6 Michael Catanzaro 2018-08-20 11:53:25 PDT
I'm struggling to understand (a) what is the current behavior, (b) how this patch changes the behavior, and (c) why you (Jan-Michael) think the change would be undesirable.

My understanding is this somehow reverts the recetn zooming/scaling change by Carlos Garcia? I've already lost track of that bug, and don't remember it at all.

How is mobile Firefox's behavior different from Safari's? In general, we do try to emulate Safari's behavior -- since we are WebKit after all -- except when there is good reason to do otherwise.
Comment 7 Build Bot 2018-08-20 11:56:48 PDT
Comment on attachment 347503 [details]
Patch

Attachment 347503 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8919107

New failing tests:
http/tests/security/local-video-source-from-remote.html
Comment 8 Build Bot 2018-08-20 11:57:00 PDT
Created attachment 347520 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 9 Michael Catanzaro 2018-08-20 12:04:47 PDT
Comment on attachment 347503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347503&action=review

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3121
> + * webkit_web_view_scale_page:
> + * @web_view: a #WebKitWebView
> + * @zoom_level: the zoom level
> + * @scaleCenterX: X coordinate of scale center in page coordinates
> + * @scaleCenterY: y coordinate of scale center in page coordinates
> + * @viewX: X coordinate where scale center should appear, in view coordinates
> + * @viewY: y coordinate where scale center should appear, in view coordinates

Looking at the parameters, I don't understand how this API could plausibly be used by a client application. Where would the coordinates come from? Could you show some example usage, e.g. by updating MiniBrowser to use this API somehow? I suspect it can only sanely be called from PageClientImpl::scale, and if so, it should be declared in WebKitWebViewPrivate.h instead of being public.

Don't we also need a companion API to get the current zoom, scale center, and view coordinates?

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3125
> + * the web view.

If this remains public, then you'll need to add:

Since: 2.24

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3155
> +

Avoid the stray whitespace change.

> Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:497
> +void PageClientImpl::scale(double scale, const IntPoint& scaleCenterInPageCoordinates, const IntPoint& newScaleCenterInViewCoordinates)
> +{
> +    if (WEBKIT_IS_WEB_VIEW(m_viewWidget)) {
> +        webkit_web_view_scale_page(WEBKIT_WEB_VIEW(m_viewWidget), scale,
> +            scaleCenterInPageCoordinates.x(), scaleCenterInPageCoordinates.y(),
> +            newScaleCenterInViewCoordinates.x(), newScaleCenterInViewCoordinates.y());
> +        return;
> +    }
> +
> +    webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(m_viewWidget))->scaleAndPanPage(scale, scaleCenterInPageCoordinates, newScaleCenterInViewCoordinates);
> +}

Why doesn't iOS need to implement this function?

> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:413
> +WEBKIT_API void
> +webkit_web_view_scale_page                           (WebKitWebView             *web_view,
> +                                                      gdouble                    zoom_level,
> +                                                      int                        scaleCenterX,
> +                                                      int                        scaleCenterY,
> +                                                      int                        viewX,
> +                                                      int                        viewY);

gint

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1125
> +    void zoom(double scale, const IntPoint& initialPoint, const IntPoint& centre) final

"center" would be more familiar to most.

> Source/WebKit/UIProcess/WebPageProxy.h:775
> +    void scaleAndPanPage(double scale, WebCore::IntPoint scaleCenterInPageCoordinates, WebCore::IntPoint newScaleCenterInViewCoordinates);

I think this is acceptable, but it is a bit unfortunate to need to add a new message here that is used only by GTK. That's why I'm curious why iOS doesn't need to implement PageClientImpl::scale.
Comment 10 Michael Catanzaro 2018-08-20 12:05:21 PDT
(In reply to Build Bot from comment #7)
> Comment on attachment 347503 [details]
> Patch
> 
> Attachment 347503 [details] did not pass win-ews (win):
> Output: https://webkit-queues.webkit.org/results/8919107
> 
> New failing tests:
> http/tests/security/local-video-source-from-remote.html

By the way, just ignore this bot. We know your patch didn't cause this unrelated test failure.
Comment 11 Jan-Michael Brummer 2018-08-20 23:31:18 PDT
@Justin: Would it be possible to show us a video before/after this change for those of us who do not have a touchscreen (Michael)?
Comment 12 Carlos Garcia Campos 2018-08-20 23:51:05 PDT
Regarding platform integration or UI related stuff we don't try to emulate Safari, but GNOME. So, pinch to zoom should be consistent with other GNOME applications. Only when the Web has a clearly different approach to other desktop applications we follow what firefox and/or chromium does, not Safari. And without any public API required, it should be transparent to the applications.
Comment 13 Justin Michaud 2018-08-21 07:24:11 PDT
https://imgur.com/a/LVrSMGj <- Before & After

This seems to me to be consistent with other gnome applications (ex: maps) and chromium on linux. The "before" behaviour is consistent with Firefox on Linux, but the "after" behaviour is consistent with Firefox and Edge on 
Windows last I checked.

Michael raises a great point about iOS not having a message for scaling. On second thought, it is not needed. I made the new message because I originally had a call to convert the scale centre from view coordinates to page coordinates, but later I realised that wasn't necessary.

To verify, it seems this is what iOS does:

``` WKWebView.mm:4111
void WebViewImpl::setMagnification(double magnification, CGPoint centerPoint)
{
    if (magnification <= 0 || isnan(magnification) || isinf(magnification))
        [NSException raise:NSInvalidArgumentException format:@"Magnification should be a positive number"];

    dismissContentRelativeChildWindowsWithAnimation(false);

    m_page->scalePageInViewCoordinates(magnification, WebCore::roundedIntPoint(centerPoint));
}
```

``` ViewGestureControllerMac.mm:141
void ViewGestureController::handleMagnificationGestureEvent(NSEvent *event, FloatPoint origin)
{
    ASSERT(m_activeGestureType == ViewGestureType::None || m_activeGestureType == ViewGestureType::Magnification);

    if (m_activeGestureType == ViewGestureType::None) {
        if (event.phase != NSEventPhaseBegan)
            return;

        // FIXME: We drop the first frame of the gesture on the floor, because we don't have the visible content bounds yet.
        m_magnification = m_webPageProxy.pageScaleFactor();
        m_webPageProxy.process().send(Messages::ViewGestureGeometryCollector::CollectGeometryForMagnificationGesture(), m_webPageProxy.pageID());
        m_lastMagnificationGestureWasSmartMagnification = false;

        return;
    }

    // We're still waiting for the DidCollectGeometry callback.
    if (!m_visibleContentRectIsValid)
        return;

    willBeginGesture(ViewGestureType::Magnification);

    double scale = event.magnification;
    double scaleWithResistance = resistanceForDelta(scale, m_magnification) * scale;

    m_magnification += m_magnification * scaleWithResistance;
    m_magnification = std::min(std::max(m_magnification, minElasticMagnification), maxElasticMagnification);

    m_magnificationOrigin = origin;

    if (m_frameHandlesMagnificationGesture)
        m_webPageProxy.scalePage(m_magnification, roundedIntPoint(origin));
    else
        m_webPageProxy.drawingArea()->adjustTransientZoom(m_magnification, scaledMagnificationOrigin(origin, m_magnification));

    if (event.phase == NSEventPhaseEnded || event.phase == NSEventPhaseCancelled)
        endMagnificationGesture();
}
```

I will have a new patch shortly. Thanks!
Comment 14 Justin Michaud 2018-08-21 08:18:01 PDT
Created attachment 347640 [details]
Patch
Comment 15 Michael Catanzaro 2018-08-21 09:22:08 PDT
Could you post the before/after videos as webm please? I think imgur is mp4.
Comment 16 Michael Catanzaro 2018-08-21 09:27:27 PDT
Comment on attachment 347640 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347640&action=review

Well the code changes, at least, are way simpler now. That looks good.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1128
> +        auto* page = webkitWebViewBaseGetPage(m_webView);

Instead of webkitWebViewBaseGetPage(), just use webView->priv->pageProxy directly.

> Source/WebKit/UIProcess/gtk/GestureController.h:54
> +    virtual void zoom(double scale, const WebCore::IntPoint& initialPoint, const WebCore::IntPoint& centre) = 0;

center here, too
Comment 18 Michael Catanzaro 2018-08-21 09:47:18 PDT
Discussed a bit with Jan-Michael on IRC. He's changed his mind and given approval.

(In reply to Carlos Garcia Campos from comment #12)
> Regarding platform integration or UI related stuff we don't try to emulate
> Safari, but GNOME. So, pinch to zoom should be consistent with other GNOME
> applications. Only when the Web has a clearly different approach to other
> desktop applications we follow what firefox and/or chromium does, not
> Safari. And without any public API required, it should be transparent to the
> applications.

Jan-Michael is saying he changed GNOME Photos from the Firefox on Linux to the iOS-style behavior last year... so it sounds like we are paving the way regardless.
Comment 19 Michael Catanzaro 2018-08-21 09:47:43 PDT
Comment on attachment 347640 [details]
Patch

r=me, but cq- due to two minor review comments
Comment 20 Michael Catanzaro 2018-08-21 09:49:29 PDT
BTW, I assume this does not regress normal non-touchscreen zooming.
Comment 21 Justin Michaud 2018-08-21 14:39:15 PDT
Created attachment 347699 [details]
Patch
Comment 22 Justin Michaud 2018-08-21 14:49:08 PDT
It seems this patch also fixes this bug: https://bugs.webkit.org/show_bug.cgi?id=154963

I fixed the Canadian-ism for center. For the second comment, webkitWebViewBaseGetPage is also used by the previous function, so I am wondering if I should change both, or leave it as-is.

For non touchscreen zooming, it works but the two zooms are independent. I wonder if this is a change that should be made in epiphany (to reset the page scaling when you press ctrl + [0-+]), or if we should reset the page scale when you set the zoom. In the first case, I think a getter / setter would need to be added to the public api. What do you think?

Thanks again!
Comment 23 Michael Catanzaro 2018-08-21 17:01:31 PDT
(In reply to Justin Michaud from comment #22)
> It seems this patch also fixes this bug:
> https://bugs.webkit.org/show_bug.cgi?id=154963
> 
> I fixed the Canadian-ism for center. For the second comment,
> webkitWebViewBaseGetPage is also used by the previous function, so I am
> wondering if I should change both, or leave it as-is.

Hm yes, I would, while you're at it, since you changed a line just below it. That's the only place in this file where it's used. Of course it makes no real difference, but our style is to access the priv variable directly when there's no difference.

> For non touchscreen zooming, it works but the two zooms are independent. I
> wonder if this is a change that should be made in epiphany (to reset the
> page scaling when you press ctrl + [0-+]), or if we should reset the page
> scale when you set the zoom. In the first case, I think a getter / setter
> would need to be added to the public api. What do you think?

OK, it sounds like this is the last bit that needs to be worked out.

I'm confused why scale and zoom are different concepts in the first place. By far the simplest solution is to have just one zoom that changes when you pinch. It would be unfortunate if we have to expose a second setting for scale.
Comment 24 Build Bot 2018-08-21 17:11:53 PDT
Comment on attachment 347699 [details]
Patch

Attachment 347699 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8936302

New failing tests:
http/wpt/workers/name-property-enhanced.html
Comment 25 Build Bot 2018-08-21 17:11:56 PDT
Created attachment 347737 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 26 Justin Michaud 2018-08-21 17:44:58 PDT
The current behaviour (before this patch) is that pinching is the same as zooming with ctrl-+.

After this patch (and on other platforms and browsers), there is a difference between scaling the page with a pinch gesture, and zooming in with ctrl-[+-] which re-lays out the page. If you want to avoid exposing a second setting for scale, we can re-set the scale when the zoom level changes. I don't see a use case where an embedder would not want this behaviour, but I wasn't sure.

I am going to:
- Replace the two use cases of webkitWebViewBaseGetPage with priv
- Reset the scale to 1 whenever the zoom level is changed

Does that sound good?
Comment 27 Build Bot 2018-08-21 19:36:47 PDT
Comment on attachment 347699 [details]
Patch

Attachment 347699 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8938988

New failing tests:
http/wpt/workers/name-property-enhanced.html
Comment 28 Build Bot 2018-08-21 19:36:49 PDT
Created attachment 347756 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 29 Michael Catanzaro 2018-08-21 19:49:50 PDT
(In reply to Justin Michaud from comment #26)
> I am going to:
> - Replace the two use cases of webkitWebViewBaseGetPage with priv
> - Reset the scale to 1 whenever the zoom level is changed
> 
> Does that sound good?

OK, that makes sense to me.

I don't know if resetting the scale is what users are going to expect from other browsers(?), but we can always change that in the future if needed.
Comment 30 Justin Michaud 2018-08-21 20:39:43 PDT
I just checked, you are right. It is just ctrl+0 that resets scaling. I guess adding a public api for the scalePage is probably the best option then. I will add that tomorrow, and hopefully everything will be good to go. I am thinking webkit_web_view_set_scale_level(WebKitWebView*, gdouble scale, gint pageX, gint pageY) + webkit_web_view_get_scale_level.

Once this lands, I can submit a patch to epiphany to fix ctrl+0.

Thanks again for the help!
Comment 31 Carlos Garcia Campos 2018-08-21 23:55:31 PDT
Comment on attachment 347699 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347699&action=review

> Source/WebKit/ChangeLog:7
> +

Please, explain the changes here.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:-1127
> -        m_webView->priv->pageClient->zoom(scale);

I think this is the only user of PageClientImpl::zoom() so it can be removed.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1131
> +        FloatPoint scaledZoomCenter = FloatPoint(initialPoint);

FloatPoint scaledZoomCenter(initialPoint);

> Source/WebKit/UIProcess/gtk/GestureController.cpp:206
> -    m_client.zoom(m_scale);
> +    m_client.zoom(m_scale, m_initialPoint, m_viewPoint);

I wonder if, instead of passing both values to the client and let them compute the center in page coordinates, we could do the conversion here and pass only the center to the client that would only need to call WebPageProxy::scalePage(scale, center);
Comment 32 Justin Michaud 2018-08-22 09:31:13 PDT
Created attachment 347807 [details]
Patch
Comment 33 Justin Michaud 2018-08-22 09:37:43 PDT
I tested the new api with the following patch to epiphany:

```
diff --git a/src/ephy-window.c b/src/ephy-window.c
index a86fd45b2..d7f005ba3 100644
--- a/src/ephy-window.c
+++ b/src/ephy-window.c
@@ -967,7 +967,7 @@ sync_tab_zoom (WebKitWebView *web_view, GParamSpec *pspec, EphyWindow *window)
 {
   GActionGroup *action_group;
   GAction *action;
-  gboolean can_zoom_in = TRUE, can_zoom_out = TRUE, can_zoom_normal = FALSE;
+  gboolean can_zoom_in = TRUE, can_zoom_out = TRUE;
   double zoom;
   GtkWidget *zoom_level_button;
   gchar *zoom_level;
@@ -991,10 +991,6 @@ sync_tab_zoom (WebKitWebView *web_view, GParamSpec *pspec, EphyWindow *window)
     can_zoom_out = FALSE;
   }
 
-  if (zoom != 1.0) {
-    can_zoom_normal = TRUE;
-  }
-
   action_group = gtk_widget_get_action_group (GTK_WIDGET (window),
                                               "win");
 
@@ -1003,7 +999,7 @@ sync_tab_zoom (WebKitWebView *web_view, GParamSpec *pspec, EphyWindow *window)
   action = g_action_map_lookup_action (G_ACTION_MAP (action_group), "zoom-out");
   g_simple_action_set_enabled (G_SIMPLE_ACTION (action), can_zoom_out);
   action = g_action_map_lookup_action (G_ACTION_MAP (action_group), "zoom-normal");
-  g_simple_action_set_enabled (G_SIMPLE_ACTION (action), can_zoom_normal);
+  g_simple_action_set_enabled (G_SIMPLE_ACTION (action), true);
 }
 
 static void
@@ -3472,6 +3468,30 @@ ephy_window_set_zoom (EphyWindow *window,
     webkit_web_view_set_zoom_level (web_view, zoom);
 }
 
+/**
+ * ephy_window_set_scale:
+ * @window: an #EphyWindow
+ * @scale: the desired zoom level
+ *
+ * Sets the scale on @window's active #EphyEmbed. A @scale of 1.0 corresponds to
+ * 100% scale (normal size).
+ **/
+void
+ephy_window_set_scale (EphyWindow *window,
+                      double      scale)
+{
+  EphyEmbed *embed;
+  WebKitWebView *web_view;
+
+  g_assert (EPHY_IS_WINDOW (window));
+
+  embed = window->active_embed;
+  g_assert (embed != NULL);
+
+  web_view = EPHY_GET_WEBKIT_WEB_VIEW_FROM_EMBED (embed);
+  webkit_web_view_set_page_scale (web_view, scale, 0, 0);
+}
+
 static void
 ephy_window_change_allow_popup_windows_state (GSimpleAction *action,
                                               GVariant      *state,
diff --git a/src/ephy-window.h b/src/ephy-window.h
index 1a6feab4d..6f66733ff 100644
--- a/src/ephy-window.h
+++ b/src/ephy-window.h
@@ -53,6 +53,9 @@ void              ephy_window_load_url            (EphyWindow *window,
 void              ephy_window_set_zoom            (EphyWindow *window,
                                                    double zoom);
 
+void              ephy_window_set_scale           (EphyWindow *window,
+                                                   double scale);
+
 void              ephy_window_activate_location   (EphyWindow *window);
 const char       *ephy_window_get_location        (EphyWindow *window);
 
diff --git a/src/window-commands.c b/src/window-commands.c
index 0d483808c..0a25bd18a 100644
--- a/src/window-commands.c
+++ b/src/window-commands.c
@@ -1782,6 +1782,7 @@ window_cmd_zoom_normal (GSimpleAction *action,
                         gpointer       user_data)
 {
   EphyWindow *window = user_data;
+  ephy_window_set_scale (window, 1.0);
   ephy_window_set_zoom (window, 1.0);
 }
``` 

With this patch, ctrl + 0 works as expected.

Thanks for your patience!
Comment 34 Michael Catanzaro 2018-08-22 12:55:09 PDT
Comment on attachment 347807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347807&action=review

I was OK with the previous patch, BTW.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3160
> + * @origin_x: the x coordinate for the origin
> + * @origin_y: the y coordinate for the origin

Well this brings back my old question: where is the supposed to get the values to pass here?

In your Epiphany patch, it always passes 0, which is fine when doing Ctrl+0.
Comment 35 Michael Catanzaro 2018-08-22 13:07:15 PDT
(In reply to Michael Catanzaro from comment #34)
> Well this brings back my old question: where is the supposed to get the
> values to pass here?

"where is the client application"
Comment 36 Justin Michaud 2018-08-22 19:31:49 PDT
No idea. I assumed that there would be some api to get the current page scroll position, but it seems there isn't. I'm really not sure what to do now.
Comment 37 Michael Catanzaro 2018-08-22 20:07:27 PDT
I was about ready to give r+ on attachment #347699 [details] before you brought back the public API.

Changing the zoom level is an unusual operation. While not great, I think it's OK if that resets the scale for now. We can always improve with new APIs in a future patch, right?
Comment 38 Carlos Garcia Campos 2018-08-22 23:35:11 PDT
Comment on attachment 347807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347807&action=review

I would also avoid to add new API. Also note that all new API must include a unit test. If apps only need to reset the scale, maybe it's better to only expose webkit_web_view_reset_scale(). If we really want to allow users to scale the page, we should provide a test case to ensure it's really possible with the proposed API. The ephy example always passing 1.0 and 0,0 as center is not enough. Note also that without new API we could merge this to 2.22 branch.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3158
> + * @scale_level: the page scale

I would use scale_factor

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3166
> + * example, a pinch-to-zoom touch gesture). The point (origin_x, origin_y)
> + * should be in page coordinates, scaled by the new scale factor.

I don't think we should receive page coordinates here. This function could be used to implement gestures differently, overriding the default WebKit behavior. In that case the user will be handling widget coords.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3168
> + * Since 2.24

Since:

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3170
> +void webkit_web_view_set_page_scale(WebKitWebView* webView, gdouble scaleLevel, gint originX, gint originY)

set_page_scale sounds like you are setting a property, but page scale isn't and the setter receives more parameters. I think this should be webkit_web_view_scale().

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3174
> +    auto& page = getPage(webView);

Here we would need to convert to page coordinates.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3179
> + * webkit_web_view_get_zoom_level:

get_page_scale

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3182
> + * Get the zoom level of @web_view, i.e. the factor by which the

zoom level -> page scale

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3187
> + * Since 2.24

Since:

> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:415
> +webkit_web_view_get_page_scale                       (WebKitWebView             *web_view);

wpe header would need to be updated too.
Comment 39 Justin Michaud 2018-08-23 08:01:21 PDT
Created attachment 347921 [details]
Patch
Comment 40 Michael Catanzaro 2018-08-23 08:25:46 PDT
Comment on attachment 347921 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347921&action=review

What does the Epiphany patch look like now? Epiphany has to call webkit_web_view_reset_page_scale() when modifying the zoom?

Is there any time a browser would want to use this API *except* when modifying the zoom? If so, r=me. If not, then WebKit should probably just reset the scale itself when the zoom changes, like you had proposed earlier?

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3160
> + * Resets the page scale of @web_view, i.e. the factor by
> + * which the page is magnified without changing the layout

OH. So the difference between scale and zoom is that zoom changes the layout?

I finally understand!

P.S. add a period at the end of this sentence.
Comment 41 Justin Michaud 2018-08-23 08:34:24 PDT
Created attachment 347924 [details]
Patch
Comment 42 Justin Michaud 2018-08-23 08:39:55 PDT
Fixed the period.

I don't know if there is any other time the page scale would need to be reset. The problem is that we need to reset the scale when the zoom is reset, but not when it is changed. I suppose it is a pretty minor detail.

I am not sure how to add a test, since there is no way to get or set the page scale from the public api. I would appreciate some guidance. I think the unit test is the last remaining part of this, if it is required.

Ephy patch:
```
diff --git a/src/ephy-window.c b/src/ephy-window.c
index a86fd45b2..366c4d50a 100644
--- a/src/ephy-window.c
+++ b/src/ephy-window.c
@@ -967,7 +967,7 @@ sync_tab_zoom (WebKitWebView *web_view, GParamSpec *pspec, EphyWindow *window)
 {
   GActionGroup *action_group;
   GAction *action;
-  gboolean can_zoom_in = TRUE, can_zoom_out = TRUE, can_zoom_normal = FALSE;
+  gboolean can_zoom_in = TRUE, can_zoom_out = TRUE;
   double zoom;
   GtkWidget *zoom_level_button;
   gchar *zoom_level;
@@ -991,10 +991,6 @@ sync_tab_zoom (WebKitWebView *web_view, GParamSpec *pspec, EphyWindow *window)
     can_zoom_out = FALSE;
   }
 
-  if (zoom != 1.0) {
-    can_zoom_normal = TRUE;
-  }
-
   action_group = gtk_widget_get_action_group (GTK_WIDGET (window),
                                               "win");
 
@@ -1003,7 +999,7 @@ sync_tab_zoom (WebKitWebView *web_view, GParamSpec *pspec, EphyWindow *window)
   action = g_action_map_lookup_action (G_ACTION_MAP (action_group), "zoom-out");
   g_simple_action_set_enabled (G_SIMPLE_ACTION (action), can_zoom_out);
   action = g_action_map_lookup_action (G_ACTION_MAP (action_group), "zoom-normal");
-  g_simple_action_set_enabled (G_SIMPLE_ACTION (action), can_zoom_normal);
+  g_simple_action_set_enabled (G_SIMPLE_ACTION (action), true);
 }
 
 static void
@@ -3472,6 +3468,27 @@ ephy_window_set_zoom (EphyWindow *window,
     webkit_web_view_set_zoom_level (web_view, zoom);
 }
 
+/**
+ * ephy_window_reset_scale:
+ * @window: an #EphyWindow
+ *
+ * Resets the page scale on @window's active #EphyEmbed.
+ **/
+void
+ephy_window_reset_scale (EphyWindow *window)
+{
+  EphyEmbed *embed;
+  WebKitWebView *web_view;
+
+  g_assert (EPHY_IS_WINDOW (window));
+
+  embed = window->active_embed;
+  g_assert (embed != NULL);
+
+  web_view = EPHY_GET_WEBKIT_WEB_VIEW_FROM_EMBED (embed);
+  webkit_web_view_reset_page_scale (web_view);
+}
+
 static void
 ephy_window_change_allow_popup_windows_state (GSimpleAction *action,
                                               GVariant      *state,
diff --git a/src/ephy-window.h b/src/ephy-window.h
index 1a6feab4d..ca8d72707 100644
--- a/src/ephy-window.h
+++ b/src/ephy-window.h
@@ -53,6 +53,8 @@ void              ephy_window_load_url            (EphyWindow *window,
 void              ephy_window_set_zoom            (EphyWindow *window,
                                                    double zoom);
 
+void              ephy_window_reset_scale         (EphyWindow *window);
+
 void              ephy_window_activate_location   (EphyWindow *window);
 const char       *ephy_window_get_location        (EphyWindow *window);
 
diff --git a/src/window-commands.c b/src/window-commands.c
index 0d483808c..a014500ad 100644
--- a/src/window-commands.c
+++ b/src/window-commands.c
@@ -1783,6 +1783,7 @@ window_cmd_zoom_normal (GSimpleAction *action,
 {
   EphyWindow *window = user_data;
   ephy_window_set_zoom (window, 1.0);
+  ephy_window_reset_scale (window);
 }
 
 void
```
Comment 43 Carlos Garcia Campos 2018-08-24 00:23:36 PDT
I prefer not to add new API when we don't know any other use case than resetting the scale when the zoom level is changed to 1.0. Because we can do that ourselves. So, I agree with Michael, let's do that for now. If that's eventually a problem, then we can simply revert the behavior and provide the public API.
Comment 44 Justin Michaud 2018-08-24 06:47:47 PDT
Created attachment 348004 [details]
Patch
Comment 45 Michael Catanzaro 2018-08-24 06:53:55 PDT
Comment on attachment 348004 [details]
Patch

Please set cq? to request commit!
Comment 46 Justin Michaud 2018-08-30 14:06:41 PDT
Hey Michael,

How long after setting commit-queue? does it take usually to get commit-queue+?
Comment 47 Adrian Perez 2018-08-30 15:29:25 PDT
Comment on attachment 348004 [details]
Patch

I think the reviewer forgot to set cq+, so I've just flipped the
flag (committers like myself can set the commit-queue flag, but
of course you still need a positive review beforehand ;D)

Thanks for your patch!
Comment 48 WebKit Commit Bot 2018-08-30 15:55:35 PDT
Comment on attachment 348004 [details]
Patch

Clearing flags on attachment: 348004

Committed r235529: <https://trac.webkit.org/changeset/235529>
Comment 49 WebKit Commit Bot 2018-08-30 15:55:37 PDT
All reviewed patches have been landed.  Closing bug.