316494 - [BEOS] F11 fullscreen does not fill screen

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Doug Shelton]

User-Agent:       Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.9a1) Gecko/20051114 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.9a1) Gecko/20051114 Firefox/1.6a1

sometime between 2005-10-21 and 2005-11-14, F11 ceased to function correctly.  F11 now expands standard window offscreen, instead of going into fullscreen mode.

Reproducible: Always

Steps to Reproduce:
1.Press F11
2.
3.

Actual Results:  
Window expands to dimensions that would fill the screen, but upper left of window doesn't move.  Bottom and right edges go off edge of screen.  Window also retained title tab.

Expected Results:  
upper left of window should move to top left of screen.  Window should fill entire screen without going off edge.

Comment 1

[Doug Shelton]

*** Bug 316496 has been marked as a duplicate of this bug. ***

Comment 2

[Frederic Bezies]

*** Bug 316495 has been marked as a duplicate of this bug. ***

Comment 3

[Sergei Dolgov]

NS_IMETHODIMP nsBaseWidget::MakeFullScreen(PRBool aFullScreen)
which we probably should override now.

Comment 4

[Doug Shelton]

I noticed other OSs (gtk, gtk2, os2, photon, etc.) all have specific code for this function.  Without BeOS-specific code, fyysik, how did this feature ever work?

Comment 5

[Sergei Dolgov]

confirming

Comment 6

[tqh]

Doug could you check if removing this if () {} fixes fullscreen?
http://lxr.mozilla.org/seamonkey/source/widget/src/beos/nsWindow.cpp#1095

I have so many other changes, but working fullscreen, that I'm not sure if that's the only thing needed to fix this. 

Btw fullscreen worked for us because of 
http://lxr.mozilla.org/seamonkey/source/widget/src/xpwidgets/nsBaseWidget.cpp#551
which calls MakeFullScreenInternal that does the resizing for us. This resize in our nsWindow called Move (which should be quite an overhead as we then send both move and then resize event) which seems to fail the check posted above. If  that's the problem, there is something fishy going on.

Comment 7

[Doug Shelton]

fyysik, that fixes it.  Is there a more specific conditional that would allow us to skip the unnecessary steps but still call it in the special case of fullscreen?

Comment 8

[tqh]

I have code for the resize(x,y,width,height ...) that I think we should use as it skips sending a move event. So we can keep that check. Although it might be something fishy with it.

Comment 9

[tqh]

Attachment: Rewrote resize(aX, aY,...) and introduced mWindow

This one makes fullscreen work as expected. The problem was our resize that called move (which failed in the first if-check. why??). I don't think it should call Move as then both a move event and an resize event is fired. Introduced mWindow to make the code nicer and also modified most places where we could benefit from it.

I didn't understand why the casting was done in GetNativeData so I removed it. Was there a reason for it?

Comment 10

[Doug Shelton]

I'll build and test today.

Comment 11

[tqh]

Attachment: Changing acc. to comments

Comment 12

[Doug Shelton]

just built with latest version.  F11 works well.  Thanks, tqh!

Comment 13

[Sergei Dolgov]

tigerdog, this patch changes much more than F11 problem, so it needs bit longer testing.

Comment 14

[tqh]

Comment on attachment 206247 [details] [diff] [review]
Changing acc. to comments

r?
I don't plan to do any other changes so putting review request. So you can review it once you have tested it.

Comment 15

[Sergei Dolgov]

Comment on attachment 206247 [details] [diff] [review]
Changing acc. to comments

1) Show()
a)Hiding mView for popups was not mistake but re-added for purpose (after dropped once) - it makes faster and nicer visual appearance of menus.
b)inspite PRBool is claimed as compatible with platfrom-bool if used in logical expressions, i wouldn't risk to use such unportable, in general, things like bState == IsHidden() (as last one is bool) Only thing which i guaranteed for int-based bool in general case is that false == 0 and true != 0 - too little to guarantee such comparisons in common case.
I'm unsure also that code 
 is now more understandable, but that's taste things on which we disagree with you permanently, so no critical.

2) Move().
Are you sure about adding 
 NS_ERROR_FAILURE? I'm imagine situation when native BView/BWindow weren't created but Move() is used to set bounds. So maybe that additional (!mWindow && check is overhead.
3)GetNativeData. All platforms cast return value to void *. We may drop (nsViewBeOS/BView *) pre-casting there, but maybe better ideas is to let (void *) to stay here.

Comment 16

[tqh]

1a) Not sure what you mean. As far as I can tell there was no hide on the mView in popup, and the code should do the same thing as before.
b) I can put (bState == PR_TRUE) == mView->IsHidden() if you think that would be better. The rewrite was done to use mWindow and to remove the very duplicated _popup case.

2) Personally I think this is the correct behaviour. When operating on a window it's the window you want to move (never mind that on BeOS it also has a view). When operating on a view you want to move the view within the window. When one fails to do this the way it should be I think NS_ERROR_FAILURE is in place. Also when NS_ERROR_FAILURE is sent we should see that in debug-builds so we can see when this bad state happens. If don't return NS_ERROR_FAILURE I think we are trying to write code that assumes it knows better than the calling code.

3) It's void * by the function definition. Casting it doesn't do anything here.

Comment 17

[Doug Shelton]

(In reply to comment #13)
> tigerdog, this patch changes much more than F11 problem, so it needs bit longer
> testing.
> 

No problem.  I've incorporated it into the next round of Bleeding Edge builds.  I'll beat on it the rest of the evening.  If you think we'll have more changes, I'll hold off posting to BeBits.  Of course, I could post the builds there anyway.  They are "bleeding edge" after all... :)

Comment 18

[Sergei Dolgov]

3)"It's void * by the function definition."
I know it. But all other platfroms, except unmatured qt port, do this casting for return. Maybe it is about code portability.
2)If you look again at other platforms, they do 3 things:
"unconditionally" return NS_OK in any case,
set new mBounds if there is reason to do it,
trying to really move native element, if it exists, but return result is independent on that attempt.
We met such problem with JavaScript popups (there is BeOS-bug for that) and it appeared that we must set mBounds and return NS_OK even if there is nothing to resize. Now it isn't so critical, as you added SetBounds in StandardWindowCreate, but it shows, at least for resize, that method can be called with expectation to success, even when native elements are missing.
If it's that way for resize, i don't know why it must be other way for Move.

1) Sorry, i removed mView Show/Hide for pupups myself long ago, but was sure for misterious reason that it is stil here. So no problem.

Comment 19

[tqh]

3) Not that I've read any C/C++ reference manuals recently but that should be an ok statement. Having casting just because other platforms has it seems like a poor argument. All it adds is code bloat and confusion to those who look upon it.

2) Well I still don't buy that either. If there actually exists a case where this happens I'd be willing to do it. If that case happens IT IS clearly an error, and the only reason to skip sending an NS_ERROR_FAILURE, when it really is, is when there is a documented problem elsewhere that needs this bad assumption. Not because 'just in case someone expect it to work when it actually fails'. I think it's better to detect errors (and fix them) than to avoid sending error.

Comment 20

[Sergei Dolgov]

3)Actually implicit type conversion is Bad Thing (TM), fact that C/C++ is language with week type restrictions and allows such thing, does not mean is good idea and style.
2)I will investigate it bit more

Comment 21

[Sergei Dolgov]

Actually only change required to put fullscreen working back is changed string in 
:Move().
if (mWindowType == eWindowType_child && (mBounds.x == aX) && (mBounds.y == aY))
instead
if (mWindowType != eWindowType_popup && (mBounds.x == aX) && (mBounds.y == aY))

We miss separate windows and widgets, and for top-level mViews we always have left-top (0,0) - in its parent (BWindow) coordinate space. As we use mView->Frame() as mBounds value.
Sure, in this case when fullscreen method calls Move() even at topwidget, it tries to move widget/window to (0,0) - but according to our messy accounting - we are already there (and always are!).

Though, don't know why and how it worked before (and it worked!). We never had our MakeFullScreen() implementation. Neither we had HideWindowChrome (which is empty in BaseWidgets). Maybe those two nsWindow cleanups we made had some effect on this problem.

Comment 22

[tqh]

I already explained how it worked before in comment 6.

2) Well the resize(x,y width, height) is very flawed as it is now. It sends an move event, which it should not. So even though fixing move is also good. It is really resize(x,y width, height) and move that is broken for fullscreen.

3) Hmm, code bloat, that doesn't give any extra clarity and adds to what the compiler has to do is a good thing(TM) then? Well I prefer using KISS and self documenting code over obscurity any day.

Comment 23

[Sergei Dolgov]

2) We rather should remove OnMove() from Move() and set mBounds directly there.
and in Resize() we should send notification via OnResize() only in case if it IsTopWidgetWinod || LISTEN_TO_RESIZE (flag supplied via Create())
I told you about LISTEN_TO_RESIZE flag when we discussed StandardWindowCreate() refactoring

Unfortunately i didn't get, inspite your explanation, why fullscreen worked some time ago and why it stopped. Maybe i missed something.
IIRC only change in BaseWidget was that MakeFullScreen was separated into two parts - call of HideChrome and MakeFullScreenInternal.

But code of MakeFullScreenInternal (or what is that name) was part of old MakeFullScreen. 

Comment 24

[Sergei Dolgov]

Well.
F11 problem is regression caused by our own changes in nsWindow.
Commment out SetBounds() in  CallMethod()
case nsWindow::ONRESIZE :

And you will get back working fullscreen toggling

Comment 25

[Sergei Dolgov]

Additional investigation - F11 is working also (this is bit more misterious for me than previous reason, when we comment out SetBounds() in StandardWindowCreate() but keep it in ::ONRESIZE:

Also problem related to non-updating pages replacing existing on in tabs (described it at blog and got several unreproducable complaints aboit it in last two months) looks also depending on usage of SetBounds(). Using Resize(x, y, width, height, PR_TRUE);  produces better results with page update, but it needs bit more investigation. 

Maybe removing SetBounds() in ::ONRESIZE: is enough to fix both problems.

Comment 26

[tqh]

Resize still shouldn't send move event. Setting bounds before resize is called will update mBounds out of sync, which move checks. The SetBounds in ONRESIZE is definatly wrong. So those two things must at least be fixed.

MakeFullscreenInternal is called thru MakeFullscreen which we don't override. It resizes the window to full screen (the border is outside the screen) and changes URL-bar. We don't need to do any override as this works as expected and there isn't any documentation saying that those nsBaseWidget methods are old.

Anyway I guess I'm forced to work on my own BeOS-code as I've grown tired of the direction you're forcing the code in.

Comment 27

[Sergei Dolgov]

What i'm trying to say, that i wish to know the reason, how, when and why it was broken.
Because patch for bug 
https://bugzilla.mozilla.org/show_bug.cgi?id=301402
actually changed nothing for us
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/widget/src/xpwidgets&command=DIFF_FRAMESET&file=nsBaseWidget.cpp&rev1=1.134&rev2=1.135&root=/cvsroot

- all the same operations in same are performed. Please, just look at that change.

Thus, i suspect our own patches, and wish to fix it all in correct way. But not add new code and refactor old, even if it is totally unrelated to problem.

There may be separate bug for that, as proper coordinate handling problem is complicate enough itself.

Comment 28

[tqh]

It only seperated the functions. I didn't see any reference to that so that was very hard for me to understand. 

I know we broke it, or it have never been right in our code, but not visible before. I'm not sure if mBounds was set correctly before in creation.

Anyway I've done work the needed so I'll spend my time where it's better used.

Comment 29

[Sergei Dolgov]

Anyway.
Current patch is no-go. As it workarounds or fixes symptomps, instead investigating and fixing real reasons. IMHO.
Plus touches unrelated things with no big reason.

If removing SetBounds() from CallMethod is enough, it is enough.
If there are some other inconsistencies in coordinate handling - that should be investigated.

Other code changes, if those bringssome convinience, like introducing cached mWindow, should be in separate bug,worked out more carefully, as even last your version let's some amount of mView->Window() calls in code.

And besides KISS principle, there is another - "If it works - don't touch - until it brings visible benefit"

Comment 30

[tqh]

Fine. Good luck then.

Comment 31

[Sergei Dolgov]

Attachment: simplest patch

Simplest patch which fixes just this bug, by removing SetBounds() introduced in some of last year patches without sufficient testing and reasoning

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

why does this work? doesn't something need to call SetBounds?

Comment 33

[Sergei Dolgov]

We never used SetBounds() explicitly in BeOS widget code before that checkin which broke F11 feature, rather informed view manager about fact of change via NS_message. Letting it to ask bounds information itself, if it needs - and let it choose which method to use - GetBounds or GetScreenBounds.

In situation with SetBounds() and our BeOS-implementation mess of nsWidget with mixed nsWindow and nsWidget, this SetBound call sets also 0,0 coordinate for thing which it thinks is toplevel window, while it is toplevel bview.
So later coordinate check in Move() method, when applied to BWindow thinks that 
it is already positioned correctly and does nothing.

But partly problem may be related to not so old changes in gkview, which ignores now NS_MOVE messages to avoid message flood. 

So i'm just removing part of old checkin which caused this problem, following Occam's razor approach atm, as TQH is dealing atm with full widget code rewrite anyway (there is bug and even patch submitted already) - with separate Window, Winget and View. So don't see reason for big and not so clean changes in current code.

Anyway, for me it is rather minor feature, so if if you think my patch isn't backed and baked enough, i wouldn't care.

Comment 34

[Doug Shelton]

I would agree that it's a minor feature, but one I use almost daily.  I've used the proposed one-line fix in builds for the last two months with no (perceptible) bad effects.  If we check in the one-line fix, we eliminate one possible frustration for people who build their own copies.

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 210472 [details] [diff] [review]
simplest patch

r=me I guess

Comment 36

[Sergei Dolgov]

Checked in trunk:
Checking in mozilla/widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.118; previous revision: 1.117 

Btw, this SetBound() thing may be related to another bug, appearing in last months from time to time - some loaded pages stay unupdated until container window is manually resized

Comment 37

[Niels Reedijk]

Not present in the branch.