From 7610e74eacde3468853c9765bd3f726c14df04c1 Mon Sep 17 00:00:00 2001 From: bokan Date: Thu, 5 Mar 2015 08:19:35 -0800 Subject: [PATCH] Always create top controls manager. This CL removes the calculate_top_controls_position flag and instead always creates the top controls manager. The methods of the manager become no-ops if the top controls height is 0 so that it becomes effectively enabled when a top controls height gets set. This fixes the attached bug because some Android Chrome UI pages don't have top controls but the manager was still created. These pages set the height to 0 to hide the top controls but the methods of the manager were still getting called. This caused a crash in Blink since ScrollBy would try to calculate the shown ratio with a 0 height, causing a NaN to propagate into Blink. BUG=460007 Review URL: https://codereview.chromium.org/961023002 Cr-Commit-Position: refs/heads/master@{#319257} --- cc/base/switches.cc | 3 -- cc/base/switches.h | 1 - cc/input/top_controls_manager.cc | 9 ++++ cc/input/top_controls_manager_unittest.cc | 48 +++++++++++++++++++++- cc/trees/layer_tree_host.cc | 10 +---- cc/trees/layer_tree_host_impl.cc | 42 +++++++------------ cc/trees/layer_tree_host_impl_unittest.cc | 4 +- cc/trees/layer_tree_settings.cc | 1 - cc/trees/layer_tree_settings.h | 1 - .../chrome/browser/ApplicationInitialization.java | 2 - .../chromeos/login/chrome_restart_request.cc | 1 - .../renderer_host/compositor_impl_android.cc | 1 - .../renderer_host/render_process_host_impl.cc | 1 - .../chromium/content/common/ContentSwitches.java | 4 -- content/renderer/gpu/render_widget_compositor.cc | 2 - 15 files changed, 74 insertions(+), 56 deletions(-) diff --git a/cc/base/switches.cc b/cc/base/switches.cc index 504e787cd66e..ae424824611e 100644 --- a/cc/base/switches.cc +++ b/cc/base/switches.cc @@ -24,9 +24,6 @@ const char kDisableMainFrameBeforeActivation[] = const char kEnableMainFrameBeforeActivation[] = "enable-main-frame-before-activation"; -const char kEnableTopControlsPositionCalculation[] = - "enable-top-controls-position-calculation"; - // Percentage of the top controls need to be hidden before they will auto hide. const char kTopControlsHideThreshold[] = "top-controls-hide-threshold"; diff --git a/cc/base/switches.h b/cc/base/switches.h index 90caef83aded..46fe922ea7e7 100644 --- a/cc/base/switches.h +++ b/cc/base/switches.h @@ -20,7 +20,6 @@ CC_EXPORT extern const char kDisableThreadedAnimation[]; CC_EXPORT extern const char kDisableCompositedAntialiasing[]; CC_EXPORT extern const char kDisableMainFrameBeforeActivation[]; CC_EXPORT extern const char kEnableMainFrameBeforeActivation[]; -CC_EXPORT extern const char kEnableTopControlsPositionCalculation[]; CC_EXPORT extern const char kJankInsteadOfCheckerboard[]; CC_EXPORT extern const char kTopControlsHideThreshold[]; CC_EXPORT extern const char kTopControlsShowThreshold[]; diff --git a/cc/input/top_controls_manager.cc b/cc/input/top_controls_manager.cc index 7362805de99a..c8bd5348721c 100644 --- a/cc/input/top_controls_manager.cc +++ b/cc/input/top_controls_manager.cc @@ -101,6 +101,9 @@ void TopControlsManager::ScrollBegin() { gfx::Vector2dF TopControlsManager::ScrollBy( const gfx::Vector2dF& pending_delta) { + if (!TopControlsHeight()) + return pending_delta; + if (pinch_gesture_active_) return pending_delta; @@ -180,6 +183,12 @@ void TopControlsManager::SetupAnimation(AnimationDirection direction) { if (top_controls_animation_ && animation_direction_ == direction) return; + if (!TopControlsHeight()) { + client_->SetCurrentTopControlsShownRatio( + direction == HIDING_CONTROLS ? 0.f : 1.f); + return; + } + top_controls_animation_ = KeyframedFloatAnimationCurve::Create(); base::TimeDelta start_time = gfx::FrameTime::Now() - base::TimeTicks(); top_controls_animation_->AddKeyframe( diff --git a/cc/input/top_controls_manager_unittest.cc b/cc/input/top_controls_manager_unittest.cc index 16d0e5d21b1b..4befa150a131 100644 --- a/cc/input/top_controls_manager_unittest.cc +++ b/cc/input/top_controls_manager_unittest.cc @@ -409,8 +409,9 @@ TEST(TopControlsManagerTest, HeightChangeMaintainsFullyVisibleControls) { TEST(TopControlsManagerTest, GrowingHeightKeepsTopControlsHidden) { MockTopControlsManagerClient client(0.f, 0.5f, 0.5f); TopControlsManager* manager = client.manager(); + client.SetTopControlsHeight(1.f); manager->UpdateTopControlsState(HIDDEN, HIDDEN, false); - EXPECT_EQ(0.f, manager->ControlsTopOffset()); + EXPECT_EQ(-1.f, manager->ControlsTopOffset()); EXPECT_EQ(0.f, manager->ContentTopOffset()); client.SetTopControlsHeight(50.f); @@ -445,5 +446,50 @@ TEST(TopControlsManagerTest, ShrinkingHeightKeepsTopControlsHidden) { EXPECT_FLOAT_EQ(0.f, manager->ContentTopOffset()); } +TEST(TopControlsManagerTest, ZeroTopControlsHeightScrolling) { + MockTopControlsManagerClient client(0.f, 0.5f, 0.5f); + client.SetCurrentTopControlsShownRatio(0.f); + TopControlsManager* manager = client.manager(); + manager->UpdateTopControlsState(BOTH, BOTH, false); + + manager->ScrollBegin(); + EXPECT_FLOAT_EQ(-10.f, manager->ScrollBy(gfx::Vector2dF(0.f, -10.f)).y()); + EXPECT_FLOAT_EQ(0.f, manager->TopControlsShownRatio()); + + client.SetTopControlsHeight(20.f); + EXPECT_FLOAT_EQ(0.f, manager->TopControlsShownRatio()); + + EXPECT_FLOAT_EQ(0.f, manager->ScrollBy(gfx::Vector2dF(0.f, -15.f)).y()); + EXPECT_FLOAT_EQ(0.75f, manager->TopControlsShownRatio()); + client.SetTopControlsHeight(0.f); + manager->ScrollEnd(); + + EXPECT_FALSE(manager->animation()); + EXPECT_FLOAT_EQ(1.f, manager->TopControlsShownRatio()); +} + +TEST(TopControlsManagerTest, ZeroTopControlsHeightAnimating) { + MockTopControlsManagerClient client(0.f, 0.5f, 0.5f); + client.SetCurrentTopControlsShownRatio(0.f); + TopControlsManager* manager = client.manager(); + + manager->UpdateTopControlsState(BOTH, HIDDEN, false); + EXPECT_FLOAT_EQ(0.f, manager->TopControlsShownRatio()); + manager->UpdateTopControlsState(BOTH, SHOWN, false); + EXPECT_FLOAT_EQ(1.f, manager->TopControlsShownRatio()); + + manager->UpdateTopControlsState(BOTH, HIDDEN, true); + EXPECT_FLOAT_EQ(0.f, manager->TopControlsShownRatio()); + EXPECT_FALSE(manager->animation()); + manager->UpdateTopControlsState(BOTH, SHOWN, true); + EXPECT_FLOAT_EQ(1.f, manager->TopControlsShownRatio()); + EXPECT_FALSE(manager->animation()); + + client.SetCurrentTopControlsShownRatio(0.3f); + manager->MainThreadHasStoppedFlinging(); + EXPECT_FALSE(manager->animation()); + EXPECT_FLOAT_EQ(0.f, manager->TopControlsShownRatio()); +} + } // namespace } // namespace cc diff --git a/cc/trees/layer_tree_host.cc b/cc/trees/layer_tree_host.cc index 92e9c2593fc2..d893b33c3374 100644 --- a/cc/trees/layer_tree_host.cc +++ b/cc/trees/layer_tree_host.cc @@ -448,11 +448,8 @@ scoped_ptr LayerTreeHost::CreateLayerTreeHostImpl( host_impl->SetUseGpuRasterization(UseGpuRasterization()); shared_bitmap_manager_ = NULL; gpu_memory_buffer_manager_ = NULL; - if (settings_.calculate_top_controls_position && - host_impl->top_controls_manager()) { - top_controls_manager_weak_ptr_ = - host_impl->top_controls_manager()->AsWeakPtr(); - } + top_controls_manager_weak_ptr_ = + host_impl->top_controls_manager()->AsWeakPtr(); input_handler_weak_ptr_ = host_impl->AsWeakPtr(); return host_impl.Pass(); } @@ -1158,9 +1155,6 @@ void LayerTreeHost::SetDeviceScaleFactor(float device_scale_factor) { void LayerTreeHost::UpdateTopControlsState(TopControlsState constraints, TopControlsState current, bool animate) { - if (!settings_.calculate_top_controls_position) - return; - // Top controls are only used in threaded mode. proxy_->ImplThreadTaskRunner()->PostTask( FROM_HERE, diff --git a/cc/trees/layer_tree_host_impl.cc b/cc/trees/layer_tree_host_impl.cc index c03f7f79528e..2f4d9a9b057c 100644 --- a/cc/trees/layer_tree_host_impl.cc +++ b/cc/trees/layer_tree_host_impl.cc @@ -241,12 +241,10 @@ LayerTreeHostImpl::LayerTreeHostImpl( TRACE_EVENT_OBJECT_CREATED_WITH_ID( TRACE_DISABLED_BY_DEFAULT("cc.debug"), "cc::LayerTreeHostImpl", id_); - if (settings.calculate_top_controls_position) { - top_controls_manager_ = - TopControlsManager::Create(this, - settings.top_controls_show_threshold, - settings.top_controls_hide_threshold); - } + top_controls_manager_ = + TopControlsManager::Create(this, + settings.top_controls_show_threshold, + settings.top_controls_hide_threshold); } LayerTreeHostImpl::~LayerTreeHostImpl() { @@ -916,8 +914,7 @@ DrawResult LayerTreeHostImpl::CalculateRenderPasses( } void LayerTreeHostImpl::MainThreadHasStoppedFlinging() { - if (top_controls_manager_) - top_controls_manager_->MainThreadHasStoppedFlinging(); + top_controls_manager_->MainThreadHasStoppedFlinging(); if (input_handler_client_) input_handler_client_->MainThreadHasStoppedFlinging(); } @@ -1423,12 +1420,10 @@ CompositorFrameMetadata LayerTreeHostImpl::MakeCompositorFrameMetadata() const { metadata.root_layer_size = active_tree_->ScrollableSize(); metadata.min_page_scale_factor = active_tree_->min_page_scale_factor(); metadata.max_page_scale_factor = active_tree_->max_page_scale_factor(); - if (top_controls_manager_) { - metadata.location_bar_offset = - gfx::Vector2dF(0.f, top_controls_manager_->ControlsTopOffset()); - metadata.location_bar_content_translation = - gfx::Vector2dF(0.f, top_controls_manager_->ContentTopOffset()); - } + metadata.location_bar_offset = + gfx::Vector2dF(0.f, top_controls_manager_->ControlsTopOffset()); + metadata.location_bar_content_translation = + gfx::Vector2dF(0.f, top_controls_manager_->ContentTopOffset()); active_tree_->GetViewportSelection(&metadata.selection_start, &metadata.selection_end); @@ -1658,7 +1653,7 @@ void LayerTreeHostImpl::UpdateViewportContainerSizes() { LayerImpl* inner_container = active_tree_->InnerViewportContainerLayer(); LayerImpl* outer_container = active_tree_->OuterViewportContainerLayer(); - if (!inner_container || !top_controls_manager_) + if (!inner_container) return; ViewportAnchor anchor(InnerViewportScrollLayer(), @@ -2356,8 +2351,7 @@ InputHandler::ScrollStatus LayerTreeHostImpl::ScrollBegin( InputHandler::ScrollInputType type) { TRACE_EVENT0("cc", "LayerTreeHostImpl::ScrollBegin"); - if (top_controls_manager_) - top_controls_manager_->ScrollBegin(); + top_controls_manager_->ScrollBegin(); DCHECK(!CurrentlyScrollingLayer()); ClearCurrentlyScrollingLayer(); @@ -2568,9 +2562,6 @@ bool LayerTreeHostImpl::ShouldTopControlsConsumeScroll( const gfx::Vector2dF& scroll_delta) const { DCHECK(CurrentlyScrollingLayer()); - if (!top_controls_manager_) - return false; - // Always consume if it's in the direction to show the top controls. if (scroll_delta.y() < 0) return true; @@ -2811,8 +2802,7 @@ void LayerTreeHostImpl::ClearCurrentlyScrollingLayer() { } void LayerTreeHostImpl::ScrollEnd() { - if (top_controls_manager_) - top_controls_manager_->ScrollEnd(); + top_controls_manager_->ScrollEnd(); ClearCurrentlyScrollingLayer(); } @@ -2933,8 +2923,7 @@ void LayerTreeHostImpl::PinchGestureBegin() { active_tree_->SetCurrentlyScrollingLayer( active_tree_->InnerViewportScrollLayer()); } - if (top_controls_manager_) - top_controls_manager_->PinchBegin(); + top_controls_manager_->PinchBegin(); } void LayerTreeHostImpl::PinchGestureUpdate(float magnify_delta, @@ -2993,8 +2982,7 @@ void LayerTreeHostImpl::PinchGestureEnd() { pinch_gesture_end_should_clear_scrolling_layer_ = false; ClearCurrentlyScrollingLayer(); } - if (top_controls_manager_) - top_controls_manager_->PinchEnd(); + top_controls_manager_->PinchEnd(); client_->SetNeedsCommitOnImplThread(); // When a pinch ends, we may be displaying content cached at incorrect scales, // so updating draw properties and drawing will ensure we are using the right @@ -3089,7 +3077,7 @@ void LayerTreeHostImpl::AnimatePageScale(base::TimeTicks monotonic_time) { } void LayerTreeHostImpl::AnimateTopControls(base::TimeTicks time) { - if (!top_controls_manager_ || !top_controls_manager_->animation()) + if (!top_controls_manager_->animation()) return; gfx::Vector2dF scroll = top_controls_manager_->Animate(time); diff --git a/cc/trees/layer_tree_host_impl_unittest.cc b/cc/trees/layer_tree_host_impl_unittest.cc index 110bee999392..4b2bc7549ed2 100644 --- a/cc/trees/layer_tree_host_impl_unittest.cc +++ b/cc/trees/layer_tree_host_impl_unittest.cc @@ -2478,7 +2478,6 @@ class LayerTreeHostImplTopControlsTest : public LayerTreeHostImplTest { : layer_size_(10, 10), clip_size_(layer_size_), top_controls_height_(50) { - settings_.calculate_top_controls_position = true; settings_.use_pinch_virtual_viewport = true; viewport_size_ = gfx::Size(clip_size_.width(), @@ -2489,7 +2488,7 @@ class LayerTreeHostImplTopControlsTest : public LayerTreeHostImplTest { scoped_ptr output_surface) override { bool init = LayerTreeHostImplTest::CreateHostImpl(settings, output_surface.Pass()); - if (init && settings.calculate_top_controls_position) { + if (init) { host_impl_->active_tree()->set_top_controls_height(top_controls_height_); host_impl_->active_tree()->SetCurrentTopControlsShownRatio(1.f); } @@ -7453,7 +7452,6 @@ class LayerTreeHostImplWithTopControlsTest : public LayerTreeHostImplTest { public: void SetUp() override { LayerTreeSettings settings = DefaultSettings(); - settings.calculate_top_controls_position = true; CreateHostImpl(settings, CreateOutputSurface()); host_impl_->active_tree()->set_top_controls_height(top_controls_height_); host_impl_->sync_tree()->set_top_controls_height(top_controls_height_); diff --git a/cc/trees/layer_tree_settings.cc b/cc/trees/layer_tree_settings.cc index 9d605aea8bdb..0088e9176658 100644 --- a/cc/trees/layer_tree_settings.cc +++ b/cc/trees/layer_tree_settings.cc @@ -39,7 +39,6 @@ LayerTreeSettings::LayerTreeSettings() scrollbar_fade_duration_ms(0), scrollbar_show_scale_threshold(1.0f), solid_color_scrollbar_color(SK_ColorWHITE), - calculate_top_controls_position(false), timeout_and_draw_when_animation_checkerboards(true), maximum_number_of_failed_draws_before_draw_is_forced_(3), layer_transforms_should_scale_layer_contents(false), diff --git a/cc/trees/layer_tree_settings.h b/cc/trees/layer_tree_settings.h index 06c319a6b8c2..ce8d8568abe4 100644 --- a/cc/trees/layer_tree_settings.h +++ b/cc/trees/layer_tree_settings.h @@ -51,7 +51,6 @@ class CC_EXPORT LayerTreeSettings { int scrollbar_fade_duration_ms; float scrollbar_show_scale_threshold; SkColor solid_color_scrollbar_color; - bool calculate_top_controls_position; bool timeout_and_draw_when_animation_checkerboards; int maximum_number_of_failed_draws_before_draw_is_forced_; bool layer_transforms_should_scale_layer_contents; diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ApplicationInitialization.java b/chrome/android/java/src/org/chromium/chrome/browser/ApplicationInitialization.java index 6f0472dc2c39..f98c1f8889bd 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/ApplicationInitialization.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/ApplicationInitialization.java @@ -35,8 +35,6 @@ public final class ApplicationInitialization { CommandLine commandLine = CommandLine.getInstance(); if (commandLine.hasSwitch(ChromeSwitches.DISABLE_FULLSCREEN)) return; - commandLine.appendSwitch(ContentSwitches.ENABLE_TOP_CONTROLS_POSITION_CALCULATION); - TypedValue threshold = new TypedValue(); resources.getValue(R.floats.top_controls_show_threshold, threshold, true); commandLine.appendSwitchWithValue( diff --git a/chrome/browser/chromeos/login/chrome_restart_request.cc b/chrome/browser/chromeos/login/chrome_restart_request.cc index 7a10b5149242..329470a1ab92 100644 --- a/chrome/browser/chromeos/login/chrome_restart_request.cc +++ b/chrome/browser/chromeos/login/chrome_restart_request.cc @@ -191,7 +191,6 @@ std::string DeriveCommandLine(const GURL& start_url, cc::switches::kEnablePinchVirtualViewport, cc::switches::kEnablePropertyTreeVerification, cc::switches::kEnableMainFrameBeforeActivation, - cc::switches::kEnableTopControlsPositionCalculation, cc::switches::kMaxTilesForInterestArea, cc::switches::kMaxUnusedResourceMemoryUsagePercentage, cc::switches::kShowCompositedLayerBorders, diff --git a/content/browser/renderer_host/compositor_impl_android.cc b/content/browser/renderer_host/compositor_impl_android.cc index 6a7c1ee21df6..65cb728e82e0 100644 --- a/content/browser/renderer_host/compositor_impl_android.cc +++ b/content/browser/renderer_host/compositor_impl_android.cc @@ -379,7 +379,6 @@ void CompositorImpl::CreateLayerTreeHost() { settings.renderer_settings.allow_antialiasing = false; settings.renderer_settings.highp_threshold_min = 2048; settings.impl_side_painting = true; - settings.calculate_top_controls_position = false; base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); settings.initial_debug_state.SetRecordRenderingStats( diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc index 7f5c54d7269a..b1eae97ef079 100644 --- a/content/browser/renderer_host/render_process_host_impl.cc +++ b/content/browser/renderer_host/render_process_host_impl.cc @@ -1324,7 +1324,6 @@ void RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer( cc::switches::kDisableThreadedAnimation, cc::switches::kEnableGpuBenchmarking, cc::switches::kEnableMainFrameBeforeActivation, - cc::switches::kEnableTopControlsPositionCalculation, cc::switches::kMaxTilesForInterestArea, cc::switches::kMaxUnusedResourceMemoryUsagePercentage, cc::switches::kShowCompositedLayerBorders, diff --git a/content/public/android/java/src/org/chromium/content/common/ContentSwitches.java b/content/public/android/java/src/org/chromium/content/common/ContentSwitches.java index 44a854c510e3..a671db7c3545 100644 --- a/content/public/android/java/src/org/chromium/content/common/ContentSwitches.java +++ b/content/public/android/java/src/org/chromium/content/common/ContentSwitches.java @@ -41,10 +41,6 @@ public abstract class ContentSwitches { // Sets the ISO country code that will be used for phone number detection. public static final String NETWORK_COUNTRY_ISO = "network-country-iso"; - // Whether to enable the auto-hiding top controls. - public static final String ENABLE_TOP_CONTROLS_POSITION_CALCULATION = - "enable-top-controls-position-calculation"; - // How much of the top controls need to be shown before they will auto show. public static final String TOP_CONTROLS_SHOW_THRESHOLD = "top-controls-show-threshold"; diff --git a/content/renderer/gpu/render_widget_compositor.cc b/content/renderer/gpu/render_widget_compositor.cc index c01231ece067..00268c29d8cd 100644 --- a/content/renderer/gpu/render_widget_compositor.cc +++ b/content/renderer/gpu/render_widget_compositor.cc @@ -275,8 +275,6 @@ void RenderWidgetCompositor::Initialize() { compositor_deps_->IsElasticOverscrollEnabled(); settings.use_image_texture_target = compositor_deps_->GetImageTextureTarget(); - settings.calculate_top_controls_position = - cmd->HasSwitch(cc::switches::kEnableTopControlsPositionCalculation); if (cmd->HasSwitch(cc::switches::kTopControlsShowThreshold)) { std::string top_threshold_str = cmd->GetSwitchValueASCII(cc::switches::kTopControlsShowThreshold); -- 2.11.4.GIT