Fix #237, fix #242: Re-add toolbar icon reset workaround. #247
Conversation
There was a problem hiding this comment.
R+. Works with the acknowledged flickering on slow connections. It will be nice once the fix is available in the Release channel for Firefox 64. Thanks!
Edit: Question for you:
You mention in #237 :
Based on my testing, this appears to be caused by the same thing as #192/#242. New iframes are clearing the browser action icon once you view the page (notice the flickering badge), and since the pages were opened in new tabs and some time is passing before you switch to them, the 5- and 10-second workarounds for resetting the badge aren't taking effect.
Is the reason why the 5 and 10s resend wasn't working because Firefox doesn't fully finish loading the page until the tab is focused as a performance measure?
| @@ -43,6 +43,10 @@ function extractProduct() { | |||
| } | |||
|
|
|||
| async function sendProductToBackground(extractedProduct) { | |||
| if (!extractedProduct) { | |||
| return null; | |||
There was a problem hiding this comment.
Why are we returning null here? Is this from the eslint consistent-return rule to return a value, since we otherwise return a promise if we have an extracted product?
There was a problem hiding this comment.
It's an async function so a promise is always returned; if we don't call return, the promise just resolves with undefined. But yes, this is to appease the eslint rule.
| } | ||
| }, | ||
| {urls: ['https://*/*', 'http://*/*'], types: ['sub_frame']}, | ||
| ); |
There was a problem hiding this comment.
I recall you had originally had something like this before implementing the resend stuff -- why did you switch to resend originally?
There was a problem hiding this comment.
The issue is intermittent, and I suspect QA verified on a version that actually didn't have the fix (because I was slow to release it), then replicated the issue and re-filed. I'm not entirely sure how I was able to replicate it off of master, but at this point the fix is so consistent that I'm trying not to question it.
Tacking this on, though it's an unrelated error. Previously, we were attempting to send messages via `tabs.sendMessage` to all content scripts both the ones running in content processes and in the background. However, the tabId for a content script in the background is -1, which is not a valid value for sending messages to content scripts in specific tabs. Since the code that introduced this error was intended to fix an issue related to badging the browserAction toolbar button to add a new product, and since that badging is only triggered by extraction in content processes, we don't need to worry about sending the message to the background. Now, we now no longer see the error: `Type error for parameter tabId (Integer -1 is too small (must be at least 0)) for tabs.sendMessage.`
Tacking this on, though it's an unrelated error. Previously, we were attempting to send messages via `tabs.sendMessage` to all content scripts both the ones running in content processes and in the background. However, the tabId for a content script in the background is -1, which is not a valid value for sending messages to content scripts in specific tabs. Since the code that introduced this error was intended to fix an issue related to badging the browserAction toolbar button to add a new product, and since that badging is only triggered by extraction in content processes, we don't need to worry about sending the message to the background. Now, we now no longer see the error: `Type error for parameter tabId (Integer -1 is too small (must be at least 0)) for tabs.sendMessage.`
No description provided.