1393489 - Add API to give Snippets in Activity Stream control over when to show onboarding

Status: RESOLVED WONTFIX

(Firefox :: New Tab Page, enhancement, P1)

Description

[Kate Hudson :k88hudson]

Because snippets occasionally have time-critical campaigns that need to be shown to a small percentage of Firefox users, let's give the snippets service the ability to override on-boarding when necessary.

In order for this to work/to prevent race conditions in what is shown, we would need to switch the final hiding/unhiding of onboarding to an API available to snippets (currently this is provided from ActivityStream via the gSnippetsMap https://github.com/mozilla/activity-stream/blob/master/docs/v2-system-addon/snippets.md#gsnippetsmap)

Perhaps onboarding could provide a "showOnboardingIfRequired" method that we could add to gSnippetsMap. Internal logic for onboarding could stay the same (i.e. which users should get onboarding, the state for which onboarding items have been shown. etc.), this would just give Snippets the ability to override onboarding for a small percentage of users during critical campaign times.

Comment 1

[Ed Lee :Mardak]

If onboarding is being triggered by activity stream / snippets by a `showIfRequired` method, it would probably not be called when the pref to show Snippets is turned off and that's the desired behavior by design: https://github.com/mozilla/activity-stream/issues/3146

Comment 2

[Kate Hudson :k88hudson]

Attachment: Bug 1393489 - Add API to give Snippets in Activity Stream control over when to show onboarding

Review commit: https://reviewboard.mozilla.org/r/179356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179356/

Comment 3

[Kate Hudson :k88hudson]

In this patch, I exposed a gOnboarding API to be used by snippets.

The corresponding patch in the snippets service can be found here: https://github.com/mozmeao/snippets-service/pull/285

Comment 4

[[e-mail]]

Comment on attachment 8907692 [details]
Bug 1393489 - Add API to give Snippets in Activity Stream control over when to show onboarding

Fischer implemented most of notification part, so he might want to take a look as well

Comment 5

[Fischer [:Fischer]]

(In reply to Kate Hudson :k88hudson from comment #0)
> Because snippets occasionally have time-critical campaigns that need to be
> shown to a small percentage of Firefox users, let's give the snippets
> service the ability to override on-boarding when necessary.
> 
> In order for this to work/to prevent race conditions in what is shown, we
> would need to switch the final hiding/unhiding of onboarding to an API
> available to snippets (currently this is provided from ActivityStream via
> the gSnippetsMap
> https://github.com/mozilla/activity-stream/blob/master/docs/v2-system-addon/
> snippets.md#gsnippetsmap)
> 
> Perhaps onboarding could provide a "showOnboardingIfRequired" method that we
> could add to gSnippetsMap. Internal logic for onboarding could stay the same
> (i.e. which users should get onboarding, the state for which onboarding
> items have been shown. etc.), this would just give Snippets the ability to
> override onboarding for a small percentage of users during critical campaign
> times.
Hi Kate,
Would like to check what "override" ability we are expecting here with you.
From the github patch [1], it seems we want to let AS decide when and should or shouldn't show onboarding notification. In this way in the future we could show some important snippet even before onboarding notifications get finished. Is my understanding correct?

[1] https://github.com/mozmeao/snippets-service/pull/285/files

Comment 6

[[e-mail]]

Comment on attachment 8907692 [details]
Bug 1393489 - Add API to give Snippets in Activity Stream control over when to show onboarding

https://reviewboard.mozilla.org/r/179356/#review185328

I don't get why we need override the onboarding notification. Since Activity Stream will always load before onboarding(onboarding runs in requestIdleCallback), You can check if pref `browser.onboarding.notification.finished` is `false`, store the restore flag and set the pref to `true` when you need to disable onboarding notification to show the urgent snippet. You can restore the pref state when urgent snippet done its task. All this change can be done within activity stream without expose `gOnboarding`.

Comment 7

[Ed Lee :Mardak]

(In reply to Fred Lin [:gasolin] from comment #6)
> I don't get why we need override the onboarding notification.
I haven't looked at all the code around the patch, but from a high level code encapsulation API perspective is that Activity Stream probably shouldn't be touching some global state (prefs) to try to get Onboarding code to do something as well as reaching into the details of Onboarding implementation to hide content (find the notification element and remove/hide it).

I suppose yes, because Activity Stream and Onboarding is on the same page, there could be some css with the appropriate selector to just hide the Onboarding notification. But that selector needs to change if Onboarding purposefully/accidentally changes the structure (different id, class, etc.) and that would break Activity Stream.

The patch provides an official way to hide/show notification through an API that is tested. That API needs to be maintained for Activity Stream to work, but it that's probably better than Activity Stream needing to know what prefs Onboarding uses internally or what elements it's creating internally.

Comment 8

[[e-mail]]

Thanks for explaination. In photon workweek in toronto we already have the convention to expose `browser.onboarding.notification.finished` to AS/snippet. Changing that pref is safe and no need to know the detail of how Onboarding UI is changed.

At this time I still think we should go with pref, by adding enableNotifications/disableNotifications behavior(from this PR) in `_initPrefObserver`, we can get Onboarding notification changed immediately via change the pref state. That would be more maintainable than patching around, and would be more managable for the post merge day patch.


It burns brain in midnight, I'll discuss with Fischer and Rex at Monday if we really need to expose `gOnboarding`.

Comment 9

[Ed Lee :Mardak]

Mossop, could you provide some insight here as you had been reviewing onboarding code. Activity Stream would like to use a JS API to turn Onboarding on/off Notifications (`gOnboarding.enableNotifications()`). Onboarding already "exposes" `browser.onboarding.notification.finished` that could be considered an API to turn things on/off by setting and restoring the pref but would need to be updated to watch for those external pref changes.

k88hudson has a patch here to do the JS API, but watching for pref changes could have fewer lines changed.

Comment 10

[Dave Townsend [:mossop]]

Given that we have to turn onboarding notifications on and off at runtime I don't think preference changes are the right signals to use here. The API approach looks sounder to me.

Comment 11

[Fischer [:Fischer]]

Comment on attachment 8907692 [details]
Bug 1393489 - Add API to give Snippets in Activity Stream control over when to show onboarding

https://reviewboard.mozilla.org/r/179356/#review185936

::: browser/extensions/onboarding/content/onboarding.js:348
(Diff revision 1)
>   * The script won't be initialized if we turned off onboarding by
>   * setting "browser.onboarding.enabled" to false.
>   */
>  class Onboarding {
>    constructor(contentWindow) {
> +    this._notificationsEnabled = Services.prefs.getBoolPref(PREF_ACTIVITY_STREAM_ENABLED) ? false : true;

We might need a more general way to determine should/shouldn't show notification, considering one of the design requirments of the Onboarding is to support about:home (If I wasn't wrong, we might still go for the old about:home page) and maybe even a 3rd type page in the future.
I could think of 2 possible approaches:
Approach 1: 
  - Totally let the external page control when to show notifications. That is, the Onboarding just exposes `shouldShowNotifications`, `showNotification` apis etc and it is the duty of the external page to use apis to do notification at the appropriate timing.
  - Pros: A consistent api design. No special case.
  - Cons: We have to update at least the old about:home to call these apis (This won't be the case if going for a AS about:home page). And in the future any newly supported page has to take care of tour notifications on its own.

Approach 2:
  - Sitll the Onboarding exposes these notification apis. Besides that, the Onboarding would *detect* some attribute, like `data-auto-onboarding-notification="false"` on `<body>`. If the attribute wasn't `false`, then do notification automatically. If was `false` then don't do notification automatically, it is the external page's job.
  - Pros: No need to update the old about:home and the newly supported page in the future.
  - Cons: What if a page without `data-auto-onboarding-notification="false"` calls these apis still? We might end with 2 notifications prompted. This is not a consistent/predictable api design.

Kate, could we know the status and the schedule right now? If we had a tight schedule, then probably Approach 2 would be a more feasible option. If a AS about:home was for sure in 57, Approach 1 might be better.

::: browser/extensions/onboarding/content/onboarding.js:919
(Diff revision 1)
>      }
>      return queue ? queue.split(",") : [];
>    }
>  
> -  showNotification() {
> +  get shouldShowNotifications() {
> +    let lastTime = this._getLastTourChangeTime();

Please move this `let lastTime = this._getLastTourChangeTime();` after `if (Services.prefs.getBoolPref("browser.onboarding.notification.finished", false)) {..`

Comment 12

[Kate Hudson :k88hudson]

Ah ok, good point about supporting about:home. Right now we are proceeding ahead as planned with shipping activity stream on about:home, but we want to reserve the capability of switching back if it is perceived as too slow.

I could implement #2 but what about checking the preference for browser.newtabpage.activity-stream.aboutHome.enabled and only show notifications automatically if that pref is false?

Comment 13

[Kate Hudson :k88hudson]

Comment on attachment 8907692 [details]
Bug 1393489 - Add API to give Snippets in Activity Stream control over when to show onboarding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179356/diff/1-2/

Comment 14

[Kate Hudson :k88hudson]

Comment on attachment 8907692 [details]
Bug 1393489 - Add API to give Snippets in Activity Stream control over when to show onboarding

https://reviewboard.mozilla.org/r/179356/#review185936

> We might need a more general way to determine should/shouldn't show notification, considering one of the design requirments of the Onboarding is to support about:home (If I wasn't wrong, we might still go for the old about:home page) and maybe even a 3rd type page in the future.
> I could think of 2 possible approaches:
> Approach 1: 
>   - Totally let the external page control when to show notifications. That is, the Onboarding just exposes `shouldShowNotifications`, `showNotification` apis etc and it is the duty of the external page to use apis to do notification at the appropriate timing.
>   - Pros: A consistent api design. No special case.
>   - Cons: We have to update at least the old about:home to call these apis (This won't be the case if going for a AS about:home page). And in the future any newly supported page has to take care of tour notifications on its own.
> 
> Approach 2:
>   - Sitll the Onboarding exposes these notification apis. Besides that, the Onboarding would *detect* some attribute, like `data-auto-onboarding-notification="false"` on `<body>`. If the attribute wasn't `false`, then do notification automatically. If was `false` then don't do notification automatically, it is the external page's job.
>   - Pros: No need to update the old about:home and the newly supported page in the future.
>   - Cons: What if a page without `data-auto-onboarding-notification="false"` calls these apis still? We might end with 2 notifications prompted. This is not a consistent/predictable api design.
> 
> Kate, could we know the status and the schedule right now? If we had a tight schedule, then probably Approach 2 would be a more feasible option. If a AS about:home was for sure in 57, Approach 1 might be better.

Ok, I added some changes to check browser.newtabpage.activity-stream.enabled and browser.newtabpage.activity-stream.aboutHome.enabled as well as some tests. However, if you think it would be preferable to add data-auto-onboarding-notification="false" to our activity stream html, that would be fine too (let me know)

Comment 15

[Fischer [:Fischer]]

(In reply to Kate Hudson :k88hudson from comment #14)
> Ok, I added some changes to check browser.newtabpage.activity-stream.enabled
> and browser.newtabpage.activity-stream.aboutHome.enabled as well as some
> tests. However, if you think it would be preferable to add
> data-auto-onboarding-notification="false" to our activity stream html, that
> would be fine too (let me know)
Please go Approach #2 as we want to keep some flexibility around about:home.
The reasons not using the prefs approach are:
1. if in the future an new request asks to put Onboarding on another page, checking these prefs wouldn't cover this case. We still want auto-notification although these prefs are enabled.
2. only need to check one attribute in Approach #2
3. Approach #2 could decouple AS(or other pages) with Onboarding to a larger degree (Approach #1 is even more)

Comment 16

[Kate Hudson :k88hudson]

Comment on attachment 8907692 [details]
Bug 1393489 - Add API to give Snippets in Activity Stream control over when to show onboarding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179356/diff/2-3/

Comment 17

[Kate Hudson :k88hudson]

Comment on attachment 8907692 [details]
Bug 1393489 - Add API to give Snippets in Activity Stream control over when to show onboarding

https://reviewboard.mozilla.org/r/179356/#review185936

> Ok, I added some changes to check browser.newtabpage.activity-stream.enabled and browser.newtabpage.activity-stream.aboutHome.enabled as well as some tests. However, if you think it would be preferable to add data-auto-onboarding-notification="false" to our activity stream html, that would be fine too (let me know)

Updated with data-auto-onboarding-notification="false"!

Comment 18

[Fischer [:Fischer]]

Comment on attachment 8907692 [details]
Bug 1393489 - Add API to give Snippets in Activity Stream control over when to show onboarding

https://reviewboard.mozilla.org/r/179356/#review188210

::: browser/extensions/onboarding/content/onboarding.js:27
(Diff revision 3)
>  const PROMPT_COUNT_PREF = "browser.onboarding.notification.prompt-count";
>  const ONBOARDING_DIALOG_ID = "onboarding-overlay-dialog";
>  const ONBOARDING_MIN_WIDTH_PX = 960;
>  const SPEECH_BUBBLE_MIN_WIDTH_PX = 1130;
> -
> +const PREF_AS_ENABLED = "browser.newtabpage.activity-stream.enabled";
> +const PREF_AS_HOME_ENABLED = "browser.newtabpage.activity-stream.aboutHome.enabled";

Please remove `PREF_AS_ENABLED`, `PREF_AS_HOME_ENABLED`

::: browser/extensions/onboarding/content/onboarding.js:354
(Diff revision 3)
> +    this._window = contentWindow;
> +
> +    // If we're loading on a page with activity stream, do not enable notifications right away.
> +    // Instead, they will be enabled by the gOnboarding API.
> +    const autoShowNotifications = this._window.document.body.getAttribute("data-auto-onboarding-notification") !== "false";
> +    this._notificationsEnabled = autoShowNotifications;

We could just do `this._notificationsEnabled = this._window.document.body.getAttribute("data-auto-onboarding-notification") !== "false";`.
And could we rename `_notificationsEnabled` to `_autoShowNotifications` or `_autoEnableNotifications` because that is closer to its purpose

::: browser/extensions/onboarding/content/onboarding.js:356
(Diff revision 3)
> +    // If we're loading on a page with activity stream, do not enable notifications right away.
> +    // Instead, they will be enabled by the gOnboarding API.
> +    const autoShowNotifications = this._window.document.body.getAttribute("data-auto-onboarding-notification") !== "false";
> +    this._notificationsEnabled = autoShowNotifications;
> +
> +    this._notificationInitializationNeeded = false;

Please remove this variable. `_autoEnableNotifications` shoudl be enough

::: browser/extensions/onboarding/content/onboarding.js:395
(Diff revision 3)
>        this._window.requestIdleCallback(() => this._resizeUI());
> +
> +    exposeOnboarding(contentWindow, this)
> +  }
> +
> +  get notificationsEnabled() {

Should be able to remove this getter

::: browser/extensions/onboarding/content/onboarding.js:399
(Diff revision 3)
> +
> +  get notificationsEnabled() {
> +    return this._notificationsEnabled;
> +  }
> +
> +  enableNotifications() {

Should be able to remove `enableNotifications` we could expose `initNotifications` to achieve the job.

::: browser/extensions/onboarding/content/onboarding.js:406
(Diff revision 3)
> +    if (this._notificationInitializationNeeded) {
> +      this.initNotifications();
> +    }
> +  }
> +
> +  disableNotifications() {

We could remove `disableNotifications` and expose `hideNotification` to achieve the job

::: browser/extensions/onboarding/content/onboarding.js:456
(Diff revision 3)
>  
>      this._initPrefObserver();
> -    // Doing tour notification takes some effort. Let's do it on idle.
> -    this._window.requestIdleCallback(() => this._initNotification());
> +
> +    this._notificationInitializationNeeded = true;
> +
> +    if (this.notificationsEnabled) {

Here could remove the above `_notificationInitializationNeeded = true` and do
```
  if (this._autoEnableNotifications) {
    this.initNotifications();
  }
```
So it gose like if auto-notification was on, then try to init notification automatically, otherwise do nothing let the external page decide. Inside `initNotifications` it would check `shouldShowNotifications` and `showNotification` at the proper doc hidden state

::: browser/extensions/onboarding/content/onboarding.js:1193
(Diff revision 3)
>  
> +// This should be called with onboarding has initialized
> +function exposeOnboarding(window, onboarding) {
> +  const gOnboarding = {
> +    shouldShowNotifications: false,
> +    notificationsEnabled: false

There should be need to expose this `notificationsEnabled` because it was decided by the external page's "data-auto-onboarding-notification" attr.

::: browser/extensions/onboarding/content/onboarding.js:1200
(Diff revision 3)
> +  if (onboarding) {
> +    gOnboarding.notificationsEnabled = onboarding.notificationsEnabled;
> +    gOnboarding.tourType = onboarding._tourType;
> +    gOnboarding.shouldShowNotifications = onboarding.shouldShowNotifications;
> +    gOnboarding.enableNotifications = () => onboarding.enableNotifications();
> +    gOnboarding.disableNotifications = () => onboarding.disableNotifications();

Here we could expose `shouldShowNotifications`, `initNotification`, `hideNotification`. The external page can do like
```
// On window load for instance
if (gOnboarding.shouldShowNotifications) gOnboarding.initNotification();

// Later user clicks somewhere on the page (not the close button on the notification)
gOnboarding.hideNotification();
```

::: browser/extensions/onboarding/test/browser/browser_onboarding_notification_enabled.js:49
(Diff revision 3)
> +  const tab = await openTabWithPrefs(ABOUT_NEWTAB_URL, [
> +    ["browser.newtabpage.activity-stream.enabled", true],
> +    ["browser.newtabpage.activity-stream.aboutHome.enabled", false],
> +  ]);
> +
> +  const shown = await checkNotificationUIShown(tab.linkedBrowser, false);

I'm a bit confused here. From the test case, it seems the notification shouldn't show on an AS about:newtab. But I thought by default AS would call api to show it. It would't show under some snippet setting and while testing, we could assume the setting is always "show notification".

::: browser/extensions/onboarding/test/browser/browser_onboarding_notification_enabled.js:61
(Diff revision 3)
> +  const tab = await openTabWithPrefs(ABOUT_NEWTAB_URL, [
> +    ["browser.newtabpage.activity-stream.enabled", false],
> +    ["browser.newtabpage.activity-stream.aboutHome.enabled", false],
> +  ]);
> +
> +  const shown = await checkNotificationUIShown(tab.linkedBrowser, false);

Here conflicts with the above "test_activity_stream_notificationsEnabled_about_newtab_with_as" test case. Here and above there both call `await checkNotificationUIShown(tab.linkedBrowser, false);` with arg of `shouldBeShown == false` but expecting the different results.
This test is testing the auto-notification on the non-AS about:newtab. We could reuse `await promiseTourNotificationOpened`

::: browser/extensions/onboarding/test/browser/browser_onboarding_notification_enabled.js:73
(Diff revision 3)
> +  const tab = await openTabWithPrefs(ABOUT_HOME_URL, [
> +    ["browser.newtabpage.activity-stream.enabled", true],
> +    ["browser.newtabpage.activity-stream.aboutHome.enabled", false],
> +  ]);
> +
> +  const shown = await checkNotificationUIShown(tab.linkedBrowser, true);

The same as "test_activity_stream_notificationsEnabled_about_newtab_without_as". Here could reuse `await promiseTourNotificationOpened` to test the non-AS about:home

::: browser/extensions/onboarding/test/browser/browser_onboarding_notification_enabled.js:85
(Diff revision 3)
> +  const tab = await openTabWithPrefs(ABOUT_HOME_URL, [
> +    ["browser.newtabpage.activity-stream.enabled", true],
> +    ["browser.newtabpage.activity-stream.aboutHome.enabled", true],
> +  ]);
> +
> +  const shown = await checkNotificationUIShown(tab.linkedBrowser, false);

The same question as "test_activity_stream_notificationsEnabled_about_newtab_with_as"

::: browser/extensions/onboarding/test/browser/browser_onboarding_notification_enabled.js:97
(Diff revision 3)
> +  const tab = await openTabWithPrefs(ABOUT_HOME_URL, [
> +    ["browser.newtabpage.activity-stream.enabled", true],
> +    ["browser.newtabpage.activity-stream.aboutHome.enabled", true],
> +  ]);
> +
> +  await promiseTourNotificationOpened(tab.linkedBrowser);

This "test_enable_notifications" conflicts with "test_activity_stream_notificationsEnabled_about_home_with_as" test case. With the same prefs settings but expects the opposite results??

::: browser/extensions/onboarding/test/browser/head.js:150
(Diff revision 3)
> +    }
> +    return content.wrappedJSObject.gOnboarding.notificationsEnabled;
> -    });
> +  });
> -  }
> +}
> -  return ContentTask.spawn(browser, {}, isOpened);
> +
> +async function promiseTourNotificationOpened(browser, waitForUi = true) {

I would suggest not to modify `promiseTourNotificationOpened` for 
1. many places [1] are using it
2. the original code listens to the DOM mutation event so as long as the auto-notification is on or external page calls apis, it would observe the notification prompt. The original approach should be sufficient.

[1] http://searchfox.org/mozilla-central/search?q=promiseTourNotificationOpened

::: browser/extensions/onboarding/test/browser/head.js:153
(Diff revision 3)
> -  }
> +}
> -  return ContentTask.spawn(browser, {}, isOpened);
> +
> +async function promiseTourNotificationOpened(browser, waitForUi = true) {
> +  await promiseWaitForOnboardingAPI(browser);
> +  await ContentTask.spawn(browser, {}, async function() {
> +    ok(content.wrappedJSObject.gOnboarding.shouldShowNotifications, "gOnboarding.shouldShowNotifications is true");

Could we move this `gOnboarding.shouldShowNotifications` api check into `promiseWaitForOnboardingAPI`?

Comment 19

[Fischer [:Fischer]]

Comment on attachment 8907692 [details]
Bug 1393489 - Add API to give Snippets in Activity Stream control over when to show onboarding

https://reviewboard.mozilla.org/r/179356/#review188210

> There should be need to expose this `notificationsEnabled` because it was decided by the external page's "data-auto-onboarding-notification" attr.

Correct myself: There should be *NO* need to expose this notificationsEnabled because it was decided by the external page's "data-auto-onboarding-notification" attr.

Comment 20

[Fischer [:Fischer]]

Comment on attachment 8907692 [details]
Bug 1393489 - Add API to give Snippets in Activity Stream control over when to show onboarding

https://reviewboard.mozilla.org/r/179356/#review188260

::: browser/extensions/onboarding/content/onboarding.js:928
(Diff revision 3)
>        }]);
>      }
>      return queue ? queue.split(",") : [];
>    }
>  
> -  showNotification() {
> +  get shouldShowNotifications() {

Right now the Onboarding could be destroyed under 960px-wide window, we might need to add
```
if (!this.uiInitialized) {
  return false;
}
```
so it would either knows `shouldShowNotifications` is false or stops proceeding inside `_initNotification`, otherwise something wrong might happen when the external page calls api

::: browser/extensions/onboarding/content/onboarding.js:981
(Diff revision 3)
>      }
>      let targetTourId = queue[0];
>      let targetTour = this._tours.find(tour => tour.id == targetTourId);
>  
>      // Show the target tour notification
>      this._notificationBar = this._renderNotificationBar();

I was thinking should we do
```
if (!this._notificationBar) {
  this._notificationBar = this._renderNotificationBar();
}
```
Because originally the onboarding 100% controlled the notification prompt, it was guaranteed that no twice prompts in one session. However, with the exposed apis that may no longer be the case.

Comment 21

[Ed Lee :Mardak]

Not sure how the priorities are being set, but without this, I believe toggling the Snippets pref in activity stream is broken, so this is something we'll need to uplift to 57.

Comment 22

[Tim Spurway [:tspurway]]

[Tracking Requested - why for this release]: we need this to be able to regulate traffic between Snippets and the Onboarding Tour on about:home and about:newtab.

Comment 23

[[e-mail]]

Comment on attachment 8907692 [details]
Bug 1393489 - Add API to give Snippets in Activity Stream control over when to show onboarding

https://reviewboard.mozilla.org/r/179356/#review189028

::: browser/extensions/onboarding/content/onboarding.js
(Diff revision 3)
> +    this._notificationInitializationNeeded = false;
>      this.init(contentWindow);
>    }
>  
>    async init(contentWindow) {
> -    this._window = contentWindow;

`this._window` is used elsewhere, so we should not remove that

Comment 24

[Ed Lee :Mardak]

Activity Stream ended up just setting the pref.

Comment 25

[Ciprian Muresan [:cmuresan], Ecosystem QA]

:Mardak, I'm unsure what is the correct resolution for this bug. The correspondent in GitHub is closed as "Fixed by #3542", but here it's closed as WONTFIX. 
Does this mean that the fix will not be uplifted in m-c?

Comment 26

[Ed Lee :Mardak]

The approach in the patches in this bug aren't needed to fix the issue because a different approach was used as per comment 6. That other approach fix has landed in GitHub and will be uplifted via bug 1403215.

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked bug 1403215