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

Throttle browser button counter updates #350

Merged
merged 2 commits into from Mar 11, 2019
Merged

Throttle browser button counter updates #350

merged 2 commits into from Mar 11, 2019

Conversation

@chrmod
Copy link
Member

@chrmod chrmod commented Mar 6, 2019

Counter updates can happen very fast and every update issues a call to chrome process. This change should improve performance - specifically for underpowered devices like mobiles.

@chrmod chrmod requested a review from jsignanini as a code owner Mar 6, 2019
@christophertino christophertino requested a review from IAmThePan Mar 7, 2019
@christophertino
Copy link
Member

@christophertino christophertino commented Mar 7, 2019

@IAmThePan do you remember what issue we had with passing tabID to button.update?

@chrmod
Copy link
Member Author

@chrmod chrmod commented Mar 7, 2019

btw. we've tested this change on mobile browser and seems to work without any issues. Would be probably better to have an update queue as there are multiple sources of "badge" updates.

@IAmThePan
Copy link
Contributor

@IAmThePan IAmThePan commented Mar 7, 2019

I don't quite remember but I think there were inconsistencies where the passed TabId wasn't for the currently active tab.
Examples:

  • What if you have two windows open? Will it incorrectly set the badge count to the other window's active tab count?
  • What if a tab closes and a Errored request is a tracker, will that incorrectly update the currently active tab.
  • What if the TabId changes (a prefetched tab becomes active)?
@chrmod
Copy link
Member Author

@chrmod chrmod commented Mar 7, 2019

regardless of correct behavior of button.update this PR should introduce no functional change other than throttling. Note the _throttleButtonUpdate called without an argument will pass undefined value as tabId, which should be no different from passing no argument.

@jsignanini jsignanini merged commit 9cae830 into develop Mar 11, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jsignanini jsignanini deleted the throttle-badge branch Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants