Last Comment Bug 311651 - [BeOS] Cleanup Move,Resize and Scroll handling in widget
: [BeOS] Cleanup Move,Resize and Scroll handling in widget
Status: RESOLVED FIXED
: fixed1.8
Product: Core Graveyard
Classification: Graveyard
Component: Widget: BeOS (show other bugs)
: Trunk
: x86 BeOS
: -- normal (vote)
: ---
Assigned To: Sergei Dolgov
:
Mentors:
Depends on:
Blocks: 266252 311032
  Show dependency treegraph
 
Reported: 2005-10-08 05:02 PDT by Sergei Dolgov
Modified: 2014-12-09 11:27 PST (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (10.08 KB, patch)
2005-10-08 05:33 PDT, Sergei Dolgov
no flags Details | Diff | Review
patch (10.40 KB, patch)
2005-10-08 08:13 PDT, Sergei Dolgov
no flags Details | Diff | Review
patch (10.41 KB, patch)
2005-10-08 14:27 PDT, Sergei Dolgov
no flags Details | Diff | Review
patch (11.22 KB, patch)
2005-10-09 09:24 PDT, Sergei Dolgov
thesuckiestemail: review-
Details | Diff | Review
patch (16.76 KB, patch)
2005-10-09 12:03 PDT, Sergei Dolgov
no flags Details | Diff | Review
patch, wider usage of HideKids method (19.49 KB, patch)
2005-10-14 06:33 PDT, Sergei Dolgov
thesuckiestemail: review+
Details | Diff | Review
patch (929 bytes, patch)
2005-10-18 09:23 PDT, Sergei Dolgov
thesuckiestemail: review+
Details | Diff | Review
cmmulative patch (19.57 KB, patch)
2005-10-18 14:15 PDT, Sergei Dolgov
thesuckiestemail: review+
mtschrep: approval1.8rc2+
Details | Diff | Review

Description Sergei Dolgov 2005-10-08 05:02:30 PDT
At the moment Resize and Move events triggered by BView::FrameMoved() and
BView::FrameResized() tend to generate too much overhead.
Patch will follow
Comment 1 Sergei Dolgov 2005-10-08 05:33:48 PDT
Created attachment 198919 [details] [diff] [review]
patch

Let Gecko to handle child widgets resize and move by self.
Generate resize/move events only from BWindow - with reduced message size
(obtain  bounds in main thread instead sending via messages).

Needs testting
Comment 2 Sergei Dolgov 2005-10-08 05:35:23 PDT
P.S. - also added kids hiding in nsWindow::Move() like we do in ::Scroll()
Comment 3 Sergei Dolgov 2005-10-08 07:40:29 PDT
don't pay attention at extra {} etc now - this isn't for review atm.
Just for functionality test
Comment 4 Sergei Dolgov 2005-10-08 08:13:13 PDT
Created attachment 198935 [details] [diff] [review]
patch

same as previous, but added HideChildren action for ceratain conditions
(aRepaint == PR_TRUE) to nsWindow::Resize()
Comment 5 Sergei Dolgov 2005-10-08 14:27:15 PDT
Created attachment 198955 [details] [diff] [review]
patch

Adding missing for misterious reason number in assertion. Thanks to Doug
Shelton.
Comment 6 Doug Shelton 2005-10-08 15:09:59 PDT
patch applies and compiles cleanly now.
Comment 7 Sergei Dolgov 2005-10-09 09:24:06 PDT
Created attachment 199004 [details] [diff] [review]
patch

additionally cleaned up OnResize() method:
1)in BeOS client area bounds are same as ::Bounds().
2)no need to call mView->Bounds() and lock looper once more - it was done in
GetBounds()
Comment 8 tqh 2005-10-09 10:02:04 PDT
I think we should not duplicate code so the 

for (nsIWidget* kid = mFirstChild; kid; kid = kid->GetNextSibling())  ...

should be in a private call, (maybe inlined). Also while we are fixing Move and
Resize I think we should skip the if's mustUnlock and haveWindow and just set
the booleans to the expressions inside the if();
So that:
if(mView->LockLooper())
  mustUnlock = true
would be
mustUnlock = mView->LockLooper();
Comment 9 Sergei Dolgov 2005-10-09 12:03:19 PDT
Created attachment 199017 [details] [diff] [review]
patch

ancient Move and Resize code refactored.
HideKids(PRBool) function added
Comment 10 Sergei Dolgov 2005-10-14 05:35:46 PDT
Comment on attachment 199017 [details] [diff] [review]
patch

obsoleting.
Better scrolling trick found, and it needs bit different HideKids() method
Comment 11 Sergei Dolgov 2005-10-14 06:33:59 PDT
Created attachment 199550 [details] [diff] [review]
patch, wider usage of HideKids method

Changes over previous version
Refactored HideKids() method (hiding-unhiding ALL VISIBLE children now),
added mIsScrolling trigger to control whether HideKids to be called, using
HideKids in Scroll() together with trigger, reducing locking usage.
Both resize and scrolling  turns better (especially scrolling) with this patch
(see ),
last remaining scrolling problem is now XUL-knob, whith ugly lag in update (as
using asynchronous version of update)
Comment 12 Sergei Dolgov 2005-10-14 07:54:28 PDT
Little explanation to whom it may concern about scrolling part of patch.
nsWindow::Scroll() triggers ON flag mIsScrolling and and HideKids hides (only
those) children which intersects with current mBounds.
nsWindow::Update() unhides children (again, only visible) if msIsScrolling is
OFF. It means - when this method is called by something other than gkview Scroll().
Then Update() siwtches mIsScrolling  OFF unconditionally. So, if no Scroll is
called, with next Update() call children will be visible again.

All that prevents very intensive Hide/Show sequence during scroll, so prevents
same huge amount of unneccessary Invalidate/Repaint events.
Comment 13 tqh 2005-10-14 11:32:21 PDT
Comment on attachment 199550 [details] [diff] [review]
patch, wider usage of HideKids method

r=thesuckiestemail@yahoo.se

Works ok. (We might want to improve hideKids in the future though)
Comment 14 Sergei Dolgov 2005-10-14 11:42:16 PDT
landed in trunk:
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.98; previous revision: 1.97 
/cvsroot/mozilla/widget/src/beos/nsWindow.h,v  <--  nsWindow.h
new revision: 1.37; previous revision: 1.36 
Comment 15 Sergei Dolgov 2005-10-14 12:02:18 PDT
marking fixed
Comment 16 Sergei Dolgov 2005-10-17 03:04:30 PDT
I got strange behaviour with context menus on some sites with crappy code and
slow download which imports advertising into iframes.
Sometimes right-clicking on that iframe causes context menu to appear far away
from click point. I suspect that we should remove HideKids from :Move() but
cannot be sure atm about reason and remedy, because i lack good testcase:(
Comment 17 Niels Reedijk 2005-10-17 12:15:25 PDT
Comment on attachment 199550 [details] [diff] [review]
patch, wider usage of HideKids method

Requesting approval for the MOZILLA_1_8_BRANCH. This is a BeOS only change and
will therefore not pose any risk for the other platforms.
Comment 18 Sergei Dolgov 2005-10-18 04:10:05 PDT
it appears that in combination with scroll HideKids used in Move() sometimes
brings bug on page with IFRAMEs. While right-click on some IFRAMEs works
correctly, other cause strnage thing - context menu appears far away from click
point. So we will remove it for now
Comment 19 Sergei Dolgov 2005-10-18 09:23:47 PDT
Created attachment 199943 [details] [diff] [review]
patch

current visibility control code is insufficient it may cause strange effects
 with sites with slowly imported IFRAME content.
Let this problem for future
Comment 20 Sergei Dolgov 2005-10-18 09:39:08 PDT
Nielx, there is little problem now with 1.8.
I'm rolling back one of changes in previous patch.
So i don't know what is better way - edit previous patch for you or you will
apply new one later too.
Comment 21 Niels Reedijk 2005-10-18 11:57:58 PDT
The patch isn't in CVS yet, so I'll just see if I can remove the approval flag.
Comment 22 Niels Reedijk 2005-10-18 11:58:23 PDT
Comment on attachment 199550 [details] [diff] [review]
patch, wider usage of HideKids method

I could!
Comment 24 Sergei Dolgov 2005-10-18 14:03:25 PDT
checked
Checking in mozilla/widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.103; previous revision: 1.102
done 
Comment 25 Sergei Dolgov 2005-10-18 14:15:18 PDT
Created attachment 199980 [details] [diff] [review]
cmmulative patch

combining both first patch and second(partial rollback)
for Nielx convinience to checkin in branch.
Comment 26 Sergei Dolgov 2005-10-18 14:17:02 PDT
Comment on attachment 199980 [details] [diff] [review]
cmmulative patch

tqh, can you put review here too, for nielx convinience - to ask single
approval and commit it with single action?
Comment 27 Sergei Dolgov 2005-10-18 14:19:08 PDT
Comment on attachment 199550 [details] [diff] [review]
patch, wider usage of HideKids method

invalid for branch
Comment 28 Sergei Dolgov 2005-10-18 14:19:44 PDT
Comment on attachment 199943 [details] [diff] [review]
patch

invalid for branch
Comment 29 Niels Reedijk 2005-11-01 06:54:38 PST
Comment on attachment 199980 [details] [diff] [review]
cmmulative patch

Requesting approval to land in the MOZILLA_1_8_BRANCH. This is a BeOS only change and will not affect the other platforms in any way.
Comment 30 Christian :Biesinger (don't email me, ping me on IRC) 2005-11-02 05:00:39 PST
checked in on MOZILLA_1_8_BRANCH

Checking in widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.91.4.7; previous revision: 1.91.4.6
done
Checking in widget/src/beos/nsWindow.h;
/cvsroot/mozilla/widget/src/beos/nsWindow.h,v  <--  nsWindow.h
new revision: 1.35.4.4; previous revision: 1.35.4.3
done

Note You need to log in before you can comment on or make changes to this bug.