Can you rephrase the title and description?  The main frame can't be an OOPIF.  :)  Did you mean RemoteFrame?

WebViewImpl::AnimateDoubleTapZoom isn't Blink>Animation - that component is for web-exposed animation APIs (e.g. CSS animations, Web Animations)

REPRO STEPS:

Manual repro steps for find-in-page case:
1. Launch chrome with --site-per-process flag
2. Navigate to http://csreis.github.io/tests/cross-site-iframes.html
3. Click "Add Frame" button 3 times (to ensure the last frame is not visible / needs to be scrolled to)
4. Navigate the last/bottom frame cross-site by clicking "Go cross-site (complex page)"
5. Scroll the main frame to the top, so that the last/bottom frame is not visible
6. Ctrl-F and search for "Waterfall"

EXPECTED BEHAVIOR: The main page scrolls to show the find-in-page results (this is the behavior without --site-per-process)

ACTUAL BEHAVIOR: The main page doesn't scroll (i.e. going between different matches scrolls the subframe containing the matches, but this has in practice no visible effect when the main page doesn't scroll)

WORK AROUND: The user can scroll the main frame manually (if the user knows about the workaround + knows which subframe contains the matches)


Based on the repro steps above I think I should bump-up the priority of this bug, because find-in-page seems effectively broken if the found matches are in a cross-site frame that is not visible (i.e. that requires scrolling of the main frame to become visible).

Actually, let's split this bug in 2:
-  issue 758347  - find-in-page seems rather broken in some OOPIF scenarios - pri2
-  issue 758348  - tap/zoom doesn't work - pri3 (because - tap gestures are Android-only AFAICT from the code *and* because OOPIFs don't yet ship on Android)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f152db88197739be6094b050d1e59ffc724dd828

commit f152db88197739be6094b050d1e59ffc724dd828
Author: Ehsan Karamad <ekaramad@chromium.org>
Date: Mon Oct 23 17:41:25 2017

Implement recursive scrolling of rect into view for OOPIFs

Functionalities such as element.scrollIntoView() API rely on recursively
scrolling LayoutBoxes until a given rectangle in view. When the LayoutObject
is that of a LocalFrameView, the scrolling is handed over to the owner
element in the parent frame. This step is currently not implemented for
OOPIFs which consequently breaks different features such as
find-in-page and element.scrollIntoView() API.

This CL adds support for recursive scroll across OOPIFs through adding
a public struct WebRemoteScrollPropteries in blink which will be passed
from a renderer process to the browser to be then forwarded to the
parent frame. The new struct along with the rectangle to be scrolled are
the missing information needed to join the scroll chain between a child
process and its parent.

The CL adds an interactive test to catch element.scrollIntoView()
regressions in OOPIFs.

Bug:  618474 ,  676037 , 734209,  768029 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Iff5a5b470721f1a6e3c5db5f8f1c427a52ddb31c
Reviewed-on: https://chromium-review.googlesource.com/679319
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: Dimitri Glazkov <dglazkov@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510838}
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/content/browser/DEPS
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/content/browser/frame_host/render_frame_proxy_host.cc
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/content/browser/frame_host/render_frame_proxy_host.h
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/content/common/DEPS
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/content/common/frame_messages.h
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/content/renderer/render_frame_impl.h
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/content/renderer/render_frame_proxy.cc
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/content/renderer/render_frame_proxy.h
[add] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/content/test/data/bounding_rect_for_frame.html
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.cpp
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.h
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/third_party/WebKit/Source/core/exported/WebRemoteFrameImpl.cpp
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/third_party/WebKit/Source/core/exported/WebRemoteFrameImpl.h
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/third_party/WebKit/Source/core/frame/LocalFrameClient.h
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/third_party/WebKit/Source/core/frame/LocalFrameView.cpp
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/third_party/WebKit/Source/core/frame/LocalFrameView.h
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/third_party/WebKit/Source/core/layout/ScrollAlignment.h
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/third_party/WebKit/Source/platform/BUILD.gn
[add] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/third_party/WebKit/Source/platform/exported/WebRemoteScrollProperties.cpp
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/third_party/WebKit/public/BUILD.gn
[add] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/third_party/WebKit/public/platform/WebRemoteScrollProperties.h
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/third_party/WebKit/public/web/WebFrameClient.h
[modify] https://crrev.com/f152db88197739be6094b050d1e59ffc724dd828/third_party/WebKit/public/web/WebRemoteFrame.h

Adding blocker label on issue 393285 given that ZoomToFindInPageRect is technically only used with text autosizing.

Looking at the lack of support for double-tap zoom, any change in page-scale-factor will have to be applied at the root-frame level, but presumably it needs information about the geometry to be zoomed-to (target element) from the subframe.

Possibly this can be done via adding page-scale information to the WebRemoteScrollProperties mechanism?

In the double-tap case, one possible implementation could be to have the target element rect returned by a previous event (similar to how TouchAction is sent to the browser). Then we send the double tap event to the main frame with the target rect. It also seems like this approach could have the animation performed on the compositor thread, making it smoother even for the non-OOPIF case.

Agreed vis-a-vis sending the rect back to the browser for propagation to the parent.

At present these animations are initiated on the main thread when WebViewImpl sends them to LayerTreeHost::StartPageScaleAnimation(). Then the animation is done on the compositor thread after the next commit.

I would imagine the parent should still start the animation in the usual way, but can do so using the rect sent from the child. Not sure if there are any security concerns here or not.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9208904bb47d4c523f4caedb733f157c342baa7e

commit 9208904bb47d4c523f4caedb733f157c342baa7e
Author: W. James MacLean <wjmaclean@chromium.org>
Date: Wed Aug 22 13:07:56 2018

Prevent kGestureDoubleTap from propagating for OOPIFs.

kGestureDoubleTap is used by Android only, and page scale animations
should only be iniitated in the main frame renderer. WebViewImpl has
special handling logic that avoids sending this event to EventHandler,
but the current (incomplete) implementation for OOPIFs in
WebFrameWidgetImpl does forward it to EventHandler, hitting a variety
of NOTREACHED points along the way.

This CL modifies WebFrameWidgetImpl to drop this event without sending
it to EventHandler, and return the same status as WebViewImpl does.

Bug: 734209
Change-Id: I900111df370a70b1db418660135e7e00e0f2508e
Reviewed-on: https://chromium-review.googlesource.com/1183942
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584990}
[modify] https://crrev.com/9208904bb47d4c523f4caedb733f157c342baa7e/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ec4dd97b26dbeb0c7fd94ae610ff2404a4c6b0c4

commit ec4dd97b26dbeb0c7fd94ae610ff2404a4c6b0c4
Author: W. James MacLean <wjmaclean@chromium.org>
Date: Thu Oct 25 19:17:41 2018

Move ComputeBlockBounds to WebFrameWidgetBase.

This CL is a re-factor in preparation for implementing double-tap
zoom for OOPIFs.

Bug: 734209
Change-Id: I97ef9dc77d4365900f4ad50b2ad5a7b8bb5aeacc
Reviewed-on: https://chromium-review.googlesource.com/c/1298111
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602824}
[modify] https://crrev.com/ec4dd97b26dbeb0c7fd94ae610ff2404a4c6b0c4/third_party/blink/renderer/core/exported/web_frame_test.cc
[modify] https://crrev.com/ec4dd97b26dbeb0c7fd94ae610ff2404a4c6b0c4/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/ec4dd97b26dbeb0c7fd94ae610ff2404a4c6b0c4/third_party/blink/renderer/core/exported/web_view_impl.h
[modify] https://crrev.com/ec4dd97b26dbeb0c7fd94ae610ff2404a4c6b0c4/third_party/blink/renderer/core/frame/web_frame_widget_base.cc
[modify] https://crrev.com/ec4dd97b26dbeb0c7fd94ae610ff2404a4c6b0c4/third_party/blink/renderer/core/frame/web_frame_widget_base.h

*** UI Mass Triage ***

adding labels for expert review

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d973a55b7929fccb10c59f594b8d5152a8fc1296

commit d973a55b7929fccb10c59f594b8d5152a8fc1296
Author: W. James MacLean <wjmaclean@chromium.org>
Date: Thu Nov 29 21:39:13 2018

Implement OOPIF double-tap-zoom.

This CL adds the required pathways to support double-tap-zoom for OOPIF.

1) Modifies WebFrameWidgetImpl to get the box_bounds of the double-
   tapped element, and
2) sends the tapped point and box_bounds to the browser, where
3) RenderFrameHostImpl transforms them into root-view coordinates, and
4) sends them to the renderer via the main-frame's RenderViewHostImpl.
5) From there RenderViewImpl invokes WebViewImpl's
   AnimateDoubleTapZoom.

Bug: 734209
Change-Id: Ic55afb6154356d676872ced93f64a243190cf289
Reviewed-on: https://chromium-review.googlesource.com/c/1298081
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612371}
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/content/common/view_messages.h
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/content/common/widget_messages.h
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/content/renderer/render_view_impl.cc
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/content/renderer/render_view_impl.h
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/content/renderer/render_widget.cc
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/content/renderer/render_widget.h
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/third_party/blink/public/web/web_view.h
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/third_party/blink/public/web/web_widget_client.h
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/third_party/blink/renderer/core/exported/web_view_impl.h
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/third_party/blink/renderer/core/page/chrome_client.h
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/third_party/blink/renderer/core/page/chrome_client_impl.cc
[modify] https://crrev.com/d973a55b7929fccb10c59f594b8d5152a8fc1296/third_party/blink/renderer/core/page/chrome_client_impl.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f936709b51647364772c31b27f702280209a2d3d

commit f936709b51647364772c31b27f702280209a2d3d
Author: W. James MacLean <wjmaclean@chromium.org>
Date: Thu Dec 06 20:23:30 2018

Refactor to split ZoomToFindInPageRect between the view/frame widgets.

The CL performs a refactor to prepare for adding an OOPIF path for
ZoomToFindInPageRect.

Bug: 734209
Change-Id: I9575f84d4850ad13b869892c23e8f109ea8f80e4
Reviewed-on: https://chromium-review.googlesource.com/c/1358912
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614475}
[modify] https://crrev.com/f936709b51647364772c31b27f702280209a2d3d/third_party/blink/public/web/web_frame_widget.h
[modify] https://crrev.com/f936709b51647364772c31b27f702280209a2d3d/third_party/blink/renderer/core/editing/finder/text_finder.cc
[modify] https://crrev.com/f936709b51647364772c31b27f702280209a2d3d/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/f936709b51647364772c31b27f702280209a2d3d/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
[modify] https://crrev.com/f936709b51647364772c31b27f702280209a2d3d/third_party/blink/renderer/core/frame/web_frame_widget_impl.h
[modify] https://crrev.com/f936709b51647364772c31b27f702280209a2d3d/third_party/blink/renderer/core/frame/web_view_frame_widget.cc
[modify] https://crrev.com/f936709b51647364772c31b27f702280209a2d3d/third_party/blink/renderer/core/frame/web_view_frame_widget.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5372eb73a82ed9038f94a71eecd5fe534252c34c

commit 5372eb73a82ed9038f94a71eecd5fe534252c34c
Author: W. James MacLean <wjmaclean@chromium.org>
Date: Wed Dec 19 12:56:36 2018

Plumb ZoomToFindInPageRect for OOPIFs.

This CL adds the necessary plumbing to allow ZoomToFindInPageRect
to work with OOPIFs. The implementation mirrors that of
AnimateDoubleTapZoom.

Bug: 734209
Change-Id: I9f7eab84b1b2fd026a7236836f587d6164fcfb44
Reviewed-on: https://chromium-review.googlesource.com/c/1367864
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617811}
[modify] https://crrev.com/5372eb73a82ed9038f94a71eecd5fe534252c34c/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/5372eb73a82ed9038f94a71eecd5fe534252c34c/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/5372eb73a82ed9038f94a71eecd5fe534252c34c/content/common/view_messages.h
[modify] https://crrev.com/5372eb73a82ed9038f94a71eecd5fe534252c34c/content/common/widget_messages.h
[modify] https://crrev.com/5372eb73a82ed9038f94a71eecd5fe534252c34c/content/renderer/render_view_impl.cc
[modify] https://crrev.com/5372eb73a82ed9038f94a71eecd5fe534252c34c/content/renderer/render_view_impl.h
[modify] https://crrev.com/5372eb73a82ed9038f94a71eecd5fe534252c34c/content/renderer/render_widget.cc
[modify] https://crrev.com/5372eb73a82ed9038f94a71eecd5fe534252c34c/content/renderer/render_widget.h
[modify] https://crrev.com/5372eb73a82ed9038f94a71eecd5fe534252c34c/third_party/blink/public/web/web_view.h
[modify] https://crrev.com/5372eb73a82ed9038f94a71eecd5fe534252c34c/third_party/blink/public/web/web_widget_client.h
[modify] https://crrev.com/5372eb73a82ed9038f94a71eecd5fe534252c34c/third_party/blink/renderer/core/exported/web_view_impl.h
[modify] https://crrev.com/5372eb73a82ed9038f94a71eecd5fe534252c34c/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/92a4519189c83e6cac800ea476371034f18010b2

commit 92a4519189c83e6cac800ea476371034f18010b2
Author: W. James MacLean <wjmaclean@chromium.org>
Date: Thu Dec 20 20:09:55 2018

Add test for OOPIF ZoomToFindInPageRect.

This CL adds a test for
https://chromium-review.googlesource.com/c/chromium/src/+/1367864.

It modifies an existing, but disabled test to do this, so it's possible
this test was flakey or otherwise had issues, so this is being landed
separately from the main CL so we can diagnose it separately.

Bug: 734209
Change-Id: I7bf8a14f0d126a62e8ac4c5815d4b1802e27b3e6
Reviewed-on: https://chromium-review.googlesource.com/c/1384940
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618307}
[modify] https://crrev.com/92a4519189c83e6cac800ea476371034f18010b2/content/browser/find_request_manager.h
[modify] https://crrev.com/92a4519189c83e6cac800ea476371034f18010b2/content/browser/find_request_manager_browsertest.cc

 Issue 758348  has been merged into this issue.

I think the work on this is complete ... the zoom animations for both AnimateDoubleTapZoom and ZoomToFindInPageRect are hooked up, and the remaining one (ZoomToMultipleTargetsRect) no longer seems to exist.

lukasza@, shall we close this now?

+dcheng@ for any insights related to the remaining IsWebLocalFrame / ToWebLocalFrame calls

I see that WebViewImpl::WidenRectWithinPageBounds still has the following DCHECK:

    // TODO(lukasza): https://crbug.com/734209: The DCHECK below holds now, but
    // only because all of the callers don't support OOPIFs and exit early if
    // the main frame is not local.
    DCHECK(MainFrame()->IsWebLocalFrame());
    max_size = MainFrame()->ToWebLocalFrame()->DocumentSize();
    scroll_offset = MainFrame()->ToWebLocalFrame()->GetScrollOffset();

Maybe this DCHECK is fine (I see that WebViewImpl::AnimateDoubleTapZoom and WebViewImpl::ZoomToFindInPageRect also DCHECK that the main frame is local).  OTOH, I wonder if these DCHECKs could go away if this functionality was instead implemented in a widget and/or local frame?  Maybe this should be tracked in a separate bug?

So it's quite right for WebViewImpl::AnimateDoubleTapZoom and WebViewImpl::ZoomToFindInPageRect to require that the MainFrame is local.

They way we've made this work is for OOPIF frames that request the service to do so by forwarding their rects to the browser process, where they get converted to main-frame coords and forwarded to the MainFrame.

Only the MainFrame renderer should ever initiate page scale animations ... since only the main frame has a ViewPort. The page scale changes in the animations propagates back to the OOPIFs via syncing the VisualProperties.

RE: #c25:

Thanks for the explanation!  I think we can close this as fixed, as soon as I get rid of the incorrect comment in WidenRectWithinPageBounds - https://crrev.com/c/1391573.

Marking this as a site isolation on Android blocker, based on  issue 758348  being merged here - mostly for tracking, since it seems that this is close to being fixed.  Though see also my comment on  https://crbug.com/758348#c17  about zoom being slightly higher than needed for OOPIF vs non-OOPIF - not sure whether that's a separate bug, or something still to be done here.

lukasz@ - any word on when your CL mentioned in #26 will land, so we can close this out?

dcheng@ - is the review of that CL held up on the check for the existence of the mainframe() ? That check has been there a long time, and we could try removing it in a separate CL, but it shouldn't block lukasz@'s CL in the meantime.