Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

tab index in state is not always accurate #14080

Closed
petemill opened this issue May 10, 2018 · 2 comments
Closed

tab index in state is not always accurate #14080

petemill opened this issue May 10, 2018 · 2 comments
Assignees
@petemill
Labels
0.22.x-single-webview Issue first seen on single-webview build against v0.22.x branch initiative/perf perf QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release-notes/exclude
Milestone
0.22.x Release 3 ...

Comments

@petemill
Copy link
Member

petemill commented May 10, 2018 •
edited
Loading

As reported at #14064 (comment), a tab's index in the state can get out of sync with muon. This is because there are different muon Tab events in a single-webview world that can affect index, and we need to make sure we call updateTabsStateForWindow.

However, this function results in a performance hit since it not only updates the state, but causes a new TAB_UPDATED action to be fired for tab which has a detected change.

After discussing with @bridiver it seems that all the properties it is comparing are not needed, since they would be handled by either chrome-tabs-updated or more specific events (such as pinned).

Therefore, we can remove the call to TAB_UPDATED inside updateTabsStateForWindow and simply make sure each tab's index is accurate.

Test plan

Same as #14064
@petemill petemill added perf initiative/perf 0.22.x-single-webview Issue first seen on single-webview build against v0.22.x branch labels May 10, 2018
@petemill petemill added this to the 0.22.x Release 3 (Beta channel) milestone May 10, 2018
@petemill petemill self-assigned this May 10, 2018
petemill added a commit that referenced this issue May 10, 2018
@petemill
tab index in appState is now reliable
91ee4a2 
Fix #14080
Reintroduce `tabs.updateTabsStateForWindow` which should be called whenever we believe muon has new tab properties that we wouldn't have otherwise received events for.
It seems that the only time this can happen anymore is when the index of tabs has changed because another tab, with a lower index, was moved or closed, so renamed the function `updateTabIndexesForWindow`.
Also allows us to remove the call to dispatch the action `UPDATE_TAB` for each tab with modified properties, which was a performance issue. Instead, since we're only concerned with index change, we make sure that this is handled in the renderer on the appropriate events (tab inserted, removed, deleted, moved, pinned (which triggers moved)).
Adds performance measuring for that function when using --debug-tab-events flag.
@petemill
Copy link
Member Author

Should be fixed in next build

petemill added a commit that referenced this issue May 10, 2018
@petemill
tab index in appState is now reliable
cf3410d 
Fix #14080
Reintroduce `tabs.updateTabsStateForWindow` which should be called whenever we believe muon has new tab properties that we wouldn't have otherwise received events for.
It seems that the only time this can happen anymore is when the index of tabs has changed because another tab, with a lower index, was moved or closed, so renamed the function `updateTabIndexesForWindow`.
Also allows us to remove the call to dispatch the action `UPDATE_TAB` for each tab with modified properties, which was a performance issue. Instead, since we're only concerned with index change, we make sure that this is handled in the renderer on the appropriate events (tab inserted, removed, deleted, moved, pinned (which triggers moved)).
Adds performance measuring for that function when using --debug-tab-events flag.
petemill added a commit that referenced this issue May 10, 2018
@petemill
tab index in appState is now reliable
8e07ced 
Fix #14080
Reintroduce `tabs.updateTabsStateForWindow` which should be called whenever we believe muon has new tab properties that we wouldn't have otherwise received events for.
It seems that the only time this can happen anymore is when the index of tabs has changed because another tab, with a lower index, was moved or closed, so renamed the function `updateTabIndexesForWindow`.
Also allows us to remove the call to dispatch the action `UPDATE_TAB` for each tab with modified properties, which was a performance issue. Instead, since we're only concerned with index change, we make sure that this is handled in the renderer on the appropriate events (tab inserted, removed, deleted, moved, pinned (which triggers moved)).
Adds performance measuring for that function when using --debug-tab-events flag.
This was referenced May 10, 2018
Closed
Closed
Closed
@LaurenWags
Copy link
Member

LaurenWags commented May 10, 2018 •
edited by btlechowski
Loading

Verified with macOS 10.12.6 using

  • 0.22.706 e11b027
  • muon 6.0.9
  • libchromiumcontent 66.0.3359.139

Verified on Ubuntu 17.10 x64

  • 0.22.706 e11b027
  • libchromiumcontent 66.0.3359.139
  • muon: 6.0.9

petemill added a commit that referenced this issue May 11, 2018
@petemill
tab.index and frame index (via index of state.frames List) in appStat…
fd9d533 
…e is now reliable

Fix #14080
Reintroduce `tabs.updateTabsStateForWindow` which should be called whenever we believe muon has new tab properties that we wouldn't have otherwise received events for.
It seems that the only time this can happen anymore is when the index of tabs has changed because another tab, with a lower index, was moved or closed, so renamed the function `updateTabIndexesForWindow`.
Also allows us to remove the call to dispatch the action `UPDATE_TAB` for each tab with modified properties, which was a performance issue. Instead, since we're only concerned with index change, we make sure that this is handled in the renderer on the appropriate events (tab inserted, removed, deleted, moved, pinned (which triggers moved)).
Adds performance measuring for that function when using --debug-tab-events flag.
petemill added a commit that referenced this issue May 11, 2018
@petemill
tab.index and frame index (via index of state.frames List) in appStat…
3612120 
…e is now reliable

Fix #14080
Reintroduce `tabs.updateTabsStateForWindow` which should be called whenever we believe muon has new tab properties that we wouldn't have otherwise received events for.
It seems that the only time this can happen anymore is when the index of tabs has changed because another tab, with a lower index, was moved or closed, so renamed the function `updateTabIndexesForWindow`.
Also allows us to remove the call to dispatch the action `UPDATE_TAB` for each tab with modified properties, which was a performance issue. Instead, since we're only concerned with index change, we make sure that this is handled in the renderer on the appropriate events (tab inserted, removed, deleted, moved, pinned (which triggers moved)).
Adds performance measuring for that function when using --debug-tab-events flag.
petemill added a commit that referenced this issue May 11, 2018
@petemill
tab.index and frame index (via index of state.frames List) in appStat…
b4f00f2 
…e is now reliable

Fix #14080
Reintroduce `tabs.updateTabsStateForWindow` which should be called whenever we believe muon has new tab properties that we wouldn't have otherwise received events for.
It seems that the only time this can happen anymore is when the index of tabs has changed because another tab, with a lower index, was moved or closed, so renamed the function `updateTabIndexesForWindow`.
Also allows us to remove the call to dispatch the action `UPDATE_TAB` for each tab with modified properties, which was a performance issue. Instead, since we're only concerned with index change, we make sure that this is handled in the renderer on the appropriate events (tab inserted, removed, deleted, moved, pinned (which triggers moved)).
Adds performance measuring for that function when using --debug-tab-events flag.
petemill added a commit that referenced this issue May 11, 2018
@petemill
tab.index and frame index (via index of state.frames List) in appStat…
f9ea977 
…e is now reliable

Fix #14080
Reintroduce `tabs.updateTabsStateForWindow` which should be called whenever we believe muon has new tab properties that we wouldn't have otherwise received events for.
It seems that the only time this can happen anymore is when the index of tabs has changed because another tab, with a lower index, was moved or closed, so renamed the function `updateTabIndexesForWindow`.
Also allows us to remove the call to dispatch the action `UPDATE_TAB` for each tab with modified properties, which was a performance issue. Instead, since we're only concerned with index change, we make sure that this is handled in the renderer on the appropriate events (tab inserted, removed, deleted, moved, pinned (which triggers moved)).
Adds performance measuring for that function when using --debug-tab-events flag.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@petemill petemill

Labels
0.22.x-single-webview Issue first seen on single-webview build against v0.22.x branch initiative/perf perf QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release-notes/exclude
Projects
None yet
Milestone
0.22.x Release 3 (Release Channel)
Development

No branches or pull requests
5 participants
@petemill @bsclifton @srirambv @LaurenWags @btlechowski