Closed
Bug 470707
Opened 16 years ago
Closed 16 years ago
[SeaMonkey] test_download_history.js fails now
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: sgautherie, Assigned: mak)
References
(Blocks 1 open bug)
Details
(Keywords: regression, verified1.9.1)
Attachments
(1 file, 2 obsolete files)
1.87 KB,
patch
|
dietrich
:
review+
|
Details | Diff | Splinter Review |
[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.
Assignee | ||
Comment 1•16 years ago
|
||
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.
Reporter | ||
Comment 2•16 years ago
|
||
Yeah, this bug case is much like bug 469593's :->
Flags: wanted1.9.2?
Assignee | ||
Comment 3•16 years ago
|
||
Assignee | ||
Updated•16 years ago
|
Attachment #354152 -
Flags: review?(ehsan.akhgari)
Updated•16 years ago
|
Attachment #354152 -
Flags: review?(ehsan.akhgari) → review-
Comment 4•16 years ago
|
||
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>.
Comment 5•16 years ago
|
||
s/download manager/places/ of course. :-)
Assignee | ||
Comment 6•16 years ago
|
||
(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.
Assignee | ||
Comment 7•16 years ago
|
||
or maybe we could move it to browser/places tests since has a dependancy on a browser component...
Comment 8•16 years ago
|
||
(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.
Reporter | ||
Comment 9•16 years ago
|
||
Bug 458849 has now/already been checked in on 1.9.1 branch :-|
No longer depends on: 458849
Flags: wanted1.9.1?
OS: Windows 2000 → All
Hardware: x86 → All
Summary: [SeaMonkey] test_download_history.js fails now, with m-c/1.9.2 → [SeaMonkey] test_download_history.js fails now
Assignee | ||
Comment 10•16 years ago
|
||
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.
Attachment #354152 -
Attachment is obsolete: true
Attachment #354152 -
Flags: review?(sdwilsh)
Comment 11•16 years ago
|
||
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.)
Comment 12•16 years ago
|
||
(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!
Assignee | ||
Comment 13•16 years ago
|
||
this simply skips the pb test if pb is not available, like other tests.
Attachment #354818 -
Flags: review?(dietrich)
Reporter | ||
Comment 14•16 years ago
|
||
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|.
Assignee | ||
Comment 15•16 years ago
|
||
fixed Serge's comments
Attachment #354818 -
Attachment is obsolete: true
Attachment #354821 -
Flags: review?(dietrich)
Attachment #354818 -
Flags: review?(dietrich)
Reporter | ||
Comment 16•16 years ago
|
||
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.)
Updated•16 years ago
|
Attachment #354821 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 17•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Comment 18•16 years ago
|
||
Keywords: fixed1.9.1
Reporter | ||
Comment 19•16 years ago
|
||
verified1.9.1, as the tinderboxes are now passing this test.
Reporter | ||
Comment 20•16 years ago
|
||
[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
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•