This repository has been archived by the owner on Dec 3, 2020. It is now read-only.
Fix #192: Resend product data after a delay to reset toolbar icon. #220
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mythmon
approved these changes
Oct 29, 2018
There was a problem hiding this comment.
Since this was the second time I've reviewed something for #192, I went and actually read the bug. It didn't really make sense until I looked at the patch in bug 1493470 (specifically the commit message), and realized this applied both to both page actions and browser actions. This makes more sense now.
If this weren't already being fixed in Firefox 64, I'd push for a better thing than just trying twice and praying, but I think this is fine since it is a temporary workaraound.
Osmose
force-pushed
the
toolbar-cleared-again
branch
from
4526a15 to
75c6c17
Compare
biancadanforth
added a commit
to biancadanforth/price-tracker
that referenced
this pull request
Nov 2, 2018
* Note: This fix is complicated by the fact that a 'price alert' badge only changes the global badge state. An 'add' product badge changes the tab-specific badge state, which takes precedence over the global badge state.
To avoid event duplication for 'badge_toolbar_button':
* Add 'hasBadgeTextChanged' function with optional 'tabId' argument, so that the event only fires if the badge text has changed.
* Previously, we were getting a bunch of redundant `badge_toolbar_button` events for ‘price_alert’ any time the state was changed and we’re on a page with an ‘add’ badge. Since `handlePriceAlerts` is called every time state changes in any way (`store.subscribe(handlePriceAlerts)`), we now check to see if the badge text has changed for a price alert badge on the global browserAction button before recording the 'badge_toolbar_button' with badge type 'price_alert' event.
* Previously, if tab 1 had badge type 'add' and tab 2 had badge type 'price_alert', and I switch to tab 2 before any calls to 'resend' (mozilla#220) are made from tab 1, I would get two extra 'badge_toolbar_button' of type 'add' events. Adding the 'tabId' optional argument fixes this bug.
biancadanforth
added a commit
to biancadanforth/price-tracker
that referenced
this pull request
Nov 6, 2018
* Note: This fix is complicated by the fact that a 'price alert' badge only changes the global badge state. An 'add' product badge changes the tab-specific badge state, which takes precedence over the global badge state.
To avoid event duplication for 'badge_toolbar_button':
* Add 'hasBadgeTextChanged' function with optional 'tabId' argument, so that the event only fires if the badge text has changed.
* Previously, we were getting a bunch of redundant `badge_toolbar_button` events for ‘price_alert’ any time the state was changed and we’re on a page with an ‘add’ badge. Since `handlePriceAlerts` is called every time state changes in any way (`store.subscribe(handlePriceAlerts)`), we now check to see if the badge text has changed for a price alert badge on the global browserAction button before recording the 'badge_toolbar_button' with badge type 'price_alert' event.
* Previously, if tab 1 had badge type 'add' and tab 2 had badge type 'price_alert', and I switch to tab 2 before any calls to 'resend' (mozilla#220) are made from tab 1, I would get two extra 'badge_toolbar_button' of type 'add' events. Adding the 'tabId' optional argument fixes this bug.
biancadanforth
added a commit
to biancadanforth/price-tracker
that referenced
this pull request
Nov 6, 2018
* Note: This fix is complicated by the fact that a 'price alert' badge only changes the global badge state. An 'add' product badge changes the tab-specific badge state, which takes precedence over the global badge state.
To avoid event duplication for 'badge_toolbar_button':
* Add 'hasBadgeTextChanged' function with optional 'tabId' argument, so that the event only fires if the badge text has changed.
* Previously, we were getting a bunch of redundant `badge_toolbar_button` events for ‘price_alert’ any time the state was changed and we’re on a page with an ‘add’ badge. Since `handlePriceAlerts` is called every time state changes in any way (`store.subscribe(handlePriceAlerts)`), we now check to see if the badge text has changed for a price alert badge on the global browserAction button before recording the 'badge_toolbar_button' with badge type 'price_alert' event.
* Previously, if tab 1 had badge type 'add' and tab 2 had badge type 'price_alert', and I switch to tab 2 before any calls to 'resend' (mozilla#220) are made from tab 1, I would get two extra 'badge_toolbar_button' of type 'add' events. Adding the 'tabId' optional argument fixes this bug.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Forgive me father for I have sinned
In my defense, this is much more reliable than any other way I could find of detecting this error. There's still a possibility of very long page loads causing the toolbar icon to be reset after the timers go off, but that's just life. And 64 and above aren't affected by this bug anyway, so it's a temporary pain.
Also I noticed an error being logged to the console a bunch while trying to fix this and included a fix for that too.