Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't presume window is the global object for js #2465

Merged
merged 1 commit into from Oct 9, 2015

Conversation

@callahad
Copy link
Contributor

commented Sep 28, 2015

The bundled Favico lib exports itself by setting this.Favico = Favico.

The orangered module then presumes that Favico will be available on the
global window object, but that's not the case for Firefox's sandbox.
From Bug 1208775's discussion, it sounds like we might WONTFIX this.

Simply omitting window. works equally well in Chrome and Firefox.

@andytuba

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2015

Could you also update https://github.com/honestbleeps/Reddit-Enhancement-Suite/blob/master/Opera/opera-footer.js#L3 so var Favicon = this.Favicon? In Opera12, all of RES runs inside a huge IIFE.. I mostly understand how it works.

Don't presume window is the global object for js
The bundled Favico lib exports itself by setting `this.Favico = Favico`.

The orangered module then presumes that Favico will be available on the
global `window` object, but that's not the case for Firefox's sandbox.
From [Bug 1208775][]'s discussion, it sounds like we might WONTFIX this.

Simply omitting `window.` works equally well in Chrome and Firefox.

[Bug 1208775]: https://bugzilla.mozilla.org/show_bug.cgi?id=1208775#c3

@callahad callahad force-pushed the callahad:fix-favico branch from 45a43b0 to 436a947 Sep 28, 2015

@callahad

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

I literally haven't touched Opera 12 in years, and don't have any experience or knowledge re: debugging add-ons for it, so this is untested. But logically, it should work. Force-pushed a new rev of the patch.

@andytuba

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2015

Cool, thanks! I'll double-check it runs before pushing Opera release.

It's great to see contributions towards prepping RES for the next generation of Firefox extension development.

@callahad

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

Thanks! 👍

@callahad

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2015

As far as I can tell, Opera 12 doesn't seem to work at all. Neither self-built master nor v4.5.4 from the Opera add-ons site run successfully.

This might be a larger policy discussion, but... maybe it's time to drop support for Opera 12? It hasn't seen a security patch in over a year, comments indicate that RES hasn't worked since at least April, and virtually no one seems to be complaining in the issues.

Heck Given the lack of updates, I'd personally feel it's irresponsible to continue supporting users on that platform, but that's your call. :)

Short term, maybe we could merge this since it certainly can't break Opera support any more than it's currently broken, and revisit Opera altogether in another bug?

@callahad

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2015

bump?

andytuba added a commit that referenced this pull request Oct 9, 2015
Merge pull request #2465 from callahad/fix-favico
Don't presume window is the global object for js

@andytuba andytuba merged commit dda0faa into honestbleeps:master Oct 9, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@andytuba

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2015

Thanks for tidy-up!

Opera12 was working for a little bit recently in dev. I got it working again mostly for entertainment value and partly to be nice to the remaining few dozen users. Maybe I'll get it running again if it's simple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.