From d606481ea573a5d664e9a830d9287a2fc9388c58 Mon Sep 17 00:00:00 2001 From: sadrul Date: Wed, 15 Jul 2015 13:32:09 -0700 Subject: [PATCH] view-manager: Create the root-view only when the viewport-metrics are known. Instead of creating the root-view at first, delay the creation until the viewport metrics are known. This avoids having to create the root-view with a default size, and then updating the size when the viewport metrics become known. With this change, a Browser knows about the device-pixel ratio as soon as it is embedded. So there's no more need for DevicePixelRatioWaiter. So that is also removed. BUG=496935 Review URL: https://codereview.chromium.org/1229893004 Cr-Commit-Position: refs/heads/master@{#338905} --- components/view_manager/display_manager.cc | 13 ++--- components/view_manager/view_manager_app.cc | 15 ++--- .../view_manager/view_manager_root_connection.cc | 21 ++++--- .../view_manager/view_manager_root_connection.h | 9 ++- .../view_manager/view_manager_root_delegate.h | 4 ++ components/view_manager/view_manager_root_impl.cc | 20 +++++-- .../view_manager/view_manager_service_unittest.cc | 42 ++++++++------ mandoline/ui/browser/browser.cc | 42 +++++++------- mandoline/ui/browser/browser.h | 6 -- mandoline/ui/browser/browser_apptest.cc | 4 +- mandoline/ui/browser/browser_delegate.h | 5 +- mandoline/ui/browser/browser_manager.cc | 64 ++-------------------- mandoline/ui/browser/browser_manager.h | 8 +-- 13 files changed, 100 insertions(+), 153 deletions(-) diff --git a/components/view_manager/display_manager.cc b/components/view_manager/display_manager.cc index 373ff57fc49b..cf3f113ac4f3 100644 --- a/components/view_manager/display_manager.cc +++ b/components/view_manager/display_manager.cc @@ -234,15 +234,14 @@ void DefaultDisplayManager::UpdateMetrics(const gfx::Size& size, if (metrics_.size_in_pixels.To() == size && metrics_.device_pixel_ratio == device_pixel_ratio) return; - mojo::ViewportMetrics metrics; - metrics.size_in_pixels = mojo::Size::From(size); - metrics.device_pixel_ratio = device_pixel_ratio; + mojo::ViewportMetrics old_metrics; + old_metrics.size_in_pixels = metrics_.size_in_pixels.Clone(); + old_metrics.device_pixel_ratio = metrics_.device_pixel_ratio; - delegate_->GetRootView()->SetBounds(gfx::Rect(size)); - delegate_->OnViewportMetricsChanged(metrics_, metrics); + metrics_.size_in_pixels = mojo::Size::From(size); + metrics_.device_pixel_ratio = device_pixel_ratio; - metrics_.size_in_pixels = metrics.size_in_pixels.Clone(); - metrics_.device_pixel_ratio = metrics.device_pixel_ratio; + delegate_->OnViewportMetricsChanged(old_metrics, metrics_); } void DefaultDisplayManager::OnBoundsChanged(const gfx::Rect& new_bounds) { diff --git a/components/view_manager/view_manager_app.cc b/components/view_manager/view_manager_app.cc index 6cbed253b048..1f9f283c6a88 100644 --- a/components/view_manager/view_manager_app.cc +++ b/components/view_manager/view_manager_app.cc @@ -100,21 +100,16 @@ void ViewManagerApp::Create(ApplicationConnection* connection, DCHECK(connection_manager_.get()); // TODO(fsamuel): We need to make sure that only the window manager can create // new roots. - scoped_ptr view_manager_root( - new ViewManagerRootImpl( - connection_manager_.get(), - is_headless_, - app_impl_, - gpu_state_)); + ViewManagerRootImpl* view_manager_root = new ViewManagerRootImpl( + connection_manager_.get(), is_headless_, app_impl_, gpu_state_); mojo::ViewManagerClientPtr client; connection->ConnectToService(&client); // ViewManagerRootConnection manages its own lifetime. - new ViewManagerRootConnectionImpl(request.Pass(), - view_manager_root.Pass(), - client.Pass(), - connection_manager_.get()); + view_manager_root->Init(new ViewManagerRootConnectionImpl( + request.Pass(), make_scoped_ptr(view_manager_root), client.Pass(), + connection_manager_.get())); } void ViewManagerApp::Create( diff --git a/components/view_manager/view_manager_root_connection.cc b/components/view_manager/view_manager_root_connection.cc index 18e7f29d4661..017ccbe0a58c 100644 --- a/components/view_manager/view_manager_root_connection.cc +++ b/components/view_manager/view_manager_root_connection.cc @@ -16,7 +16,6 @@ ViewManagerRootConnection::ViewManagerRootConnection( service_(nullptr), connection_manager_(manager), connection_closed_(false) { - root_->Init(this); } ViewManagerRootConnection::~ViewManagerRootConnection() { @@ -40,6 +39,9 @@ ViewManagerServiceImpl* ViewManagerRootConnection::GetViewManagerService() { return service_; } +void ViewManagerRootConnection::OnDisplayInitialized() { +} + void ViewManagerRootConnection::OnDisplayClosed() { CloseConnection(); } @@ -50,17 +52,18 @@ ViewManagerRootConnectionImpl::ViewManagerRootConnectionImpl( mojo::ViewManagerClientPtr client, ConnectionManager* manager) : ViewManagerRootConnection(root.Pass(), manager), - binding_(view_manager_root(), request.Pass()) { - binding_.set_connection_error_handler([this]() { CloseConnection(); }); - - connection_manager()->AddRoot(this); - set_view_manager_service(connection_manager()->EmbedAtView( - kInvalidConnectionId, - view_manager_root()->root_view()->id(), - client.Pass())); + binding_(view_manager_root(), request.Pass()), + client_(client.Pass()) { } ViewManagerRootConnectionImpl::~ViewManagerRootConnectionImpl() { } +void ViewManagerRootConnectionImpl::OnDisplayInitialized() { + connection_manager()->AddRoot(this); + set_view_manager_service(connection_manager()->EmbedAtView( + kInvalidConnectionId, view_manager_root()->root_view()->id(), + client_.Pass())); +} + } // namespace view_manager diff --git a/components/view_manager/view_manager_root_connection.h b/components/view_manager/view_manager_root_connection.h index 4d1d8b3defb0..cc89c2206cc5 100644 --- a/components/view_manager/view_manager_root_connection.h +++ b/components/view_manager/view_manager_root_connection.h @@ -46,11 +46,12 @@ class ViewManagerRootConnection : public ViewManagerRootDelegate { protected: ~ViewManagerRootConnection() override; - private: // ViewManagerRootDelegate: - ViewManagerServiceImpl* GetViewManagerService() override; + void OnDisplayInitialized() override; void OnDisplayClosed() override; + ViewManagerServiceImpl* GetViewManagerService() override; + private: scoped_ptr root_; ViewManagerServiceImpl* service_; ConnectionManager* connection_manager_; @@ -71,7 +72,11 @@ class ViewManagerRootConnectionImpl : public ViewManagerRootConnection { private: ~ViewManagerRootConnectionImpl() override; + // ViewManagerRootDelegate: + void OnDisplayInitialized() override; + mojo::Binding binding_; + mojo::ViewManagerClientPtr client_; DISALLOW_COPY_AND_ASSIGN(ViewManagerRootConnectionImpl); }; diff --git a/components/view_manager/view_manager_root_delegate.h b/components/view_manager/view_manager_root_delegate.h index 3984bd6e78c0..b85440c7f229 100644 --- a/components/view_manager/view_manager_root_delegate.h +++ b/components/view_manager/view_manager_root_delegate.h @@ -17,6 +17,10 @@ class ViewManagerServiceImpl; // when the Display of the root is closed. class ViewManagerRootDelegate { public: + // Called when the window associated with the root is completely initialized + // (i.e. the ViewportMetrics for the display is known). + virtual void OnDisplayInitialized() = 0; + // Called when the window associated with the root is closed. virtual void OnDisplayClosed() = 0; diff --git a/components/view_manager/view_manager_root_impl.cc b/components/view_manager/view_manager_root_impl.cc index 6301ebf238e0..00dc13374554 100644 --- a/components/view_manager/view_manager_root_impl.cc +++ b/components/view_manager/view_manager_root_impl.cc @@ -19,12 +19,8 @@ ViewManagerRootImpl::ViewManagerRootImpl( const scoped_refptr& gpu_state) : delegate_(nullptr), connection_manager_(connection_manager), - root_(connection_manager->CreateServerView( - RootViewId(connection_manager->GetAndAdvanceNextRootId()))), display_manager_( DisplayManager::Create(is_headless, app_impl, gpu_state)) { - root_->SetBounds(gfx::Rect(800, 600)); - root_->SetVisible(true); display_manager_->Init(this); } @@ -33,6 +29,8 @@ ViewManagerRootImpl::~ViewManagerRootImpl() { void ViewManagerRootImpl::Init(ViewManagerRootDelegate* delegate) { delegate_ = delegate; + if (delegate_ && root_) + delegate_->OnDisplayInitialized(); } ViewManagerServiceImpl* ViewManagerRootImpl::GetViewManagerService() { @@ -96,8 +94,18 @@ void ViewManagerRootImpl::OnDisplayClosed() { void ViewManagerRootImpl::OnViewportMetricsChanged( const mojo::ViewportMetrics& old_metrics, const mojo::ViewportMetrics& new_metrics) { - // TODO(fsamuel: We shouldn't broadcast this to all connections but only those - // within a window root. + if (!root_) { + root_.reset(connection_manager_->CreateServerView( + RootViewId(connection_manager_->GetAndAdvanceNextRootId()))); + root_->SetBounds(gfx::Rect(new_metrics.size_in_pixels.To())); + root_->SetVisible(true); + if (delegate_) + delegate_->OnDisplayInitialized(); + } else { + root_->SetBounds(gfx::Rect(new_metrics.size_in_pixels.To())); + } + // TODO(fsamuel): We shouldn't broadcast this to all connections but only + // those within a window root. connection_manager_->ProcessViewportMetricsChanged(old_metrics, new_metrics); } diff --git a/components/view_manager/view_manager_service_unittest.cc b/components/view_manager/view_manager_service_unittest.cc index d3701ada340d..72a986a31559 100644 --- a/components/view_manager/view_manager_service_unittest.cc +++ b/components/view_manager/view_manager_service_unittest.cc @@ -117,11 +117,12 @@ class TestClientConnection : public ClientConnection { public: explicit TestClientConnection(scoped_ptr service_impl) : ClientConnection(service_impl.Pass(), &client_) {} - ~TestClientConnection() override {} TestViewManagerClient* client() { return &client_; } private: + ~TestClientConnection() override {} + TestViewManagerClient client_; DISALLOW_COPY_AND_ASSIGN(TestClientConnection); @@ -178,19 +179,20 @@ class TestConnectionManagerDelegate : public ConnectionManagerDelegate { class TestViewManagerRootConnection : public ViewManagerRootConnection { public: - TestViewManagerRootConnection( - scoped_ptr root, - ConnectionManager* manager) - : ViewManagerRootConnection(root.Pass(), manager) { + TestViewManagerRootConnection(scoped_ptr root, + ConnectionManager* manager) + : ViewManagerRootConnection(root.Pass(), manager) {} + ~TestViewManagerRootConnection() override {} + + private: + // ViewManagerRootDelegate: + void OnDisplayInitialized() override { connection_manager()->AddRoot(this); set_view_manager_service(connection_manager()->EmbedAtView( kInvalidConnectionId, view_manager_root()->root_view()->id(), mojo::ViewManagerClientPtr())); } - ~TestViewManagerRootConnection() override {} - - private: DISALLOW_COPY_AND_ASSIGN(TestViewManagerRootConnection); }; @@ -202,7 +204,15 @@ class TestDisplayManager : public DisplayManager { ~TestDisplayManager() override {} // DisplayManager: - void Init(DisplayManagerDelegate* delegate) override {} + void Init(DisplayManagerDelegate* delegate) override { + // It is necessary to tell the delegate about the ViewportMetrics to make + // sure that the ViewManagerRootConnection is correctly initialized (and a + // root-view is created). + mojo::ViewportMetrics metrics; + metrics.size_in_pixels = mojo::Size::From(gfx::Size(400, 300)); + metrics.device_pixel_ratio = 1.f; + delegate->OnViewportMetricsChanged(mojo::ViewportMetrics(), metrics); + } void SchedulePaint(const ServerView* view, const gfx::Rect& bounds) override { } void SetViewportSize(const gfx::Size& size) override {} @@ -286,16 +296,14 @@ class ViewManagerServiceTest : public testing::Test { DisplayManager::set_factory_for_testing(&display_manager_factory_); // TODO(fsamuel): This is probably broken. We need a root. connection_manager_.reset(new ConnectionManager(&delegate_)); - scoped_ptr root( - new ViewManagerRootImpl(connection_manager_.get(), - true /* is_headless */, - nullptr, - scoped_refptr())); + ViewManagerRootImpl* root = new ViewManagerRootImpl( + connection_manager_.get(), true /* is_headless */, nullptr, + scoped_refptr()); // TODO(fsamuel): This is way too magical. We need to find a better way to // manage lifetime. - root_connection_ = - new TestViewManagerRootConnection(root.Pass(), - connection_manager_.get()); + root_connection_ = new TestViewManagerRootConnection( + make_scoped_ptr(root), connection_manager_.get()); + root->Init(root_connection_); wm_client_ = delegate_.last_client(); } diff --git a/mandoline/ui/browser/browser.cc b/mandoline/ui/browser/browser.cc index 5acf8ae25571..4f1683867542 100644 --- a/mandoline/ui/browser/browser.cc +++ b/mandoline/ui/browser/browser.cc @@ -58,7 +58,25 @@ void Browser::ReplaceContentWithRequest(mojo::URLRequestPtr request) { Embed(request.Pass()); } -void Browser::OnDevicePixelRatioAvailable() { +mojo::ApplicationConnection* Browser::GetViewManagerConnectionForTesting() { + return view_manager_init_.connection(); +} + +void Browser::OnEmbed(mojo::View* root) { + // Browser does not support being embedded more than once. + CHECK(!root_); + + // Make it so we get OnWillEmbed() for any Embed()s done by other apps we + // Embed(). + root->view_manager()->SetEmbedRoot(); + + // TODO(beng): still unhappy with the fact that both this class & the UI class + // know so much about these views. Figure out how to shift more to + // the UI class. + root_ = root; + + delegate_->InitUIIfNecessary(this, root_); + content_ = root_->view_manager()->CreateView(); ui_->Init(root_); @@ -81,28 +99,6 @@ void Browser::OnDevicePixelRatioAvailable() { } } -mojo::ApplicationConnection* Browser::GetViewManagerConnectionForTesting() { - return view_manager_init_.connection(); -} - -void Browser::OnEmbed(mojo::View* root) { - // Browser does not support being embedded more than once. - CHECK(!root_); - - // Make it so we get OnWillEmbed() for any Embed()s done by other apps we - // Embed(). - root->view_manager()->SetEmbedRoot(); - - // TODO(beng): still unhappy with the fact that both this class & the UI class - // know so much about these views. Figure out how to shift more to - // the UI class. - root_ = root; - - if (!delegate_->InitUIIfNecessary(this, root_)) - return; // We'll be called back from OnDevicePixelRatioAvailable(). - OnDevicePixelRatioAvailable(); -} - void Browser::OnEmbedForDescendant(mojo::View* view, mojo::URLRequestPtr request, mojo::ViewManagerClientPtr* client) { diff --git a/mandoline/ui/browser/browser.h b/mandoline/ui/browser/browser.h index c5c13d29e6dc..6e4a760e1bc4 100644 --- a/mandoline/ui/browser/browser.h +++ b/mandoline/ui/browser/browser.h @@ -54,12 +54,6 @@ class Browser : public mojo::ViewManagerDelegate, const GURL& current_url() const { return current_url_; } - // Called once a valid device_pixel_ratio is determined. We gate construction - // of the UI until the device_pixel_ratio is available as it's necessary to - // properly setup the ui. - // TODO(sky): remove this. Only here until we move to viewmanager. - void OnDevicePixelRatioAvailable(); - private: FRIEND_TEST_ALL_PREFIXES(BrowserTest, ClosingBrowserClosesAppConnection); FRIEND_TEST_ALL_PREFIXES(BrowserTest, TwoBrowsers); diff --git a/mandoline/ui/browser/browser_apptest.cc b/mandoline/ui/browser/browser_apptest.cc index 25cd9d716c10..f99a842ceb0c 100644 --- a/mandoline/ui/browser/browser_apptest.cc +++ b/mandoline/ui/browser/browser_apptest.cc @@ -106,9 +106,7 @@ class BrowserTest : public mojo::test::ApplicationTestBase, } } - bool InitUIIfNecessary(Browser* browser, mojo::View* root_view) override { - return true; - } + void InitUIIfNecessary(Browser* browser, mojo::View* root_view) override {} private: mojo::ApplicationImpl* app_; diff --git a/mandoline/ui/browser/browser_delegate.h b/mandoline/ui/browser/browser_delegate.h index 51985f67555b..3446a2e059fd 100644 --- a/mandoline/ui/browser/browser_delegate.h +++ b/mandoline/ui/browser/browser_delegate.h @@ -21,9 +21,8 @@ class BrowserDelegate { // opportunity to perform some cleanup. virtual void BrowserClosed(Browser* browser) = 0; - // Requests initialization of state to display the browser on screen. Returns - // whether UI was initialized at this point. - virtual bool InitUIIfNecessary(Browser* browser, mojo::View* root_view) = 0; + // Requests initialization of state to display the browser on screen. + virtual void InitUIIfNecessary(Browser* browser, mojo::View* root_view) = 0; protected: virtual ~BrowserDelegate() {} diff --git a/mandoline/ui/browser/browser_manager.cc b/mandoline/ui/browser/browser_manager.cc index 97782e731a24..527e2652a951 100644 --- a/mandoline/ui/browser/browser_manager.cc +++ b/mandoline/ui/browser/browser_manager.cc @@ -17,46 +17,6 @@ const char kGoogleURL[] = "http://www.google.com"; } // namespace -// TODO(sky): make ViewManager not do anything until device_pixel_ratio is -// determined. At which point this can be nuked. -class BrowserManager::DevicePixelRatioWaiter : mojo::ViewObserver { - public: - DevicePixelRatioWaiter(BrowserManager* manager, - Browser* browser, - mojo::View* view) - : manager_(manager), browser_(browser), view_(view) { - view_->AddObserver(this); - } - ~DevicePixelRatioWaiter() override { RemoveObserver(); } - - private: - void RemoveObserver() { - if (!view_) - return; - view_->RemoveObserver(this); - view_ = nullptr; - } - - // mojo::ViewObserver: - void OnViewViewportMetricsChanged( - mojo::View* view, - const mojo::ViewportMetrics& old_metrics, - const mojo::ViewportMetrics& new_metrics) override { - if (new_metrics.device_pixel_ratio == 0) - return; - - RemoveObserver(); - manager_->OnDevicePixelRatioAvailable(browser_, view); - } - void OnViewDestroyed(mojo::View* view) override { RemoveObserver(); } - - BrowserManager* manager_; - Browser* browser_; - mojo::View* view_; - - DISALLOW_COPY_AND_ASSIGN(DevicePixelRatioWaiter); -}; - BrowserManager::BrowserManager() : app_(nullptr) { } @@ -72,16 +32,6 @@ Browser* BrowserManager::CreateBrowser(const GURL& default_url) { return browser; } -void BrowserManager::OnDevicePixelRatioAvailable(Browser* browser, - mojo::View* view) { - device_pixel_ratio_waiter_.reset(); -#if defined(USE_AURA) - DCHECK(!aura_init_.get()); - aura_init_.reset(new AuraInit(view, app_->shell())); -#endif - browser->OnDevicePixelRatioAvailable(); -} - void BrowserManager::LaunchURL(const mojo::String& url) { DCHECK(!browsers_.empty()); mojo::URLRequestPtr request(mojo::URLRequest::New()); @@ -120,18 +70,12 @@ void BrowserManager::BrowserClosed(Browser* browser) { app_->Terminate(); } -bool BrowserManager::InitUIIfNecessary(Browser* browser, mojo::View* view) { - if (view->viewport_metrics().device_pixel_ratio > 0) { +void BrowserManager::InitUIIfNecessary(Browser* browser, mojo::View* view) { + DCHECK_GT(view->viewport_metrics().device_pixel_ratio, 0); #if defined(USE_AURA) - if (!aura_init_) - aura_init_.reset(new AuraInit(view, app_->shell())); + if (!aura_init_) + aura_init_.reset(new AuraInit(view, app_->shell())); #endif - return true; - } - DCHECK(!device_pixel_ratio_waiter_.get()); - device_pixel_ratio_waiter_.reset( - new DevicePixelRatioWaiter(this, browser, view)); - return false; } void BrowserManager::Create(mojo::ApplicationConnection* connection, diff --git a/mandoline/ui/browser/browser_manager.h b/mandoline/ui/browser/browser_manager.h index 11334e494b5c..10be6f48554f 100644 --- a/mandoline/ui/browser/browser_manager.h +++ b/mandoline/ui/browser/browser_manager.h @@ -41,10 +41,6 @@ class BrowserManager : public mojo::ApplicationDelegate, Browser* CreateBrowser(const GURL& default_url); private: - class DevicePixelRatioWaiter; - - void OnDevicePixelRatioAvailable(Browser* browser, mojo::View* view); - // Overridden from LaunchHandler: void LaunchURL(const mojo::String& url) override; @@ -54,7 +50,7 @@ class BrowserManager : public mojo::ApplicationDelegate, mojo::ApplicationConnection* connection) override; // Overridden from BrowserDelegate: - bool InitUIIfNecessary(Browser* browser, mojo::View* view) override; + void InitUIIfNecessary(Browser* browser, mojo::View* view) override; void BrowserClosed(Browser* browser) override; // Overridden from mojo::InterfaceFactory: @@ -69,8 +65,6 @@ class BrowserManager : public mojo::ApplicationDelegate, mojo::WeakBindingSet launch_handler_bindings_; std::set browsers_; - scoped_ptr device_pixel_ratio_waiter_; - DISALLOW_COPY_AND_ASSIGN(BrowserManager); }; -- 2.11.4.GIT