Exit Fullscreen when screen is locked

Exit Fullscreen when screen is locked

BUG=chromium-os:4212
TEST=TestFullscreenExit added

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53226

[Paweł Hajdan Jr., 2010-07-20 22:15:23]

Drive-by with a test comment. Please let me take another look before committing.

http://codereview.chromium.org/2832063/diff/4001/5002
File chrome/browser/chromeos/login/screen_locker_browsertest.cc (right):

http://codereview.chromium.org/2832063/diff/4001/5002#newcode58
chrome/browser/chromeos/login/screen_locker_browsertest.cc:58: while
(browser()->window()->IsFullscreen() != fullscreen) {
Please don't write code like this. Don't loop in tests, wait for a notification.

[oshima, 2010-07-20 22:27:37]

http://codereview.chromium.org/2832063/diff/4001/5002
File chrome/browser/chromeos/login/screen_locker_browsertest.cc (right):

http://codereview.chromium.org/2832063/diff/4001/5002#newcode58
chrome/browser/chromeos/login/screen_locker_browsertest.cc:58: while
(browser()->window()->IsFullscreen() != fullscreen) {
On 2010/07/20 22:15:23, Paweł Hajdan Jr. wrote:
> Please don't write code like this. Don't loop in tests, wait for a
notification.

I agree it's better if there is a notification, but there is no notification for
this. What is the issue with busy loop?

[Paweł Hajdan Jr., 2010-07-20 22:32:31]

http://codereview.chromium.org/2832063/diff/4001/5002
File chrome/browser/chromeos/login/screen_locker_browsertest.cc (right):

http://codereview.chromium.org/2832063/diff/4001/5002#newcode58
chrome/browser/chromeos/login/screen_locker_browsertest.cc:58: while
(browser()->window()->IsFullscreen() != fullscreen) {
On 2010/07/20 22:27:37, oshima wrote:
> On 2010/07/20 22:15:23, Paweł Hajdan Jr. wrote:
> > Please don't write code like this. Don't loop in tests, wait for a
> notification.
> 
> I agree it's better if there is a notification, but there is no notification
for
> this. What is the issue with busy loop?

Please add a notification. The reason is that a loop is just a sloppy way of
doing that. While fixing some test flakiness issues I've determined that it's
much better when tests wait for a notification instead of polling on a
condition.

[oshima, 2010-07-20 23:15:30]

On Tue, Jul 20, 2010 at 3:32 PM, <phajdan.jr@chromium.org> wrote:

>
> http://codereview.chromium.org/2832063/diff/4001/5002
> File chrome/browser/chromeos/login/screen_locker_browsertest.cc (right):
>
> http://codereview.chromium.org/2832063/diff/4001/5002#newcode58
> chrome/browser/chromeos/login/screen_locker_browsertest.cc:58: while
> (browser()->window()->IsFullscreen() != fullscreen) {
> On 2010/07/20 22:27:37, oshima wrote:
>
>> On 2010/07/20 22:15:23, Paweł Hajdan Jr. wrote:
>> > Please don't write code like this. Don't loop in tests, wait for a
>> notification.
>>
>
>  I agree it's better if there is a notification, but there is no
>>
> notification for
>
>> this. What is the issue with busy loop?
>>
>
> Please add a notification. The reason is that a loop is just a sloppy
> way of doing that. While fixing some test flakiness issues I've

determined that it's much better when tests wait for a notification
> instead of polling on a condition.


Busy wait is a valid solution for certain cases (including this IMHO).
It's not necessarily sloppy and it won't cause any flaky-ness (unlike
sleep).
Adding notification and the code just for testing in the chrome itself
doesn't
best solution either, but I'm happy to be proven wrong. The one real
drawback
I can think of is that it may use a bit more CPU because it will run a few
more loops until X sends event back. Is this your concern?

- oshima



>
>
> http://codereview.chromium.org/2832063/show
>

[oshima, 2010-07-21 03:37:07]

Changed to use gtk event for fullscreen state update.
Conditional loop is still necessary because the order of
fullscreen and lock events is not deterministic.

[Paweł Hajdan Jr., 2010-07-21 17:16:26]

The test code LGTM.

Thanks a lot for writing it this way.

[zel, 2010-07-21 18:11:05]

LGTM