Closed Bug 1937194 Opened 8 months ago Closed 29 days ago

Use NotificationDB directly from parent process

Categories

(Core :: DOM: Notifications, task)

Product:
Core
Component:
DOM: Notifications
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
142 Branch
Tracking Flags:
Tracking Status
firefox142 --- fixed

People

(Reporter: saschanaz, Assigned: saschanaz)

References

Details

Attachments

(5 files)

Bug 1937194 - Part 1: Migrate NotificationStorage to moz-src r=asuth,hsingh
48 bytes, text/x-phabricator-request
Details | Review
Bug 1937194 - Part 4: Remove Services.ppmm/cpmm usage in NotificationDB/Storage r=asuth,hsingh
48 bytes, text/x-phabricator-request
Details | Review
Bug 1937194 - Part 2: Use add_task for old unit tests r=asuth,hsingh
48 bytes, text/x-phabricator-request
Details | Review
Bug 1937194 - Part 3: Use db.queueTask directly instead of addAndSend r=asuth,hsingh
48 bytes, text/x-phabricator-request
Details | Review
Bug 1937194 - Part 5: Adjust browser_handle_notification.js r=mossop
48 bytes, text/x-phabricator-request
Details | Review

NotificationStorage is a proxy to NotificationDB for content process, but yet the parent process is still using it through PersistNotification(). This still works because somehow Services.cpmm works in the parent process too, but there's no need to use it.

Done by ./mach use-moz-src dom/notification/moz.build.

Also merged the two separate JS module declarations, which was done when it was behind a build flag.

Attachment #9499584 - Attachment description: WIP: Bug 1937194 - Part 1: Migrate NotificationStorage to moz-src → Bug 1937194 - Part 1: Migrate NotificationStorage to moz-src r=asuth,hsingh

It's more modern way to run async tests.

Mostly just simple textual replacement but there's some more complex replacement where Services.cpmm is used directly, to be able to await instead of managing the test flow in more complex way.

This completely removes Services.cpmm usage from the unit tests.

The diff doesn't look as simple as I hoped, but hopefully not too hard to see the equivalence.

Attachment #9499585 - Attachment description: WIP: Bug 1937194 - Part 2: Remove Services.ppmm/cpmm usage in NotificationDB/Storage → Bug 1937194 - Part 4: Remove Services.ppmm/cpmm usage in NotificationDB/Storage r=asuth,hsingh
Attachment #9499784 - Attachment description: Bug 1937194 - Part 3: Use db.queueTask direclty instead of addAndSend r=asuth,hsingh → Bug 1937194 - Part 3: Use db.queueTask directly instead of addAndSend r=asuth,hsingh
Pushed by smolnar@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/e33b20f5d5b1 https://hg.mozilla.org/integration/autoland/rev/3efcb41bd3ee Revert "Bug 1937194 - Part 4: Remove Services.ppmm/cpmm usage in NotificationDB/Storage r=asuth" for causing bc failures @ browser_openwindow_without_window
Flags: needinfo?(krosylight)
Pushed by chorotan@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/44b59bcb6a9c https://hg.mozilla.org/integration/autoland/rev/99aa21d58d05 Revert "Bug 1937194 - Part 4: Remove Services.ppmm/cpmm usage in NotificationDB/Storage r=asuth" for causing notification related failures

Err. So the simulate-click-handler is the real bug that's fixed in the updated patch.

But browser_handle_notification failure is a test bug, in this case simulateNotificationClickWithNewWindow got faster to open a tab (👍🏻), resulting test behavior change.

  // In case the tab is already opened...
  let newTab = newWin.gBrowser.tabs.find(
    tab => tab.linkedBrowser.currentURI.spec === "https://example.com/"
  );

  // If not let's wait for one
  if (!newTab) {
    newTab = await BrowserTestUtils.waitForNewTab(
      newWin.gBrowser,
      "https://example.com/"
    );
  }

Here, newWin.gBrowser.tabs already got the new tab open, but the tab is yet on about:blank before being navigated to example.com. At that point calling waitForNewTab is too late and hangs forever. And it's unclear whether I can detect there's already a tab that is navigating to example.com.

Not sure what to do, perhaps mossop knows?

Flags: needinfo?(krosylight) → needinfo?(dtownsend)

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #11)

Err. So the simulate-click-handler is the real bug that's fixed in the updated patch.

But browser_handle_notification failure is a test bug, in this case simulateNotificationClickWithNewWindow got faster to open a tab (👍🏻), resulting test behavior change.

  // In case the tab is already opened...
  let newTab = newWin.gBrowser.tabs.find(
    tab => tab.linkedBrowser.currentURI.spec === "https://example.com/"
  );

  // If not let's wait for one
  if (!newTab) {
    newTab = await BrowserTestUtils.waitForNewTab(
      newWin.gBrowser,
      "https://example.com/"
    );
  }

Here, newWin.gBrowser.tabs already got the new tab open, but the tab is yet on about:blank before being navigated to example.com. At that point calling waitForNewTab is too late and hangs forever. And it's unclear whether I can detect there's already a tab that is navigating to example.com.

Not sure what to do, perhaps mossop knows?

There's a few options here I think. Probably the easiest is to use BrowserTestUtils.waitForLocationChange which will wait for the url you care about to load in any tab, present or future.

Flags: needinfo?(dtownsend)
QA Whiteboard: [qa-triage-done-c143/b142]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: