Mac fullscreen low power video: Add FullscreenLowPowerControllerCocoa

Mac fullscreen low power video: Add FullscreenLowPowerControllerCocoa

This is where the logic for transitioning in and out of fullscreen
low power mode lives. Do not hook up to the BrowserWindowController
yet.

BUG=594460
Committed: https://crrev.com/335667904fb55b54dc75c1be172347986b8f3763
Cr-Commit-Position: refs/heads/master@{#393311}

[ccameron, 2016-05-05 02:26:22]

ccameron@chromium.org changed reviewers:
+ erikchen@chromium.org

[ccameron, 2016-05-05 02:26:23]

This succeeds in getting into detached mode when hooked up to
BrowserWindowController (which isn't done in this patch, since, when we do that,
we'll need to make sure we suppress FSLP mode in all of the appropriate
situations).

https://codereview.chromium.org/1952163002/diff/1/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_low_power_coordinator.h (right):

https://codereview.chromium.org/1952163002/diff/1/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_low_power_coordinator.h:21: NSWindow*
GetFullscreenLowPowerWindow();
It may be that we'll want to have some sort of scoped suppressor structure
instead and/or in addition to this.

[erikchen, 2016-05-05 17:47:38]

lgtm

https://codereview.chromium.org/1952163002/diff/1/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_low_power_coordinator.h (right):

https://codereview.chromium.org/1952163002/diff/1/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_low_power_coordinator.h:13: class
FullscreenLowPowerCoordinatorCocoa
Write a comment describing what this class does so that an OWNER of c/b/u/cocoa
who hasn't been a part of our discussion will understand the point of this
class.

https://codereview.chromium.org/1952163002/diff/1/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_low_power_coordinator.h:48: // The balance of
alls to Add/RemoveLowPowerModeSuppression. This must be
s/alls/calls

https://codereview.chromium.org/1952163002/diff/1/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_low_power_coordinator.mm (right):

https://codereview.chromium.org/1952163002/diff/1/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_low_power_coordinator.mm:8: NSWindow*
eventTargetWindow_;
I prefer to never use raw pointers for NSObjects, but I see that it gets
retained down in FLPCC, so up to you.

https://codereview.chromium.org/1952163002/diff/1/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_low_power_coordinator.mm:101:
setStyleMask:[low_power_window_ styleMask] | NSFullScreenWindowMask];
We don't unset this mask when exiting low power mode? Can we not set it when we
create it? What happens if this mask gets set halfway through the fullscreen
transition animation? I guess we can suppress during animations?

https://codereview.chromium.org/1952163002/diff/1/ui/accelerated_widget_mac/a...
File ui/accelerated_widget_mac/accelerated_widget_mac.mm (right):

https://codereview.chromium.org/1952163002/diff/1/ui/accelerated_widget_mac/a...
ui/accelerated_widget_mac/accelerated_widget_mac.mm:98: }
should we be reseting the FSLPlayer?

https://codereview.chromium.org/1952163002/diff/1/ui/accelerated_widget_mac/f...
File ui/accelerated_widget_mac/fullscreen_low_power_coordinator.h (right):

https://codereview.chromium.org/1952163002/diff/1/ui/accelerated_widget_mac/f...
ui/accelerated_widget_mac/fullscreen_low_power_coordinator.h:22: // or
destroyed. In this call, the coordinator will call back into the
s/will/must/?

[ccameron, 2016-05-05 19:27:18]

Thanks! updated.

https://codereview.chromium.org/1952163002/diff/1/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_low_power_coordinator.h (right):

https://codereview.chromium.org/1952163002/diff/1/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_low_power_coordinator.h:13: class
FullscreenLowPowerCoordinatorCocoa
On 2016/05/05 17:47:38, erikchen wrote:
> Write a comment describing what this class does so that an OWNER of
c/b/u/cocoa
> who hasn't been a part of our discussion will understand the point of this
> class.

Done.

https://codereview.chromium.org/1952163002/diff/1/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_low_power_coordinator.h:48: // The balance of
alls to Add/RemoveLowPowerModeSuppression. This must be
On 2016/05/05 17:47:38, erikchen wrote:
> s/alls/calls

Done.

https://codereview.chromium.org/1952163002/diff/1/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_low_power_coordinator.mm (right):

https://codereview.chromium.org/1952163002/diff/1/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_low_power_coordinator.mm:8: NSWindow*
eventTargetWindow_;
On 2016/05/05 17:47:38, erikchen wrote:
> I prefer to never use raw pointers for NSObjects, but I see that it gets
> retained down in FLPCC, so up to you.

Oh, I'd prefer scoped_nsobject.

https://codereview.chromium.org/1952163002/diff/1/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_low_power_coordinator.mm:101:
setStyleMask:[low_power_window_ styleMask] | NSFullScreenWindowMask];
On 2016/05/05 17:47:38, erikchen wrote:
> We don't unset this mask when exiting low power mode? Can we not set it when
we
> create it? What happens if this mask gets set halfway through the fullscreen
> transition animation? I guess we can suppress during animations?

Setting it at creation time results in us not hitting detached mode.

Going into FSLP mode during transitions looks nasty -- we'll need to suppress
during transition animations for those reasons (not sure if it breaks detached
mode or not).

https://codereview.chromium.org/1952163002/diff/1/ui/accelerated_widget_mac/a...
File ui/accelerated_widget_mac/accelerated_widget_mac.mm (right):

https://codereview.chromium.org/1952163002/diff/1/ui/accelerated_widget_mac/a...
ui/accelerated_widget_mac/accelerated_widget_mac.mm:98: }
On 2016/05/05 17:47:38, erikchen wrote:
> should we be reseting the FSLPlayer?

There's no upside to resetting it -- we can reuse the same FSLPLayer across
different NSWindows without any issues (reusing NSWindows, however, has issues).

https://codereview.chromium.org/1952163002/diff/1/ui/accelerated_widget_mac/f...
File ui/accelerated_widget_mac/fullscreen_low_power_coordinator.h (right):

https://codereview.chromium.org/1952163002/diff/1/ui/accelerated_widget_mac/f...
ui/accelerated_widget_mac/fullscreen_low_power_coordinator.h:22: // or
destroyed. In this call, the coordinator will call back into the
On 2016/05/05 17:47:38, erikchen wrote:
> s/will/must/?

Done.

[ccameron, 2016-05-05 22:36:40]

Adding avi@ for chrome/browser/ui OWNER stamp

[ccameron, 2016-05-06 20:08:00]

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

[ccameron, 2016-05-06 20:08:01]

Adding avi@ for OWNER RS.

[Avi (use Gerrit), 2016-05-06 20:37:33]

lgtm

stamp

[ccameron, 2016-05-06 20:42:25]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-05-06 20:42:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1952163002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-06 20:42:46]

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

[commit-bot: I haz the power, 2016-05-06 20:49:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-06 20:49:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ccameron, 2016-05-09 19:49:07]

ccameron@chromium.org changed reviewers:
+ sky@chromium.org

[ccameron, 2016-05-09 19:49:08]

Adding sky@ for chrome/browser/ui/BUILD.gn OWNER

[sky, 2016-05-12 15:05:00]

LGTM

[ccameron, 2016-05-12 17:12:58]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-05-12 17:13:26]

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

[commit-bot: I haz the power, 2016-05-12 18:28:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-12 18:30:51]

Description was changed from

==========
Mac fullscreen low power video: Add FullscreenLowPowerControllerCocoa

This is where the logic for transitioning in and out of fullscreen
low power mode lives. Do not hook up to the BrowserWindowController
yet.

BUG=594460
==========

to

==========
Mac fullscreen low power video: Add FullscreenLowPowerControllerCocoa

This is where the logic for transitioning in and out of fullscreen
low power mode lives. Do not hook up to the BrowserWindowController
yet.

BUG=594460

Committed: https://crrev.com/335667904fb55b54dc75c1be172347986b8f3763
Cr-Commit-Position: refs/heads/master@{#393311}
==========

[commit-bot: I haz the power, 2016-05-12 18:30:52]

Patchset 3 (id:??) landed as
https://crrev.com/335667904fb55b54dc75c1be172347986b8f3763
Cr-Commit-Position: refs/heads/master@{#393311}