244290 - [FIXr]Propagate view update batching to the root viewmanager

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

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).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch, more or less

Comment 2

[Boris Zbarsky [:bzbarsky]]

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).

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 :-)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Good catch on ProcessPendingUpdates.  What aboug UpdateViews() and
ProcessWidgetChanges()?  I'm not quite sure when some of this stuff is expected
to be called...

Comment 5

[Boris Zbarsky [:bzbarsky]]

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?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

OK, that works.  Maybe I was recalling the control flow for the
propagation-delay version.  ;)

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Attempt to deal with that

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?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Er, I meant this one

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Ok.  I should just take this.  ;)

I'll probably write some inline getters/setters for these members.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Current iteration

Comment 15

[Boris Zbarsky [:bzbarsky]]

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...

Comment 16

[Boris Zbarsky [:bzbarsky]]

+  // mRefreshEnabled should never be accessed on any viewmanager but
+  // the root one.
   PRPackedBool      mRefreshEnabled;
   PRPackedBool      mPainting;

That should say "mRefreshEnabled and mPainting".  Fixed locally.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 149712 [details] [diff] [review]
Current iteration

minusing in response to comments

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Current work

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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Ready to go

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!!!

Comment 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK

Comment 25

[Boris Zbarsky [:bzbarsky]]

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?

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, I think so.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments, with walking the view tree in ForceUpdate()

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Synced to tip

Comment 29

[Boris Zbarsky [:bzbarsky]]

Checked in last night.  This seemed to improve Tdhtml a tad..

Comment 30

[Boris Zbarsky [:bzbarsky]]

This has caused bug 269402 and bug 267557