Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2202123002:
Make WebView respond to device scale change (Closed)
|
Created:
4 years, 4 months ago by Tima Vaisburd Modified:
3 years, 9 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake WebView respond to device scale change
This CL listens to the device scale change in
AwContents.onConfigurationChanged() and updates the device
scale factor in SharedDeviceDisplayInfo, ContentViewCore
and AwContents areas.
BUG=620929
Patch Set 1 #
Total comments: 14
Patch Set 2 : Made WindowAndroid::content_offset() device scale independent, restored AwContents.onSizeChanged(),… #
Total comments: 3
Patch Set 3 : Moved WindowAndroid::content_offset() work out #Patch Set 4 : Rebase only #Patch Set 5 : Combined two setSize methods again with question #
Total comments: 1
Messages
Total messages: 16 (3 generated)
Description was changed from ========== Make WebView respond to device scale change Propagate the new device scale to the DeviceDisplayInfo and ContentViewCore in a synchronized way. BUG=620929 ========== to ========== Make WebView respond to device scale change This CL listens to the device scale change in AwContents.onConfigurationChanged() and updates the device scale factor in SharedDeviceDisplayInfo, ContentViewCore and AwContents areas. BUG=620929 ==========
timav@chromium.org changed reviewers: + boliu@chromium.org, sgurun@chromium.org
I managed to make the scale change work, at least in my limited testing. There are some questions. Please take a look. There are still two authorities that deliver device scale factor: Screen related and ContentViewCore related which is less than ideal. https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (left): https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:3187: mContentViewCore.onPhysicalBackingSizeChanged(w, h); onPhysicalBackingSizeChanges() calls nativeWasResized() that eventually gathers ResizeParams and calls RenderWidget::OnResize(). ContentViewCoreImpl determines the viewport size in dip by getting the pixels size and dividing it by the scale. At the time of this call the pixels size is not yet updated and the ResizeParams have |physical_backing_size| equal to the new size and the |visible_viewport_size| corresponding to the old one. At the next line mContentViewCore.onSizeChanged() calls RenderWidget::OnResize() again, now with correct sizes, but somehow it is too late. https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:712: mContainerView.requestLayout(); Do we need to request layout on the container?
https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (left): https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:3187: mContentViewCore.onPhysicalBackingSizeChanged(w, h); On 2016/08/02 17:27:19, Tima Vaisburd wrote: > onPhysicalBackingSizeChanges() calls nativeWasResized() that eventually gathers > ResizeParams and calls RenderWidget::OnResize(). > ContentViewCoreImpl determines the viewport size in dip by getting the pixels > size and dividing it by the scale. > At the time of this call the pixels size is not yet updated and the ResizeParams > have |physical_backing_size| equal to the new size and the > |visible_viewport_size| corresponding to the old one. > > At the next line mContentViewCore.onSizeChanged() calls > RenderWidget::OnResize() again, now with correct sizes, but somehow it is too > late. what do you mean by "late"? It sounds like you are describing how things work today. why is this a problem with this density change? basically I don't see why these two calls needs to be merged into one (even though I agree it is weird they are separate) https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:712: mContainerView.requestLayout(); On 2016/08/02 17:27:19, Tima Vaisburd wrote: > Do we need to request layout on the container? Don't think so. If the scale ends up needing a redraw, then wait until blink sends the redraw message up through the normal invalidate path. https://codereview.chromium.org/2202123002/diff/1/content/browser/android/con... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2202123002/diff/1/content/browser/android/con... content/browser/android/content_view_core_impl.cc:926: dpi_scale_ = device_scale; this should probably do more than just set the value, what about updating everything that depends on this value? https://codereview.chromium.org/2202123002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2202123002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1658: public void onBackingAndViewportSizeChanged( if the above two methods needs to be merged, then just merge them, and fix all the callsites in clank as well, don't add a new one also I don't quite understand why they need to be merged (see other comment)
https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (left): https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:3187: mContentViewCore.onPhysicalBackingSizeChanged(w, h); On 2016/08/02 18:09:27, boliu wrote: > On 2016/08/02 17:27:19, Tima Vaisburd wrote: > > onPhysicalBackingSizeChanges() calls nativeWasResized() that eventually > gathers > > ResizeParams and calls RenderWidget::OnResize(). > > ContentViewCoreImpl determines the viewport size in dip by getting the pixels > > size and dividing it by the scale. > > At the time of this call the pixels size is not yet updated and the > ResizeParams > > have |physical_backing_size| equal to the new size and the > > |visible_viewport_size| corresponding to the old one. > > > > At the next line mContentViewCore.onSizeChanged() calls > > RenderWidget::OnResize() again, now with correct sizes, but somehow it is too > > late. > > what do you mean by "late"? > > It sounds like you are describing how things work today. why is this a problem > with this density change? basically I don't see why these two calls needs to be > merged into one (even though I agree it is weird they are separate) I tried to explain why I merged these two calls but I did not get to the root cause of the problem, that is, why the second call does not fix the issue.
https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (left): https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:3187: mContentViewCore.onPhysicalBackingSizeChanged(w, h); On 2016/08/02 18:53:27, Tima Vaisburd wrote: > On 2016/08/02 18:09:27, boliu wrote: > > On 2016/08/02 17:27:19, Tima Vaisburd wrote: > > > onPhysicalBackingSizeChanges() calls nativeWasResized() that eventually > > gathers > > > ResizeParams and calls RenderWidget::OnResize(). > > > ContentViewCoreImpl determines the viewport size in dip by getting the > pixels > > > size and dividing it by the scale. > > > At the time of this call the pixels size is not yet updated and the > > ResizeParams > > > have |physical_backing_size| equal to the new size and the > > > |visible_viewport_size| corresponding to the old one. > > > > > > At the next line mContentViewCore.onSizeChanged() calls > > > RenderWidget::OnResize() again, now with correct sizes, but somehow it is > too > > > late. > > > > what do you mean by "late"? > > > > It sounds like you are describing how things work today. why is this a problem > > with this density change? basically I don't see why these two calls needs to > be > > merged into one (even though I agree it is weird they are separate) > > I tried to explain why I merged these two calls but I did not get to the root > cause of the problem, that is, why the second call does not fix the issue. Why would you expect it to "fix" anything? You said the DIPScale hasn't changed in these calls here, right?
On 2016/08/02 19:00:02, boliu wrote: > https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... > File android_webview/java/src/org/chromium/android_webview/AwContents.java > (left): > > https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... > android_webview/java/src/org/chromium/android_webview/AwContents.java:3187: > mContentViewCore.onPhysicalBackingSizeChanged(w, h); > On 2016/08/02 18:53:27, Tima Vaisburd wrote: > > On 2016/08/02 18:09:27, boliu wrote: > > > On 2016/08/02 17:27:19, Tima Vaisburd wrote: > > > > onPhysicalBackingSizeChanges() calls nativeWasResized() that eventually > > > gathers > > > > ResizeParams and calls RenderWidget::OnResize(). > > > > ContentViewCoreImpl determines the viewport size in dip by getting the > > pixels > > > > size and dividing it by the scale. > > > > At the time of this call the pixels size is not yet updated and the > > > ResizeParams > > > > have |physical_backing_size| equal to the new size and the > > > > |visible_viewport_size| corresponding to the old one. > > > > > > > > At the next line mContentViewCore.onSizeChanged() calls > > > > RenderWidget::OnResize() again, now with correct sizes, but somehow it is > > too > > > > late. > > > > > > what do you mean by "late"? > > > > > > It sounds like you are describing how things work today. why is this a > problem > > > with this density change? basically I don't see why these two calls needs to > > be > > > merged into one (even though I agree it is weird they are separate) > > > > I tried to explain why I merged these two calls but I did not get to the root > > cause of the problem, that is, why the second call does not fix the issue. > > Why would you expect it to "fix" anything? You said the DIPScale hasn't changed > in these calls here, right? It actually did :) Google search page locates the bottom bar in a specific way that was broken until I made RenderWidget::Resize() accept parameters that are _always_ internally consistent. The sequence was (1) Resize(dip_scale:3.5, physical_backing_size:1440x2052, visible_viewport_size:412x566) (2) Resize(dip_scale:3.5, physical_backing_size:1440x2052, visible_viewport_size:412x587) 2052 / 3.5 = 586.3, so should be 587, not 566. 566 came from the old physical size 1440x1980 I agree with you that I did not explain why the second call with correct data did not fix all issues, and that, I guess, I should do.
On 2016/08/02 19:21:36, Tima Vaisburd wrote: > On 2016/08/02 19:00:02, boliu wrote: > > > https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... > > File android_webview/java/src/org/chromium/android_webview/AwContents.java > > (left): > > > > > https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... > > android_webview/java/src/org/chromium/android_webview/AwContents.java:3187: > > mContentViewCore.onPhysicalBackingSizeChanged(w, h); > > On 2016/08/02 18:53:27, Tima Vaisburd wrote: > > > On 2016/08/02 18:09:27, boliu wrote: > > > > On 2016/08/02 17:27:19, Tima Vaisburd wrote: > > > > > onPhysicalBackingSizeChanges() calls nativeWasResized() that eventually > > > > gathers > > > > > ResizeParams and calls RenderWidget::OnResize(). > > > > > ContentViewCoreImpl determines the viewport size in dip by getting the > > > pixels > > > > > size and dividing it by the scale. > > > > > At the time of this call the pixels size is not yet updated and the > > > > ResizeParams > > > > > have |physical_backing_size| equal to the new size and the > > > > > |visible_viewport_size| corresponding to the old one. > > > > > > > > > > At the next line mContentViewCore.onSizeChanged() calls > > > > > RenderWidget::OnResize() again, now with correct sizes, but somehow it > is > > > too > > > > > late. > > > > > > > > what do you mean by "late"? > > > > > > > > It sounds like you are describing how things work today. why is this a > > problem > > > > with this density change? basically I don't see why these two calls needs > to > > > be > > > > merged into one (even though I agree it is weird they are separate) > > > > > > I tried to explain why I merged these two calls but I did not get to the > root > > > cause of the problem, that is, why the second call does not fix the issue. > > > > Why would you expect it to "fix" anything? You said the DIPScale hasn't > changed > > in these calls here, right? > > It actually did :) Google search page locates the bottom bar in a specific way > that was broken > until I made RenderWidget::Resize() accept parameters that are _always_ > internally consistent. What bottom bar? Is this the GSA app or the search page loaded in like the webview shell? > The sequence was > (1) Resize(dip_scale:3.5, physical_backing_size:1440x2052, > visible_viewport_size:412x566) > (2) Resize(dip_scale:3.5, physical_backing_size:1440x2052, > visible_viewport_size:412x587) > > 2052 / 3.5 = 586.3, so should be 587, not 566. 566 came from the old physical > size 1440x1980 > > I agree with you that I did not explain why the second call with correct data > did not fix all > issues, and that, I guess, I should do.
https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (left): https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:3187: mContentViewCore.onPhysicalBackingSizeChanged(w, h); On 2016/08/02 19:00:02, boliu wrote: > On 2016/08/02 18:53:27, Tima Vaisburd wrote: > > On 2016/08/02 18:09:27, boliu wrote: > > > On 2016/08/02 17:27:19, Tima Vaisburd wrote: > > > > onPhysicalBackingSizeChanges() calls nativeWasResized() that eventually > > > gathers > > > > ResizeParams and calls RenderWidget::OnResize(). > > > > ContentViewCoreImpl determines the viewport size in dip by getting the > > pixels > > > > size and dividing it by the scale. > > > > At the time of this call the pixels size is not yet updated and the > > > ResizeParams > > > > have |physical_backing_size| equal to the new size and the > > > > |visible_viewport_size| corresponding to the old one. > > > > > > > > At the next line mContentViewCore.onSizeChanged() calls > > > > RenderWidget::OnResize() again, now with correct sizes, but somehow it is > > too > > > > late. > > > > > > what do you mean by "late"? > > > > > > It sounds like you are describing how things work today. why is this a > problem > > > with this density change? basically I don't see why these two calls needs to > > be > > > merged into one (even though I agree it is weird they are separate) > > > > I tried to explain why I merged these two calls but I did not get to the root > > cause of the problem, that is, why the second call does not fix the issue. > > Why would you expect it to "fix" anything? You said the DIPScale hasn't changed > in these calls here, right? I restored the orininal code which means I need to find the root cause why these two calls do not work. https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:711: setDIPScale(newDIPScale); Bo, as far as I understand you suggested to force resize here (I was going to call ContentViewCoreImpl::WasResized()). But in my experiments the actual resize always came after device scale change, no matter if hardcorded or in full screen window. I guess Android does it for us? https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:712: mContainerView.requestLayout(); On 2016/08/02 18:09:27, boliu wrote: > On 2016/08/02 17:27:19, Tima Vaisburd wrote: > > Do we need to request layout on the container? > > Don't think so. If the scale ends up needing a redraw, then wait until blink > sends the redraw message up through the normal invalidate path. Removed mContainerView.requestLayout(); https://codereview.chromium.org/2202123002/diff/1/content/browser/android/con... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2202123002/diff/1/content/browser/android/con... content/browser/android/content_view_core_impl.cc:926: dpi_scale_ = device_scale; On 2016/08/02 18:09:27, boliu wrote: > this should probably do more than just set the value, what about updating > everything that depends on this value? Did not quite get your idea. I think everything that uses dpi_scale() (which is private) is either event coordinates or rectangle coordinates and they are transient, except for one thing - AndroidWindow::content_offset() - which I fixed now. https://codereview.chromium.org/2202123002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2202123002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1658: public void onBackingAndViewportSizeChanged( On 2016/08/02 18:09:27, boliu wrote: > if the above two methods needs to be merged, then just merge them, and fix all > the callsites in clank as well, don't add a new one > > also I don't quite understand why they need to be merged (see other comment) Removed this method for now.
when you rebase, upload a rebase patch set only first, then upload your additional changes, then I can just diff the additional changes https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2202123002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:711: setDIPScale(newDIPScale); On 2016/08/16 00:21:00, Tima Vaisburd wrote: > Bo, as far as I understand you suggested to force resize here (I was going to > call ContentViewCoreImpl::WasResized()). But in my experiments the actual resize > always came after device scale change, no matter if hardcorded or in full screen > window. I guess Android does it for us? That's just coming from the re-layout, because views above webview changed size. We can't rely on it happening, hence calling WasResized here.
https://codereview.chromium.org/2202123002/diff/20001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2202123002/diff/20001/content/browser/android... content/browser/android/content_view_core_impl.cc:432: view_.GetWindowAndroid()->set_content_offset(content_offset); this change and it's fallout can be in a separate CL that's landed first https://codereview.chromium.org/2202123002/diff/20001/content/browser/android... content/browser/android/content_view_core_impl.cc:915: dpi_scale_ = device_scale; what about all the places where dpi_scale() is called? that's a lot of places
https://codereview.chromium.org/2202123002/diff/20001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2202123002/diff/20001/content/browser/android... content/browser/android/content_view_core_impl.cc:432: view_.GetWindowAndroid()->set_content_offset(content_offset); On 2016/08/16 15:03:03, boliu wrote: > this change and it's fallout can be in a separate CL that's landed first Created https://codereview.chromium.org/2249243002/
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2202123002/diff/100001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2202123002/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:3224: mContentViewCore.onPhysicalAndViewportSizeChanged(w, h); I still did not understand why the second call does not work. I saw the first call issued and extra blink::FrameView::layout() triggered by the style change detection and this layout was missed on the second call, but this is not the root cause. During rotation we have the similar wrong size followed by the right size, but in the case of rotation the first wrong size is same as the old one and it is optimized away completely. In the other places onPhysicalBackingSizeChanged() and onSizeChanged() do not come together, the first one (as far as I get) is bound to the surface size change, the second to the view size change. AwContents is the only place they come together. Maybe it makes sense to combine them on one call, like in this PS?
can you please close this?
On 2017/03/01 01:01:21, sgurun wrote: > can you please close this? This is initial prototype that did not go into production. Closing. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
