Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2268323003:
Initialize RWHV with default bounds rather than only size (Closed)
|
Created:
4 years, 4 months ago by Yusuf Modified:
4 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, yusukes+watch_chromium.org, nona+watch_chromium.org, tburkard+watch_chromium.org, shuchen+watch_chromium.org, jam, gavinp+prer_chromium.org, lizeb+watch-custom-tabs_chromium.org, darin-cc_chromium.org, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitialize RWHV with default bounds rather than only size
This starts sending bounds information from exteral prerenders
and uses that to store a default_bounds_ rectangle in RWHV Android
rather than a size. This basically updates the
default size to also take into account the top controls offset needed in the physical backing size.
With this we can get the page composited properly during prerendering.
Note that this doesn't resolve the bug yet, since we still need an
abstraction layer (possibly a NativeView) that will hold this
default_bounds_ information.
BUG=628302
Committed: https://crrev.com/8c230b2f62cd2e5f83983047f6ba56ecc9a024aa
Cr-Commit-Position: refs/heads/master@{#415112}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fixed brackets #
Total comments: 2
Patch Set 3 : Nit #
Total comments: 2
Patch Set 4 : nit #
Total comments: 2
Messages
Total messages: 34 (17 generated)
yusufo@chromium.org changed reviewers: + sievers@chromium.org
sievers@ for RWHVA changes. I want to get an OK from you, before I send this out to owners. This basically starts plumbing the right size info to RWHVA. The next patch will add the abstraction layer.
The CQ bit was checked by yusufo@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
You might want to update the description to say that this basically updates the default size to also take into account the top controls offset needed in the physical backing size. https://codereview.chromium.org/2268323003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2268323003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:387: // Ignore the given size as only the Java code has the power to Hmm looks like we don't implement this function correctly and don't call WasResized(). I wonder why. https://codereview.chromium.org/2268323003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:568: if (!content_view_core_) missing braces
Description was changed from ========== Initialize RWHV with default bounds rather than only size This starts sending bounds information from exteral prerenders and uses that to store a default_bounds_ rectangle in RWHV Android rather than a size. This way physical backing size is also initialized and we can get the page composited properly during prerendering Note that this doesn't resolve the bug yet, since we still need an abstraction layer (possibly a NativeView) that will hold this default_bounds_ information. BUG=628302 ========== to ========== Initialize RWHV with default bounds rather than only size This starts sending bounds information from exteral prerenders and uses that to store a default_bounds_ rectangle in RWHV Android rather than a size. This basically updates the default size to also take into account the top controls offset needed in the physical backing size. With this we can get the page composited properly during prerendering. Note that this doesn't resolve the bug yet, since we still need an abstraction layer (possibly a NativeView) that will hold this default_bounds_ information. BUG=628302 ==========
Description was changed from ========== Initialize RWHV with default bounds rather than only size This starts sending bounds information from exteral prerenders and uses that to store a default_bounds_ rectangle in RWHV Android rather than a size. This basically updates the default size to also take into account the top controls offset needed in the physical backing size. With this we can get the page composited properly during prerendering. Note that this doesn't resolve the bug yet, since we still need an abstraction layer (possibly a NativeView) that will hold this default_bounds_ information. BUG=628302 ========== to ========== Initialize RWHV with default bounds rather than only size This starts sending bounds information from exteral prerenders and uses that to store a default_bounds_ rectangle in RWHV Android rather than a size. This basically updates the default size to also take into account the top controls offset needed in the physical backing size. With this we can get the page composited properly during prerendering. Note that this doesn't resolve the bug yet, since we still need an abstraction layer (possibly a NativeView) that will hold this default_bounds_ information. BUG=628302 ==========
https://codereview.chromium.org/2268323003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2268323003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:387: // Ignore the given size as only the Java code has the power to On 2016/08/23 21:24:39, sievers wrote: > Hmm looks like we don't implement this function correctly and don't call > WasResized(). I wonder why. I think until now, we pretty much ignored any setSize or setBounds calls that is originating from native since CVC was the only source of truth.
On 2016/08/23 21:30:48, Yusuf wrote: > https://codereview.chromium.org/2268323003/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://codereview.chromium.org/2268323003/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/render_widget_host_view_android.cc:387: // Ignore > the given size as only the Java code has the power to > On 2016/08/23 21:24:39, sievers wrote: > > Hmm looks like we don't implement this function correctly and don't call > > WasResized(). I wonder why. > > I think until now, we pretty much ignored any setSize or setBounds calls that is > originating from native since CVC was the only source of truth. Well, that's not true since you are using this API to override the size. We just do it in an odd way, and it's not obvious why, which is that we *save* the size and then we only use it to respond with it to certain queries while we are detached from the CVC. While on desktop this simply resizes the contents.
On 2016/08/23 21:44:22, sievers wrote: > On 2016/08/23 21:30:48, Yusuf wrote: > > > https://codereview.chromium.org/2268323003/diff/1/content/browser/renderer_ho... > > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > > > > https://codereview.chromium.org/2268323003/diff/1/content/browser/renderer_ho... > > content/browser/renderer_host/render_widget_host_view_android.cc:387: // > Ignore > > the given size as only the Java code has the power to > > On 2016/08/23 21:24:39, sievers wrote: > > > Hmm looks like we don't implement this function correctly and don't call > > > WasResized(). I wonder why. > > > > I think until now, we pretty much ignored any setSize or setBounds calls that > is > > originating from native since CVC was the only source of truth. > > Well, that's not true since you are using this API to override the size. We just > do it in an odd way, and it's not obvious why, which is that we *save* the size > and then we only use it to respond with it to certain queries while we are > detached from the CVC. While on desktop this simply resizes the contents. Yeah, that is right, since even before my changes default_size_ update was doing "something" in the end. So are you saying SetSize and SetBounds should call WasResized now and make it explicit?
On 2016/08/23 21:52:44, Yusuf wrote: > On 2016/08/23 21:44:22, sievers wrote: > > On 2016/08/23 21:30:48, Yusuf wrote: > > > > > > https://codereview.chromium.org/2268323003/diff/1/content/browser/renderer_ho... > > > File content/browser/renderer_host/render_widget_host_view_android.cc > (right): > > > > > > > > > https://codereview.chromium.org/2268323003/diff/1/content/browser/renderer_ho... > > > content/browser/renderer_host/render_widget_host_view_android.cc:387: // > > Ignore > > > the given size as only the Java code has the power to > > > On 2016/08/23 21:24:39, sievers wrote: > > > > Hmm looks like we don't implement this function correctly and don't call > > > > WasResized(). I wonder why. > > > > > > I think until now, we pretty much ignored any setSize or setBounds calls > that > > is > > > originating from native since CVC was the only source of truth. > > > > Well, that's not true since you are using this API to override the size. We > just > > do it in an odd way, and it's not obvious why, which is that we *save* the > size > > and then we only use it to respond with it to certain queries while we are > > detached from the CVC. While on desktop this simply resizes the contents. > > Yeah, that is right, since even before my changes default_size_ update was doing > "something" > in the end. So are you saying SetSize and SetBounds should call WasResized now > and make > it explicit? We might want to give it a shot. But also probably doesn't belong in this patch.
On 2016/08/23 21:57:37, sievers wrote: > On 2016/08/23 21:52:44, Yusuf wrote: > > On 2016/08/23 21:44:22, sievers wrote: > > > On 2016/08/23 21:30:48, Yusuf wrote: > > > > > > > > > > https://codereview.chromium.org/2268323003/diff/1/content/browser/renderer_ho... > > > > File content/browser/renderer_host/render_widget_host_view_android.cc > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2268323003/diff/1/content/browser/renderer_ho... > > > > content/browser/renderer_host/render_widget_host_view_android.cc:387: // > > > Ignore > > > > the given size as only the Java code has the power to > > > > On 2016/08/23 21:24:39, sievers wrote: > > > > > Hmm looks like we don't implement this function correctly and don't call > > > > > WasResized(). I wonder why. > > > > > > > > I think until now, we pretty much ignored any setSize or setBounds calls > > that > > > is > > > > originating from native since CVC was the only source of truth. > > > > > > Well, that's not true since you are using this API to override the size. We > > just > > > do it in an odd way, and it's not obvious why, which is that we *save* the > > size > > > and then we only use it to respond with it to certain queries while we are > > > detached from the CVC. While on desktop this simply resizes the contents. > > > > Yeah, that is right, since even before my changes default_size_ update was > doing > > "something" > > in the end. So are you saying SetSize and SetBounds should call WasResized now > > and make > > it explicit? > > We might want to give it a shot. But also probably doesn't belong in this patch. So I think the bots were unhappy because I had missed brackets. This patch is good to go AFAIC :). sievers@ let me know what you think.
The CQ bit was checked by yusufo@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...
lgtm w/nit https://codereview.chromium.org/2268323003/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2268323003/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:569: if (default_bounds_.IsEmpty()) return gfx::Size(); nit: newline before 'return'
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yusufo@chromium.org changed reviewers: + mariakhomenko@chromium.org, sky@chromium.org
sky@ for web_contents_sizer.cc mariakhomenko@ for other android changes This one actually starts using the bounds info from the previous patch in https://codereview.chromium.org/2259533003. Sorry for the separate patches with little change. In hindsight I should have probably merged these into one. But wanted to really keep the first one as no-op since it was on all platforms. https://codereview.chromium.org/2268323003/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2268323003/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:569: if (default_bounds_.IsEmpty()) return gfx::Size(); On 2016/08/26 21:35:02, sievers wrote: > nit: newline before 'return' Done.
LGTM
lgtm https://codereview.chromium.org/2268323003/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandler.java (right): https://codereview.chromium.org/2268323003/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandler.java:144: screenBounds.top = (int) Math.ceil(screenBounds.top / density); I think you should do left here as well. At some point it could become a calculation above and then they are likely to forget the conversion here.
https://codereview.chromium.org/2268323003/diff/30001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandler.java (right): https://codereview.chromium.org/2268323003/diff/30001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/prerender/ExternalPrerenderHandler.java:144: screenBounds.top = (int) Math.ceil(screenBounds.top / density); On 2016/08/29 16:41:15, Maria wrote: > I think you should do left here as well. At some point it could become a > calculation above and then they are likely to forget the conversion here. Done.
The CQ bit was checked by yusufo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org, mariakhomenko@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2268323003/#ps50001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Initialize RWHV with default bounds rather than only size This starts sending bounds information from exteral prerenders and uses that to store a default_bounds_ rectangle in RWHV Android rather than a size. This basically updates the default size to also take into account the top controls offset needed in the physical backing size. With this we can get the page composited properly during prerendering. Note that this doesn't resolve the bug yet, since we still need an abstraction layer (possibly a NativeView) that will hold this default_bounds_ information. BUG=628302 ========== to ========== Initialize RWHV with default bounds rather than only size This starts sending bounds information from exteral prerenders and uses that to store a default_bounds_ rectangle in RWHV Android rather than a size. This basically updates the default size to also take into account the top controls offset needed in the physical backing size. With this we can get the page composited properly during prerendering. Note that this doesn't resolve the bug yet, since we still need an abstraction layer (possibly a NativeView) that will hold this default_bounds_ information. BUG=628302 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:50001)
Message was sent while issue was closed.
Description was changed from ========== Initialize RWHV with default bounds rather than only size This starts sending bounds information from exteral prerenders and uses that to store a default_bounds_ rectangle in RWHV Android rather than a size. This basically updates the default size to also take into account the top controls offset needed in the physical backing size. With this we can get the page composited properly during prerendering. Note that this doesn't resolve the bug yet, since we still need an abstraction layer (possibly a NativeView) that will hold this default_bounds_ information. BUG=628302 ========== to ========== Initialize RWHV with default bounds rather than only size This starts sending bounds information from exteral prerenders and uses that to store a default_bounds_ rectangle in RWHV Android rather than a size. This basically updates the default size to also take into account the top controls offset needed in the physical backing size. With this we can get the page composited properly during prerendering. Note that this doesn't resolve the bug yet, since we still need an abstraction layer (possibly a NativeView) that will hold this default_bounds_ information. BUG=628302 Committed: https://crrev.com/8c230b2f62cd2e5f83983047f6ba56ecc9a024aa Cr-Commit-Position: refs/heads/master@{#415112} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8c230b2f62cd2e5f83983047f6ba56ecc9a024aa Cr-Commit-Position: refs/heads/master@{#415112}
Message was sent while issue was closed.
twellington@chromium.org changed reviewers: + twellington@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2268323003/diff/50001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (left): https://codereview.chromium.org/2268323003/diff/50001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:257: android:exported="false" /> This activity should not be exported because it allows incognito tabs to be opened (non-exported for security). Why was this changed?
Message was sent while issue was closed.
https://codereview.chromium.org/2268323003/diff/50001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (left): https://codereview.chromium.org/2268323003/diff/50001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:257: android:exported="false" /> On 2016/09/08 23:14:37, Theresa Wellington wrote: > This activity should not be exported because it allows incognito tabs to be > opened (non-exported for security). Why was this changed? Looks accidental (possibly testing things for me)? I'm going to change it back if there are no objections. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
