Summary: | [GTK] Touchscreen pinch to zoom should scale the page like other platforms | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Justin Michaud <justin> | ||||||||||||||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Minor | CC: | aperez, berto, bugs-noreply, carlosg, cgarcia, commit-queue, ews-watchlist, gustavo, jan.brummer, mcatanzaro, rniwa | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=154963 | ||||||||||||||||||||||||
Attachments: |
|
Description
Justin Michaud
2018-08-20 09:18:38 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 (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. Created attachment 347503 [details]
Patch
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 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... 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 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 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 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. (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. @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)? 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. 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! Created attachment 347640 [details]
Patch
Could you post the before/after videos as webm please? I think imgur is mp4. 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 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 on attachment 347640 [details]
Patch
r=me, but cq- due to two minor review comments
BTW, I assume this does not regress normal non-touchscreen zooming. Created attachment 347699 [details]
Patch
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! (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 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 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
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 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 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
(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. 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 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); Created attachment 347807 [details]
Patch
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 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. (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" 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. 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 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. Created attachment 347921 [details]
Patch
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. Created attachment 347924 [details]
Patch
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 ``` 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. Created attachment 348004 [details]
Patch
Comment on attachment 348004 [details]
Patch
Please set cq? to request commit!
Hey Michael, How long after setting commit-queue? does it take usually to get commit-queue+? 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 on attachment 348004 [details] Patch Clearing flags on attachment: 348004 Committed r235529: <https://trac.webkit.org/changeset/235529> All reviewed patches have been landed. Closing bug. |