New issue
Advanced search Search tips

Issue 590981 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Content embedders shouldn't need to call WebContents::WasShown / WasHidden

Project Member Reported by tapted@chromium.org, Mar 1 2016

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
 
Labels: -OS-Mac M-51 OS-All
You started fixing this bug over two years ago. Are you still working on it? 
Cc: tapted@chromium.org
Components: UI>Browser>TabContents
Labels: -Pri-2 Pri-3
Owner: ----
Status: Available (was: Started)
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