Skip to content

Commit

Permalink
Bug 1842252 - use correct destruction callback and improve handling o…
Browse files Browse the repository at this point in the history 
…f hiding the sidebar, r=jhirsch

Differential Revision: https://phabricator.services.mozilla.com/D185310
  • Loading branch information
@gijsk
gijsk committed Aug 5, 2023
1 parent eb81778 commit cd68a40
Show file tree
Hide file tree
Showing 3 changed files with 98 additions and 45 deletions.
3 changes: 3 additions & 0 deletions browser/base/content/browser.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9970,6 +9970,9 @@ var ShoppingSidebarManager = {
},

_updateVisibility() {
if (window.closed) {
return;
}
let optedOut = this.optedInPref === 2;
let isPBM = PrivateBrowsingUtils.isWindowPrivate(window);

Expand Down
61 changes: 52 additions & 9 deletions browser/components/shopping/ShoppingSidebarChild.sys.mjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ export class ShoppingSidebarChild extends RemotePageChild {
gAllActors.add(this);
}

actorDestroyed() {
super.actorDestroyed();
didDestroy() {
this._destroyed = true;
super.didDestroy?.();
gAllActors.delete(this);
this.#product?.uninit();
}
Expand All @@ -46,12 +47,17 @@ export class ShoppingSidebarChild extends RemotePageChild {
switch (message.name) {
case "ShoppingSidebar:UpdateProductURL":
let { url } = message.data;
let uri = Services.io.newURI(url);
if (this.#productURI?.equalsExceptRef(uri)) {
let uri = url ? Services.io.newURI(url) : null;
// If we're going from null to null, bail out:
if (!this.#productURI && !uri) {
return;
}
// Otherwise, check if we now have a product:
if (uri && this.#productURI?.equalsExceptRef(uri)) {
return;
}
this.#productURI = uri;
this.updateContent();
this.updateContent({ haveUpdatedURI: true });
break;
}
}
Expand Down Expand Up @@ -79,7 +85,37 @@ export class ShoppingSidebarChild extends RemotePageChild {
return this.#productURI;
}

async updateContent() {
/**
* This callback is invoked whenever something changes that requires
* re-rendering content. The expected cases for this are:
* - page navigations (both to new products and away from a product once
* the sidebar has been created)
* - opt in state changes.
*
* @param {object?} options
* Optional parameter object.
* @param {bool} options.haveUpdatedURI = false
* Whether we've got an up-to-date URI already. If true, we avoid
* fetching the URI from the parent, and assume `this.#productURI`
* is current. Defaults to false.
*
*/
async updateContent({ haveUpdatedURI = false } = {}) {
// updateContent is an async function, and when we're off making requests or doing
// other things asynchronously, the actor can be destroyed, the user
// might navigate to a new page, the user might disable the feature ... -
// all kinds of things can change. So we need to repeatedly check
// whether we can keep going with our async processes. This helper takes
// care of these checks.
let canContinue = (currentURI, checkURI = true) => {
if (this._destroyed || !this.canFetchAndShowData) {
return false;
}
if (!checkURI) {
return true;
}
return currentURI && currentURI == this.#productURI;
};
this.#product?.uninit();
// We are called either because the URL has changed or because the opt-in
// state has changed. In both cases, we want to clear out content
Expand All @@ -91,10 +127,14 @@ export class ShoppingSidebarChild extends RemotePageChild {
});
if (this.canFetchAndShowData) {
if (!this.#productURI) {
// If we already have a URI and it's just null, bail immediately.
if (haveUpdatedURI) {
return;
}
let url = await this.sendQuery("GetProductURL");

// Bail out if we opted out in the meantime, or don't have a URI.
if (lazy.optedIn !== 1 || !url) {
if (!canContinue(null, false)) {
return;
}

Expand All @@ -107,8 +147,8 @@ export class ShoppingSidebarChild extends RemotePageChild {
console.error("Failed to fetch product analysis data", err);
return { error: err };
});
// Check if the product URI or opt in changed while we waited.
if (uri != this.#productURI || !this.canFetchAndShowData) {
// Check if we got nuked from orbit, or the product URI or opt in changed while we waited.
if (!canContinue(uri)) {
return;
}
this.sendToContent("Update", {
Expand All @@ -120,6 +160,9 @@ export class ShoppingSidebarChild extends RemotePageChild {
}

sendToContent(eventName, detail) {
if (this._destroyed) {
return;
}
let win = this.contentWindow;
let evt = new win.CustomEvent(eventName, {
bubbles: true,
Expand Down
79 changes: 43 additions & 36 deletions browser/components/shopping/tests/browser/browser_shopping_urlbar.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ add_task(async function test_button_inactive() {
});

// Switching Tabs shows and hides the button
add_task(async function test_button_changes_with_location() {
add_task(async function test_button_changes_with_tabswitch() {
Services.prefs.setBoolPref("browser.shopping.experience2023.active", true);

let shoppingButton = document.getElementById("shopping-sidebar-button");
Expand Down Expand Up @@ -107,41 +107,45 @@ add_task(async function test_button_changes_with_location() {
add_task(async function test_button_toggles_sidebars() {
Services.prefs.setBoolPref("browser.shopping.experience2023.active", false);

let shoppingButton = document.getElementById("shopping-sidebar-button");
let browser = gBrowser.selectedBrowser;
let browserPanel = gBrowser.getPanel(browser);
await BrowserTestUtils.withNewTab(CONTENT_PAGE, async function (browser) {
let shoppingButton = document.getElementById("shopping-sidebar-button");
let browserPanel = gBrowser.getPanel(browser);

BrowserTestUtils.loadURIString(browser, PRODUCT_PAGE);
await BrowserTestUtils.browserLoaded(browser);
BrowserTestUtils.loadURIString(browser, PRODUCT_PAGE);
await BrowserTestUtils.browserLoaded(browser);

let sidebar = browserPanel.querySelector("shopping-sidebar");
let sidebar = browserPanel.querySelector("shopping-sidebar");

is(sidebar, null, "Shopping sidebar should be closed");
is(sidebar, null, "Shopping sidebar should be closed");

// open
shoppingButton.click();
await BrowserTestUtils.waitForMutationCondition(
shoppingButton,
{
attributeFilter: ["shoppingsidebaropen"],
},
() => shoppingButton.getAttribute("shoppingsidebaropen") == "true"
);

sidebar = browserPanel.querySelector("shopping-sidebar");
ok(BrowserTestUtils.is_visible(sidebar), "Shopping sidebar should be open");
// open
shoppingButton.click();
await BrowserTestUtils.waitForMutationCondition(
shoppingButton,
{
attributeFilter: ["shoppingsidebaropen"],
},
() => shoppingButton.getAttribute("shoppingsidebaropen") == "true"
);

// close
shoppingButton.click();
await BrowserTestUtils.waitForMutationCondition(
shoppingButton,
{
attributeFilter: ["shoppingsidebaropen"],
},
() => shoppingButton.getAttribute("shoppingsidebaropen") == "false"
);
sidebar = browserPanel.querySelector("shopping-sidebar");
ok(BrowserTestUtils.is_visible(sidebar), "Shopping sidebar should be open");

// close
shoppingButton.click();
await BrowserTestUtils.waitForMutationCondition(
shoppingButton,
{
attributeFilter: ["shoppingsidebaropen"],
},
() => shoppingButton.getAttribute("shoppingsidebaropen") == "false"
);

ok(BrowserTestUtils.is_hidden(sidebar), "Shopping sidebar should be closed");
ok(
BrowserTestUtils.is_hidden(sidebar),
"Shopping sidebar should be closed"
);
});
});

// Button changes all Windows
Expand All @@ -150,8 +154,7 @@ add_task(async function test_button_toggles_all_windows() {

let shoppingButton = document.getElementById("shopping-sidebar-button");

BrowserTestUtils.loadURIString(gBrowser.selectedBrowser, PRODUCT_PAGE);
await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, PRODUCT_PAGE);

let browserPanelA = gBrowser.getPanel(gBrowser.selectedBrowser);
let sidebarA = browserPanelA.querySelector("shopping-sidebar");
Expand All @@ -164,12 +167,15 @@ add_task(async function test_button_toggles_all_windows() {
);
await BrowserTestUtils.browserLoaded(newWindow.gBrowser.selectedBrowser);

let browserPanelB = gBrowser.getPanel(newWindow.gBrowser.selectedBrowser);
let browserPanelB = newWindow.gBrowser.getPanel(
newWindow.gBrowser.selectedBrowser
);
let sidebarB = browserPanelB.querySelector("shopping-sidebar");

ok(
BrowserTestUtils.is_hidden(sidebarA),
"Shopping sidebar should be closed in current window"
is(
sidebarA,
null,
"Shopping sidebar should not exist yet for new tab in current window"
);
is(sidebarB, null, "Shopping sidebar closed in new window");

Expand Down Expand Up @@ -212,5 +218,6 @@ add_task(async function test_button_toggles_all_windows() {
"Shopping sidebar should be closed in new window"
);

BrowserTestUtils.removeTab(tab);
await BrowserTestUtils.closeWindow(newWindow);
});

0 comments on commit cd68a40

Please sign in to comment.