483781 - make tests that require the toolkit dlmgr UI not fail with new SeaMonkey dlmgr UI work

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Robert Kaiser]

Bug 381157 will make SeaMonkey implement its own implementation of nsIDownloadManagerUI, ultimately pointing to the new tree-based DM window created in bug 472001.

The problem that arises with that is that SeaMonkey will fail all tests in toolkit that require the toolkit UI.

The solution we have come up with is to implement a hidden pref in our nsIDownloadManagerUI implementation that will switch us to use the toolkit UI. The name of browser.download.manager.useToolkitUI is generic enough that everyone else doing a similar thing (Fennec?) could do the same.

Now, to not make tests fail, those that actually need it, should also set this pref to true so that they get the UI they expect.

Ideally, this can land on 1.9.1 before SeaMonkey lands bug 381157 and bug 472001 so our test boxes never need to go orange for this.

Comment 1

[Justin Wood (:Callek)]

My latest patch on Bug 472001 enables the SeaMonkey use of this pref, (but does depend on review of Bug 381157). I tested that if the pref is set that these tests pass, regardless of the SeaMonkey browser.download.manager.behavior setting.

The following tests were tested by me:
_CHROME_FILES = \
  test_basic_functionality.xul \
  test_cleanup_search.xul \
  test_clear_button_disabled.xul \
  test_close_download_manager.xul \
  test_delete_key_removes.xul \
  test_esc_key_closes_clears.xul \
  test_multi_select.xul \
  test_multiword_search.xul \
  test_pause_button_state.xul \
  test_removeDownload_updates_ui.xul \
  test_search_clearlist.xul \
  test_search_keys.xul \
  test_select_all.xul \
  test_space_key_pauses_resumes.xul \
  test_ui_stays_open_on_alert_clickback.xul \
  test_unkownContentType_dialog_layout.xul \
  test_bug_412360.xul \
  test_bug_429247.xul \

The only one of these that this pref is not needed for is   test_unkownContentType_dialog_layout.xul

The mac-only test: test_backspace_key_removes.xul I suspect will need this pref, but did not test.

Ideally these tests will set this pref, and reset it when they finish.

I am not sure the best way to go about that in the tests themselves though.

Comment 2

[Robert Kaiser]

Attachment: set .useToolkitUI=true while executing the UI tests

This patch sets the pref while executing the tests and clears it again when finishing every test. This doesn't affect Firefox at all as it doesn't listen to that pref but SeaMonkey stays green with that patch even when the patches in bug 472001 land and any other toolkit-based app that ships with a different dlmgr implementation can implement the pref the same way and get the same effect :)

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 372999 [details] [diff] [review]
set .useToolkitUI=true while executing the UI tests

So, we may have talked about this on irc, and I may have said it was OK (I think that's the case, but I'm not sure).

However, thinking about this some more, I'm not happy with the setup.  New tests will have to do the same thing, but won't fail on Firefox, where people are likely adding the tests.

I'm going to suggest we make a utility js file, like http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/utils.js, and define dmui a method, like getDMUI (better names welcome), that will return the service.  The trick is to get the service by classID, which shouldn't ever change, and if it does, I'd want the tests to fail anyway.

Make sense?

Comment 4

[Robert Kaiser]

(In reply to comment #3)
> The trick is to get the service by classID, which shouldn't ever
> change, and if it does, I'd want the tests to fail anyway.

As explained on IRC, that might work nice we we would package both the toolkit and the SeaMonkey service, which would work nicely if we were a XULRunner app - but here's the caveat: We aren't, so we would have to deliver them in the same directory and component manager isn't predictable in the order in which it loads components from the same directory, which means we wouldn't know which UI would override which one and be used. Because of that, we need to go ugly and not ship the toolkit implementation of nsIDownloadManagerUI and we can't grab it right there. D'Oh!

With that situation, I'm not really sure of a better solution than the patch I attached here.

Comment 5

[Shawn Wilsher :sdwilsh]

How about this:
Make a utility function that checks if we have the classID in Components.classesById and returns a boolean.  Add that to the start of every test and bail early if it returns false.  No preference setting, and resetting needed.

Comment 6

[Robert Kaiser]

Per IRC, we'll just resort to not running those tests in SeaMonkey for now, with an ifdef in the toolkit Makefile. Patch upcoming as soon as I am sober, awake and have time :)

Comment 7

[Justin Wood (:Callek)]

sdwilsh, would setting this pref in the toolkit/ head.js (auto-included in every test) be ok?  we can unset in the cleanup() function (we'd set it AFTER the first-auto-run of cleanup)

And in *our* (seamonkey) DLMGR tests we do similarly but just resetting it if set?

Comment 8

[Robert Kaiser]

Callek, I somehow feel it would be too unclean to not ensure things are cleaned up in *every* test that also calls the setting of the pref. We've had enough bugs already on one test changing execution of another by not cleaning up correctly, I don't want to introduce more of that. I feel better with SeaMonkey not testing toolkit UI automatically than with polluting the pref set of other tests.

Comment 9

[Robert Kaiser]

Attachment: v2: ensure going through toolkit CID

The real fun is here - the new patch actually combined what Shawn says in comment #3 and comment #5 ;-)

It turns out that while we aren't _packaging_ nsDownloadManagerUI.js, we are actually building it. So when running tests from dist/bin, it's actually there, when running them (potentially) from a packaged build, it's not.

With this in mind, I made the utility function for getting the UI (by CID) just return false if the class ID is not found. With this, we can easily either bail out prematurely and note that in logs with a todo() or actually force going through the toolkit service if it exists.

I tested this both with nsDownloadManagerUI.js present in components/ and not and on both SeaMonkey with the dlmgr patches and Minefield and all 4 (!) variants work without failures, either passing all tests or returning a todo() for all (with the exception of the test for private browsing that always returns todo on SeaMonkey).

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 374781 [details] [diff] [review]
v2: ensure going through toolkit CID

nit: @returns instead of @return

r=sdwilsh

Comment 11

[Robert Kaiser]

Pushed as http://hg.mozilla.org/mozilla-central/rev/ab664753cc72 and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/62896a9d16b7