Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1020)

Issue 248383002: Revert of mac: History swiping doesn't work right with iframes. (Closed)

Created:
6 years, 8 months ago by erikchen
Modified:
6 years, 8 months ago
Reviewers:
Avi (use Gerrit), Nico
CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, jam, penghuang+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, James Su, miu+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Revert of mac: History swiping doesn't work right with iframes. (https://codereview.chromium.org/227043012/) Reason for revert: This CL causes regressions in vertical content scrolling. https://code.google.com/p/chromium/issues/detail?id=365034 Original issue's description: > mac: History swiping doesn't work right with iframes. > > This CL is a reland of crrev.com/194713016. The reason that the previous > attempt to land this CL failed was because Blink was claiming to handle events > that it was actually ignoring. This has been fixed in crrev.com/219473003, > which also added unit-tests to ensure that the new behavior is correct. > > In addition, this reland removes all logic related to the isPinnedLeft and > isPinnedRight properties of the history swiper. They are no longer required > because they are a subset of the state that determines whether blink will > handle an event, which is now being correctly propagated to the history swiper. > This fixes a long outstanding bug (I don't think it's ever worked) where the > pinned state is not correctly propagated to the history swiper, which prevents > history navigation. (Blink only sends callbacks for when a user scrolls or the > window is resized. If the user performs a history navigation, the pinned state > might change, but Blink will not send a callback to update the state of the > history swiper). > > ----------------Description from first attempt to land this CL---------------- > > In the previous behavior, it was possible for a user to start scrolling an > iframe with the touch pad, and then trigger history navigation. This happened > because the Cocoa view only received callbacks for whether an event in a > gesture wasn't handled. If any event in a gesture happened to not be handled by > blink, then that event could trigger history swiping, even though blink may > have handled earlier events in the same gesture! > > My solution is to propagate the information for whether an event was handled to > the Cocoa view. History swiping is only enabled if no event is ever handled by > blink. > > BUG=321437 > TEST=Navigate to www.google.com. Then navigate to > http://codepen.io/anon/pen/ujsec. Place the cursor in the box that says "place > cursor here", and start a downwards 2-finger swipe. Half way through the swipe, > change directions and swipe to the left (or right, depending on your settings) > to navigate backwards in history. Prior to this change, the history swipe arrow > would appear. In this change, the history swipe arrow no longer appears. > > Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262572 TBR=thakis@chromium.org,avi@chromium.org BUG=321437 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265548

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+141 lines, -89 lines) Patch
M chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.h View 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm View 1 chunk +13 lines, -2 lines 0 comments Download
M chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.h View 3 chunks +17 lines, -13 lines 0 comments Download
M chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm View 5 chunks +31 lines, -10 lines 0 comments Download
M chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_unit_test.mm View 4 chunks +9 lines, -23 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.cc View 1 chunk +4 lines, -7 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_unittest.cc View 1 chunk +3 lines, -6 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.h View 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_base.h View 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_base.cc View 1 chunk +2 lines, -3 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_mac.h View 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_mac.mm View 4 chunks +33 lines, -7 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_mac_unittest.mm View 1 chunk +4 lines, -3 lines 0 comments Download
M content/port/browser/render_widget_host_view_port.h View 1 chunk +2 lines, -6 lines 0 comments Download
M content/public/browser/render_widget_host_view_mac_delegate.h View 2 chunks +9 lines, -5 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
erikchen
Created Revert of mac: History swiping doesn't work right with iframes.
6 years, 8 months ago (2014-04-22 23:11:58 UTC) #1
erikchen
The CQ bit was checked by erikchen@chromium.org
6 years, 8 months ago (2014-04-22 23:12:52 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/248383002/1
6 years, 8 months ago (2014-04-22 23:13:37 UTC) #3
Avi (use Gerrit)
lgtm OK. :(
6 years, 8 months ago (2014-04-23 02:12:32 UTC) #4
commit-bot: I haz the power
6 years, 8 months ago (2014-04-23 06:13:07 UTC) #5
Message was sent while issue was closed.
Change committed as 265548

Powered by Google App Engine
This is Rietveld 408576698