New issue
Advanced search Search tips

Issue 868983 link

Starred by 1 user
Status: Fixed
Owner:
pdr@chromium.org
Closed: Aug 14
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Feature



Sign in to add a comment

PreferredSizeChangedMode causes an additional lifecycle update after every layout

Project Member Reported by pdr@chromium.org, Jul 30 
PreferredSizeChangedMode causes an additional blink lifecycle update after every layout.

Testcase:
----------------8<----------------
<!doctype html>
<div id="div" style="width: 100px; height: 100px; background: lightblue;"></div>
<script>
setTimeout(function() {
  console.log('about to change layout')
  div.style.width = "200px";
}, 1500);
</script>
----------------8<----------------

Output:
----------------8<----------------
[CONSOLE(5)] "about to change layout"
[local_frame_view.cc(2355)] UpdateLifecyclePhasesInternal
[local_frame_view.cc(2355)] UpdateLifecyclePhasesInternal
----------------8<----------------

This mode is on by default on MacOS in case the user clicks the green zoom button on the window:
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_contents_view_mac.mm?l=418
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 31 

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

commit 5d41d7b54772050c9180e52d02ccdf988c59ad76
Author: Philip Rogers <pdr@chromium.org>
Date: Tue Jul 31 15:34:51 2018

Only update layout from WebViewImpl::ContentsPreferredMinimumSize

WebViewImpl::ContentsPreferredMinimumSize only needs layout information
for MinPreferredLogicalWidth and ScrollHeight. This patch updates the
lifecycle update call to only do a layout lifecycle update, (similar to
OffsetWidth).

Bug:  868983 

Change-Id: Id31b6fb884b2c182b541eab5e8baf61b17579158
Reviewed-on: https://chromium-review.googlesource.com/1153979
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579415}
[modify] https://crrev.com/5d41d7b54772050c9180e52d02ccdf988c59ad76/third_party/blink/renderer/core/exported/web_view_impl.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 1 

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

commit 0796588190d581e4430e4a93493f362f9887463a
Author: Philip Rogers <pdr@chromium.org>
Date: Wed Aug 01 23:19:09 2018

Rename DidUpdateLayout to DidUpdateMainFrameLayout

There are several DidUpdateLayout functions but it is not obvious that
these are only called when the main frame lays out (see the callsite in
Document::LayoutUpdated). I confirmed this locally. This patch renames
these to DidUpdateMainFrameLayout so it is more obvious that this is
only called for main frame layouts.

The comments indicate that DidUpdateMainFrameLayout should only be
called with clean layout, but this is not true for the callsite in
RenderViewImpl::OnEnablePreferredSizeChangedMode.  A TODO has been added
to clean up RenderViewImpl::OnEnablePreferredSizeChangedMode.

Bug:  868983 
Change-Id: I66de3e8c402e7edeff9a8b1e7991716f55b771a8
Reviewed-on: https://chromium-review.googlesource.com/1156991
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579987}
[modify] https://crrev.com/0796588190d581e4430e4a93493f362f9887463a/android_webview/renderer/aw_render_view_ext.cc
[modify] https://crrev.com/0796588190d581e4430e4a93493f362f9887463a/android_webview/renderer/aw_render_view_ext.h
[modify] https://crrev.com/0796588190d581e4430e4a93493f362f9887463a/content/public/renderer/render_view_observer.h
[modify] https://crrev.com/0796588190d581e4430e4a93493f362f9887463a/content/renderer/render_view_impl.cc
[modify] https://crrev.com/0796588190d581e4430e4a93493f362f9887463a/content/renderer/render_view_impl.h
[modify] https://crrev.com/0796588190d581e4430e4a93493f362f9887463a/third_party/blink/public/web/web_view_client.h
[modify] https://crrev.com/0796588190d581e4430e4a93493f362f9887463a/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/0796588190d581e4430e4a93493f362f9887463a/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/0796588190d581e4430e4a93493f362f9887463a/third_party/blink/renderer/core/exported/web_view_impl.h
[modify] https://crrev.com/0796588190d581e4430e4a93493f362f9887463a/third_party/blink/renderer/core/frame/fullscreen_controller.cc
[modify] https://crrev.com/0796588190d581e4430e4a93493f362f9887463a/third_party/blink/renderer/core/frame/fullscreen_controller.h
[modify] https://crrev.com/0796588190d581e4430e4a93493f362f9887463a/third_party/blink/renderer/core/page/chrome_client.h
[modify] https://crrev.com/0796588190d581e4430e4a93493f362f9887463a/third_party/blink/renderer/core/page/chrome_client_impl.cc
[modify] https://crrev.com/0796588190d581e4430e4a93493f362f9887463a/third_party/blink/renderer/core/page/chrome_client_impl.h
[modify] https://crrev.com/0796588190d581e4430e4a93493f362f9887463a/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.h
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 7 

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

commit 601fd07dc26269d097d6ef6bd90f1203d361a72c
Author: Philip Rogers <pdr@chromium.org>
Date: Tue Aug 07 22:58:22 2018

Remove lifecycle updates for preferred content size changes

On platforms that need preferred content size updates (MacOS and
Android), an additional blink lifecycle update is issued if layout
changes because ContentsPreferredMinimumSize is called after layout. In
most cases this is unnecessary because layout is already up-to-date.

On platforms that use PreferredSizeChangedMode (e.g, MacOS), the
preferred size is updated after layout (RVI::DidUpdateMainFrameLayout)
and when the PreferredSizeChangedMode setting is enabled. When called
from DidUpdateMainFrameLayout, no lifecycle update is needed. When the
setting is first enabled, this patch now explicitly updates the
lifecycle before updating the preferred size.

On Android (AwRenderViewExt), the preferred size update does not appear
to need to force a lifecycle update because the common path in
AwRenderViewExt::UpdateContentsSize uses DocumentSize which does not
force a layout update.

This patch removes the forced lifecycle update from
ContentsPreferredMinimumSize and adds a DCHECK that is is only called
with up-to-date layout. An integration test has been added that ensures
no extra lifecycle updates occur. This would have caught
 https://crbug.com/866981 .

This patch changes the expectations for media/video-zoom-controls.html
which was different on MacOS compared to Windows and Linux. The media
controls code is racy with layout and will put up a bad frame (see:
MediaControlsImpl::NotifyElementSizeChanged). Before this patch, the
preferred content size layout timer would occur before the media
controls layout timer and cause different controls to show up on MacOS.
With this patch, the expectation on MacOS now matches other platforms.

Bug:  868983 

Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I80098edd844bb88e3c64a54cc42b54063ec1c583
Reviewed-on: https://chromium-review.googlesource.com/1153972
Reviewed-by: Philip Rogers <pdr@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581383}
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/android_webview/renderer/aw_render_view_ext.cc
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/android_webview/renderer/aw_render_view_ext.h
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/content/renderer/render_view_browsertest.cc
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/content/renderer/render_view_impl.cc
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/content/renderer/render_view_impl.h
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/WebKit/LayoutTests/VirtualTestSuites
[add] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/WebKit/LayoutTests/lifecycle/background-change-lifecycle-count.html
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/WebKit/LayoutTests/platform/mac-mac10.12/media/video-zoom-controls-expected.png
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/WebKit/LayoutTests/platform/mac-mac10.12/virtual/video-surface-layer/media/video-zoom-controls-expected.png
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/WebKit/LayoutTests/platform/mac/media/video-zoom-controls-expected.png
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/WebKit/LayoutTests/platform/mac/media/video-zoom-controls-expected.txt
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/WebKit/LayoutTests/platform/mac/virtual/video-surface-layer/media/video-zoom-controls-expected.png
[add] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/WebKit/LayoutTests/platform/mac/virtual/video-surface-layer/media/video-zoom-controls-expected.txt
[add] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/WebKit/LayoutTests/virtual/threaded/lifecycle/README.txt
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/public/web/web_view.h
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/public/web/web_widget.h
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/editing/frame_selection_test.cc
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/editing/granularity_strategy_test.cc
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/editing/spellcheck/spell_checker_test.cc
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/exported/web_frame_test.cc
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/exported/web_view_test.cc
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/frame/local_frame_view.h
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/html/forms/text_control_element_test.cc
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/page/page_animator.cc
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/page/page_animator.h
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/page/page_widget_delegate.cc
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/testing/internals.cc
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/testing/internals.h
[modify] https://crrev.com/601fd07dc26269d097d6ef6bd90f1203d361a72c/third_party/blink/renderer/core/testing/internals.idl

Comment 4 by pdr@chromium.org, Aug 14

Status: Fixed (was: Assigned)
►

Sign in to add a comment