Maybe as part of introducing the test interface, we stop using GetLastScrollOffset in those cases. Assigning to jonross@ for now.

Jon are you actively working on this? Saman says this is not only used in tests. It's used in other places, like taking screenshots, and that's likely the hard part. It's unclear whether that's a strict Requirement. 

+miu since this involves CopyRequests. I wonder if we can assign tokens to CopyRequests that match a token in a CompositorFrame? 

I haven't started looking at this one yet

Marking as available.

I looked at this last night, and the only non-test use of this is here [1]. The code does not actually care about the offset, it simply needs to know if the page is scrolled at the top, or not. So replacing this with a standalone renderer->browser message should be easy.

For the uses in tests, we can probably use the frame-sink-observer idea we discussed elsewhere before (where the browser would add a frame-sink-observer, and get notified of the compositor-frames the renderer submits).

[1] https://cs.chromium.org/chromium/src/chrome/browser/thumbnails/thumbnail_tab_helper.cc?l=293

I am currently working on this bug now.

Technically this would be racy, because when the browser requests a snapshot there is no guarantee that the renderer has not scrolled in the meantime. There is a non-racy solution is that I explained to Yi which is non-racy, but requires a bit more work. Or we can just say we don't care about the race because honestly in practice it shouldn't happen.

Yeah, there are other options (e.g. the CopyFromSurface() callback receives both a SkBitmap and a bool at_top) that is probably cleaner. If we can do without introducing a new separate renderer->browser ipc with racy behaviour, that'd be very nice.

I'm fine with Viz returning at_top for now. Long-term we shouldn't make Viz track scroll offset but for now it's a solution that involves fewer IPC messages and is simpler than other solutions I can think of.

I chatted with Fady. We can do something like this.
The renderer allocates a new LocalSurfaceId when scroll offset changes from zero to non-zero and vice versa. The renderer notifies the browser about the properties of its surfaces, so given a LocalSurfaceId the browser knows whether the renderer is at top or not. When the browser requests readback for the renderer, in addition to the bitmap it'll receive the LocalSurfaceId at the time that the snapshot was taken. The browser can figure out at_top based on the LocalSurfaceId.

cc'ing Fady

The cool thing about trying this approach is it lets us validate child allocated IDs while scrolling now in parallel with some of the work cblume@ is doing.

Just getting the bitmap and a bool-flag seems much simpler than having to allocate LocalSurfaceId for scroll + remember the LocalSurfaceIds in the browser though?

Allocating a LocalSurfaceId must happen anyway for top bar control scrolling and text selection handles on Android. The code needs to get written regardless. I'm OK with doing the easy thing first, but I strongly support working towards the other solution as well.

Sure. But that feels more like a different android-specific issue, so probably should happen in a separate bug?

The top-bar control scrolling can happen even when the page is scrolled half-way, e.g. you swipe-up to scroll down a fair bit, then lift, then swipe-down to scroll up. the top-bar control immediately scrolls down, even if the page scroll-offset does not initially change (and then goes from non-top to still non-top). This seems fairly different from knowing whether the page was scrolled at the top when a screenshot/thumbnail is taken.

It just feels wrong to have a special case for browser in Viz. 

Knowing whether a screenshot/thumbnail was taken while the page was scrolled at top, vs. top-bar scrolling/text-selection handles sync on android, seem like two different problems. And having two different fixes for two different problems is not special-casing.

Right now we capture thumbnails and then throw them away if they're not at the top of the page, whereas with the SurfaceId approach, we only make a copyrequest if the frame is at the top of the page. I think the SurfaceId approach is likely to be more efficient.

I'd like to consult with gklassen@ and/or anicolao@ on this bug w.r.t. priorities. I think validating child-allocated surface sync across two threads (main and impl) alone makes this valuable to do.

> Right now we capture thumbnails and then throw them away if
> they're not at the top of the page, ...

What makes you think we do that, because from reading the code [1], I don't think we do that (I mean, if that's what we were doing, then we wouldn't just try to get the thumbnail at all?).

[1] https://cs.chromium.org/chromium/src/chrome/browser/thumbnails/thumbnail_tab_helper.cc?l=293

OK, having not read the code in a long time, I think I have confused myself between the proposal and the current state of the code. The proposal through Viz would involve throwing away screenshots, right? I think because the display compositor is on the same thread as the browser then we do the right thing on capture but the proposal is suggesting we capture and throw away captured things that don't have the top bit?

#22: I'm not sure what you mean. If I understood correctly your proposal is that Viz sends the at_top bit along with the bitmap. This is a special-casing for browser, because no other client needs it. I was under the impression that Viz doesn't care and is not supposed to know about scrolling in clients, and root_scroll_offset in CompositorFrameMetadata is going away?

/cc+ rjkroege@

#26: Sending at_top in response to the screenshot/thumbnail-request is one option, yes (others being renderer notifying the browser when scrolled-at-top changes, or maybe even browser asking renderer about it when it asks to get the thumbnail). Taking screenshot/thumbnail right now is in itself a special-case for browser I think? So it would make sense for the API to meet its needs? Even if there are other users, it's probably not a lot of effort maintaining |root_scroll_offset|? If it turns out to be, then sure, we can explore other options (stated above, or new ones).

#25: I am not sure I understand what you are saying/asking. None of the proposals that I have seen include throwing away the captured thumbnail. The 'at top' bit is used to score the thumbnails [1], where we treat at-top thumbnail as being better than not-at-top thumbnail.

[1] https://cs.chromium.org/chromium/src/components/history/core/common/thumbnail_score.cc?l=22

#27: Ability to take screenshot is a privileged API but I wouldn't call it special-casing for browser. root_scroll_offset is not hard to maintain; I'm just concerned about whether it belongs in Viz or not.

#25: The thumbnailing code already throws away the captured thumbnail if it's not better than the last one. Sadrul's proposal doesn't make it better or worse. 

Not sure if it's in the scope of this bug or not, but just wanted to mention that CompositorFrameMetadata::root_scroll_offset is also used on Android for overscroll effects (pull-to-refresh and overscroll glow):

https://cs.chromium.org/chromium/src/content/browser/android/overscroll_controller_android.cc?rcl=92c106628b0058fd14b1f5b6fc937b80ab49a7dd&l=307

Maybe instead of having an at_top bit, we can have a CapturedFrameMetadata that includes information about the captured frame (currently only scroll offset)? This will allow us to add more information if we need to, and feels less hacky.

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

commit 0d291cb42d9784a8c329b960d3017bd583f9e98b
Author: yiyix <yiyix@chromium.org>
Date: Wed Mar 28 19:30:59 2018

Adding bool |scroll_offset_at_top| to RenderFrameMetadata

GetLastScrollOffset() is updated to return whether the scroll
offset is equal to 0, i.e., whether the thumb of the scroll bar
is at top. This is because the only usage of GetLastScrollOffset
is in thumbnailing, and it's used to check if the scroll
offset is 0.

Added bool |scroll_offset_at_top| to RenderFrameMetadata. Since
|scroll_offset_at_top| does not change with every frame, it's added
to low frequency fields.

Bug:771360

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: If90b776905b7b7cb65b96faca6deb78a46a399fb
Reviewed-on: https://chromium-review.googlesource.com/980704
Commit-Queue: Yi Xu <yiyix@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546565}
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/cc/trees/render_frame_metadata.cc
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/cc/trees/render_frame_metadata.h
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/chrome/browser/thumbnails/thumbnail_tab_helper.cc
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/browser/renderer_host/render_widget_host_view_aura.h
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/browser/renderer_host/render_widget_host_view_base.cc
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/browser/renderer_host/render_widget_host_view_base.h
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/browser/renderer_host/render_widget_host_view_browsertest.cc
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/browser/renderer_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/browser/renderer_host/render_widget_host_view_child_frame.h
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/browser/renderer_host/render_widget_host_view_mac.h
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/browser/site_per_process_hit_test_browsertest.cc
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/common/render_frame_metadata.mojom
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/common/render_frame_metadata_struct_traits.cc
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/common/render_frame_metadata_struct_traits.h
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/public/browser/render_widget_host_view.h
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/test/test_render_view_host.cc
[modify] https://crrev.com/0d291cb42d9784a8c329b960d3017bd583f9e98b/content/test/test_render_view_host.h