Remove WebContents::Was{Hidden,Shown}() from the content public interface

Remove WebContents::Was{Hidden,Shown}() from the content public interface

The WebContentsView typically knows best when this should be invoked.
Excluding tests, Mac's TabStripController is currently the only place in
Chrome that invokes WebContents::WasShown().

Tests use WasShown() and WasHidden() in simulations, so provide access
for tests via content::WebContentsTester.

Then, WebContents::WasHidden() otherwise 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.

BUG=590981
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

[tapted, 2016-02-29 05:04:39]

Description was changed from

==========
Remove WebContents::Was{Hidden,Shown}() from the content public interface

The WebContentsView typically knows best when this should be invoked.
Excluding tests, Mac's TabStripController is currently the only place in
Chrome that invokes WebContents::WasShown().

Tests use WasShown() and WasHidden() in simulations, so provide access
for tests via content::WebContentsTester.

WebContents::WasHidden() otherwise 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.

BUG=
==========

to

==========
Remove WebContents::Was{Hidden,Shown}() from the content public interface

The WebContentsView typically knows best when this should be invoked.
Excluding tests, Mac's TabStripController is currently the only place in
Chrome that invokes WebContents::WasShown().

Tests use WasShown() and WasHidden() in simulations, so provide access
for tests via content::WebContentsTester.

WebContents::WasHidden() otherwise 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.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[tapted, 2016-02-29 06:49:36]

Description was changed from

==========
Remove WebContents::Was{Hidden,Shown}() from the content public interface

The WebContentsView typically knows best when this should be invoked.
Excluding tests, Mac's TabStripController is currently the only place in
Chrome that invokes WebContents::WasShown().

Tests use WasShown() and WasHidden() in simulations, so provide access
for tests via content::WebContentsTester.

WebContents::WasHidden() otherwise 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.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove WebContents::Was{Hidden,Shown}() from the content public interface

The WebContentsView typically knows best when this should be invoked.
Excluding tests, Mac's TabStripController is currently the only place in
Chrome that invokes WebContents::WasShown().

Tests use WasShown() and WasHidden() in simulations, so provide access
for tests via content::WebContentsTester.

Then, WebContents::WasHidden() otherwise 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.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2016-02-29 17:36:35]

jam@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2016-02-29 17:36:36]

https://codereview.chromium.org/1743143002/diff/20001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1743143002/diff/20001/content/public/browser/...
content/public/browser/web_contents.h:387: virtual void MarkBackgrounded() = 0;
what's the other analogous method?

i.e. it seems this is for undoing a previous call.where is that call?
shouldn't they be together?

[tapted, 2016-02-29 22:42:33]

This CL was a kind of exploration to scope out the problem.  I think this
comment will publish the CL out - it's not for a final review yet, but I'm glad
you saw this in its early stages :).

Please see my answer to your question below.

However, I think before this CL, we should act on those `TODOs` that the CL adds
as annotations. i.e. remove the call sites where Chrome calls `WasHidden` when
it doesn't need to. Then `MarkBackgrounded` should become unnecessary to land
this.

https://codereview.chromium.org/1743143002/diff/20001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1743143002/diff/20001/content/public/browser/...
content/public/browser/web_contents.h:387: virtual void MarkBackgrounded() = 0;
On 2016/02/29 17:36:36, jam wrote:
> what's the other analogous method?
> 
> i.e. it seems this is for undoing a previous call.where is that call?
> shouldn't they be together?

Ah, that's the thing :). `MarkBackgrounded` actually shouldn't exist, and
embedders don't really need the analogous method. That's because they're
currently using calls to WebContents::WasHidden() shortly after creating a
WebContents, but with that WebContents created with
WebContents::InitParams::initially_hidden set to false, rather than true. For
Aura platforms, the `WasShown` call to balance a WasHidden is always made by the
content layer, by WebContentsView. (and http://crrev.com/1733173002 makes that
true for Mac as well as Aura).

Exposing `WasHidden` can lead to bugs - e.g. http://crbug.com/590476 which came
up recently. Part of the issue is that calls to `WasHidden` (or MarkBackgrounded
which this CL renames it to) can be called on WebContents that are already
hidden.

This CL annotates all the `WasHidden` calls that Chrome was making. I think they
can all be removed with appropriate use of
WebContents::InitParams::initially_hidden . (Or perhaps
`InitParams::initial_state = {BACKGROUND_PRELOAD, FOREGROUND}` if that's
clearer. Maybe there's a use case for `UNLOADED` as well, but there doesn't seem
to be).

[tapted, 2016-03-01 06:48:40]

Description was changed from

==========
Remove WebContents::Was{Hidden,Shown}() from the content public interface

The WebContentsView typically knows best when this should be invoked.
Excluding tests, Mac's TabStripController is currently the only place in
Chrome that invokes WebContents::WasShown().

Tests use WasShown() and WasHidden() in simulations, so provide access
for tests via content::WebContentsTester.

Then, WebContents::WasHidden() otherwise 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.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove WebContents::Was{Hidden,Shown}() from the content public interface

The WebContentsView typically knows best when this should be invoked.
Excluding tests, Mac's TabStripController is currently the only place in
Chrome that invokes WebContents::WasShown().

Tests use WasShown() and WasHidden() in simulations, so provide access
for tests via content::WebContentsTester.

Then, WebContents::WasHidden() otherwise 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.

BUG=590981
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[tapted, 2016-03-01 08:36:33]

On 2016/02/29 22:42:33, tapted wrote:
> However, I think before this CL, we should act on those `TODOs` that the CL
adds
> as annotations. i.e. remove the call sites where Chrome calls `WasHidden` when
> it doesn't need to. Then `MarkBackgrounded` should become unnecessary to land
> this.

So removing the first couple of unnecessary `WasHidden` calls seems
straightforward -- https://codereview.chromium.org/1747143002/ -- but I want to
add a test for the (tiny) RenderWidgetHostViewMac change I needed there.

I've also added a BUG= -> http://crbug.com/590981 about this. E.g. I think
RenderWidgetHostView::Show/Hide can be removed from the public API too. And
e.g., RenderWidgetHostView says "It is not legal to call Hide() multiple times
in a row.", but with WebContents::WasHidden exposed, this is easy to do (and
chrome does it).

[jam, 2016-03-14 16:06:29]

On 2016/02/29 22:42:33, tapted wrote:
> This CL was a kind of exploration to scope out the problem.  I think this
> comment will publish the CL out - it's not for a final review yet, but I'm
glad
> you saw this in its early stages :).
> 
> Please see my answer to your question below.
> 
> However, I think before this CL, we should act on those `TODOs` that the CL
adds
> as annotations. i.e. remove the call sites where Chrome calls `WasHidden` when
> it doesn't need to. Then `MarkBackgrounded` should become unnecessary to land
> this.
> 
>
https://codereview.chromium.org/1743143002/diff/20001/content/public/browser/...
> File content/public/browser/web_contents.h (right):
> 
>
https://codereview.chromium.org/1743143002/diff/20001/content/public/browser/...
> content/public/browser/web_contents.h:387: virtual void MarkBackgrounded() =
0;
> On 2016/02/29 17:36:36, jam wrote:
> > what's the other analogous method?
> > 
> > i.e. it seems this is for undoing a previous call.where is that call?
> > shouldn't they be together?
> 
> Ah, that's the thing :). `MarkBackgrounded` actually shouldn't exist, and
> embedders don't really need the analogous method. That's because they're
> currently using calls to WebContents::WasHidden() shortly after creating a
> WebContents, but with that WebContents created with
> WebContents::InitParams::initially_hidden set to false, rather than true. For
> Aura platforms, the `WasShown` call to balance a WasHidden is always made by
the
> content layer, by WebContentsView. (and http://crrev.com/1733173002 makes that
> true for Mac as well as Aura).
> 
> Exposing `WasHidden` can lead to bugs - e.g. http://crbug.com/590476 which
came
> up recently. Part of the issue is that calls to `WasHidden` (or
MarkBackgrounded
> which this CL renames it to) can be called on WebContents that are already
> hidden.
> 
> This CL annotates all the `WasHidden` calls that Chrome was making. I think
they
> can all be removed with appropriate use of
> WebContents::InitParams::initially_hidden . (Or perhaps
> `InitParams::initial_state = {BACKGROUND_PRELOAD, FOREGROUND}` if that's
> clearer. Maybe there's a use case for `UNLOADED` as well, but there doesn't
seem
> to be).

sounds good to me, thanks for the explanation
(Avi had pointed me to this cl, hence the comment on the unpublished review :) )