108817 - [FIX]Make nsListControlFrame timer simpler, flush reflows appropriately

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[John Keiser (jkeiser)]

The nsListControlFrame timer has a lot of extra stuff in it.  Now that the bug
34297 patch has landed, its only job is to tell the select when to reflow so
that we aren't doing it constantly.  Thus I propose it have just one flag,
mShouldReflow, when when flown will cause a reflow when the timer stops.

In the ideal world, mShouldReflow should be set when:

- an option's text changes
- an option is added
- an option is removed
- size is changed

The first three affect horizontal size, add/remove options affect dropdown
vertical size (I believe), and size of course affects vertical size.  Reflowing
when an option's text changes might actually fix some other option bugs on that
subject ...

Other ideas of when to set the flag are welcome.  When the flag is set, and the
timer goes off, the flush will occur.

This will entail making sure selected indices get reset as soon as options are
removed or added (so that that doesn't get lost in the bargain).  We could add a
flag in the timer for that, but I'm thinking setting 3-4 ints isn't going to be
much slower than setting a flag in the timer, and certainly not enough to
increase complexity by doing that.

I have a start on this, but I'd like to hear comment.

rods?  This change is triggered by the problem/solution you found for reflows
(we aren't flushing reflows every time an option is added but doing it on every
add would be too costly so we need it in a timer).  I can't reproduce the bug,
probably because it's a timing thing or possibly because GFX controls do things
differently.  But the timer needs cleaning up anyway.

Comment 1

[rods (gone)]

It is easy to reproduce on Windows, I will attach an example to this bug. And 
yes, we will need a timer to do the flush after the JS has completed executing 
(adding and removing items)

Comment 2

[rods (gone)]

Here is the example:
http://bugzilla.mozilla.org/attachment.cgi?id=54662&action=view

Click on the "Add 100 Items" button.

Comment 3

[John Keiser (jkeiser)]

So do you agree with the other parts of it (flushing on text change and size
change)?  I am by no means expert on when to flush, but my general impression is
you flush whenever the size of the box changes (or could change) horizontally or
vertically.

Comment 4

[Johnny Stenback (:jst)]

FlushPendingNotifications() (on nsIPresShell) should only be called when size
and position (i.e. reflow results) are *asked for* and the answer is needed
synchronously. I'm by no means an expert in reflow or any layout code, but I
don't see why the timer would need to *flush*, a *flush* should only be needed
to force layout to synchronously complete what would happen asynchronously
anyway. Shouldn't the timer simply notify the frames about the changes so that
the proper reflow commands are queued, which would cause the right thing to
happen once we're reflowing again?

Correct me if I'm wrong.

Comment 5

[John Keiser (jkeiser)]

That makes sense to me.  In fact, I think that's what rods was talking about and
I misread it--the FlushPendingNotifications he was talking about, commented out
in nsComboboxControlFrame::SelectionChanged(), is like this:

//      nsCOMPtr<nsIPresShell> presShell;
//      mPresContext->GetShell(getter_AddRefs(presShell));
//      presShell->FlushPendingNotifications(PR_FALSE);

Can you confirm the looking-bad problem BTW?  (I'm on Linux and it's not
happening here.)

Comment 6

[Johnny Stenback (:jst)]

I can't say I see any display problems in that testcase either, I tested on
Win2k and Linux, I click the "Add 100 items" button and it adds 100 items and
selects the 10th item in all selects, and from looking at the JS in the testcase
it seems like that's what it should do. Rods, care to explain what the problem
you're seeing is?

Comment 7

[John Keiser (jkeiser)]

Attachment: First Cut Patch (WIP)

Comment 8

[John Keiser (jkeiser)]

This is a first cut patch for this stuff.  rods, could you try it out and see if
you have any issues with things not showing up?  Anyone else from Windows, too,
but you seem to catch those errors more often than anyone else.

This patch doesn't handle size change yet (we apparently never did) and it
doesn't handle option text size change either (same).  The main thing you'll
notice is, it's not flushing reflows, it's adding a style reflow flush command
to the presentation shell (for later execution I assume).  This seems to work,
but I'm open for suggestions.  I really don't know how flushing works--how *are*
you supposed to mark a component for reflow?  Is adding it the command to the
shell the Right Thing?

Comboboxes need work in terms of this.  I am not yet sure what needs to be done
in place of the commented-out flush rods found, but I'd rather we just mark it
dirty somehow (perhaps adding a reflow command to the shell).

One neat new gimmick of this patch does is that it protects the selected index
against addition and removal of options ... i.e. if you click on an option, add
an option into the select, and then hit shift+click on another option, it will
work as expected (try it out at
http://www.johnkeiser.com/mozilla/select/jsadd.html).

Comment 9

[John Keiser (jkeiser)]

BTW, the only thing the timer does now is scroll the select.  If we:

(1) do not scroll the select except when the select is first created
or
(2) scroll the select immediately when selectedIndex changes

Then we can do away with the timer completely!  That would be a big hip hip hooray!

So I'd like comment on this too, especially from rods :)

Comment 10

[John Keiser (jkeiser)]

Attachment: First Cut Patch w/o rejects

I had some foo in that patch from another patch which I just checked in, so it
fails now--this patch works.

Comment 11

[John Keiser (jkeiser)]

Attachment: Much Cooler But Costlier Patch

This patch gets rid of the timer entirely, and hurts performance a little. 
Tests show between imperceptible and 50% slowdown when adding huge numbers of
options--it ends up being "still really really small amount of time," but it's
a bigger small amount of time.	The culprit is probably the thing that adds the
reflow request into the pres shell on AddOption().

There is an optimization that can be made to keep initial page load speed down
to current levels (specifically, don't trigger reflow until after all options
have been addedm or do it on every 20th one or when finished adding options,
whichever comes first) but JS addition will still suffer these penalties.

If it is deemed a worthy sacrifice, great, if not, that's fine too.  I am
thinking I don't want to deal with reflow issues here; because of (probably
unnecessarily complex) combobox interactions, select frames are really hard to
deal with.  I'd rather implement <select> using XUL/XBL/Outliner ... it really
does need a clean wipe. :)

Comment 12

[Vedran Miletic]

retargeting

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to current trunk

This kills off the timer without the perf hit by not forcing full reflows like
jkeiser's last patch did.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 155301 [details] [diff] [review]
Patch to current trunk

As far as I can tell, this preserves the old behavior (including when we reset,
lazy resetting, etc) without using a timer.  We just put off the reset till
after the reflow that was triggered by option addition/removal.

While I was in here I removed the mPresContext member, since it's not needed.

Tests show that performance is actually a tad (5%) better with this patch,
since we no longer have to deal with all the timer-munging and associated
locking.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed.