New issue
Advanced search Search tips

Issue 885223 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 25
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Buggy RenderWidgetHostViewAura::DoBrowserControlsShrinkBlinkSize()

Project Member Reported by afakhry@chromium.org, Sep 18

Issue description

Scrolling a page in tablet mode using the mouse wheel leads the renderer shrinking the viewport size when it shouldn't (see attached screenshot).

- While scrolling is in progress, content::RenderWidgetHostViewAura::DoBrowserControlsShrinkBlinkSize() returns `false`: 
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_aura.cc?type=cs&sq=package:chromium&q=content::RenderWidgetHostViewAura::DoBrowserControlsShrinkBlinkSize&g=0&l=838.

- If blink::WebViewImpl::UpdateICBAndResizeViewport() gets called during this time, the `icb_size` will be shrunk: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/exported/web_view_impl.cc?type=cs&q=blink::WebViewImpl::UpdateICBAndResizeViewport&g=0&l=1362

This is what happened in the attached example:

|+++++++++++++++++++++++++++
|virtual bool content::RenderWidgetHostViewAura::DoBrowserControlsShrinkBlinkSize() const
|val = 0
|Took: 27 us
|---------------------------
|+++++++++++++++++++++++++++
|void blink::WebViewImpl::UpdateICBAndResizeViewport()
|icb_size.ToString() = "679x641"
|1
|icb_size.ToString() = "679x570"
|Took: 84 us
|---------------------------




 
Selection_101.jpg
196 KB View Download
Sounds like Blink's version of BrowserControls::shrink_viewport_ might be out of sync? Put some logging around where we change that, if the browser side is false then the Blink side should be as well.
bokan@, They're not out of sync, they both return false while scrolling is in progress. The problem is that it seems RenderWidgetHostViewAura::DoBrowserControlsShrinkBlinkSize() can be called at any time, even during scrolling, so if we are during scrolling, we return false, causing icb_size to shrink, when we finish scrolling, we return true, causing icb_size to unshrink.

In both situations the shown_ratio remained at 1.f.


It's not that DoBrowserControlsShrinkBlinkSize must be false during scrolling, it's that it must not change while we're scrolling.

So say you have:

1) Page loads, browser controls are fully shown and the page is stabilized
  
  shrinks_blink_size = true on both sides

2) We now scroll down, hiding the URL bar but without lifting the finger

  shown_ratio = 0 but shrinks_blink_size = true still

  While in this state, the ICB should not change size since we loaded the 
  page. If we get a resize while scrolling for some reason, it should account 
  for the URL bar and prevent changing the web contents size.

3) Release the finger
  
  This causes a resize of the web contents and shown_ratio = false. The ICB 
  size should still not change, it will now be smaller than the web_contents 
  size.


The invariant here is that DoBrowserControlsShrinkBlinkSize should only be true when we've resized the web contents (by sending a resize IPC) to be smaller by the top controls height - that should happen in step #3 above.
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 25

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

commit 6efd6eb84601cef66b85eb37cb8b4af0c38f9862
Author: Ahmed Fakhry <afakhry@google.com>
Date: Tue Sep 25 17:35:45 2018

Refactor and fix bugs in sliding top-chrome with page scrolls

This CL is a preparation for turning the feature on by default.
It fixes the following issues:
- Editable elements focus change notifications are broadcasted
  to all listeners from all render view hosts. Handle it only
  for the active webcontents.
- The value of DoBrowserControlsShrinkBlinkSize() should never
  change while scrolling is in progress. It should also return
  false when the feature is disabled regardless of the current
  shown ratio.
- A gray patch that used to show at the bottom of the page. We
  should never set the shown ratio of the browser immediately
  without asking the renderer to do it first and call us when
  done. For that we need to defer switching the status to disabled
  until the renderer calls us with the final shown ratio value, only
  then we know both the browser and the renderer are synchronized
  and we can layout the browser view.
- The active webcontents can be null in unit tests.
- Expose top-controls related fields from behind OS_ANDROID.

BUG=867063,  885223 , 884453
TEST=Manual, passes existing tests, passes all tests with the
     feature enabled by default.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I3bc7747a745619dd6f789d25a2a1609bbe8f15db
Reviewed-on: https://chromium-review.googlesource.com/1217754
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593988}
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/cc/trees/render_frame_metadata.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/chrome/browser/ui/browser.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/chrome/browser/ui/browser.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/chrome/browser/ui/browser_window.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/chrome/browser/ui/cocoa/browser_window_cocoa.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/chrome/browser/ui/cocoa/browser_window_cocoa.mm
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/chrome/browser/ui/views/frame/browser_view.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/chrome/browser/ui/views/frame/top_controls_slide_controller.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/chrome/browser/ui/views/frame/top_controls_slide_controller_chromeos.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/chrome/browser/ui/views/frame/top_controls_slide_controller_chromeos.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/chrome/browser/ui/views/frame/top_controls_slide_controller_chromeos_browsertest.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/chrome/test/base/test_browser_window.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/chrome/test/base/test_browser_window.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/components/embedder_support/android/delegate/web_contents_delegate_android.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/components/embedder_support/android/delegate/web_contents_delegate_android.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/components/viz/common/quads/compositor_frame_metadata.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/devtools/protocol/page_handler.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/frame_host/interstitial_page_impl.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/renderer_host/render_view_host_delegate_view.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/renderer_host/render_view_host_delegate_view.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/renderer_host/render_widget_host_delegate.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/renderer_host/render_widget_host_delegate.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/renderer_host/render_widget_host_view_aura.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/renderer_host/render_widget_host_view_base.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/renderer_host/render_widget_host_view_base.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/browser/web_contents/web_contents_view_android.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/common/common_param_traits_unittest.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/common/content_param_traits_macros.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/common/render_widget_surface_properties.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/common/render_widget_surface_properties.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/public/browser/web_contents_delegate.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/public/browser/web_contents_delegate.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/content/public/test/android/dom_utils.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/media/base/video_frame_metadata.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/services/viz/public/cpp/compositing/compositor_frame_metadata_struct_traits.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/services/viz/public/cpp/compositing/compositor_frame_metadata_struct_traits.h
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/services/viz/public/cpp/compositing/struct_traits_unittest.cc
[modify] https://crrev.com/6efd6eb84601cef66b85eb37cb8b4af0c38f9862/services/viz/public/interfaces/compositing/compositor_frame_metadata.mojom

Status: Fixed (was: Assigned)

Sign in to add a comment