[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20081217 SeaMonkey/2.0a3pre] (home, optim default) (W2Ksp4) (http://hg.mozilla.org/mozilla-central/rev/4a4a42520901 +http://hg.mozilla.org/comm-central/rev/877154dd96a2 + bug 469606 patch) { *** test pending *** test finished *** exiting *** PASS *** } [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20081221 SeaMonkey/2.0a3pre] (home, optim default) (W2Ksp4) (http://hg.mozilla.org/mozilla-central/rev/b839ff0630c6 +http://hg.mozilla.org/comm-central/rev/2a4c4c1f0feb + bug 469606 patch) { *** test pending TypeError: tests is undefined *** FAIL *** [...] ../../../../_tests/xpcshell-simple/test_places/unit/test_download_history.js:53: TypeError: Cc['@mozilla.org/privatebrowsing;1'] is undefined } This is blamed to bug 458849.

i suppose that's for the lack of provateBrowsing mode, probably for now the best fix would be to split the test and put the pb part into private browsing unit tests, but Places should probably provide an in_memory_database mode and pb should use it, because actually we have code in toolkit dependant on pb that is a browser component.

Yeah, this bug case is much like bug 469593's :->

Comment on attachment 354152 [details] [diff] [review] patch v1 I'm not a peer, but I really don't like these tests to move into browser/, when they're actually testing the download manager code functionality. The correct approach here would be to wrap the code to get the PB service in a try/catch block, and only test the private browsing part if the private browsing service actually exists. For an example of this, check out <http://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/places/tests/unit/test_248970.js>.

s/download manager/places/ of course. :-)

(In reply to comment #4) > (From update of attachment 354152 [details] [diff] [review]) > I'm not a peer, but I really don't like these tests to move into browser/, when > they're actually testing the download manager code functionality. that code path is only pertinent to private browsing, and since it is not a toolkit component the only available place to test that is with pb tests. > > The correct approach here would be to wrap the code to get the PB service in a > try/catch block, and only test the private browsing part if the private > browsing service actually exists. that would work but since private browsing is not part of toolkit would be wrong imho.

or maybe we could move it to browser/places tests since has a dependancy on a browser component...

(In reply to comment #6) > that code path is only pertinent to private browsing, and since it is not a > toolkit component the only available place to test that is with pb tests. That part of the private browsing functionality is implemented in toolkit, not in browser/components/privatebrowsing. > > The correct approach here would be to wrap the code to get the PB service in a > > try/catch block, and only test the private browsing part if the private > > browsing service actually exists. > > that would work but since private browsing is not part of toolkit would be > wrong imho. Since we don't have a central place for the private browsing code to live, we left the tests for each component's functionality along with the component itself. (In reply to comment #7) > or maybe we could move it to browser/places tests since has a dependancy on a > browser component... Since this functionality is implemented in toolkit, browser/components/places looks irrelevant.

Bug 458849 has now/already been checked in on 1.9.1 branch :-|

Comment on attachment 354152 [details] [diff] [review] patch v1 obsoleting, due to ehsan comment, seems like the current path for these tests is to try catch the getService, so we'll move there for now.

Probably a good idea to leave it in toolkit but just not execute the pb part if the pb service isn't available. And this doesn't test downloadmanager but places, right? (I'm just asking because SeaMonkey doesn't build toolkit DM for now - that's planned for the hopefully next few weeks though.)

(In reply to comment #11) > And this doesn't test downloadmanager but places, right? (I'm just asking > because SeaMonkey doesn't build toolkit DM for now - that's planned for the > hopefully next few weeks though.) Yes, I tried to correct myself in comment 5, but apparently the damage was already done!

this simply skips the pb test if pb is not available, like other tests.

Comment on attachment 354818 [details] [diff] [review] patch v1.1 Nits only: >+ // PrivateBrowsing component is not always available to Places implementers I like comments with an ending point. >+ var pb = Cc["@mozilla.org/privatebrowsing;1"]. >+ getService(Ci.nsIPrivateBrowsingService); I prefer |var| before |try|.

fixed Serge's comments

Comment on attachment 354821 [details] [diff] [review] patch v1.2 >+ var pb = null; Nit: |= null| isn't really needed in this case. (Yet, it does no harm.)

verified1.9.1, as the tinderboxes are now passing this test.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090110 SeaMonkey/2.0a3pre] (experimental/_m-c_, home, optim default) (W2Ksp4) (http://hg.mozilla.org/mozilla-central/rev/6acaaa957e0a +http://hg.mozilla.org/comm-central/rev/865f907bb16b) V.Fixed