[BeOS] Cleanup Move,Resize and Scroll handling in widget

RESOLVED FIXED

Status

Core Graveyard
Widget: BeOS
RESOLVED FIXED
12 years ago
2 years ago

People

(Reporter: Sergei Dolgov, Assigned: Sergei Dolgov)

Tracking

({fixed1.8})

Trunk
x86
BeOS
fixed1.8
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

12 years ago
At the moment Resize and Move events triggered by BView::FrameMoved() and
BView::FrameResized() tend to generate too much overhead.
Patch will follow
(Assignee)

Comment 1

12 years ago
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
Assignee: nobody → sergei_d
Status: NEW → ASSIGNED
(Assignee)

Comment 2

12 years ago
P.S. - also added kids hiding in nsWindow::Move() like we do in ::Scroll()
(Assignee)

Comment 3

12 years ago
don't pay attention at extra {} etc now - this isn't for review atm.
Just for functionality test
(Assignee)

Comment 4

12 years ago
Created attachment 198935 [details] [diff] [review]
patch

same as previous, but added HideChildren action for ceratain conditions
(aRepaint == PR_TRUE) to nsWindow::Resize()
Attachment #198919 - Attachment is obsolete: true
(Assignee)

Comment 5

12 years ago
Created attachment 198955 [details] [diff] [review]
patch

Adding missing for misterious reason number in assertion. Thanks to Doug
Shelton.
(Assignee)

Updated

12 years ago
Attachment #198935 - Attachment is obsolete: true

Updated

12 years ago
Blocks: 311032

Comment 6

12 years ago
patch applies and compiles cleanly now.
(Assignee)

Comment 7

12 years ago
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()
Attachment #198955 - Attachment is obsolete: true
Attachment #199004 - Flags: review?(thesuckiestemail)

Comment 8

12 years ago
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();

Updated

12 years ago
Attachment #199004 - Flags: review?(thesuckiestemail) → review-
(Assignee)

Comment 9

12 years ago
Created attachment 199017 [details] [diff] [review]
patch

ancient Move and Resize code refactored.
HideKids(PRBool) function added
(Assignee)

Updated

12 years ago
Attachment #199004 - Attachment is obsolete: true
(Assignee)

Comment 10

12 years ago
Comment on attachment 199017 [details] [diff] [review]
patch

obsoleting.
Better scrolling trick found, and it needs bit different HideKids() method
Attachment #199017 - Attachment is obsolete: true
(Assignee)

Comment 11

12 years ago
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)
Attachment #199550 - Flags: review?(thesuckiestemail)
(Assignee)

Updated

12 years ago
Summary: [BeOS] Cleanup Move and Resize handling in widget → [BeOS] Cleanup Move,Resize and Scroll handling in widget
(Assignee)

Comment 12

12 years ago
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

12 years ago
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)
Attachment #199550 - Flags: review?(thesuckiestemail) → review+
(Assignee)

Comment 14

12 years ago
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 
(Assignee)

Comment 15

12 years ago
marking fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

12 years ago
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

12 years ago
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.
Attachment #199550 - Flags: approval1.8rc1?

Updated

12 years ago
Attachment #199550 - Flags: approval1.8rc1? → approval1.8rc1+
(Assignee)

Comment 18

12 years ago
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
(Assignee)

Comment 19

12 years ago
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
Attachment #199943 - Flags: review?(thesuckiestemail)
(Assignee)

Updated

12 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 20

12 years ago
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

12 years ago
The patch isn't in CVS yet, so I'll just see if I can remove the approval flag.

Comment 22

12 years ago
Comment on attachment 199550 [details] [diff] [review]
patch, wider usage of HideKids method

I could!
Attachment #199550 - Flags: approval1.8rc1+

Comment 23

12 years ago
Comment on attachment 199943 [details] [diff] [review]
patch

r=thesuckiestemail@yahoo.se
Attachment #199943 - Flags: review?(thesuckiestemail) → review+
(Assignee)

Comment 24

12 years ago
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 
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 25

12 years ago
Created attachment 199980 [details] [diff] [review]
cmmulative patch

combining both first patch and second(partial rollback)
for Nielx convinience to checkin in branch.
(Assignee)

Comment 26

12 years ago
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?
Attachment #199980 - Flags: review?(thesuckiestemail)
(Assignee)

Comment 27

12 years ago
Comment on attachment 199550 [details] [diff] [review]
patch, wider usage of HideKids method

invalid for branch
Attachment #199550 - Attachment is obsolete: true
(Assignee)

Comment 28

12 years ago
Comment on attachment 199943 [details] [diff] [review]
patch

invalid for branch
Attachment #199943 - Attachment is obsolete: true

Updated

12 years ago
Attachment #199980 - Flags: review?(thesuckiestemail) → review+

Updated

12 years ago
Blocks: 266252

Comment 29

12 years ago
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.
Attachment #199980 - Flags: approval1.8rc2?

Updated

12 years ago
Attachment #199980 - Flags: approval1.8rc2? → approval1.8rc2+
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
Keywords: fixed1.8
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.