Mac: Use Mavericks occlusion APIs for power savings.

Mac: Use Mavericks occlusion APIs for power savings.

Use NSWindowDidChangeOcclusionStateNotification (new in 10.9 Mavericks) to
detect when the application is hidden, the window is minimized or obscured by
another window.

BUG=310374

[Andre, 2014-10-27 22:47:25]

andresantoso@chromium.org changed reviewers:
+ ccameron@chromium.org, miu@chromium.org

[Andre, 2014-10-27 22:47:25]

miu@, ccameron@, PTAL.
I'm not familiar with tab capture, do I need to do something special to account
for it?

[ccameron, 2014-10-27 22:54:50]

This may be too heavyweight -- the call to WasHidden will cause the CALayer
displaying contents to be destroyed -- this will probably result in the window
flashing white briefly if it is quickly un-occluded.

If indeed this doesn't cause any issues, then that's great (and this lgtm), but
be sure to test it quite a bit locally.

[Andre, 2014-10-27 23:18:06]

On 2014/10/27 22:54:50, ccameron1 wrote:
> This may be too heavyweight -- the call to WasHidden will cause the CALayer
> displaying contents to be destroyed -- this will probably result in the window
> flashing white briefly if it is quickly un-occluded.
> 
> If indeed this doesn't cause any issues, then that's great (and this lgtm),
but
> be sure to test it quite a bit locally.

Uploaded patch 2 to avoid destroying the layer of occluded active tab.

[ccameron, 2014-10-27 23:44:37]

On 2014/10/27 23:18:06, Andre wrote:
> On 2014/10/27 22:54:50, ccameron1 wrote:
> > This may be too heavyweight -- the call to WasHidden will cause the CALayer
> > displaying contents to be destroyed -- this will probably result in the
window
> > flashing white briefly if it is quickly un-occluded.
> > 
> > If indeed this doesn't cause any issues, then that's great (and this lgtm),
> but
> > be sure to test it quite a bit locally.
> 
> Uploaded patch 2 to avoid destroying the layer of occluded active tab.

Hmm, looks like there are issues with tab capture. Taking a look at them

[ccameron, 2014-10-28 00:37:32]

On 2014/10/27 23:44:37, ccameron1 wrote:
> On 2014/10/27 23:18:06, Andre wrote:
> > On 2014/10/27 22:54:50, ccameron1 wrote:
> > > This may be too heavyweight -- the call to WasHidden will cause the
CALayer
> > > displaying contents to be destroyed -- this will probably result in the
> window
> > > flashing white briefly if it is quickly un-occluded.
> > > 
> > > If indeed this doesn't cause any issues, then that's great (and this
lgtm),
> > but
> > > be sure to test it quite a bit locally.
> > 
> > Uploaded patch 2 to avoid destroying the layer of occluded active tab.
> 
> Hmm, looks like there are issues with tab capture. Taking a look at them

Added instructions to reproduce the issue to the bug. I don't immediately see
where the issue is originating.

[miu, 2014-10-28 04:01:14]

On 2014/10/28 00:37:32, ccameron1 wrote:
> On 2014/10/27 23:44:37, ccameron1 wrote:
> > On 2014/10/27 23:18:06, Andre wrote:
> > > On 2014/10/27 22:54:50, ccameron1 wrote:
> > > > This may be too heavyweight -- the call to WasHidden will cause the
> CALayer
> > > > displaying contents to be destroyed -- this will probably result in the
> > window
> > > > flashing white briefly if it is quickly un-occluded.
> > > > 
> > > > If indeed this doesn't cause any issues, then that's great (and this
> lgtm),
> > > but
> > > > be sure to test it quite a bit locally.
> > > 
> > > Uploaded patch 2 to avoid destroying the layer of occluded active tab.
> > 
> > Hmm, looks like there are issues with tab capture. Taking a look at them
> 
> Added instructions to reproduce the issue to the bug. I don't immediately see
> where the issue is originating.

The problem is that this change circumvents the logic in
WebContentsImpl::WasHidden() and WebContentsImpl::DecrementCapturerCount(). 
Note that it's not just about tab capture, but all the WebContentsObservers that
respond override WebContentsObserver::WasHidden().

Have you considered moving this functionality to a higher abstraction layer? 
For example, it may work well to have this in
chrome/browser/ui/cocoa/tabs/tab_controller.mm, with code there to call
WebContents::WasShown/Hidden() in response to occlusion notifications.

[Andre, 2014-10-28 18:20:56]

On 2014/10/28 04:01:14, miu wrote:
> On 2014/10/28 00:37:32, ccameron1 wrote:
> > On 2014/10/27 23:44:37, ccameron1 wrote:
> > > On 2014/10/27 23:18:06, Andre wrote:
> > > > On 2014/10/27 22:54:50, ccameron1 wrote:
> > > > > This may be too heavyweight -- the call to WasHidden will cause the
> > CALayer
> > > > > displaying contents to be destroyed -- this will probably result in
the
> > > window
> > > > > flashing white briefly if it is quickly un-occluded.
> > > > > 
> > > > > If indeed this doesn't cause any issues, then that's great (and this
> > lgtm),
> > > > but
> > > > > be sure to test it quite a bit locally.
> > > > 
> > > > Uploaded patch 2 to avoid destroying the layer of occluded active tab.
> > > 
> > > Hmm, looks like there are issues with tab capture. Taking a look at them
> > 
> > Added instructions to reproduce the issue to the bug. I don't immediately
see
> > where the issue is originating.
> 
> The problem is that this change circumvents the logic in
> WebContentsImpl::WasHidden() and WebContentsImpl::DecrementCapturerCount(). 
> Note that it's not just about tab capture, but all the WebContentsObservers
that
> respond override WebContentsObserver::WasHidden().
> 
> Have you considered moving this functionality to a higher abstraction layer? 
> For example, it may work well to have this in
> chrome/browser/ui/cocoa/tabs/tab_controller.mm, with code there to call
> WebContents::WasShown/Hidden() in response to occlusion notifications.

I think we should do this so that it will also work on the (experimental) views
toolkit based Mac Chrome (MacViews project), and even on Mac content shell.
WDYT about patch #3?
I still need to figure out why the content fades in after getting un-occluded.

[miu, 2014-10-28 20:15:06]

On 2014/10/28 18:20:56, Andre wrote:
> I think we should do this so that it will also work on the (experimental)
views
> toolkit based Mac Chrome (MacViews project), and even on Mac content shell.
> WDYT about patch #3?
> I still need to figure out why the content fades in after getting un-occluded.

Good point.  If you are taking this approach, however, I'd suggest adding a new
method in RenderWidgetHostDelegate (WebContentsImpl is an implementer of this
interface) to *request* hiding/showing to optimize for occlusion.  Then, the
implementation of the delegate interface rightly contains the logic to take
action iff the situation permits.  The code in RWHVMac would then become
something like:

  - (void)windowChangedOcclusionState:(NSNotification*)notification {
    DCHECK(base::mac::IsOSMavericksOrLater());
    if (!host_ || !host_->delegate())
      return;
    NSWindow* window = [notification object];
    host_->delegate()->SetViewIsOccluded(!([window occlusionState] &
NSWindowOcclusionStateVisible));
  }

Side note: All this code in RWHVMac (not your changes) directly calling into
WebContents via GetWebContents() feels very hacky to me.  No other RWHV
implementations do this, and it's actually wrong since not all
RenderWidgetHosts's are RenderViewHost's.  For example, a Flash fullscreen
widget is not a RenderViewHost, but *is* the foreground widget w.r.t. user
interaction and visibility.

Side note 2: Another approach to explore is to put this functionality into
WebContentsViewMac.  This has the advantage of not needing the extra method in
RenderWidgetHostDelegate, since WebContentsView has direct access to its
WebContentsImpl.

[Andre, 2014-10-28 21:30:15]

On 2014/10/28 20:15:06, miu wrote:
> On 2014/10/28 18:20:56, Andre wrote:
> > I think we should do this so that it will also work on the (experimental)
> views
> > toolkit based Mac Chrome (MacViews project), and even on Mac content shell.
> > WDYT about patch #3?
> > I still need to figure out why the content fades in after getting
un-occluded.
> 
> Good point.  If you are taking this approach, however, I'd suggest adding a
new
> method in RenderWidgetHostDelegate (WebContentsImpl is an implementer of this
> interface) to *request* hiding/showing to optimize for occlusion.  Then, the
> implementation of the delegate interface rightly contains the logic to take
> action iff the situation permits.  The code in RWHVMac would then become
> something like:
> 
>   - (void)windowChangedOcclusionState:(NSNotification*)notification {
>     DCHECK(base::mac::IsOSMavericksOrLater());
>     if (!host_ || !host_->delegate())
>       return;
>     NSWindow* window = [notification object];
>     host_->delegate()->SetViewIsOccluded(!([window occlusionState] &
> NSWindowOcclusionStateVisible));
>   }
> 
> Side note: All this code in RWHVMac (not your changes) directly calling into
> WebContents via GetWebContents() feels very hacky to me.  No other RWHV
> implementations do this, and it's actually wrong since not all
> RenderWidgetHosts's are RenderViewHost's.  For example, a Flash fullscreen
> widget is not a RenderViewHost, but *is* the foreground widget w.r.t. user
> interaction and visibility.
> 
> Side note 2: Another approach to explore is to put this functionality into
> WebContentsViewMac.  This has the advantage of not needing the extra method in
> RenderWidgetHostDelegate, since WebContentsView has direct access to its
> WebContentsImpl.

Thanks, that makes sense.
I'm not clear on what we want to do on occlusion vs hidden.
If I understand correctly, hidden means the tab is inactive (not the foreground
tab).
Occlusion means the tab is active, but the window is minimized, hidden or
covered
by another window.
One difference is we don't want to call DestroyBrowserCompositor() on occlusion.
Do we want everything in RenderWidgetHostImpl::WasHidden()? Or only the part
where we send a msg to the renderer?

[Andre, 2014-10-29 20:08:34]

Patchset #4 (id:60001) has been deleted

[miu, 2014-10-30 18:35:32]

Is this change being discontinued?  ...in favor of:
https://codereview.chromium.org/684343002

[Andre, 2014-10-30 18:36:17]

On 2014/10/30 18:35:32, miu wrote:
> Is this change being discontinued?  ...in favor of:
> https://codereview.chromium.org/684343002

Yes. Closing.