Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2219823002:
Move content offset to ViewAndroid (Closed)
|
Created:
4 years, 4 months ago by Jinsuk Kim Modified:
3 years, 11 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove content offset to ViewAndroid
Content offset is specific to a given web contents/render widget.
This CL moves the offset info from WindwoAndroid to ViewAndroid,
and it is used to get the correct bounds for snapshot API.
BUG=617750
Review-Url: https://codereview.chromium.org/2219823002
Cr-Commit-Position: refs/heads/master@{#442442}
Committed: https://chromium.googlesource.com/chromium/src/+/8f74b8e7c03b8b37f1b2cfcc6a28a9b639e3822b
Patch Set 1 #
Total comments: 7
Patch Set 2 : pass right bounds to snapshot api #
Total comments: 3
Patch Set 3 : offset in RWH #
Total comments: 2
Patch Set 4 : updated comments #
Messages
Total messages: 50 (32 generated)
The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jinsukkim@chromium.org changed reviewers: + aelias@chromium.org, sievers@chromium.org, twellington@chromium.org
Removing one TODO at a time... sievers@chromium.org: Please review changes in content/ twellington@chromium.org: Please review changes in ui/android/ aelias@chromium.org: Please review changes in ui/snapshot/
> This still works for WindowAndroid since it inherits ViewAndroid. Except for that nobody sets it correctly for the WindowAndroid :) For now this seems an improvement since ViewAndroid is a better place than WindowAndroid to store the content offset (and multiple Views might clobber the current offset on the Window in the current code which is a theoretical bug). Eventually I think we can remove the notion of |content_offset| from the ViewAndroid and just give it a bounds rect that includes the content offset. See comment in snapshot_android.cc. I think if you fix that call site then this will work better than before, and we can also just live with the fact that with your patch the WindowAndroid's content_offset() will always be zero. https://codereview.chromium.org/2219823002/diff/1/ui/snapshot/snapshot_androi... File ui/snapshot/snapshot_android.cc (right): https://codereview.chromium.org/2219823002/diff/1/ui/snapshot/snapshot_androi... ui/snapshot/snapshot_android.cc:27: return GrabWindowSnapshot( You can actually fix this too here and call GrabViewSnapshotAsync() passing |view| which will then use the correct offset for that view.
https://codereview.chromium.org/2219823002/diff/1/content/browser/android/con... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2219823002/diff/1/content/browser/android/con... content/browser/android/content_view_core_impl.cc:432: view_.set_content_offset(gfx::ScaleVector2d(content_offset, dpi_scale_)); Actually with your change this can now be done in render_widget_host_view_android.cc instead.
https://codereview.chromium.org/2219823002/diff/1/ui/snapshot/snapshot_androi... File ui/snapshot/snapshot_android.cc (left): https://codereview.chromium.org/2219823002/diff/1/ui/snapshot/snapshot_androi... ui/snapshot/snapshot_android.cc:55: source_rect_in_pixel.y() + offset.y())), Argh, this code makes no sense. The caller passes in a rect. Then we somehow adjust it for an offset by *translating* it (and keeping the size)? Ok so let's look at the call sites which we support: 1) For devtools/telemetry/tracing from RenderWidgetHostImpl, see GrabViewSnapshot*() call sites. This queries the RWHV for the bounds and passes that in. Looking at the code this already ends up accounting for the the top control heights in ContentViewCoreImpl::GetViewSize(). So looks like this is currently broken. 2) chrome/browser/android/feedback/screenshot_task.cc This grabs a whole window screenshots ('createCompositorScreenShot') and queries the Android root view/window for the total size that it passes in. It would then shift the rect down out of bounds which doesn't make sense. So also broken. That being said, we should probably remove View/WindowAndroid::content_offset() instead. Looks like the call sites should (and would already) end up passing in the correct rect. (And only call site 1) really has a notion of a content offset.) I think you can test both call sites. 1) through tracing with frame tracing/grabbing enabled, and 2) through the feedback reporter.
https://codereview.chromium.org/2219823002/diff/1/ui/snapshot/snapshot_androi... File ui/snapshot/snapshot_android.cc (left): https://codereview.chromium.org/2219823002/diff/1/ui/snapshot/snapshot_androi... ui/snapshot/snapshot_android.cc:55: source_rect_in_pixel.y() + offset.y())), On 2016/08/05 19:32:12, sievers wrote: > Argh, this code makes no sense. > > The caller passes in a rect. Then we somehow adjust it for an offset by > *translating* it (and keeping the size)? > > Ok so let's look at the call sites which we support: > > 1) For devtools/telemetry/tracing from RenderWidgetHostImpl, see > GrabViewSnapshot*() call sites. > > This queries the RWHV for the bounds and passes that in. > Looking at the code this already ends up accounting for the the top control > heights in ContentViewCoreImpl::GetViewSize(). So looks like this is currently > broken. > > 2) chrome/browser/android/feedback/screenshot_task.cc > > This grabs a whole window screenshots ('createCompositorScreenShot') and queries > the Android root view/window for the total size that it passes in. It would then > shift the rect down out of bounds which doesn't make sense. So also broken. > > That being said, we should probably remove View/WindowAndroid::content_offset() > instead. Looks like the call sites should (and would already) end up passing in > the correct rect. (And only call site 1) really has a notion of a content > offset.) > > I think you can test both call sites. 1) through tracing with frame > tracing/grabbing enabled, and 2) through the feedback reporter. Ah sorry, I was slightly off: gfx::Rect RenderWidgetHostViewAndroid::GetViewBounds() const { if (!content_view_core_) return gfx::Rect(default_size_); if (base::CommandLine::ForCurrentProcess()->HasSwitch( switches::kEnableOSKOverscroll)) return gfx::Rect(content_view_core_->GetViewSizeWithOSKHidden()); return gfx::Rect(content_view_core_->GetViewSize()); } So content_view_core_->GetViewSize() shrinks for the size but then converts this to a rect with offset (0,0). I think this option makes the least sense (vs. either including the omnibox and saying the origin is 0, or not including it and saying the content is translated). We might want to attempt to change it but have to be a bit careful and look at all the call sites to see what it might break.
The CQ bit was checked by jinsukkim@chromium.org
The CQ bit was unchecked by jinsukkim@chromium.org
The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/05 19:32:12, no sievers wrote: > https://codereview.chromium.org/2219823002/diff/1/ui/snapshot/snapshot_androi... > File ui/snapshot/snapshot_android.cc (left): > > https://codereview.chromium.org/2219823002/diff/1/ui/snapshot/snapshot_androi... > ui/snapshot/snapshot_android.cc:55: source_rect_in_pixel.y() + offset.y())), > Argh, this code makes no sense. > > The caller passes in a rect. Then we somehow adjust it for an offset by > *translating* it (and keeping the size)? > > Ok so let's look at the call sites which we support: > > 1) For devtools/telemetry/tracing from RenderWidgetHostImpl, see > GrabViewSnapshot*() call sites. > > This queries the RWHV for the bounds and passes that in. > Looking at the code this already ends up accounting for the the top control > heights in ContentViewCoreImpl::GetViewSize(). So looks like this is currently > broken. > > 2) chrome/browser/android/feedback/screenshot_task.cc > > This grabs a whole window screenshots ('createCompositorScreenShot') and queries > the Android root view/window for the total size that it passes in. It would then > shift the rect down out of bounds which doesn't make sense. So also broken. > > That being said, we should probably remove View/WindowAndroid::content_offset() > instead. Looks like the call sites should (and would already) end up passing in > the correct rect. (And only call site 1) really has a notion of a content > offset.) > > I think you can test both call sites. 1) through tracing with frame > tracing/grabbing enabled, and 2) through the feedback reporter. Updated RWHVA::GetViewBounds() to return bounds with correct offset/size. Snapshot API doesn't need to adjust offset but simply uses the passed bounds. This helps fix a bug of snapshot of window not including omnibox (see https://bugs.chromium.org/p/chromium/issues/detail?id=419514#c16). Tested both call sites and verified the results are as expected. I didn't remove content_offset like suggested above but still keep in ViewAndroid to use it in computing the right bounds.
jinsukkim@chromium.org changed reviewers: + boliu@chromium.org - aelias@chromium.org, sievers@chromium.org, twellington@chromium.org
Bo, could you take a look? https://codereview.chromium.org/2219823002/diff/1/content/browser/android/con... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2219823002/diff/1/content/browser/android/con... content/browser/android/content_view_core_impl.cc:432: view_.set_content_offset(gfx::ScaleVector2d(content_offset, dpi_scale_)); On 2016/08/05 18:22:44, no sievers wrote: > Actually with your change this can now be done in > render_widget_host_view_android.cc instead. Done. BTW I'm not clear about what to do with the content offset of this view(owned by WebContentsViewAndroid). Shouldn't this be updated in sync with RWVHA's view content offset? https://codereview.chromium.org/2219823002/diff/1/ui/snapshot/snapshot_androi... File ui/snapshot/snapshot_android.cc (right): https://codereview.chromium.org/2219823002/diff/1/ui/snapshot/snapshot_androi... ui/snapshot/snapshot_android.cc:27: return GrabWindowSnapshot( On 2016/08/05 18:15:14, no sievers wrote: > You can actually fix this too here and call GrabViewSnapshotAsync() passing > |view| which will then use the correct offset for that view. This is for synchronous version which is not preferred on Android as per the comment below. Moved it above these two methods to make it more visible.
Description was changed from ========== Move content offset to ViewAndroid Content offset is specific to a given web contents/render widget. This still works for WindowAndroid since it inherits ViewAndroid. BUG=617750 ========== to ========== Move content offset to ViewAndroid Content offset is specific to a given web contents/render widget. This CL moves the offset info from WindwoAndroid to ViewAndroid, and it is used to get the correct bounds for snapshot API. BUG=617750 ==========
https://codereview.chromium.org/2219823002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2219823002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2351: gfx::Rect snapshot_bounds(GetView()->GetViewBounds()); Feels like GrabViewSnapshotAsync should grab the offset directly from ViewAndroid, then this code doesn't need to be specialized. Also RWHVA::GetViewBounds don't need to change either I think?
https://codereview.chromium.org/2219823002/diff/1/content/browser/android/con... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2219823002/diff/1/content/browser/android/con... content/browser/android/content_view_core_impl.cc:432: view_.set_content_offset(gfx::ScaleVector2d(content_offset, dpi_scale_)); On 2016/12/22 12:35:48, Jinsuk Kim wrote: > On 2016/08/05 18:22:44, no sievers wrote: > > Actually with your change this can now be done in > > render_widget_host_view_android.cc instead. > > Done. BTW I'm not clear about what to do with the content offset of this > view(owned by WebContentsViewAndroid). Shouldn't this be updated in sync with > RWVHA's view content offset? Hmm, debatable. Ideally, the offset should be on RWHVA only, not WCVA. WCVA should not contribute to the rendering output (I think?), so no one should try to read from it should always just delegate to the active RWHVA. The answer will also depend on how chrome top controls works in the new ViewAndroid tree world. I'm not sure what the right answer there is. I think for now, unless there is code that depends on this, let's leave it out of the WCVA?
https://codereview.chromium.org/2219823002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2219823002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2351: gfx::Rect snapshot_bounds(GetView()->GetViewBounds()); On 2016/12/22 21:14:59, boliu wrote: > Feels like GrabViewSnapshotAsync should grab the offset directly from > ViewAndroid, then this code doesn't need to be specialized. Also > RWHVA::GetViewBounds don't need to change either I think? Oh, I see sievers complained about the translation in https://codereview.chromium.org/2219823002/diff/1/ui/snapshot/snapshot_androi... So maybe not do what I said above.. Documentation for GrabViewSnapshotAsync says source_rect is in *layer* space. This means that right now RWHVA::GetViewBounds is wrong. The translation should be moved out of GetViewBounds to here, because we don't want to read the entire layer. Sucks to have platform-specific code here, but I guess that makes sense because only android has top controls. Also did you test all the cases that he mentioned in that comment, and everything still works as expected after this CL?
https://codereview.chromium.org/2219823002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2219823002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2351: gfx::Rect snapshot_bounds(GetView()->GetViewBounds()); On 2016/12/22 21:28:54, boliu_back_jan3 wrote: > On 2016/12/22 21:14:59, boliu wrote: > > Feels like GrabViewSnapshotAsync should grab the offset directly from > > ViewAndroid, then this code doesn't need to be specialized. Also > > RWHVA::GetViewBounds don't need to change either I think? > > Oh, I see sievers complained about the translation in > https://codereview.chromium.org/2219823002/diff/1/ui/snapshot/snapshot_androi... > > So maybe not do what I said above.. > > Documentation for GrabViewSnapshotAsync says source_rect is in *layer* space. > This means that right now RWHVA::GetViewBounds is wrong. The translation should > be moved out of GetViewBounds to here, because we don't want to read the entire > layer. Sucks to have platform-specific code here, but I guess that makes sense > because only android has top controls. I take it that you want the change in RWVHA::GetViewBounds() reverted? > > Also did you test all the cases that he mentioned in that comment, and > everything still works as expected after this CL? Yes, quoting my own comment in case you might have missed it: > This helps fix a bug of snapshot of window not including omnibox > (see https://bugs.chromium.org/p/chromium/issues/detail?id=419514#c16). > Tested both call sites and verified the results are as expected.
lgtm
The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jinsukkim@chromium.org changed reviewers: + aelias@chromium.org, twellington@chromium.org
aelias@chromium.org: Please review changes in ui/snapshot twellington@chromium.org: Please review changes in ui/android
ui/android lgtm https://codereview.chromium.org/2219823002/diff/80001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2219823002/diff/80001/ui/android/view_android... ui/android/view_android.h:65: void set_content_offset(const gfx::Vector2dF& content_offset) { nit: I think mentioning that the content offset is in CSS pixels in this comment makes it easier for clients to know what is expected without reading through the rest of the header file.
ui/snapshot lgtm
https://codereview.chromium.org/2219823002/diff/80001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2219823002/diff/80001/ui/android/view_android... ui/android/view_android.h:65: void set_content_offset(const gfx::Vector2dF& content_offset) { On 2017/01/09 16:03:53, Theresa wrote: > nit: I think mentioning that the content offset is in CSS pixels in this comment > makes it easier for clients to know what is expected without reading through the > rest of the header file. Done.
The CQ bit was checked by jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, aelias@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2219823002/#ps100001 (title: "updated comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484004675791400,
"parent_rev": "0d32da520686f09a43107b63e9ab365fa5af580f", "commit_rev":
"8f74b8e7c03b8b37f1b2cfcc6a28a9b639e3822b"}
Message was sent while issue was closed.
Description was changed from ========== Move content offset to ViewAndroid Content offset is specific to a given web contents/render widget. This CL moves the offset info from WindwoAndroid to ViewAndroid, and it is used to get the correct bounds for snapshot API. BUG=617750 ========== to ========== Move content offset to ViewAndroid Content offset is specific to a given web contents/render widget. This CL moves the offset info from WindwoAndroid to ViewAndroid, and it is used to get the correct bounds for snapshot API. BUG=617750 Review-Url: https://codereview.chromium.org/2219823002 Cr-Commit-Position: refs/heads/master@{#442442} Committed: https://chromium.googlesource.com/chromium/src/+/8f74b8e7c03b8b37f1b2cfcc6a28... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/8f74b8e7c03b8b37f1b2cfcc6a28... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
