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

Significant rewrite & bugfixes #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

m-khvoinitsky
Copy link

@m-khvoinitsky m-khvoinitsky commented Jan 21, 2018

I've started hacking your add-on to fix annoying bug (see first point below) and eventually end up with some major rewrite.

  • fixed bug when no hostname (or whatever) was inserted in title under some circumstances (content script insertion happens after load event)
    It's not needed to wait for load event because content script is inserted at document_end which means that all DOM is already constructed. Meanwhile, if some script would change title, you have MutationObserver for that.
    Most obvious example is a page without any external content (stylesheets, images, etc) — in such case it's very likely that load event will fire before content script insertion. However, there are occasional real world pages which are affected by this issue (I suspect, Addon not working on some pages #7 is about that).
  • all preferences apply immediately
  • configurable delimiter (Allow delimiter between title and hostname to be changed #2)
  • slightly different choices of what to insert
    Note: I haven't wrote an updater from previous version of preferences.
  • using storage.sync
  • added add-on id (extracted actual value from addons.mozilla.org)
  • using browser_style: true for options_ui page so it looks more natural inside about:addons UI. However, it has some bug which under some circumstances leads to unreadable text. But there is a workaround for it.

* fixed bug when no hostname (or whatever) was inserted in title under some circumstances (content script insertion happens after load event)
* all preferences apply immediately
* configurable delimiter
* slightly different choices of what to insert
* using storage.sync
* added add-on id (extracted actual value from addons.mozilla.org)
@m-khvoinitsky
Copy link
Author

Any comments?

@MegaphoneJon
Copy link

I tested this for about a week and it works as advertised. What can folks do to help get this merged?

@benjivm
Copy link

benjivm commented Apr 2, 2019

Seems as though @flamingspaz hasn't been active in a good while. Is it worth forking this into a new extension?

@flamingspaz
Copy link
Owner

I believe this has already been forked into a different extension, and in any case should be rewritten to use the Window API. I'm not sure when I'd get to this.

@m-khvoinitsky
Copy link
Author

I believe this has already been forked into a different extension

No, I didn't managed to find time to complete this.

in any case should be rewritten to use the Window API

Sure, however it's too restrictive — it only allows setting prefix of title, not another positions, unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants