604093 - references to undefined variable "XULWindowBrowser" and calls to undefined function setStatusText in browser.js

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

This looks to have been introduced by http://hg.mozilla.org/mozilla-central/rev/bb2db707bfcb (bug 574688). "XULWindowBrowser" looks like it was meant to be XULBrowserWindow, but it doesn't have a setStatusText method either as far as I can tell.

I don't think this impacts any behavior, apart from perhaps dragging things on the downloads button, since it's almost always the last line in the dragLeave handler.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: v1

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 482929 [details] [diff] [review]
v1

To fully remove this, we'd need to also remove browserDragAndDrop.dragOver()'s statusString argument, and its call to XULBrowserWindow.setStatusText, and likely all of the strings that correspond to the string names passed to it.

The other alternative is to use the URL bar link thing to show this status text, but I don't know if that makes any sense from a UX perspective.

Comment 4

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #2)
> The other alternative is to use the URL bar link thing to show this status
> text, but I don't know if that makes any sense from a UX perspective.

It doesn't make any sense, since there is a state possible, when status text should be shown and the cursor is above an URL, so it's target should be also shown.

Comment 5

[Dietrich Ayala (:dietrich)]

See also bug 600647.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #2)
> The other alternative is to use the URL bar link thing to show this status
> text, but I don't know if that makes any sense from a UX perspective.

Or to leave it in as an empty stub for addons who want to implement status showing, as bug 600647 suggests.

Comment 8

[:Margaret Leibovic]

(In reply to comment #2)
> The other alternative is to use the URL bar link thing to show this status
> text, but I don't know if that makes any sense from a UX perspective.

I'm doing this in bug 603777. I'm not sure how that affects this bug, but I figured I should bring it to your attention.

Comment 9

[Dietrich Ayala (:dietrich)]

Attachment: v2

keep it intact, since it'll be used.

Comment 10

[Matthew Turnbull [Bluefang]]

Comment on attachment 494734 [details] [diff] [review]
v2

Shouldn't this also include a stub implementation for XULBrowserWindow.setStatusText, even if it is just an empty function?

Regardless of whether or not this is actually used by the browser, I think it is important to keep it around for extensions. Unless extensions first check if it exists, using XULBrowserWindow.setStatusText would cause an error if it wasn't implemented.

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: v3

Implements setStatusText, and depends on the work in bug 603777.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Bug 603777 isn't a blocker, so I'm not sure it's a good dependency to have here. I'm actually leaning more towards just removing all the dead code (per comment 2). Theoretical future extensions that want to show status text are unlikely to care too much about drag drop feedback for random toolbar buttons, and if they do they can pretty easily add drag listeners themselves, so us bearing the maintenance burden of code that we don't use doesn't seem like a great idea.

Comment 13

[Dietrich Ayala (:dietrich)]

Ok. I guess it also makes sense to not cloud the cleanup with the extensibility question.

Comment 14

[Dietrich Ayala (:dietrich)]

Attachment: v4 - removal

Comment 15

[Dietrich Ayala (:dietrich)]

https://hg.mozilla.org/mozilla-central/rev/7527bd2dff2a