1873780 - Move surface_displayed tracking state from browser elements to a WeakSet in the parent actor

Status: NEW

(Firefox :: Shopping, enhancement, P3)

Description

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

In https://phabricator.services.mozilla.com/D195727 (patch for bug 1868602), we started tracking whether a tab had loaded an eligible product page by setting an expando property on the tab's browser via JS. In the review, mconley suggested we instead track the list of surface_displayed eligible tabs through a WeakSet stored in the ShoppingSidebarParent, which would accomplish the same goals without spreading the state all over the place. Here's the comment for posterity:

(from https://phabricator.services.mozilla.com/D195727#6798384)

So I think this is fine, given the time pressure here, but I might suggest a refactor here down the line. Instead of setting an expando property on the <browser>, and doing the bookkeeping to keep it up to date, what we could do is have a WeakSet within the ShoppingSidebarParent that contains browser.permanentKey's for browsers that have distinct product page visits.

browser.permanentKey is an empty object whose sole purpose is to be an object identifier for a <browser> that gets transferred during tab tear out / tear in. It can also be used as a key in a WeakSet.

So then, down below, where we set , we could instead:

// At top of ShoppingSidebarParent.sys.mjs... 
let gDistinctProductPageVisits = new WeakSet();
 
// ... down in _maybeToggleSidebar, where we originally set `isDistinctProductPageVisit = true`:

gDistinctProductPageVisits.add(aBrowser.permanentKey);

Then, checks can be done via gDistinctProductPageVisits.has(aBrowser.permanentKey).

This has the added advantage of being contained all within ShoppingSidebarParent, so that state isn't leaked via the expando property.

Again, this doesn't block - but it's a suggestion for a follow-up.