1821826 - Refactor FeatureCallout to support generic triggers, routing messages from messaging system to FeatureCallout

Status: VERIFIED FIXED

(Firefox :: Messaging System, task, P1)

Description

[Shane Hughes [:aminomancer]]

Currently we can't test new FeatureCallout messages by any means except stubbing FeatureCalloutMessages or just modifying source code. So working on a new message for an experiment is a lot more involved than for example testing new spotlights.

This is because FeatureCallout isn't an always-available messaging surface like spotlight, CFR, etc. There are certain contexts where we instantiate a single FeatureCallout — for a single page load of about:firefoxview or the PDF.js viewer. In those situations, FeatureCallout itself fires a featureCalloutCheck trigger in order to request a message from ASRouter.

Normally, triggers are fired by trigger listeners or specific landmarks in the application lifecycle, and the source of the trigger isn't concerned with the return value, because the trigger is not a request for information but the start of a call stack that ultimately culminates in the display of a message. But in FeatureCallout's case, the trigger itself doesn't invoke any global feature callout broker.

The method that normally processes messages and routes them to their handlers perceives a feature callout message as a spotlight message, and then directs it to the spotlight handler, which checks if there's a gDialogBox in the ownerGlobal for the browser that was sent. Because FeatureCallout sends a document in place of a browser, there isn't one, so it ends there. But it returns the matched message, so FeatureCallout is able to use sendTriggerMessage—normally used to fire a trigger, find a matching message, and show that message—to request a matching message, so it can show that message itself.

This works for messages in ASRouter, but it means we can't really trigger a message from outside of FeatureCallout. The most we can do is provide it with messages for it to naturally display.

So, I think it's very desirable for us to refactor FeatureCallout, and to give it its own template feature_callout (instead of the current approach of borrowing spotlight). But this is difficult, because a feature callout is something that can be instantiated in either chrome or content. A while back, we considered the possibility of making a new JSActor for feature callout, in order to display feature callouts in the PDF viewer, as opposed to in the chrome, on top of the PDF viewer. But we wanted to show feature callouts in the chrome anyway, so we went in that direction instead.

So the way it's currently set up, gBrowser sets up a per-chrome window FeatureCallout when visiting PDF.js, immediately tries to show a feature callout, then tears everything down once the callout is closed. Parallel to that, Firefox View sets up a per-content window FeatureCallout on load. Communicating with both of these will at least require some changes to how the chrome FeatureCallout is set up and stored. Fortunately, the only place we show FeatureCallout in content is Firefox View, which runs in the parent process. That means ASRouter can directly invoke the Firefox View FeatureCallout instance, meaning we don't need to set up a JSActor or something.

And of course we will also need to change FeatureCallout itself so that it does not send triggers itself or request messages to be shown. Rather, the source that wants to show a feature callout (Firefox View or PDF.js) should send a trigger, ASRouter should handle the trigger and route it to a feature callout broker, then the feature callout broker should send it to whichever FeatureCallout instance is relevant for that trigger and that message. This might require some changes to the featureCalloutCheck trigger.

This is likely to be a pretty meaty patch, but I don't really see a good way to split it up. I think once we've done the work of refactoring FeatureCallout, that's 99% of the work here — I don't think we'd actually need to do anything to add support to ASRouter devtools, and supporting feature callouts for about:messagepreview should be barely any more work. But if you have ideas for splitting this up let me know.

Comment 1

[Shane Hughes [:aminomancer]]

Also because this is a pretty difficult job I'd say this is probably P3, maybe P2. We'd really like to be able to test feature callouts much faster but the amount of effort involved could be big. Probably best to address the low hanging fruit of cfr and about:welcome support in about:messagepreview first (that is bug 1821820 and bug 1821824)

Comment 2

[Shane Hughes [:aminomancer]]

Attachment: Bug 1821826 - Refactor FeatureCallout to support generic triggers. r=#omc-reviewers

Comment 3

[Shane Hughes [:aminomancer]]

[Tracking Requested - why for this release]:

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b5f216cf6aa
Refactor FeatureCallout to support generic triggers. r=omc-reviewers,fxview-reviewers,tabbrowser-reviewers,dao,jprickett,sclements

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/5b5f216cf6aa

Comment 6

[Shane Hughes [:aminomancer]]

We've decided not to uplift these to 116.

Comment 7

[Marius Coman [:mcoman], Ecosystem QA]

I have tested this task using the steps from the received Test Plan and I can confirm the following:

• Feature 1 - Firefox View Feature Tour is disabled
  • No Feature Callout is triggered when navigating to the Firefox View page on a new profile.
• Feature 3 - Feature Callouts can be force-shown from the New Tab Page DevTools
  • Feature Callouts can be force-shown from the New Tab Page DevTools and respect the specification from the "Feature 3" section of the receive "Test Plan".
• Feature 4 - Feature Callouts can be entirely contained within an experiment and triggered by generic triggers
  • Feature Callouts can be entirely contained within an experiment and triggered by generic triggers and respect the specification from the "Feature 4" section of the receive "Test Plan".

However, I have not managed to trigger the "PDF.js Feature Tour" callout from the Feature 2 - PDF.js Feature Tour still works section of the Test Plan. In order to trigger the callout I have used the following steps:

[Steps]:

1. Created a new Firefox profile.
2. Opened a new tab and navigated to "https://www.clickdimensions.com/links/TestPDFfile.pdf".
3. Observe the behavior.

[Expected result]:

• The "PDF.js Feature Tour" feature callout is successfully triggered.

[Actual result]:

• The "PDF.js Feature Tour" feature callout is not triggered.

@Shane could you please take a look over this behavior in order to verify if I've used the correct steps or if should I log a separate issue for this?

Tested using the latest Firefox Nightly 117.0a1 (Build ID: 20230716213402) on Windows 10 x64, macOS 13.4.1, and Linux Mint 21.1.

Comment 8

[Shane Hughes [:aminomancer]]

Sorry Marius, I forgot to add a test step. Here are the corrected test steps:

Feature 2 - PDF.js Feature Tour still works

• Go to about:config and set browser.pdfjs.feature-tour so its value is {"screen":"FEATURE_CALLOUT_1_A","complete":false}
• Open a PDF file in a new tab
• There should be a Feature Callout anchored to the "Text" button in the PDF.js toolbar

Thanks!

Comment 9

[Marius Coman [:mcoman], Ecosystem QA]

Hi, Shane, and thank you for the steps. I can confirm that after setting the browser.pdfjs.feature-tour pref to {"screen":"FEATURE_CALLOUT_1_A","complete":false} the PDF Feature Callout is successfully displayed anchored to the "Text" button.

Based on this and my previous comment I am marking this task as Verified - Fixed.