User-Agent: Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.8b2) Gecko/20050604 Firefox/1.0+ Build Identifier: Removing and moving some code around. nsWindows now start out with transparent and relies on BaseWidget to set some variables (border and window type is set in StandardWindowCreate anyway). OnPaint removed code (wasn't that code obsolete, Sergei) and refactored to get away from ifs in ifs ugliness (IMO). SetBackgroundColor only sets backgroundcolor, the view set itself transparent when attached to a window instead. We set the lastViewWidth and height in the constructor from the frame instead. Reproducible: Always Steps to Reproduce:

Sergei, I went thru my experimental nsWindow, and I think these might be worth adding. What do you think?

Added the StandardWindowCreate I wrote, which is dependent on the other changes herein. Otherwise it blinks so it's probably fixed when we set views transparent.

The new StandardWindowCreate is dependent on bug 296823 as it creates fewer views.

good step in general. Will investigate it bit more.

It's needed as I have a few questionmarks. Even though I think it works good enough, as it has been tested in quite some months in my test-releases. When I first tested it it shaved of 2 seconds in startup-time, but now I have a faster laptop with slower harddrive so it's mostly harddrive-dependent.

Changed title to more reflect the StandardWindowCreate changes.

some of the changes proposed in the original patch have crept into CVS through other means. Just updating the patch to match CVS.

actually i wish we leave there at least comment about "invisible" case, and use Run() for BLooper, unrelated to windows type. I still have feeling that we cannot rely on Mozilla's nsWidget::Show() in our case, in order to keep message chain consistent.

Patch derived from CVS after clearing of dependent bug. Feedback appreciated (so we can keep things moving).

i need to test if re-casting of nsViewBeOS and also regarding changes in *.h file are done. Also parenthesis identation isn't unified in current patch. Also some comments may be added, so after testing patch this evening i probably contact Doug in private

Only comment I have is on OnPaint, as I don't like long if() blocks the 'if((r.width || r.height) && mEventCallback)'-line can be refactored and combined with the 'if(!mView)'-line to return instead.

fyysik, where did we leave off on this? Seems like we'll need an updated patch after landing scrolling changes in bug 129310. Is this something I should create or will you create one after addressing tqh's concerns in comment #12? It'd be good to land this one, too, I think.

Doug, i think this is now tqh's deal. As he added some more changes, quite logical, to StandardWindowCreate. Don't rush. I try to discuss it with him, and anyway, scrolling patch isn't in the tree.

I think we should be able to do a better patch once the scrolling is done. This has mostly been delayed because it hasn't been discussed and tested properly, but with the scrolling changes that's no longer the case. Doug, keep up the good work.

The next patch will probably depend on bug 310090

Doug, what is this dependency on 192310?

Ok, saw the error in the dependency.

Removed a lot of unused code, changed a couple of bools to packedbools. There doesn't seem to be a reason for us to have our own setBackgroundColor and setForegroundColor especially with the lockingoverhead. Also fixed some warnings. Note my comment in StandardWindowCreate when a childview is created but without parent. That might need investigation before we commit this.

latest patch applies clean, builds OK but firefox won't start. Attachment includes stack crawl and console output. Note the "NO PARENT" warning in the console output right before it dies.

Comment on attachment 197471 [details] patch breaks startup problem caused by dependency on 310090.

I've noticed quite a bit of console debug output in the form ###!!! ASSERTION: invalid break array: 'aBreaks[aNumBreaks - 1] == aLength', file /boot/home/develop/mozilla/gfx/src/beos/nsRenderingContextBeOS.cpp, line 1151 Break: at file /boot/home/develop/mozilla/gfx/src/beos/nsRenderingContextBeOS.cpp, line 1151 An early workaround from fyysik eliminated this message. While we're cleaning things, can we also take care of this message since it seems to be irrelevant?

We might, but it's in gfs/src/beos and has not very much in common with this, so maybe it should be fixed on it's own.

Doug, aBreak assertion fix should be actually in previous patch, about nsRenderingContextBeOS, but we forgot it - i was sure for unknown reason that we fixed this long ago:) But i submitted new patch in "Japanese" bug, which fixes this issue too. You was CC-ed.

confirmed aBreaks issue fixed with Japanese monospace font patch, as fyysik indicates. I've been running with tqh's latest patch for this for about 2 days, with no apparent problems. May be good for review, tqh.

we might want to make the printf("NO PARENT!!!\n") warning only showup on debug builds. I can't begin to guess why the code falls through to this point, but it doesn't seem to hurt the operation of the browser. I've only seen it at startup, as noted above.

That is the plan, but before I submit the final one I wanted one that shows that there actually is such a case. Before we commit it I want to find out why as well.

This one handles all windowtypes I could test correctly. That means that we have Firefox 'Customize toolbar' without border. It's very rewritten, but with some comments.

Hmm, looking at Firefox in windows I see that Prefs have close, title, and resize. No minimize, maximize. And customize toolbar has: title, minimize, maximize, close and resize.

I've applied the patch over clean nsWindow from CVS. Hunk #9 (line 626) failed to apply cleanly, for some reason, but was easily applied by hand since it appears to be cleanup only and not new code. No adverse effects yet and it sure is nice to be able to resize the preferences window!

(In reply to comment #27) > That is the plan, but before I submit the final one I wanted one that shows that > there actually is such a case. Before we commit it I want to find out why as well. Any luck finding out why we have windows withouth parents?

Havn't looked at that yet, I'm trying to see how window-styles should be.

have you made any further progress on this, tqh? I'm about to do a bunch of Firefox builds and would like to include the work-in-progress, if only because it makes the options/preferences window much easier to use under BeOS.

My motivation is at an all time low, it depends on factors which I have no control over.

(In reply to comment #34) > My motivation is at an all time low, it depends on factors which I have no > control over. tqh, I'm truly sorry to hear you're so bummed out. The work you've done so far is already a huge improvement over previous window behavior. Please let me know if there's anything at all I can do to help you feel better.

Well, it only lasted a short while :) Apparently I was mistaken about some things. Still my laptop is having wifi problems so I have to work on other things.

(In reply to comment #36) > Well, it only lasted a short while :) I'm happy for you! but bummer about your laptop. Anything I can do to help in the meantime?

Changes: GetNativeData changed so needed to call with other type. SetBounds is called so that redraw is done correctly. IME is added, but not sure it will work. mParent is set. Reordered variables (so that they might align better) or if we want to change to PRPackedBool.

Looks ok, testing. Though, probably printf("UNKNOWN or not handled windowtype!!!\n"); should be put in assertion or ifdef debug in future

//NS_NATIVE_WIDGET maybe? //Parent may be a BView if we embed. BView *parent= (BView *) (aParent ? aParent->GetNativeData(NS_NATIVE_GRAPHIC) : aNativeParent); We may change GetNativeData() method back if needed. It might be also NS_NATIVE_WIDGET here.

1) I don't see break or return after case eWindowType_plugin: Is NS_NOTYETIMPLEMENTED("*") macro for that? Or it only printouts? 2) I'm unsure if toplevel BView/nsViewBeOS needs B_FRAME_EVENTS flags. Only native resize event we're getting now is from BWindow::FrameResized()

hmm, i got some strange feeling that invisible window is working as coordinate origin/parent, common for windows, which are in some sence in parent/sibling relations, like main window and (preferences) dialog. So, mouse coordinates and maybe some other events use its bounds as origin to calculate relative coordinates. I will try to investigate it more, but maybe we will need to reallow BaseCreate and create event dispatching for it again.

case eWindowType_plugin fallbacks to childview (and should be tested with plugins), it just prints out a warning before it does.

We need one more module-level variable to be initialized in Create: mListenForResizes = aInitData->mListenForResizes; to use it then in ::Resize() method: if (mListenForResizes || mIsTopWidgetWindow) OnResize(mBounds); in order to avoid unneeded message sending

I think it's time to close this one if the patch came out alright. r?

Comment on attachment 205170 [details] [diff] [review] Some cleanup and suggested changes there was an error in one of the default flags. Corrected patch on its way.

r?

Comment on attachment 205173 [details] [diff] [review] Removed B_NO_WORKSPACE_ACTIVATION for default toplevel windows r=sergei_d actually i dislike BRect aViewRect... aFoo is for argument. No need for such style for variable which you will put in function call, it is confusing, as in this case, for illustration, we should look for this among StandardWindowCreate() parameters. Actually, maybe we don't need to create there this temp variable at all, but use conversion from screen inside new nsViewBeOS() call instead.

excellent! I just built with the version 3 patch and my nasty bug with session history max viewers (fastback bug 301894) seems to be gone! I could not duplicate the problem using any of the sites/techniques that caused trouble before.

Doug, I think you linked the wrong bug :) Sergei, I agree. It shouldn't be needed.

s/301834/ bug 308194

Sergei your comment made me look into that. I think I have some changes: mView = new nsViewBeOS(this, w->Bounds(), "Toplevel view", 0, (mWindowType == eWindowType_popup ? B_WILL_DRAW: 0) | B_FRAME_EVENTS); and removing the mView->ResizeTo line and the line above. Think we need to discuss this, seems to work. B_FOLLOW_ALL is removed btw.

Oops, it seems that I may have been fooled and it used an older lib. It might need B_FOLLOW_ALL after all.

for toplevel we DO need FOLLOW_ALL but DON'T need (with current implementation) FRAME_EVENTS. Maybe you confused with those two.

r? This one I think should be checked in.

I have incorporated patch 4 into my build. The problems with fastback cache have come back. I'll switch it back and rebuild to make sure the only change is patch 3 compared to patch 4.

No explanation: I went back and rebuilt with 3 and now I'm reproducing the problem again. I can't explain - I doublechecked settings, etc. I did a clean build with the version 3 patch. I apologize for the bad information.

Comment on attachment 205316 [details] [diff] [review] changed some lines at nsWindowBeOS creation r=sergei_d

Comment on attachment 205173 [details] [diff] [review] Removed B_NO_WORKSPACE_ACTIVATION for default toplevel windows obsoleting

Checking in mozilla/widget/src/beos/nsWindow.cpp; /cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v <-- nsWindow.cpp new revision: 1.113; previous revision: 1.112 done Checking in mozilla/widget/src/beos/nsWindow.h; /cvsroot/mozilla/widget/src/beos/nsWindow.h,v <-- nsWindow.h new revision: 1.48; previous revision: 1.47 done landed in trunk. Wating for first build from cvs to close bug.

sergei, I think you can close the bug. I deleted widget/src/beos directory and checked out a new one from CVS, then did a make -f client.mk clean build. No compile errors, clean build. I'm using it now.

closing

Requesting permission to land on the MOZILLA_1_8_BRANCH. This is a BeOS-only change and does not affect other platforms.

Comment on attachment 233136 [details] [diff] [review] Patch for 1_8 branch BeOS only change

Comment on attachment 233136 [details] [diff] [review] Patch for 1_8 branch a=beltzner on behalf of drivers for the 1.8.1 branch (NPOTB)

I'm confused. I received an update to this bug earlier today but now cannot see the attachment represented by this activity: steffen.wilberg@web.de 2007-05-31 11:03:07 PDT Attachment #195957 [details] [diff]Flag review?(sergei_d@fi.tartu.ee) It appears Steffen didn't realize the patch was committed and tried to introduce additional changes. Was a new bug created with those additional changes?

All Steffen did, which you can see by viewing https://bugzilla.mozilla.org/show_activity.cgi?id=296856, was remove an old review request.

(In reply to comment #67) > All Steffen did, which you can see by viewing > https://bugzilla.mozilla.org/show_activity.cgi?id=296856, was remove an old > review request. > Ah, got it now. Thank you Gavin!

If I'm reading the code correctly, the patch for the 1.8 branch does not appear to have been committed. Neilx, if you're reading this can you please try to verify?

It does not have the appropriate keywords, and I have not found any commits in bonsai between those dates, so I'm pretty sure this one wasn't applied.

sergei, it looks like this is another one to apply to branch. It was approved but maybe never actually placed in the branch code.

(In reply to comment #71) > sergei, it looks like this is another one to apply to branch. It was approved > but maybe never actually placed in the branch code. > If it still applies and works without side-effects, and this approval is still valid, I will check that in

(In reply to comment #71) > sergei, it looks like this is another one to apply to branch. It was approved > but maybe never actually placed in the branch code. > Floating/Modal code is already in branch.

(In reply to comment #73) > (In reply to comment #71) > > sergei, it looks like this is another one to apply to branch. It was approved > > but maybe never actually placed in the branch code. Windows creation patch does not apply here for branch.