30958 - tree widget should call FlushPendingNotifications, not ProcessReflowCommands

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[buster]

Today, the tree control calls ProcessReflowCommands on the pres shell. It's
safer and more correct to call FlushPendingNotifications.  This bug is a
reminder to either make it possible for hyatt to do the right thing, or make it
safe for him to do what he is doing today.

Below is a thread that's been bouncing around on this issue.

------------------------------ 1. Steve wrote --------------------------------

nsTreeRowGroupFrame calls ProcessReflowCommands() directly.  It does not get a
reflow lock first.

I think nsTreeRowGroupFrame should call FlushPendingNotifications() instead of
ProcessReflowCommands(), shouldn't
it?

My fix would protect the tree code, vidur's would not.  Though Vidur's change
makes plenty of sense.


------------------------------ 2 Vidur wrote ----------------------------------
nsTreeRowGroupFrame calls ProcessReflowCommands directly?! That sounds evil to
me. Did Nisheeth approve that,
Dave? Yeah, FlushPendingNotifications is probably what you want.

------------------------------ 3 Troy wrote ----------------------------------
nsTreeRowGroupFrame really should not be calling ProcessReflowCommands
directly.I hope this is just a temporary measure?

I remember Nisheeth and I discussed the tree widget and reflow commands and I
thought we had a solution that work.
David, did Nisheeth talk with you about this?

------------------------------- 4 Hyatt wrote ---------------------------------
I had to call this because of nisheeth's changes to make reflows asynchronous.
I cannot call FlushPendingNotifications because I am needing to process reflow
commands while a lock has already been grabbed.  In my situation,
FlushPendingNotifications ended up being a no-op, so I couldn't call it.

The real problem keeping me from fixing this is that we grab locks too
aggressively, i.e., we grab a lock on an AttributeChanged before we even know
that we're going to do a reflow.  In my case, although no reflow is required, a
lock is still grabbed, and so I can't use FlushPendingNotifications.

Essentially FlushPendingNotifications is useless when used inside
AttributeChanged/Contentremoved/Contentappended/Contentinserted, because we
ALWAYS grab locks.

Comment 1

[Nisheeth Ranjan]

Accepting bug and setting target milestone to M16...

Comment 2

[Nisheeth Ranjan]

I have a fix for this and will check it in once the tree opens for M16.  I'm
going to attach the patch.  Troy and Hyatt, please code review it.  Thanks.

Comment 3

[Nisheeth Ranjan]

Attachment: Patch

Comment 4

[Nisheeth Ranjan]

I just checked this in.