Closed Bug 244290 Opened 16 years ago Closed 16 years ago

[FIXr]Propagate view update batching to the root viewmanager

Categories

(Core :: Web Painting, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8alpha5

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: perf)

Attachments

(1 file, 8 obsolete files)

See bug 244238 and bug 243726 comment 13 and 14.  We need to propagate the batch
notifications, set the "has pending updates" flags etc, all on the root
viewmanager (really the first step towards having a single viewmanager per window).
Attached patch Patch, more or less (obsolete) — Splinter Review
Comment on attachment 149125 [details] [diff] [review]
Patch, more or less

This should work, pretty much.	Teardown has me worried -- if a parent gets
taken out of the tree, how's it to let its kids know that the root has changed?
 Or is that a non-issue?

Apart from that hitch, this should work (and can brobably be modified easily to
work with the root view changes).
Attachment #149125 - Flags: superreview?(roc)
Attachment #149125 - Flags: review?(roc)
I think teardown should be OK. Child viewmanagers should be destroyed before
their parents in all cases. View reparenting can't change the view manager
hierarchy, because reflow should never move frames across document boundaries
--- one hopes :-).

ProcessPendingUpdates doesn't cross view manager boundaries, see this check:
    if (childView->GetViewManager() == this) {
You'll want to remove this check, otherwise updates for non-root documents won't
get processed, I guess. Having to crawl the entire view hierarchy across all
documents every time we do an update sounds bad, but I suppose it will only show
up in really pathological testcases. If necessary we can address it with a "has
pending invalidates" flag on the view manager, or possibly with a "this view
subtree has pending invalidates" flag on views.

I can't think of any other problems right now, it looks deceptively simple :-)
Good catch on ProcessPendingUpdates.  What aboug UpdateViews() and
ProcessWidgetChanges()?  I'm not quite sure when some of this stuff is expected
to be called...
Though really, very very few views will ever have a dirty region, as far as I
can tell.  Apart from floating views, we store all the dirty region stuff on the
root view with a widget, no?
UpdateViews is only supposed to invalidate the views within the current view
manager, so that's OK.

ProcessWidgetChanges is only called if CACHE_WIDGET_CHANGES is defined, which it
currently isn't. That stuff probably doesn't work at all right now anyway.
I believe that currently any view with a widget can have a dirty region.

With that damage-propagation-delay patch I gave you, only root views will have
dirty regions.
Attached patch Updated to comments (obsolete) — Splinter Review
OK, that works.  Maybe I was recalling the control flow for the
propagation-delay version.  ;)
Attachment #149125 - Attachment is obsolete: true
Attachment #149130 - Flags: superreview?(roc)
Attachment #149130 - Flags: review?(roc)
Attachment #149125 - Flags: superreview?(roc)
Attachment #149125 - Flags: superreview-
Attachment #149125 - Flags: review?(roc)
Attachment #149125 - Flags: review-
Comment on attachment 149130 [details] [diff] [review]
Updated to comments

Hmm... actually, there is a problem here.  If UpdateWidgetArea() is called on
some view that's not in the root viewmanager, there are some issues.
Attachment #149130 - Flags: superreview?(roc)
Attachment #149130 - Flags: superreview-
Attachment #149130 - Flags: review?(roc)
Attachment #149130 - Flags: review-
Attached patch Attempt to deal with that (obsolete) — Splinter Review
OK, this makes sure to call through to the root for Composite() as well.  At
this point, basically, the following member variables only make sense (and
should only be used by) the root viewmanager:

mMouseGrabber
mUpdateCnt
mUpdateBatchCnt
mRefreshEnabled
mHasPendingInvalidates

It's almost worth creating inline getters/setters for these with asserts that
only the root is accessing them... :(

There are a few other variables that I sorta wonder about:

mPainting
mAllowDoubleBuffering

can those really be set differently on root and kids?
Attachment #149130 - Attachment is obsolete: true
Attached patch Er, I meant this one (obsolete) — Splinter Review
Attachment #149147 - Attachment is obsolete: true
mPainting should be hoisted to the root view manager too.

I'm not sure if mAllowDoubleBuffering is realy being used. The only call is
here:
http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsObjectFrame.cpp#803
and I'm not sure if Mac users have that pref set typically, or not.

If we continue to support it, then we can continue to use it as-is, I think. It
can be set differently for root and kids because it gets set when we discover
that the document contains a plugin. But typically the nsViewManager::Refresh is
executed in the view manager associated with the right document.
Ok.  I should just take this.  ;)

I'll probably write some inline getters/setters for these members.
Assignee: roc → bzbarsky
Blocks: 244366
Attached patch Current iteration (obsolete) — Splinter Review
Attachment #149149 - Attachment is obsolete: true
Comment on attachment 149712 [details] [diff] [review]
Current iteration

I had to change UpdateViews() to recurse into kids so that the deferred
recursive paints stuff would work.  I'm not sure I like that, but it seemed
like the simplest way to deal...
Attachment #149712 - Flags: superreview?(roc)
Attachment #149712 - Flags: review?(roc)
+  // mRefreshEnabled should never be accessed on any viewmanager but
+  // the root one.
   PRPackedBool      mRefreshEnabled;
   PRPackedBool      mPainting;

That should say "mRefreshEnabled and mPainting".  Fixed locally.
My build with this patch is showing weird issues (like not painting subframes if
I load new pages in them by clicking on a link in the subframe).  Possibly
because Composite() is being pushed up to the root vm?  Not sure.

It looks like just hacking this in won't really work -- I'll need to sort out
what the current code does (including the interaction with widgets and events),
decide what the new painting model should be, and then implement that... This
could take a while... :(  I certainly doubt I can do that in the 1.8 timeframe.
Comment on attachment 149712 [details] [diff] [review]
Current iteration

minusing in response to comments
Attachment #149712 - Flags: superreview?(roc)
Attachment #149712 - Flags: superreview-
Attachment #149712 - Flags: review?(roc)
Attachment #149712 - Flags: review-
Blocks: 243726
OS: Linux → All
Hardware: PC → All
Attached patch Current work (obsolete) — Splinter Review
Subframes paint fine now.  I'm still building a debug build to see whether any
of those assertions fire, but this looks like it might be money.
Attachment #149712 - Attachment is obsolete: true
Attached patch Ready to go (obsolete) — Splinter Review
This compiles and runs debug, doesn't crash, behaves right, and doesn't assert
on frameset documents, even with DHTML animations running in a frame.
Attachment #162003 - Attachment is obsolete: true
Comment on attachment 162035 [details] [diff] [review]
Ready to go

Summary of changes:

nsIViewManager: Just dumping out my thoughts

nsViewManager: documented the new setup and its reasons for existence.	Added
some functions for safe member access.	Considered a separate nsRootViewManager
class that would have the extra members, but decided that really wasn't worth
it.

nsView: init a pointer to null (not really relevant to this bug, but it was in
my tree in the view module, so...)

nsWindow impls: these were the only two nsIWidget impls the walked the kids on
Update().  So I moved the walk to nsViewManager::ForceUpdate() and removed it
from this code.  I've verified that UpdateIdle() actually walks the list of all
windows that have a draw queued, so the non-forced drawing path is not affected
by this change except insofar as we won't paint widgets that didn't really need
painting anymore.
Attachment #162035 - Flags: superreview?(roc)
Attachment #162035 - Flags: review?(roc)
Priority: -- → P1
Summary: Propagate view update batching to the root viewmanager → [FIX]Propagate view update batching to the root viewmanager
Target Milestone: --- → mozilla1.8alpha5
Comment on attachment 162035 [details] [diff] [review]
Ready to go

Looks good. Just move the root-only nsViewManager fields to one clearly
delimited block, for clarity. THanks!!!
Attachment #162035 - Flags: superreview?(roc)
Attachment #162035 - Flags: superreview+
Attachment #162035 - Flags: review?(roc)
Attachment #162035 - Flags: review+
Actually, I may need to redo part of this because popup widgets will no longer
be in the childlist of nsIWidget.  So my walk in Update() won't work to update
child widgets....  We'll see how bug 264245 goes, and I'll look at this again
once we have that and the other bug 238493 regressions fixed.
Summary: [FIX]Propagate view update batching to the root viewmanager → [FIXr]Propagate view update batching to the root viewmanager
roc, with the fix for bug 264245 popups are no longer in the widget tree.  So
calling Update() on the VM won't repaint them with my patch (it walks the widget
tree).  Should I be walking the view tree instead and calling Update() on the
widgets as I hit them?
Blocks: 264915
Attachment #163029 - Attachment is obsolete: true
Checked in last night.  This seemed to improve Tdhtml a tad..
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
This has caused bug 269402 and bug 267557
Depends on: 267557, 269402
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.