292236 - Tabbrowser iterates through progress listeners in reverse order

Status: NEW

(SeaMonkey :: Tabbed Browser, defect)

Description

[Alex Vincent [:WeirdAl]]

http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/tabbrowser.xml#196

The original progress listener order is from last to first.  tabbrowser works
from first to last.

This caused a really lovely regression with our corporate product when I removed
tabbrowser from the loop...

Patch will be forthcoming.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: Reverse order of tabbrowser progress listeners, to follow doc loader

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: Reverse order of tabbrowser progress listeners, to follow doc loader

I missed one...

Comment 3

[Boris Zbarsky [:bzbarsky]]

I'm not a peer for either xpfe or toolkit; I suggest getting reviews from neil
and mconnor.

Comment 4

[[e-mail]]

Comment on attachment 182083 [details] [diff] [review]
Reverse order of tabbrowser progress listeners, to follow doc loader

You missed one? More like six.

Comment 5

[Alex Vincent [:WeirdAl]]

Attachment: Reverse order of tabbrowser progress listeners, to follow doc loader

... and six more makes a baker's dozen.

Comment 6

[[e-mail]]

Comment on attachment 182101 [details] [diff] [review]
Reverse order of tabbrowser progress listeners, to follow doc loader

r=me by code inspection.

I wish the syntax for a downward-counting loop wasn't so ugly :-/

Comment 7

[[e-mail]]

(In reply to comment #5)
>... and six more makes a baker's dozen.
A baker's dozen is 13, not 11 :-P

Comment 8

[Alex Vincent [:WeirdAl]]

Comment on attachment 182101 [details] [diff] [review]
Reverse order of tabbrowser progress listeners, to follow doc loader

Oh, I know perfectly well it's 13.

1 I filed the second patch for.  6 you claimed for one of the tabbrowser.xml
files.	I claim six more for the other.

Comment 9

[jag (Peter Annema)]

Hmm, ok, I see what you mean now. I guess it's a good thing to make tabbrowser
do what nsDocLoader does, but I'm not sure we've ever guaranteed you can depend
on the order of notification. I'm also not sure we want to start doing that.

Comment 10

[jag (Peter Annema)]

Comment on attachment 182101 [details] [diff] [review]
Reverse order of tabbrowser progress listeners, to follow doc loader

That said, sr=jag

Let's hope no one's depending on tabbrowser iterating from first to last.

Comment 11

[Alex Vincent [:WeirdAl]]

I've no plans to seek approval for checkin now -- but I'll ask someone to land
this patch shortly after trunk thaws.

Comment 12

[Hermann Schwab]

When should this patch be checked in? In the 1.9 cycle? When it is bit-rotten?

Comment 13

[Alex Vincent [:WeirdAl]]

I'm thinking 1.9 cycle -- it's just not important enough to justify for the 1.8
cycle this late in the game.

Comment 14

[Alex Vincent [:WeirdAl]]

Attachment: Reverse order of tabbrowser progress listeners, to follow doc loader

Patch has bitrotted (grrr).  Neil, please advise.  In xpfe...tabbrowser, there
is:

for each (var p in this.mProgressListeners)
  if (p && 'onLinkIconAvailable' in p)
    p.onLinkIconAvailable(href);

This was revision 1.119.  I really don't know which ordering will result from a
for...in search.

Other than that, here's the new patch.

Comment 15

[James Ross]

for..in has no defined ordering, I believe. In Spidermonkey, however, the order
seems to be based on the last setting of each property - but I would not want to
rely on anything as subtle as that!

Comment 16

[[e-mail]]

Comment on attachment 192604 [details] [diff] [review]
Reverse order of tabbrowser progress listeners, to follow doc loader

As it happens, we only grow the array using push(), so the order we happen to
enumerate is 0 up order. Oddly unshift() also enumerates in 0 up order...

Comment 17

[Alex Vincent [:WeirdAl]]

Comment on attachment 192604 [details] [diff] [review]
Reverse order of tabbrowser progress listeners, to follow doc loader

jag, do you want to carry over the sr, or actively re-sr this patch?

Comment 18

[jag (Peter Annema)]

Comment on attachment 192604 [details] [diff] [review]
Reverse order of tabbrowser progress listeners, to follow doc loader

Maybe I'm missing something, but I don't see the change for the |for each| line
you asked about.

Comment 19

[Alex Vincent [:WeirdAl]]

You're not missing anything.  It's not included in the diff, because I wasn't
sure what we're supposed to do there.

Comment 20

[Alex Vincent [:WeirdAl]]

Attachment: followup to attachment 192604

Neil and I discussed the for...each loop and realized it wasn't going to work.

Comment 21

[[e-mail]]

Comment on attachment 192610 [details] [diff] [review]
followup to attachment 192604 [details] [diff] [review]

So what happened to my idea of unshifting the progress listeners into the array
instead of pushing them?

Comment 22

[Alex Vincent [:WeirdAl]]

(In reply to comment #21)
> So what happened to my idea of unshifting the progress listeners into the array
> instead of pushing them?

I'm not convinced it will produce the same results. 

Comment 23

[jag (Peter Annema)]

That for each simply iterates over the progress listeners. It replaced a 0..n-1
iteration. This bug is about reversing that iteration so we do the same as
before tabbrowser. Therefore, can't we simply just do the n-1..0 iteration and
be done? No trickery, no "what if this undefined order changes in the future",
and best of all, it'll look just like all the other loops in this file.

Comment 24

[Alex Vincent [:WeirdAl]]

(In reply to comment #23)
> That for each simply iterates over the progress listeners. It replaced a 0..n-1
> iteration. This bug is about reversing that iteration so we do the same as
> before tabbrowser. Therefore, can't we simply just do the n-1..0 iteration and
> be done? No trickery, no "what if this undefined order changes in the future",
> and best of all, it'll look just like all the other loops in this file.

jag, I thought the manual loops I gave did exactly that.  :)

That said, I realize now that Neil is right, according to xpcshell:

js> var x = [];
js> x.unshift("one");
1
js> x.unshift("two");
2
js> x
two,one
js> var y
js> for (y in x) { dump(y + "\n") };
0
1
js> for each (y in x) { dump(y + "\n") };
two
one

We really could go either way, with my patches as posted, or with the route Neil
suggested (changing push to unshift in precisely two places).  jag, if you want
me to go that route, sr- both patches and it will be done.

Comment 25

[jag (Peter Annema)]

Alex: oops, I missed the "Created an attachment" part, I thought you were just
commenting on it.

Neil, Alex: I don't care wether we |unshift| and |for each| or use |for n-1..0|,
as long as we use it consistently. The only problem I have with |for each| is
that there appears to be no guarantee about the order, while this bug is about
iterating in a specific known order. 

Comment 26

[jag (Peter Annema)]

Comment on attachment 192610 [details] [diff] [review]
followup to attachment 192604 [details] [diff] [review]

So sr=jag, but ignore if Neil prefers |for each|, though I'd almost say we
should change all the other iteration loops to |for each| in that case too.

Comment 27

[jag (Peter Annema)]

Comment on attachment 192610 [details] [diff] [review]
followup to attachment 192604 [details] [diff] [review]

The one thing I don't like about this though is that you're reusing |i|. It
works in this case because we return if we get a match in the outer loop, but
this is code asking for subtle bugs if someone changes the flow.

Comment 28

[Alex Vincent [:WeirdAl]]

(In reply to comment #27)
> (From update of attachment 192610 [details] [diff] [review] [edit])
> The one thing I don't like about this though is that you're reusing |i|. It
> works in this case because we return if we get a match in the outer loop, but
> this is code asking for subtle bugs if someone changes the flow.

Zut!  That should've been a "var j", not "i".  Good catch, and I hope whoever
checks it in (I don't have checkin privs) notes that.

Comment 29

[[e-mail]]

Ok, so how about this solution:
* Change the push into an unshift, which fixes all the 0 .. n loops
* Change the foreach into an 0 .. n loop just to be quite safe

Comment 30

[Alex Vincent [:WeirdAl]]

Sounds perfectly reasonable; I'll get it done in the next 24 hours.

Comment 31

[[e-mail]]

Strike my last comment. New idea: count down as before, but additionally tweak
removeProgressListener to really remove it, thus saving on null checks (this
optimization isn't safe with the existing count up loops, and don't ask me what
happens to the for each loop...)