Use ports to avoid race condition #18

Merged
merged 1 commit into from Dec 8, 2017

Conversation

Projects
None yet
2 participants
Collaborator

biancadanforth commented Dec 7, 2017

There was, with using normal browser.runtime.onMessage/sendMessage, a race condition where the content script would sometimes load before the background script on extension startup and try to send it a message before its message listener had been added.

Using a port between the content script and background script eliminates this problem, as the content script is notified when it is connected with the background script and can then send and receive messages.

I also looked at the tooltip on several popular websites and added some neutralizing styles (and cleaned up the CSS block in background.js). One known shortcoming is tootip visibility when the match word is on a window boundary; depending on location it can be completely or partially obscured.

Use ports to avoid race condition; neutralize more tooltip styles
There was, with using normal browser.runtime.onMessage/sendMessage, a race condition where the content script would sometimes load before the background script on extension startup and try to send it a message before its message listener had been added.

Using a port between the content script and background script eliminates this problem, as the content script is notified when it is connected with the background script and can send and receive messages.

I also looked at the tooltip on several popular websites and added some neutralizing styles (and cleaned up the CSS block in background.js). One known shortcoming is tootip visibility when the match word is on a window boundary; depending on location it can be completely or partially obscured.
Member

gregglind commented Dec 8, 2017

I accept those visibility limitiations. This Tooltip is "always on the right".

- .donotdelete:hover .donotdelete-tooltip {
- visibility: visible;
- z-index: 50;
+ /* Neutralizes case where <a> have after pseudoelements like ">>" */
@gregglind

gregglind Dec 8, 2017

Member

wow, this can happen! Jiminy. Good catch!

+ * in a given text node
+ */
+ .donotdelete:hover {
+ z-index: 999 !important;
@gregglind

gregglind Dec 8, 2017

Member

Good catch.

- default:
- throw new Error(`Message type not recognized: ${msg}`);
- }
+ connected(p) {
@gregglind

gregglind Dec 8, 2017

Member

this feels so weird and abstract. Some future world, let's get this pattern to be written better.

@@ -185,7 +159,7 @@ class PersistentPageModificationEffect {
// Each time a tab is updated, add CSS for that tab.
browser.tabs.onUpdated.addListener((id, changeInfo, tab) => {
- if (this.protocolIsApplicable(tab.url)) {
+ if (this.protocolIsApplicable(tab.url) && tab.status === "complete") {
@gregglind

gregglind Dec 8, 2017

Member

interesting, interesting! Good catch.

Member

gregglind commented Dec 8, 2017

OK with me! Merging.

@gregglind gregglind merged commit 79b6f72 into mozilla:master Dec 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment