Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the content layer

Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the content layer

This makes it behave the same way as on Aura platforms, and allows a
views::WebView to not worry about calling WasShown/Hidden on Mac.

After this, WasShown no longer needs to be part of the public
WebContents API. However, a lot of calls are still made in tests, so
they will be transitioned to a test header in a follow-up.

On Mac, WasShown was only called for WebContents in browser Tabs (i.e.
when the TabStripController switched tabs). So this change could (in
theory) affect extensions (e.g. BrowserAction popups), Platform Apps,
and PrintPreview. However, since the windows containing these do not
manipulate their view hierarchy, nothing about the behaviour should
actually change.

BUG=587030
Committed: https://crrev.com/65ff2ea7a5599fa69fed2f2bbae68470af0baf97
Cr-Commit-Position: refs/heads/master@{#378605}

[tapted, 2016-02-25 04:55:12]

Description was changed from

==========
Mac: Make WasShown/WasHidden the responsibility of the content layer

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

to

==========
Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the
content layer

This makes it behave the same way as on Aura platforms.

BUG=587030
==========

[tapted, 2016-02-26 01:59:48]

Description was changed from

==========
Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the
content layer

This makes it behave the same way as on Aura platforms.

BUG=587030
==========

to

==========
Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the
content layer

This makes it behave the same way as on Aura platforms.

After this, WasShown/Hidden no longer need to be part of the public
WebContents API. However, a lot of calls are still made in tests, so
they will be transitioned to a test header in a follow-up.

There will be a behaviour change on Mac: Previously WasShown/Hidden was
only called for WebContents in browser Tabs (i.e. when the
TabStripController switched tabs). After this change, WasShown/Hidden
may also be triggered for extensions (e.g. BrowserAction popups),
Platform Apps, and PrintPreview.

BUG=587030
==========

[tapted, 2016-02-26 02:12:15]

Description was changed from

==========
Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the
content layer

This makes it behave the same way as on Aura platforms.

After this, WasShown/Hidden no longer need to be part of the public
WebContents API. However, a lot of calls are still made in tests, so
they will be transitioned to a test header in a follow-up.

There will be a behaviour change on Mac: Previously WasShown/Hidden was
only called for WebContents in browser Tabs (i.e. when the
TabStripController switched tabs). After this change, WasShown/Hidden
may also be triggered for extensions (e.g. BrowserAction popups),
Platform Apps, and PrintPreview.

BUG=587030
==========

to

==========
Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the
content layer

This makes it behave the same way as on Aura platforms, and allows a
views::WebView to not worry about calling WasShown/Hidden on Mac.

After this, WasShown/Hidden no longer need to be part of the public
WebContents API. However, a lot of calls are still made in tests, so
they will be transitioned to a test header in a follow-up.

On Mac, WasShown/Hidden was only called for WebContents in browser Tabs
(i.e. when the TabStripController switched tabs). So this change could
(in theory) affect extensions (e.g. BrowserAction popups), Platform
Apps, and PrintPreview. However, since the windows containing these do
not manipulate their view hierarchy, nothing should about the behaviour
should actually change.

BUG=587030
==========

[tapted, 2016-02-26 02:12:39]

Patchset #4 (id:60001) has been deleted

[tapted, 2016-02-26 02:19:37]

Description was changed from

==========
Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the
content layer

This makes it behave the same way as on Aura platforms, and allows a
views::WebView to not worry about calling WasShown/Hidden on Mac.

After this, WasShown/Hidden no longer need to be part of the public
WebContents API. However, a lot of calls are still made in tests, so
they will be transitioned to a test header in a follow-up.

On Mac, WasShown/Hidden was only called for WebContents in browser Tabs
(i.e. when the TabStripController switched tabs). So this change could
(in theory) affect extensions (e.g. BrowserAction popups), Platform
Apps, and PrintPreview. However, since the windows containing these do
not manipulate their view hierarchy, nothing should about the behaviour
should actually change.

BUG=587030
==========

to

==========
Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the
content layer

This makes it behave the same way as on Aura platforms, and allows a
views::WebView to not worry about calling WasShown/Hidden on Mac.

After this, WasShown/Hidden no longer need to be part of the public
WebContents API. However, a lot of calls are still made in tests, so
they will be transitioned to a test header in a follow-up.

On Mac, WasShown/Hidden was only called for WebContents in browser Tabs
(i.e. when the TabStripController switched tabs). So this change could
(in theory) affect extensions (e.g. BrowserAction popups), Platform
Apps, and PrintPreview. However, since the windows containing these do
not manipulate their view hierarchy, nothing about the behaviour should
actually change.

BUG=587030
==========

[tapted, 2016-02-26 04:04:10]

Patchset #2 (id:20001) has been deleted

[tapted, 2016-02-26 04:04:17]

Patchset #1 (id:1) has been deleted

[tapted, 2016-02-26 05:41:31]

Patchset #5 (id:140001) has been deleted

[tapted, 2016-02-26 05:42:12]

Patchset #3 (id:100001) has been deleted

[tapted, 2016-02-26 08:20:02]

tapted@chromium.org changed reviewers:
+ avi@chromium.org

[tapted, 2016-02-26 08:20:48]

Hi Avi, please take a look

[Avi (use Gerrit), 2016-02-26 17:50:23]

Naming issues, but I'm liking it.

https://codereview.chromium.org/1733173002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/1733173002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:682: // Remove the old view
first, to ensure ConstraineWindowSheets keyed to the
Typo in "ConstraineWindowSheets".

https://codereview.chromium.org/1733173002/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1733173002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4803: if (!is_or_was_visible_)
{
I'm still struggling here.

This variable starts as false, and gets flipped to true the first time
UpdateWebContentsVisibility is called with true, and then stays as true for the
rest of the WebContents's life? If so, can we name it did_first_set_visible_ or
something like that? There already is a "did_first_visually_non_empty_paint_"
which has a similar flip.

https://codereview.chromium.org/1733173002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4804: // We should not hide
the web contents before it was shown the first time,
"we"?

// If this WebContents has not yet been set to be visible for the first time,
// ignore any requests to make it hidden, since resources [...]

https://codereview.chromium.org/1733173002/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1733173002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1174: // WasShown() message
when first switching to that tab.
This is a rationale of _why_ we need this, but it's not clear what this boolean
holds. Please make the comment explain _what_ is held in this variable, and save
the _why_ for the function that manipulates it.

Perhaps:

// This tracks whether this WebContents was ever set to be visible.
bool did_first_set_visible_;

or something like that. I think that's its meaning, though I'm still not
entirely sure.

https://codereview.chromium.org/1733173002/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_mac.mm (right):

https://codereview.chromium.org/1733173002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_view_mac.mm:615: -
(void)checkWebContentsShowState {
We're not checking it, we're updating it, and I don't know what a "show state"
is.

We could totally reuse the name -updateWebContentsVisibility here, as it works.

https://codereview.chromium.org/1733173002/diff/200001/ui/views/controls/webv...
File ui/views/controls/webview/webview_unittest.cc (right):

https://codereview.chromium.org/1733173002/diff/200001/ui/views/controls/webv...
ui/views/controls/webview/webview_unittest.cc:49: // Provides functionaity to
observe events on a WebContents like WasShown/
Gah, typo in "functionality".

[tapted, 2016-02-29 05:05:34]

Description was changed from

==========
Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the
content layer

This makes it behave the same way as on Aura platforms, and allows a
views::WebView to not worry about calling WasShown/Hidden on Mac.

After this, WasShown/Hidden no longer need to be part of the public
WebContents API. However, a lot of calls are still made in tests, so
they will be transitioned to a test header in a follow-up.

On Mac, WasShown/Hidden was only called for WebContents in browser Tabs
(i.e. when the TabStripController switched tabs). So this change could
(in theory) affect extensions (e.g. BrowserAction popups), Platform
Apps, and PrintPreview. However, since the windows containing these do
not manipulate their view hierarchy, nothing about the behaviour should
actually change.

BUG=587030
==========

to

==========
Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the
content layer

This makes it behave the same way as on Aura platforms, and allows a
views::WebView to not worry about calling WasShown/Hidden on Mac.

After this, WasShown no longer needs to be part of the public
WebContents API. However, a lot of calls are still made in tests, so
they will be transitioned to a test header in a follow-up.

On Mac, WasShown/Hidden was only called for WebContents in browser Tabs
(i.e. when the TabStripController switched tabs). So this change could
(in theory) affect extensions (e.g. BrowserAction popups), Platform
Apps, and PrintPreview. However, since the windows containing these do
not manipulate their view hierarchy, nothing about the behaviour should
actually change.

BUG=587030
==========

[tapted, 2016-02-29 06:43:13]

Description was changed from

==========
Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the
content layer

This makes it behave the same way as on Aura platforms, and allows a
views::WebView to not worry about calling WasShown/Hidden on Mac.

After this, WasShown no longer needs to be part of the public
WebContents API. However, a lot of calls are still made in tests, so
they will be transitioned to a test header in a follow-up.

On Mac, WasShown/Hidden was only called for WebContents in browser Tabs
(i.e. when the TabStripController switched tabs). So this change could
(in theory) affect extensions (e.g. BrowserAction popups), Platform
Apps, and PrintPreview. However, since the windows containing these do
not manipulate their view hierarchy, nothing about the behaviour should
actually change.

BUG=587030
==========

to

==========
Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the
content layer

This makes it behave the same way as on Aura platforms, and allows a
views::WebView to not worry about calling WasShown/Hidden on Mac.

After this, WasShown no longer needs to be part of the public
WebContents API. However, a lot of calls are still made in tests, so
they will be transitioned to a test header in a follow-up.

On Mac, WasShown was only called for WebContents in browser Tabs (i.e.
when the TabStripController switched tabs). So this change could (in
theory) affect extensions (e.g. BrowserAction popups), Platform Apps,
and PrintPreview. However, since the windows containing these do not
manipulate their view hierarchy, nothing about the behaviour should
actually change.

BUG=587030
==========

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

PTAL

I've also explored the follow-up to remove WebShown/WasHidden from the public
interface: https://codereview.chromium.org/1743143002/

And... I said before they were unused outside of tests. Well, I was mainly
checking `WasShown`, and that _is_ the case for WasShown. However, there is a
smattering of calls to 'WasHidden'. Except, they're all really dubious -- I
think there's still a good argument for removing these from the public
interface, since it's effectively being "abused" -- callers are mostly creating
a WebContents then calling `WasHidden()` rather than simply passing
`WebContents::InitParams::initially_hidden = true`. Anyway, I've annotated the
call sites in http://crrev.com/1743143002 and renamed `WasHidden` to
`MarkBackgrounded`. See what you think...

https://codereview.chromium.org/1733173002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/1733173002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:682: // Remove the old view
first, to ensure ConstraineWindowSheets keyed to the
On 2016/02/26 17:50:23, Avi wrote:
> Typo in "ConstraineWindowSheets".

Done.

https://codereview.chromium.org/1733173002/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1733173002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4803: if (!is_or_was_visible_)
{
On 2016/02/26 17:50:23, Avi wrote:
> I'm still struggling here.
> 
> This variable starts as false, and gets flipped to true the first time
> UpdateWebContentsVisibility is called with true, and then stays as true for
the
> rest of the WebContents's life? If so, can we name it did_first_set_visible_
or
> something like that? There already is a "did_first_visually_non_empty_paint_"
> which has a similar flip.

Done.

https://codereview.chromium.org/1733173002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4804: // We should not hide
the web contents before it was shown the first time,
On 2016/02/26 17:50:23, Avi wrote:
> "we"?
> 
> // If this WebContents has not yet been set to be visible for the first time,
> // ignore any requests to make it hidden, since resources [...]

Done.

https://codereview.chromium.org/1733173002/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1733173002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1174: // WasShown() message
when first switching to that tab.
On 2016/02/26 17:50:23, Avi wrote:
> This is a rationale of _why_ we need this, but it's not clear what this
boolean
> holds. Please make the comment explain _what_ is held in this variable, and
save
> the _why_ for the function that manipulates it.
> 
> Perhaps:
> 
> // This tracks whether this WebContents was ever set to be visible.
> bool did_first_set_visible_;
> 
> or something like that. I think that's its meaning, though I'm still not
> entirely sure.

Done. Clarified it a bit. When it was just used for aura, the comment went "On
Windows we can run into problems if resources get released within the
initialization phase while the content (and its dimensions) are not known."
Which is a bit vague.

Also, on Mac, it was clear things were relying on the initial WasShown which
could be suppressed without this.

I did a bit more spelunking - this came about in r280187 - included a bit more
so it's clear what it's for.

https://codereview.chromium.org/1733173002/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_mac.mm (right):

https://codereview.chromium.org/1733173002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_view_mac.mm:615: -
(void)checkWebContentsShowState {
On 2016/02/26 17:50:23, Avi wrote:
> We're not checking it, we're updating it, and I don't know what a "show state"
> is.
> 
> We could totally reuse the name -updateWebContentsVisibility here, as it
works.

Done.

https://codereview.chromium.org/1733173002/diff/200001/ui/views/controls/webv...
File ui/views/controls/webview/webview_unittest.cc (right):

https://codereview.chromium.org/1733173002/diff/200001/ui/views/controls/webv...
ui/views/controls/webview/webview_unittest.cc:49: // Provides functionaity to
observe events on a WebContents like WasShown/
On 2016/02/26 17:50:23, Avi wrote:
> Gah, typo in "functionality".

Done.

[Avi (use Gerrit), 2016-02-29 17:27:24]

lgtm

👍

https://codereview.chromium.org/1733173002/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/1733173002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:683: // old WebContents are
removed before adding new ones.
As a side note, this sounds like a limitation of the constrained window code
that should be fixed. :)

[tapted, 2016-03-01 03:01:44]

tapted@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[tapted, 2016-03-01 03:02:46]

+Paweł for OWNERS in chrome/test/BUILD.gn (just removing a sources += 'file'
which is now in a gyp variable instead)

https://codereview.chromium.org/1733173002/diff/220001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (left):

https://codereview.chromium.org/1733173002/diff/220001/chrome/test/BUILD.gn#o...
chrome/test/BUILD.gn:1740: sources += [
"//ui/views/controls/webview/webview_unittest.cc" ]
(the removed line: no longer needed since this file now appears in
chrome_unit_tests_views_sources)

https://codereview.chromium.org/1733173002/diff/220001/chrome/test/BUILD.gn#o...
chrome/test/BUILD.gn:2056:
chrome_tests_unit_gypi_values.chrome_unit_tests_views_sources,
(which is down here)

[Paweł Hajdan Jr., 2016-03-01 14:38:50]

LGTM

[tapted, 2016-03-01 22:22:24]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2016-03-01 22:23:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1733173002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1733173002/220001

[commit-bot: I haz the power, 2016-03-01 23:39:07]

Description was changed from

==========
Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the
content layer

This makes it behave the same way as on Aura platforms, and allows a
views::WebView to not worry about calling WasShown/Hidden on Mac.

After this, WasShown no longer needs to be part of the public
WebContents API. However, a lot of calls are still made in tests, so
they will be transitioned to a test header in a follow-up.

On Mac, WasShown was only called for WebContents in browser Tabs (i.e.
when the TabStripController switched tabs). So this change could (in
theory) affect extensions (e.g. BrowserAction popups), Platform Apps,
and PrintPreview. However, since the windows containing these do not
manipulate their view hierarchy, nothing about the behaviour should
actually change.

BUG=587030
==========

to

==========
Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the
content layer

This makes it behave the same way as on Aura platforms, and allows a
views::WebView to not worry about calling WasShown/Hidden on Mac.

After this, WasShown no longer needs to be part of the public
WebContents API. However, a lot of calls are still made in tests, so
they will be transitioned to a test header in a follow-up.

On Mac, WasShown was only called for WebContents in browser Tabs (i.e.
when the TabStripController switched tabs). So this change could (in
theory) affect extensions (e.g. BrowserAction popups), Platform Apps,
and PrintPreview. However, since the windows containing these do not
manipulate their view hierarchy, nothing about the behaviour should
actually change.

BUG=587030
==========

[commit-bot: I haz the power, 2016-03-01 23:39:08]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2016-03-01 23:40:54]

Description was changed from

==========
Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the
content layer

This makes it behave the same way as on Aura platforms, and allows a
views::WebView to not worry about calling WasShown/Hidden on Mac.

After this, WasShown no longer needs to be part of the public
WebContents API. However, a lot of calls are still made in tests, so
they will be transitioned to a test header in a follow-up.

On Mac, WasShown was only called for WebContents in browser Tabs (i.e.
when the TabStripController switched tabs). So this change could (in
theory) affect extensions (e.g. BrowserAction popups), Platform Apps,
and PrintPreview. However, since the windows containing these do not
manipulate their view hierarchy, nothing about the behaviour should
actually change.

BUG=587030
==========

to

==========
Mac: Make calling WebContents::WasShown/WasHidden the responsibility of the
content layer

This makes it behave the same way as on Aura platforms, and allows a
views::WebView to not worry about calling WasShown/Hidden on Mac.

After this, WasShown no longer needs to be part of the public
WebContents API. However, a lot of calls are still made in tests, so
they will be transitioned to a test header in a follow-up.

On Mac, WasShown was only called for WebContents in browser Tabs (i.e.
when the TabStripController switched tabs). So this change could (in
theory) affect extensions (e.g. BrowserAction popups), Platform Apps,
and PrintPreview. However, since the windows containing these do not
manipulate their view hierarchy, nothing about the behaviour should
actually change.

BUG=587030

Committed: https://crrev.com/65ff2ea7a5599fa69fed2f2bbae68470af0baf97
Cr-Commit-Position: refs/heads/master@{#378605}
==========

[commit-bot: I haz the power, 2016-03-01 23:40:55]

Patchset 7 (id:??) landed as
https://crrev.com/65ff2ea7a5599fa69fed2f2bbae68470af0baf97
Cr-Commit-Position: refs/heads/master@{#378605}