Correct NSScreen comparison on PPAPI fullscreen switching

Correct NSScreen comparison on PPAPI fullscreen switching

From OS X 10.10, NSApplicationDidChangeScreenParametersNotification
will be sent when window enters fullscreen, and [NSScreen screens]
will be updated with new NSScreen instances equal to the previous
ones but with different pointer values. As a result, using == to
compare pointers of desiredScreen and current menu bar/dock screen
will break as the containsObject: check in -update does isEqual:
comparison.

This doesn't affect Chrome as Chrome is using EmbedsFullscreenWidget().

Committed: https://crrev.com/b31efc78ccea78706ebd298a5e82340cd563691f
Cr-Commit-Position: refs/heads/master@{#298989}

[Jiang Jiang, 2014-10-08 15:29:19]

jiangj@opera.com changed reviewers:
+ erikchen@chromium.org

[Jiang Jiang, 2014-10-08 15:29:20]

Hi,

Do you feel like looking at it?

It's should be possible to reproduce with Chromium with Pepper Flash on for
Youtube and switch to fullscreen, in this case neither Dock nor Menubar are
hidden.

I'm not entire sure if the behavior only apply to applications linking to newer
(>=10.9) SDKs though. Though Chrome has different fullscreen handling from Opera
but it shouldn't affect Pepper Flash fullscreen code path, should it?

- Jiang

[erikchen, 2014-10-08 16:24:53]

I'll take a look today. Is there an associated bug?

[Jiang Jiang, 2014-10-08 16:53:22]

On 2014/10/08 16:24:53, erikchen wrote:
> I'll take a look today. Is there an associated bug?

I can create one. But it's not as easy to reproduce it with Chrome as YouTube
forces HTML5 mode when browsing with Chrome. Do you happen to know any other
PPAPI Flash test case for fullscreen mode?

[erikchen, 2014-10-08 21:14:36]

On 2014/10/08 16:53:22, Jiang Jiang wrote:
> On 2014/10/08 16:24:53, erikchen wrote:
> > I'll take a look today. Is there an associated bug?
> 
> I can create one. But it's not as easy to reproduce it with Chrome as YouTube
> forces HTML5 mode when browsing with Chrome. Do you happen to know any other
> PPAPI Flash test case for fullscreen mode?

tl dr: I am willing to lgtm this CL, although something funky is going on.

Some thoughts:
- I wasn't able to reproduce this with Chrome. (twitch.tv shows Flash videos
that support fullscreen).
  - Is this code path dead in Chrome? A comment in
render_widget_host_view_mac.mm indicates that this code should be called when
"pp::FlashFullScreen::SetFullscreen() is called", which isn't accurate.
- I've confirmed that the behavior of this class changes between 10.9 and 10.10,
and that the extra callbacks happen on 10.10, although I don't know the source
of them.
- I've confirmed that pointer comparison is the wrong way to do this, although I
was never able to find an instance where the existing logic would actually
misbehave.
- The logic of GetMenuBarScreen is wrong. It assumes that only one screen has a
menubar, which is no longer true as of 10.9, with "displays have separate
spaces" setting set to YES (defaults to YES in 10.10).

[Jiang Jiang, 2014-10-09 08:48:27]

On 2014/10/08 21:14:36, erikchen wrote:
> On 2014/10/08 16:53:22, Jiang Jiang wrote:
> > On 2014/10/08 16:24:53, erikchen wrote:
> > > I'll take a look today. Is there an associated bug?
> > 
> > I can create one. But it's not as easy to reproduce it with Chrome as
YouTube
> > forces HTML5 mode when browsing with Chrome. Do you happen to know any other
> > PPAPI Flash test case for fullscreen mode?
> 
> tl dr: I am willing to lgtm this CL, although something funky is going on.
> 
> Some thoughts:
> - I wasn't able to reproduce this with Chrome. (twitch.tv shows Flash videos
> that support fullscreen).
>   - Is this code path dead in Chrome? A comment in
> render_widget_host_view_mac.mm indicates that this code should be called when
> "pp::FlashFullScreen::SetFullscreen() is called", which isn't accurate.
> - I've confirmed that the behavior of this class changes between 10.9 and
10.10,
> and that the extra callbacks happen on 10.10, although I don't know the source
> of them.
> - I've confirmed that pointer comparison is the wrong way to do this, although
I
> was never able to find an instance where the existing logic would actually
> misbehave.
> - The logic of GetMenuBarScreen is wrong. It assumes that only one screen has
a
> menubar, which is no longer true as of 10.9, with "displays have separate
> spaces" setting set to YES (defaults to YES in 10.10).

Thanks for the detailed analysis. I will check Chromium build locally to see why
the code path wasn't triggered there.

[Jiang Jiang, 2014-10-09 13:11:35]

On 2014/10/09 08:48:27, Jiang Jiang wrote:
> On 2014/10/08 21:14:36, erikchen wrote:
> > On 2014/10/08 16:53:22, Jiang Jiang wrote:
> > > On 2014/10/08 16:24:53, erikchen wrote:
> > > > I'll take a look today. Is there an associated bug?
> > > 
> > > I can create one. But it's not as easy to reproduce it with Chrome as
> YouTube
> > > forces HTML5 mode when browsing with Chrome. Do you happen to know any
other
> > > PPAPI Flash test case for fullscreen mode?
> > 
> > tl dr: I am willing to lgtm this CL, although something funky is going on.
> > 
> > Some thoughts:
> > - I wasn't able to reproduce this with Chrome. (twitch.tv shows Flash videos
> > that support fullscreen).
> >   - Is this code path dead in Chrome? A comment in
> > render_widget_host_view_mac.mm indicates that this code should be called
when
> > "pp::FlashFullScreen::SetFullscreen() is called", which isn't accurate.
> > - I've confirmed that the behavior of this class changes between 10.9 and
> 10.10,
> > and that the extra callbacks happen on 10.10, although I don't know the
source
> > of them.
> > - I've confirmed that pointer comparison is the wrong way to do this,
although
> I
> > was never able to find an instance where the existing logic would actually
> > misbehave.
> > - The logic of GetMenuBarScreen is wrong. It assumes that only one screen
has
> a
> > menubar, which is no longer true as of 10.9, with "displays have separate
> > spaces" setting set to YES (defaults to YES in 10.10).
> 
> Thanks for the detailed analysis. I will check Chromium build locally to see
why
> the code path wasn't triggered there.

It's because WebContentsDelegate in chrome implements EmbedsFullscreenWidget():

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...

So this code path might potentially be a problem for content shell, but content
shell never supported Pepper Flash directly.

[erikchen, 2014-10-09 17:07:07]

lgtm

[Jiang Jiang, 2014-10-09 18:05:23]

The CQ bit was checked by jiangj@opera.com

[commit-bot: I haz the power, 2014-10-09 18:06:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/635363002/1

[erikchen, 2014-10-09 18:14:17]

You're going to need an OWNER review. avi or rsesek is probably your best bet.

[commit-bot: I haz the power, 2014-10-09 18:18:49]

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

[commit-bot: I haz the power, 2014-10-09 18:18:51]

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

[Jiang Jiang, 2014-10-09 18:35:35]

jiangj@opera.com changed reviewers:
+ avi@chromium.org

[Jiang Jiang, 2014-10-09 18:35:35]

jiangj@opera.com changed reviewers:
+ avi@chromium.org

[Jiang Jiang, 2014-10-09 18:35:35]

Avi, can you please take a look?

[Jiang Jiang, 2014-10-09 18:35:35]

Avi, can you please take a look?

[Avi (use Gerrit), 2014-10-09 18:38:18]

Why do they do this kind of thing? I'm feeling grateful that at least -isEqual:
still works...

LGTM

[erikchen, 2014-10-09 18:43:44]

On 2014/10/09 18:38:18, Avi wrote:
> Why do they do this kind of thing? I'm feeling grateful that at least
-isEqual:
> still works...
> 
> LGTM

When I forced this logic to run on Yosemite by adding some hacks to Chrome, the
pointer comparison still worked (although as I mentioned in #5, the pointers did
change over time across callbacks). jiangj: Were you deterministically able to
reproduce the problem of the pointer comparison failing?

[Jiang Jiang, 2014-10-09 19:10:33]

On 2014/10/09 18:43:44, erikchen wrote:
> On 2014/10/09 18:38:18, Avi wrote:
> > Why do they do this kind of thing? I'm feeling grateful that at least
> -isEqual:
> > still works...
> > 
> > LGTM
> 
> When I forced this logic to run on Yosemite by adding some hacks to Chrome,
the
> pointer comparison still worked (although as I mentioned in #5, the pointers
did
> change over time across callbacks). jiangj: Were you deterministically able to
> reproduce the problem of the pointer comparison failing?

Yes, I can reproduce it every time I tried.

How did the pointer comparison work for you if the one in [NSScreen screens]
changed?

[erikchen, 2014-10-09 19:52:49]

On 2014/10/09 19:10:33, Jiang Jiang wrote:
> On 2014/10/09 18:43:44, erikchen wrote:
> > On 2014/10/09 18:38:18, Avi wrote:
> > > Why do they do this kind of thing? I'm feeling grateful that at least
> > -isEqual:
> > > still works...
> > > 
> > > LGTM
> > 
> > When I forced this logic to run on Yosemite by adding some hacks to Chrome,
> the
> > pointer comparison still worked (although as I mentioned in #5, the pointers
> did
> > change over time across callbacks). jiangj: Were you deterministically able
to
> > reproduce the problem of the pointer comparison failing?
> 
> Yes, I can reproduce it every time I tried.
> 
> How did the pointer comparison work for you if the one in [NSScreen screens]
> changed?

The pointers only change across callbacks. During each callback, [NSScreen
screens] is called multiple times in quick succession, and the pointers do not
change.

[Jiang Jiang, 2014-10-09 21:50:13]

On 2014/10/09 19:52:49, erikchen wrote:
> On 2014/10/09 19:10:33, Jiang Jiang wrote:
> > On 2014/10/09 18:43:44, erikchen wrote:
> > > On 2014/10/09 18:38:18, Avi wrote:
> > > > Why do they do this kind of thing? I'm feeling grateful that at least
> > > -isEqual:
> > > > still works...
> > > > 
> > > > LGTM
> > > 
> > > When I forced this logic to run on Yosemite by adding some hacks to
Chrome,
> > the
> > > pointer comparison still worked (although as I mentioned in #5, the
pointers
> > did
> > > change over time across callbacks). jiangj: Were you deterministically
able
> to
> > > reproduce the problem of the pointer comparison failing?
> > 
> > Yes, I can reproduce it every time I tried.
> > 
> > How did the pointer comparison work for you if the one in [NSScreen screens]
> > changed?
> 
> The pointers only change across callbacks. During each callback, [NSScreen
> screens] is called multiple times in quick succession, and the pointers do not
> change.

What I observed is that desiredScreen_ is first initialized to the value passed
to the constructor. When it received -onScreenChanged: and call -update,
[[NSScreen screens] containsObject:desiredScreen_] returns YES so desiredScreen_
is not updated, but GetMenuBarScreen()/GetDockScreen() has already returned a
new pointer, resulting both desiredScreen_ == GetMenuBarScreen() and
desiredScreen_ == GetDockScreen() checks fail, so we end up with newMode =
base::mac::kFullScreenModeNormal and not hiding the menu bar and dock when
switching to fullscreen in this mode.

I receive two NSApplicationDidChangeScreenParametersNotification notifications
each time I enter or leave fullscreen mode on 10.10, the first time the pointer
in [NSScreen screens] didn't change, but the second time it changes.

[erikchen, 2014-10-09 21:53:14]

Gotcha. Thanks for the detailed update!



On Thu, Oct 9, 2014 at 2:50 PM, <jiangj@opera.com> wrote:

> On 2014/10/09 19:52:49, erikchen wrote:
>
>> On 2014/10/09 19:10:33, Jiang Jiang wrote:
>> > On 2014/10/09 18:43:44, erikchen wrote:
>> > > On 2014/10/09 18:38:18, Avi wrote:
>> > > > Why do they do this kind of thing? I'm feeling grateful that at
>> least
>> > > -isEqual:
>> > > > still works...
>> > > >
>> > > > LGTM
>> > >
>> > > When I forced this logic to run on Yosemite by adding some hacks to
>>
> Chrome,
>
>> > the
>> > > pointer comparison still worked (although as I mentioned in #5, the
>>
> pointers
>
>> > did
>> > > change over time across callbacks). jiangj: Were you deterministically
>>
> able
>
>> to
>> > > reproduce the problem of the pointer comparison failing?
>> >
>> > Yes, I can reproduce it every time I tried.
>> >
>> > How did the pointer comparison work for you if the one in [NSScreen
>> screens]
>> > changed?
>>
>
>  The pointers only change across callbacks. During each callback, [NSScreen
>> screens] is called multiple times in quick succession, and the pointers
>> do not
>> change.
>>
>
> What I observed is that desiredScreen_ is first initialized to the value
> passed
> to the constructor. When it received -onScreenChanged: and call -update,
> [[NSScreen screens] containsObject:desiredScreen_] returns YES so
> desiredScreen_
> is not updated, but GetMenuBarScreen()/GetDockScreen() has already
> returned a
> new pointer, resulting both desiredScreen_ == GetMenuBarScreen() and
> desiredScreen_ == GetDockScreen() checks fail, so we end up with newMode =
> base::mac::kFullScreenModeNormal and not hiding the menu bar and dock when
> switching to fullscreen in this mode.
>
> I receive two NSApplicationDidChangeScreenParametersNotification
> notifications
> each time I enter or leave fullscreen mode on 10.10, the first time the
> pointer
> in [NSScreen screens] didn't change, but the second time it changes.
>
> https://codereview.chromium.org/635363002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jiang Jiang, 2014-10-09 22:06:11]

The CQ bit was checked by jiangj@opera.com

[commit-bot: I haz the power, 2014-10-09 22:07:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/635363002/1

[commit-bot: I haz the power, 2014-10-09 22:12:06]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-10-09 22:12:49]

Patchset 1 (id:??) landed as
https://crrev.com/b31efc78ccea78706ebd298a5e82340cd563691f
Cr-Commit-Position: refs/heads/master@{#298989}