search

kustodian / extended-statusbar

Firefox addon which adds a statusbar with speed, percentage, time and loaded size (similar to Opera's one)
https://addons.mozilla.org/en-US/firefox/addon/extended-statusbar/
GNU General Public License v3.0
18 stars 6 forks source link

Preliminary Review Bugs (Important) #33

Closed kustodian closed 9 years ago

kustodian commented 9 years ago

ESB just got reviewed, but it didn't pass a full review, so we need to fix these as soon as we can, so that ESB is visible to everyone.

Here is what I got from the reviewer:

This version didn't pass full review because of the following issues:

1) Your add-on calls the setTimeout or setInterval functions with string rather than function arguments, which is normally not allowed. For instance, setTimeout('object.doStuff()') should be written as setTimeout(function () { object.doStuff() }) instead.

2) Stylesheets loaded via the stylesheet service are global in nature, and therefore may not be used without an @-moz-document block to limit their application to the intended documents.

3) Your bootstrap.js compartment remains resident after your add-on has been disabled, which means the you're failing to completely cleanup at shutdown.

You need to correct them to get full approval. Thanks.

Tested on Linux with Firefox 30.0

To learn more about the review process, please visit https://addons.mozilla.org/developers/docs/policies/reviews#selection

If you have any questions or comments on this review, please reply to this email or join #amo-editors on irc.mozilla.org

Mozilla Add-ons https://addons.mozilla.org

kustodian commented 9 years ago

If you need anymore details I'm here to help.

kustodian commented 9 years ago

I fixed the first problem, but that was a no-brainer.

@sipertruk do you have time to check 2 and 3?

kustodian commented 9 years ago

I'm not sure, but this should fix the global CSS problem, but could anyone check if this is good enough?

kustodian commented 9 years ago

@adoxa would you have time to check out problem 3 so that Mozilla can finally release version 2.0 to the public?

Would be awesome if you could check if my second commit fixes problem 2.

adoxa commented 9 years ago

I would, but I don't know what is needed for 3 (maybe just need to set something to null?), and I don't know how to tell if they worked. (And while I'm here, what's the difference between function () { func(); } and just func as an argument?)

Besides, do you really want to release without the proper data value? If you're not going to merge my version, it would be better to remove it (and speed) altogether, 'cos it's just totally wrong as it is.

kustodian commented 9 years ago

I know that the data is wrong, but it was like that from the start. We will merge your changes for the next version, we need to fix the unloading bug first.

I think the main problem with the function thing is that it was called with double quotes. If it was called without quotes all would be good. Because if you call the function with double quotes, then JavaScript needs to do an internal eval, which is a well known security issue.

Regarding the unloading problem, last time when I disabled ESB, in the Browser Console, I could see that it was reporting some missing ESB XUL elements, as if the XUL was removed, but the listener wasn't. But I'm not exactly sure.

adoxa commented 9 years ago

It seems it's the unload listener (to store collapsed state) causing it to remain resident. I've reworked it, but still think it's the wrong approach. However, I couldn't get persist or document.persist to work and I don't know what else to do (normally sipertruk has beaten me to it...).

kustodian commented 9 years ago

It didn't help, at least for the problem I noticed. When I disable ESB and open Browser Console, it still runs updateTime function and it generates the following errors:

TypeError: XULExtendedStatusbarChrome.esbXUL.esbstrings is null
adoxa commented 9 years ago

Something similar happened the first time I disabled, but I hadn't been able to replicate it. Tried again just now and the TabSelect event was still firing, but now I can't replicate that.

kustodian commented 9 years ago

I just have a lot of open tabs: Gmail, Twitter and 5 more. This happens to me every time I disable ESB.

adoxa commented 9 years ago

It happened again - seemed to occur opening the add-ons page itself, where it started the timer twice, but only stopped it once (but it might be related to other pages, since I haven't been able to do it again). In any event, the solution (or perhaps work-around) is simple - just stop the timer if the strings don't exist. I've updated my unload branch to do just that (as well as only starting it once, not continuously stopping & starting, same as I did with split-timer).

kustodian commented 9 years ago

Could you please make a pull request so that I can merge the changes?