diff options
author | sky <sky@chromium.org> | 2015-08-04 09:51:21 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-08-04 16:54:18 +0000 |
commit | f5fdada71957d7ce856045b6ddfb0433a1e187c0 (patch) | |
tree | 3929e30ab751e2d4161025f012897332a5e08154 /components/html_viewer | |
parent | 9b6e596f7a3c246b7e895b2816fe35496a5a9262 (diff) | |
download | chromium_src-f5fdada71957d7ce856045b6ddfb0433a1e187c0.zip chromium_src-f5fdada71957d7ce856045b6ddfb0433a1e187c0.tar.gz chromium_src-f5fdada71957d7ce856045b6ddfb0433a1e187c0.tar.bz2 |
Fixes shutdown with multiple documents using the same frame tree
web_widget_->close() isn't always enough.
BUG=479172,490221
TEST=covered by tests
R=ben@chromium.org
Review URL: https://codereview.chromium.org/1269503006
Cr-Commit-Position: refs/heads/master@{#341738}
Diffstat (limited to 'components/html_viewer')
-rw-r--r-- | components/html_viewer/html_frame.cc | 60 | ||||
-rw-r--r-- | components/html_viewer/html_frame.h | 7 | ||||
-rw-r--r-- | components/html_viewer/html_frame_tree_manager.cc | 3 |
3 files changed, 43 insertions, 27 deletions
diff --git a/components/html_viewer/html_frame.cc b/components/html_viewer/html_frame.cc index a311fa6..ed879d0 100644 --- a/components/html_viewer/html_frame.cc +++ b/components/html_viewer/html_frame.cc @@ -135,7 +135,7 @@ void HTMLFrame::Init(mojo::View* local_view, // TODO(sky): need to plumb through scope and other args correctly for frame // creation. if (!parent_) { - CreateWebWidget(); + CreateRootWebWidget(); // This is the root of the tree (aka the main frame). // Expected order for creating webframes is: // . Create local webframe (first webframe must always be local). @@ -145,7 +145,6 @@ void HTMLFrame::Init(mojo::View* local_view, blink::WebLocalFrame::create(blink::WebTreeScopeType::Document, this); // We need to set the main frame before creating children so that state is // properly set up in blink. - // TODO(sky): I don't like these casts. web_view()->setMainFrame(local_web_frame); const gfx::Size size_in_pixels(local_view->bounds().width, local_view->bounds().height); @@ -161,7 +160,7 @@ void HTMLFrame::Init(mojo::View* local_view, web_frame_ = remote_web_frame; } } else if (local_view && id_ == local_view->id()) { - // Frame represents the local frame. + // Frame represents the local frame, and it isn't the root of the tree. HTMLFrame* previous_sibling = GetPreviousSibling(this); blink::WebFrame* previous_web_frame = previous_sibling ? previous_sibling->web_frame() : nullptr; @@ -169,15 +168,16 @@ void HTMLFrame::Init(mojo::View* local_view, web_frame_ = parent_->web_frame()->toWebRemoteFrame()->createLocalChild( blink::WebTreeScopeType::Document, "", blink::WebSandboxFlags::None, this, previous_web_frame); - CreateWebWidget(); + CreateLocalRootWebWidget(web_frame_->toWebLocalFrame()); } else if (!parent_->IsLocal()) { web_frame_ = parent_->web_frame()->toWebRemoteFrame()->createRemoteChild( blink::WebTreeScopeType::Document, remote_frame_name, blink::WebSandboxFlags::None, this); } else { - // This is hit if we're asked to create a new local frame with a local - // parent. This should never happen (if we create a local child we don't - // call Init()). + // This is hit if we're asked to create a child frame of a local parent. + // This should never happen (if we create a local child we don't call + // Init(), and the frame server should not being creating child frames of + // this frame). NOTREACHED(); } @@ -193,8 +193,14 @@ void HTMLFrame::Init(mojo::View* local_view, void HTMLFrame::Close() { if (web_widget_) { - // Closing the widget implicitly detaches the frame. + // Closing the root widget (WebView) implicitly detaches. For children + // (which have a WebFrameWidget) a detach() is required. Use a temporary + // as if 'this' is the root the call to web_widget_->close() deletes + // 'this'. + const bool is_child = parent_ != nullptr; web_widget_->close(); + if (is_child) + web_frame_->detach(); } else { web_frame_->detach(); } @@ -286,26 +292,34 @@ mojo::ApplicationImpl* HTMLFrame::GetLocalRootApp() { } void HTMLFrame::SetView(mojo::View* view) { - // TODO(sky): figure out way to cleanup view. + // TODO(sky): figure out way to cleanup view. In particular there may already + // be a view. This happens if we go from local->remote->local. if (view_) view_->RemoveObserver(this); view_ = view; view_->AddObserver(this); } -void HTMLFrame::CreateWebWidget() { +void HTMLFrame::CreateRootWebWidget() { DCHECK(!web_widget_); - if (parent_) { - // TODO(sky): this isn't quite right. I should only have a WebFrameWidget - // for local roots. And the cast to local fram definitely isn't right. - web_widget_ = - blink::WebFrameWidget::create(this, web_frame_->toWebLocalFrame()); - } else if (view_ && view_->id() == id_) { - web_widget_ = blink::WebView::create(this); - } else { - web_widget_ = blink::WebView::create(nullptr); - } + blink::WebViewClient* web_view_client = + (view_ && view_->id() == id_) ? this : nullptr; + web_widget_ = blink::WebView::create(web_view_client); + + InitializeWebWidget(); + + ConfigureSettings(web_view()->settings()); +} +void HTMLFrame::CreateLocalRootWebWidget(blink::WebLocalFrame* local_frame) { + DCHECK(!web_widget_); + DCHECK(IsLocal()); + web_widget_ = blink::WebFrameWidget::create(this, local_frame); + + InitializeWebWidget(); +} + +void HTMLFrame::InitializeWebWidget() { // Creating the widget calls initializeLayerTreeView() to create the // |web_layer_tree_view_impl_|. As we haven't yet assigned the |web_widget_| // we have to set it here. @@ -314,9 +328,6 @@ void HTMLFrame::CreateWebWidget() { web_layer_tree_view_impl_->set_view(view_); UpdateWebViewSizeFromViewSize(); } - - if (web_view()) - ConfigureSettings(web_view()->settings()); } void HTMLFrame::UpdateFocus() { @@ -359,7 +370,8 @@ void HTMLFrame::FinishSwapToRemote() { blink::WebRemoteFrame* remote_frame = blink::WebRemoteFrame::create(scope_, this); remote_frame->initializeFromFrame(web_frame_->toWebLocalFrame()); - // swap() ends up calling us back and we then close the frame. + // swap() ends up calling us back and we then close the frame, which deletes + // it. web_frame_->swap(remote_frame); web_layer_.reset(new WebLayerImpl(this)); remote_frame->setRemoteWebLayer(web_layer_.get()); diff --git a/components/html_viewer/html_frame.h b/components/html_viewer/html_frame.h index 97e7f4e..58005fe 100644 --- a/components/html_viewer/html_frame.h +++ b/components/html_viewer/html_frame.h @@ -136,7 +136,10 @@ class HTMLFrame : public blink::WebFrameClient, void SetView(mojo::View* view); // Creates the appropriate WebWidget implementation for the Frame. - void CreateWebWidget(); + void CreateRootWebWidget(); + void CreateLocalRootWebWidget(blink::WebLocalFrame* local_frame); + + void InitializeWebWidget(); void UpdateFocus(); @@ -231,6 +234,8 @@ class HTMLFrame : public blink::WebFrameClient, HTMLFrameTreeManager* frame_tree_manager_; HTMLFrame* parent_; + // |view_| is non-null for local frames or remote frames that were once + // local. mojo::View* view_; // The id for this frame. If there is a view, this is the same id as the // view has. diff --git a/components/html_viewer/html_frame_tree_manager.cc b/components/html_viewer/html_frame_tree_manager.cc index e7252b6..2c080ae 100644 --- a/components/html_viewer/html_frame_tree_manager.cc +++ b/components/html_viewer/html_frame_tree_manager.cc @@ -126,8 +126,7 @@ HTMLFrameTreeManager::HTMLFrameTreeManager(GlobalState* global_state) : global_state_(global_state), root_(nullptr), local_root_(nullptr) {} HTMLFrameTreeManager::~HTMLFrameTreeManager() { - DCHECK(!root_); - DCHECK(!local_root_); + DCHECK(!root_ || !local_root_); RemoveFromInstances(); } |