Content embedders shouldn't need to call WebContents::WasShown / WasHidden |
||
Issue description... nor should they need to call RenderWidgetHostView::Show()/Hide() for that matter. RenderWidgetHostView has // Shows/hides the view. These must always be called together in pairs. // It is not legal to call Hide() multiple times in a row. virtual void Show() = 0; virtual void Hide() = 0; The RWHV::Show/Hide calls are currently only invoked outside of src/content by extension_view_views.cc https://chromium.googlesource.com/chromium/src/+/4a9d9829bc889e6a2cc02dbc443d3032e1d2c81f/chrome/browser/ui/views/extensions/extension_view_views.cc#49 Which goes views::WebView::SetVisible(is_visible); // Also tell RenderWidgetHostView the new visibility. Despite its name, it // is not part of the View hierarchy and does not know about the change // unless we tell it. /* host_view->Show()/Hide() */ But that smells funny. WebContentsViewAura::OnWindowVisibilityChanged should be doing this since r272662. It will call WebContents::WasHidden() which will trigger a duplicate call to Hide(). (And on Mac, until http://crrev.com/1733173002 WebContents::WasShown/Hidden wouldn't have been called at all, so why does Aura need this. Ngh.) We should just get rid of the public RenderWidgetHostView::Show()/Hide(). Then WebContents::WasShown() is actually not called by chrome except in tests (at least once http://crrev.com/1733173002 lands for Mac). WebContents::WasHidden *is* called, but probably shouldn't be. There's no guarding against duplicate calls to RWHV::Hide(). It has caused known bugs like Issue 590476 . WebContents::WasShown/WasHidden seems to be misused by Chrome code, to move newly-created WebContents to a kind of "background" state. WebContents::CreateParams::initially_hidden should be used for this instead. Really, WebContentsView knows best when this should be invoked. Tests also use WasShown() and WasHidden() in simulations, so we provide access for tests via content::WebContentsTester. See https://codereview.chromium.org/1743143002/ which removes WasShown/WasHidden from the public WebContents API. So, what about the remaining calls in chrome that do WebContents::WasHidden? Well, it looks like they're straightforward to remove after fixing some simple bugs. See, e.g., https://codereview.chromium.org/1747143002/ which removes the WasHidden calls in chrome::AddRestoredTab() and TabLoader::LoadNextTab(). Chrome Version : 50.0.2652.0
,
Jan 11
You started fixing this bug over two years ago. Are you still working on it?
,
Jan 15
I think this is still valid, but the payoff is not high and it's a lot of work. |
||
►
Sign in to add a comment |
||
Comment 1 by tapted@chromium.org
, Mar 1 2016