437063 - nsIDownloadManagerUI::visible isn't accurate

Status: REOPENED

(Toolkit :: Downloads API, defect)

Description

[Shawn Wilsher :sdwilsh]

It will say the window is visible when it's open, but not focused.

Should check that |this.recentWindow| == nsIWindowWatcher::activeWindow

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 2

[Ed Lee :Mardak]

Comment on attachment 323575 [details] [diff] [review]
v1.0

>+++ b/toolkit/components/downloads/public/nsIDownloadManagerUI.idl
>+  * @throws NS_ERROR_UNEXPECTED if the UI is not opened.
Is this the only way to figure out if the download manager window is opened? Call getAttention and check for an exception? Perhaps we should add an |opened| property? .. perhaps a separate bug ;)

With the opened property, some of the code changes gets simplified..

get visible(): if (!this.opened) return false; ..
getAttention(): if (!this.opened) throw ..

>+++ b/toolkit/components/downloads/test/browser/browser_nsIDownloadManagerUI.js
>@@ -51,9 +51,12 @@ function test_getAttention_opened()
>+  is(dmui.visible, false,
>+     "nsIDownloadManagerUI inidicates that the UI is not visible");
>+
>   dmui.getAttention();
>-  is(dmui.visible, true,
>-     "nsIDownloadManagerUI indicates that the UI is visible");
>+  is(dmui.visible, false,
>+     "nsIDownloadManagerUI indicates that the UI is not visible");
> }
Can we also get a test to assert transitions from true -> false and false -> true? Perhaps update test_visibility_open() to assert true, focus window, assert false, show window, assert true again.

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

addresses review comments

Comment 4

[Ed Lee :Mardak]

Comment on attachment 323582 [details] [diff] [review]
v2.0

r=Mardak

>+  readonly attribute boolean open;
Up to you if you want to keep it as "open". Open is also a verb, so it could be confused as a method. But that's why there's documentation! ;)

Comment 5

[Shawn Wilsher :sdwilsh]

Left it as is since it's an attribute in the idl.

Pushed to mozilla-central with changeset a26c86b184f7

Comment 6

[Shawn Wilsher :sdwilsh]

unit tests failed on linux.  Weird.

Comment 7

[Shawn Wilsher :sdwilsh]

The cause of the orange is documented in bug 437283.