Robert Kaiser Assignee
	Description • 15 years ago

Once bug 498596 switches us to places bookmarks, we also should get tests for those, starting porting the Firefox tests over to SeaMonkey.

	Robert Kaiser Assignee
	Comment 1 • 14 years ago

This patch adds all the tests that succeed, I spent too many hours trying to make others work, esp. in the browser-chrome area, and I gave up on them for now.

	Justin Wood (:Callek)
	Comment 2 • 14 years ago

Comment on attachment 458244 [details] [diff] [review]
add those tests that succeed

rs+ based on a skim... but

>+++ b/suite/common/places/tree.xml
>-            for (let j = min.value; j <= max.value; ++j)
>-              nodes.push(resultview.nodeForTreeIndex(j));
>+            if (max.value > -1)
>+              for (let j = min.value; j <= max.value; ++j)
>+                nodes.push(resultview.nodeForTreeIndex(j));

Was this another patches remnant, or needed for a test here, if so can you please explain it for posterity.

(If I find time to actually review the tests, I may steal review from neil; but I would think my rs+ is ok for the test-only changes, especially if they all pass.)

	Robert Kaiser Assignee
	Comment 3 • 14 years ago

(In reply to comment #2)
> Was this another patches remnant, or needed for a test here, if so can you
> please explain it for posterity.

Thanks for catching that, it's indeed a remnant, though from testing why some of the not added tests failed. It's not wrong, but it probably doesn't belong there.

	Robert Kaiser Assignee
	Comment 4 • 14 years ago

Pushed as http://hg.mozilla.org/comm-central/rev/b9e980d6f85d

	Serge Gautherie (:sgautherie)
	Comment 5 • 14 years ago

(In reply to comment #3)
> It's not wrong, but it probably doesn't belong there.

This |if (max.value > -1)| got pushed anyway.
Should we revert it (ftb)?
Or try to add it to Firefox too?