Fix Android tab navigation "stretch" regression.

Fix Android tab navigation "stretch" regression.

Fixes a regression in the tab-switching UI caused by
https://codereview.chromium.org/2702093002. The root cause was my
misunderstanding of the difference between RenderWidgtHostViewAndroid's
|current_frame_size_| and the "view size" value returned by
ContentViewCoreImpl.

Note: While this change fixes the regression, there still seems to be a
small (~2 pixel) width-stretching problem (barely noticeable). However,
I confirmed that behavior is the same on current Clank stable; so, this
isn't related to the bug at-hand. Something to look into later...

This change revisits all code paths leading to CopyFromSurface() on
Android, and fixes a few other pre-existing issues:

1. RWHVAndroid::GetScaledContentBitmap(), was substituting
|current_surface_size_| instead of ContentViewCore::GetViewSize() when
the source rect argument is empty. This is actually the same bug that I
fixed above to resolve the regression. ;-)

2. RWHVAndroid::SynchronousCopyContents() does not copy the correct
source region if the rect's origin is not at the point (0,0).

3. The plumbing from CustomTabObserver (Java code) into the eventual
RWHVA::CopyFromSurface() was simplified.

BUG=698157, 698974
TEST=Manually tested on Nexus 6, per bug repro steps.

Review-Url: https://codereview.chromium.org/2734043003
Cr-Commit-Position: refs/heads/master@{#455595}
Committed: https://chromium.googlesource.com/chromium/src/+/caa6a6165d28c3bb499fc0aec9bf391c6bda1578

[miu, 2017-03-07 04:02:50]

Patchset #1 (id:1) has been deleted

[miu, 2017-03-07 04:06:26]

Patchset #1 (id:20001) has been deleted

[miu, 2017-03-07 04:07:29]

Patchset #1 (id:40001) has been deleted

[miu, 2017-03-07 04:09:36]

miu@chromium.org changed reviewers:
+ aelias@chromium.org, yusufo@chromium.org

[miu, 2017-03-07 04:09:36]

aelias: OWNERS RS please.

yusufo: PTAL.

[Yusuf, 2017-03-07 18:55:28]

miu@, although I occasionally need to modify the content/browser/renderer_host/
code for prerender and such (hence my general knowledge on how size vs bounds
etc works here), I can't really claim a complete grasp of all of the general
flow, especially now since there are a few moving parts here.

I will still take a look, but I feel like a renderer_host/ owner giving a proper
look would be ideal here.

aelias, do you mind adding someone from your team for review on this?

[miu, 2017-03-07 20:28:33]

On 2017/03/07 18:55:28, Yusuf wrote:
> miu@, although I occasionally need to modify the
content/browser/renderer_host/
> code for prerender and such (hence my general knowledge on how size vs bounds
> etc works here), I can't really claim a complete grasp of all of the general
> flow, especially now since there are a few moving parts here.
> 
> I will still take a look, but I feel like a renderer_host/ owner giving a
proper
> look would be ideal here.
> 
> aelias, do you mind adding someone from your team for review on this?

yusufo: I put you down as reviewer because you originally wrote the code in
chrome/.../tab_content_manager.*, which is the only caller of
RWHVAndroid::GetScaledContentBitmap(). Otherwise, I myself am one of two main
authors for the CopyFromSurface() code, so I'm pretty confident I understand the
call patterns/semantics here. :) Mainly, if you could sanity-check the size
calculations (and use of GetVisibleViewPortSize() instead of
current_surface_size_), that should be a satisfactory review of the
functionality here.

As always, if there are "more-expert" folks to take a look, that's fine too. You
mentioned there are folks working on getting rid of ContentViewCoreImpl: Perhaps
one of them should take a look at this change?

[aelias_OOO_until_Jul13, 2017-03-07 21:08:01]

https://codereview.chromium.org/2734043003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2734043003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:542: gfx::Rect
src_subrect_in_pixel,
It looks like this is always called with empty rect (callsites start in
ChromeActivity.java and CustomTabObserver.java), so please delete this parameter
throughout the entire call chain and also your else {} block.

https://codereview.chromium.org/2734043003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:953: gfx::Rect
src_subrect_in_pixel = gfx::ConvertRectToPixel(
I don't think making a change here is necessary at all given that you've got the
empty-rect case covered in the higher-level spot GetScaledContentBitmap(),
right?  Could you just not touch this?  I consider doing something totally
different on empty rect to be an antipattern and I'd rather not introduce it
here.

https://codereview.chromium.org/2734043003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1208:
callback.Run(SkBitmap(), READBACK_FAILED);
Hmm, this is just replacing one bug by another from my POV.  I don't think it
would be that hard to apply the appropriate translation on the skia canvas to
make this work, but failing that I'd prefer for you to just add a comment
instead of changing anything (this bug has been present since this function was
written it looks like, no reason to rock the boat).

[miu, 2017-03-08 01:41:18]

Description was changed from

==========
Fix Android tab navigation "stretch" regression.

Fixes a regression in the tab-switching UI caused by
https://codereview.chromium.org/2702093002. The root cause was my
misunderstanding of the difference between RenderWidgtHostViewAndroid's
|current_frame_size_| and the "view size" value returned by
ContentViewCoreImpl.

Note: While this change fixes the regression, there still seems to be a
small (~2 pixel) width-stretching problem (barely noticeable). However,
I confirmed that behavior is the same on current Clank stable; so, this
isn't related to the bug at-hand. Something to look into later...

This change revisits all code paths leading to CopyFromSurface() on
Android, and fixes a couple other pre-existing issues:

1. RWHVAndroid::GetScaledContentBitmap(), was substituting
|current_surface_size_| instead of ContentViewCore::GetViewSize() when
the source rect argument is empty. This is actually the same bug that I
fixed above to resolve the regression. ;-)

2. RWHVAndroid::SynchronousCopyContents() does not copy the correct
source region if the rect's origin is not at the point (0,0).

BUG=698157,698974
TEST=Manually tested on Nexus 6, per bug repro steps.
==========

to

==========
Fix Android tab navigation "stretch" regression.

Fixes a regression in the tab-switching UI caused by
https://codereview.chromium.org/2702093002. The root cause was my
misunderstanding of the difference between RenderWidgtHostViewAndroid's
|current_frame_size_| and the "view size" value returned by
ContentViewCoreImpl.

Note: While this change fixes the regression, there still seems to be a
small (~2 pixel) width-stretching problem (barely noticeable). However,
I confirmed that behavior is the same on current Clank stable; so, this
isn't related to the bug at-hand. Something to look into later...

This change revisits all code paths leading to CopyFromSurface() on
Android, and fixes a couple other pre-existing issues:

1. RWHVAndroid::GetScaledContentBitmap(), was substituting
|current_surface_size_| instead of ContentViewCore::GetViewSize() when
the source rect argument is empty. This is actually the same bug that I
fixed above to resolve the regression. ;-)

2. RWHVAndroid::SynchronousCopyContents() does not copy the correct
source region if the rect's origin is not at the point (0,0).

3. The plumbing from CustomTabObserver (Java code) into the eventual
RWHVA::CopyFromSurface() was simplified.

BUG=698157,698974
TEST=Manually tested on Nexus 6, per bug repro steps.
==========

[miu, 2017-03-08 01:41:29]

Description was changed from

==========
Fix Android tab navigation "stretch" regression.

Fixes a regression in the tab-switching UI caused by
https://codereview.chromium.org/2702093002. The root cause was my
misunderstanding of the difference between RenderWidgtHostViewAndroid's
|current_frame_size_| and the "view size" value returned by
ContentViewCoreImpl.

Note: While this change fixes the regression, there still seems to be a
small (~2 pixel) width-stretching problem (barely noticeable). However,
I confirmed that behavior is the same on current Clank stable; so, this
isn't related to the bug at-hand. Something to look into later...

This change revisits all code paths leading to CopyFromSurface() on
Android, and fixes a couple other pre-existing issues:

1. RWHVAndroid::GetScaledContentBitmap(), was substituting
|current_surface_size_| instead of ContentViewCore::GetViewSize() when
the source rect argument is empty. This is actually the same bug that I
fixed above to resolve the regression. ;-)

2. RWHVAndroid::SynchronousCopyContents() does not copy the correct
source region if the rect's origin is not at the point (0,0).

3. The plumbing from CustomTabObserver (Java code) into the eventual
RWHVA::CopyFromSurface() was simplified.

BUG=698157,698974
TEST=Manually tested on Nexus 6, per bug repro steps.
==========

to

==========
Fix Android tab navigation "stretch" regression.

Fixes a regression in the tab-switching UI caused by
https://codereview.chromium.org/2702093002. The root cause was my
misunderstanding of the difference between RenderWidgtHostViewAndroid's
|current_frame_size_| and the "view size" value returned by
ContentViewCoreImpl.

Note: While this change fixes the regression, there still seems to be a
small (~2 pixel) width-stretching problem (barely noticeable). However,
I confirmed that behavior is the same on current Clank stable; so, this
isn't related to the bug at-hand. Something to look into later...

This change revisits all code paths leading to CopyFromSurface() on
Android, and fixes a few other pre-existing issues:

1. RWHVAndroid::GetScaledContentBitmap(), was substituting
|current_surface_size_| instead of ContentViewCore::GetViewSize() when
the source rect argument is empty. This is actually the same bug that I
fixed above to resolve the regression. ;-)

2. RWHVAndroid::SynchronousCopyContents() does not copy the correct
source region if the rect's origin is not at the point (0,0).

3. The plumbing from CustomTabObserver (Java code) into the eventual
RWHVA::CopyFromSurface() was simplified.

BUG=698157,698974
TEST=Manually tested on Nexus 6, per bug repro steps.
==========

[miu, 2017-03-08 01:42:04]

https://codereview.chromium.org/2734043003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2734043003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:542: gfx::Rect
src_subrect_in_pixel,
On 2017/03/07 21:08:01, aelias wrote:
> It looks like this is always called with empty rect (callsites start in
> ChromeActivity.java and CustomTabObserver.java), so please delete this
parameter
> throughout the entire call chain and also your else {} block.

Yeah, I noticed this. Wasn't going to expand the scope of this change, but I'm
always a fan of reducing entropy in the codebase. ;-)

Done. In fact, I was able to delete this method altogether and have
WebContentsAndroid call CopyFromSurface() directly. I removed the bitmap format
and source rect from all the Java→JNI→C++ method signatures, and instead of a
scale factor, the caller must provide the exact width/height of the resulting
bitmap.

https://codereview.chromium.org/2734043003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:953: gfx::Rect
src_subrect_in_pixel = gfx::ConvertRectToPixel(
On 2017/03/07 21:08:01, aelias wrote:
> I don't think making a change here is necessary at all given that you've got
the
> empty-rect case covered in the higher-level spot GetScaledContentBitmap(),
> right?  Could you just not touch this?  I consider doing something totally
> different on empty rect to be an antipattern and I'd rather not introduce it
> here.

Actually, this is necessary. This method is the Android impl for the public
RenderWidgetHostView::CopyFromSurface() API. It is very common to want to
request "the whole content region" and many call points throughout Chromium
leverage that aspect of the public API.

Ref:
https://cs.chromium.org/chromium/src/content/public/browser/render_widget_hos...

https://codereview.chromium.org/2734043003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1208:
callback.Run(SkBitmap(), READBACK_FAILED);
On 2017/03/07 21:08:01, aelias wrote:
> Hmm, this is just replacing one bug by another from my POV.  I don't think it
> would be that hard to apply the appropriate translation on the skia canvas to
> make this work, but failing that I'd prefer for you to just add a comment
> instead of changing anything (this bug has been present since this function
was
> written it looks like, no reason to rock the boat).

Done.

[miu, 2017-03-08 01:43:10]

miu@chromium.org changed reviewers:
+ tedchoc@chromium.org

[miu, 2017-03-08 01:43:10]

tedchoc: Need OWNERS approval for:
 chrome/android/.../ChromeActivity.java
 content/browser/web_contents/web_contents_android.*

[miu, 2017-03-08 01:43:21]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 01:44:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aelias_OOO_until_Jul13, 2017-03-08 02:22:03]

renderer_host/ lgtm

https://codereview.chromium.org/2734043003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabObserver.java
(right):

https://codereview.chromium.org/2734043003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabObserver.java:67:
Math.min(desiredWidth / bounds.width(), desiredHeight / bounds.height());
Yusuf -- is this logic useful anymore?  The goal of it seems to be to get a
bitmap fitting in desiredWidth anyway.  Now that we're passing in the
width/height directly, can we just always  "mContentBitmapWidth = desiredWidth;
mContentBitmapHeight = 
 desiredHeight;" and avoid the complexity?

[commit-bot: I haz the power, 2017-03-08 02:24:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 02:24:22]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-03-08 18:59:27]

lgtm

https://codereview.chromium.org/2734043003/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(left):

https://codereview.chromium.org/2734043003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1267:
webContents.getContentBitmapAsync(Bitmap.Config.ARGB_8888, 1.f, EMPTY_RECT,
callback);
Can you remove EMPTY_RECT as it looks to only be used here.

https://codereview.chromium.org/2734043003/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/2734043003/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:345:
public void getContentBitmapAsync(int width, int height, ContentBitmapCallback
callback);
Can you add a comment here?  In particular, ChromeTabbedActivity passes 0 here,
which I assume means just use the natural dimens instead of any scaled version.

[Yusuf, 2017-03-08 19:08:21]

https://codereview.chromium.org/2734043003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabObserver.java
(right):

https://codereview.chromium.org/2734043003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabObserver.java:67:
Math.min(desiredWidth / bounds.width(), desiredHeight / bounds.height());
On 2017/03/08 02:22:03, aelias wrote:
> Yusuf -- is this logic useful anymore?  The goal of it seems to be to get a
> bitmap fitting in desiredWidth anyway.  Now that we're passing in the
> width/height directly, can we just always  "mContentBitmapWidth =
desiredWidth;
> mContentBitmapHeight = 
>  desiredHeight;" and avoid the complexity?

Yes, I think we can get rid of the logic above.

[miu, 2017-03-08 21:01:39]

yusufo: PTAL at my response regarding the "desired width/height" code in
CustomTabObserver.java. LGTY?

https://codereview.chromium.org/2734043003/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(left):

https://codereview.chromium.org/2734043003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1267:
webContents.getContentBitmapAsync(Bitmap.Config.ARGB_8888, 1.f, EMPTY_RECT,
callback);
On 2017/03/08 18:59:26, Ted C wrote:
> Can you remove EMPTY_RECT as it looks to only be used here.

Done (as of PS2).

https://codereview.chromium.org/2734043003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabObserver.java
(right):

https://codereview.chromium.org/2734043003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabObserver.java:67:
Math.min(desiredWidth / bounds.width(), desiredHeight / bounds.height());
On 2017/03/08 19:08:21, Yusuf wrote:
> On 2017/03/08 02:22:03, aelias wrote:
> > Yusuf -- is this logic useful anymore?  The goal of it seems to be to get a
> > bitmap fitting in desiredWidth anyway.  Now that we're passing in the
> > width/height directly, can we just always  "mContentBitmapWidth =
> desiredWidth;
> > mContentBitmapHeight = 
> >  desiredHeight;" and avoid the complexity?
> 
> Yes, I think we can get rid of the logic above. 

Actually, there's one subtlely, if IIUC: We can't just use the desiredWidth and
height because we have to preserve the aspect ratio of the content. So, it seems
this is the ideal solution here. I've added a comment to explain this.

I'm not going to remove this logic for now because the change at-hand is a
regression fix, I don't want to take a risk introducing a new regression, and
I'm concerned this change would be getting too large for a merge into M58. :)

If you still think this logic here should change, I can file a crbug for
follow-up.

https://codereview.chromium.org/2734043003/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/2734043003/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:345:
public void getContentBitmapAsync(int width, int height, ContentBitmapCallback
callback);
On 2017/03/08 18:59:26, Ted C wrote:
> Can you add a comment here?  In particular, ChromeTabbedActivity passes 0
here,
> which I assume means just use the natural dimens instead of any scaled
version.

Done.

[Yusuf, 2017-03-08 21:17:25]

https://codereview.chromium.org/2734043003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabObserver.java
(right):

https://codereview.chromium.org/2734043003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabObserver.java:67:
Math.min(desiredWidth / bounds.width(), desiredHeight / bounds.height());
On 2017/03/08 21:01:39, miu wrote:
> On 2017/03/08 19:08:21, Yusuf wrote:
> > On 2017/03/08 02:22:03, aelias wrote:
> > > Yusuf -- is this logic useful anymore?  The goal of it seems to be to get
a
> > > bitmap fitting in desiredWidth anyway.  Now that we're passing in the
> > > width/height directly, can we just always  "mContentBitmapWidth =
> > desiredWidth;
> > > mContentBitmapHeight = 
> > >  desiredHeight;" and avoid the complexity?
> > 
> > Yes, I think we can get rid of the logic above. 
> 
> Actually, there's one subtlely, if IIUC: We can't just use the desiredWidth
and
> height because we have to preserve the aspect ratio of the content. So, it
seems
> this is the ideal solution here. I've added a comment to explain this.
> 
> I'm not going to remove this logic for now because the change at-hand is a
> regression fix, I don't want to take a risk introducing a new regression, and
> I'm concerned this change would be getting too large for a merge into M58. :)
> 
> If you still think this logic here should change, I can file a crbug for
> follow-up.

lgtm, I do think the current values we are getting through estimate.. and the
hardcoded values do align for aspect ratio and I think I was a bit paranoid to
add this check, but I understand the reasoning for not expanding the scope of
the CL.

[miu, 2017-03-08 22:43:04]

The CQ bit was checked by miu@chromium.org

[miu, 2017-03-08 22:43:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, aelias@chromium.org, yusufo@chromium.org
Link to the patchset: https://codereview.chromium.org/2734043003/#ps120001
(title: "REBASE")

[commit-bot: I haz the power, 2017-03-08 22:43:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 23:36:12]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489012984722650,
"parent_rev": "aefa25eb71bb437576f9b0f1b093b9909a211f62", "commit_rev":
"caa6a6165d28c3bb499fc0aec9bf391c6bda1578"}

[commit-bot: I haz the power, 2017-03-08 23:36:53]

Description was changed from

==========
Fix Android tab navigation "stretch" regression.

Fixes a regression in the tab-switching UI caused by
https://codereview.chromium.org/2702093002. The root cause was my
misunderstanding of the difference between RenderWidgtHostViewAndroid's
|current_frame_size_| and the "view size" value returned by
ContentViewCoreImpl.

Note: While this change fixes the regression, there still seems to be a
small (~2 pixel) width-stretching problem (barely noticeable). However,
I confirmed that behavior is the same on current Clank stable; so, this
isn't related to the bug at-hand. Something to look into later...

This change revisits all code paths leading to CopyFromSurface() on
Android, and fixes a few other pre-existing issues:

1. RWHVAndroid::GetScaledContentBitmap(), was substituting
|current_surface_size_| instead of ContentViewCore::GetViewSize() when
the source rect argument is empty. This is actually the same bug that I
fixed above to resolve the regression. ;-)

2. RWHVAndroid::SynchronousCopyContents() does not copy the correct
source region if the rect's origin is not at the point (0,0).

3. The plumbing from CustomTabObserver (Java code) into the eventual
RWHVA::CopyFromSurface() was simplified.

BUG=698157,698974
TEST=Manually tested on Nexus 6, per bug repro steps.
==========

to

==========
Fix Android tab navigation "stretch" regression.

Fixes a regression in the tab-switching UI caused by
https://codereview.chromium.org/2702093002. The root cause was my
misunderstanding of the difference between RenderWidgtHostViewAndroid's
|current_frame_size_| and the "view size" value returned by
ContentViewCoreImpl.

Note: While this change fixes the regression, there still seems to be a
small (~2 pixel) width-stretching problem (barely noticeable). However,
I confirmed that behavior is the same on current Clank stable; so, this
isn't related to the bug at-hand. Something to look into later...

This change revisits all code paths leading to CopyFromSurface() on
Android, and fixes a few other pre-existing issues:

1. RWHVAndroid::GetScaledContentBitmap(), was substituting
|current_surface_size_| instead of ContentViewCore::GetViewSize() when
the source rect argument is empty. This is actually the same bug that I
fixed above to resolve the regression. ;-)

2. RWHVAndroid::SynchronousCopyContents() does not copy the correct
source region if the rect's origin is not at the point (0,0).

3. The plumbing from CustomTabObserver (Java code) into the eventual
RWHVA::CopyFromSurface() was simplified.

BUG=698157,698974
TEST=Manually tested on Nexus 6, per bug repro steps.

Review-Url: https://codereview.chromium.org/2734043003
Cr-Commit-Position: refs/heads/master@{#455595}
Committed:
https://chromium.googlesource.com/chromium/src/+/caa6a6165d28c3bb499fc0aec9bf...
==========

[commit-bot: I haz the power, 2017-03-08 23:36:54]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/caa6a6165d28c3bb499fc0aec9bf...