1335374 - [Flash study] Create add-on to handle flash study

Status: RESOLVED FIXED

(Shield :: Actions, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

This bug is for the creation of the Shield add-on that will handle the flash study.

The add-on should:

- Choose the right cohort and set the prefs correctly
- Do the UI changes according to the UX spec
- Listen for clicks and other events related to the data gathering
- Aggregate the data and submit it as an external ping to telemetry


(The set-up of this study on Shield is tracked by bug 1335371)

Comment 1

[Alex Thayer [:alexical] (she/her)]

Some questions for bram regarding the UX spec:

- When we enter the "Can you tell us more about your problems with this page" flow due to the user refreshing the page, we're not sure the user even was having problems with the page. I don't know if it's just me, but I'll often refresh pages just to update the content of them. For instance, it's usually easier for me to just hit F5 in GMail than it is to click they're little refresh button within the page. Accordingly, should we ask them something more like "Are you having problems with this page?" and give them a button to say "No"?

  - Additionally, once we prompt them, should we remember that domain and make sure we don't prompt them again if they refresh it?

- In your UX diagram you mention "some time later..." a few times. What do you think is a good time span for this? A minute? Should we do anything if they navigate away from that site?

Comment 2

[Bram Pitoyo [:bram]]

Hi Doug. It’s great to know that we’ll be working together on this project!

(In reply to Doug Thayer [:dthayer] from comment #1)

> - When we enter the "Can you tell us more about your problems with this
> page" flow due to the user refreshing the page, we're not sure the user even
> was having problems with the page […]

Good point. Let’s add this doorhanger.

The logic looks like this:

* IF user has never interacted with the brick icon
* AND IF user refreshes the page for the first time
* THEN open doorhanger: “Are you having problems with this page? [Yes] [No]”
* IF Yes, THEN goto doorhanger: “Can you tell us more about your problems with this page?”
* IF No, THEN close doorhanger

You can find the new logic on the “Flash blocking and UI” doc online (I’ve posted it as a revision, so.


>   - Additionally, once we prompt them, should we remember that domain and
> make sure we don't prompt them again if they refresh it?

Yes. Once you interacted with this dialogue (answering either Yes or No), then it goes away forever.

If you refresh the page again without interacting, I think that we should still show this doorhanger. But alternatively, we can also take the second refresh as a sign of “don’t annoy me!” and not show the doorhanger again. My logic diagram uses the former.

 
> - In your UX diagram you mention "some time later..." a few times. What do
> you think is a good time span for this? A minute? Should we do anything if
> they navigate away from that site?

1 minute was the time span that we had talked about. I’ve included it in the new user flow diagram.

Comment 3

[Alex Thayer [:alexical] (she/her)]

Alright, I've got a draft of this fully implemented, so I have some questions for Felipe before I start tidying it up.

- Do you have a list of all the prefs I should be setting for this? `plugins.flashBlock.enabled` has been sufficient for testing the UI paths, but I'd like to get the full picture.

- I've had to do some moderately complex/hacky things to get the information that the UI needs in order to show up at the right times and function properly, and I'm wondering if doing this inside the add-on is the right place. It would probably be cleaner if Firefox could just shoot out the specific events that I need in order to do what I want to do, but that's A) code that no one else needs, and B) more code that would have to be uplifted, right? To give an example of the hackiness I'm talking about:

Exhibit A (changing the "Learn more..." link in the CTP notification into a "Report a problem..." link):

```
    // Note: this is quite a hack just to get the "Report a problem..." link to show
    // up. That link only makes sense in the context of our study, so it kind of makes
    // sense to contain it here, but we could also just put it behind a pref.
    chromeGlobal.PopupNotifications.panel.addEventListener('popupshowing', () => {
      const ctpNotification = doc.getElementById("click-to-play-plugins-notification");
      if (ctpNotification) {
        const link = doc.getAnonymousElementByAttribute(ctpNotification,
                                                      "anonid",
                                                      "click-to-play-plugins-notification-link");

        link.textContent = localized("report_a_problem");

        link.classList.remove('text-link');
        link.classList.add('ef-text-link');

        link.addEventListener('click', (e) => {
          // Unfortunately we have to reach into PopupNotification's insides in order
          // to dismiss the notification without making the brick go away.
          chromeGlobal.PopupNotifications._dismiss();

          showView('tell-us-more');
        }, { once: true });
      }
    });
```

Exhibit B (just detecting whether Flash is blocked - `Services.perms` and `SitePermissions` won't work since they don't take our deny/allow lists into account, so to do this more directly I think I would need to be in the content process. I could add a frame script to let me know from the content process, but that seems more complicated):

```
function getFlashPluginFromNotification(notification) {
  for (const plugin of notification.options.pluginData.values()) {
    if (plugin.pluginTag.name == "Shockwave Flash") {
      return plugin;
    }
  }
  return null;
}

function flashIsBlockedForWindow(chromeGlobal) {
  const notification = getPluginNotificationForWindow(chromeGlobal);

  const flash = getFlashPluginFromNotification(notification);

  if (flash) {
    return flash.fallbackType != PLUGIN_ACTIVE;
  } else {
    return false;
  }
}
```

- Where should this code go as far as source control goes? GitHub? I can't seem to dig up examples.

I think that's all the questions I have for now. If I think of any more I'll post them below.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

(In reply to Doug Thayer [:dthayer] from comment #3)
> Alright, I've got a draft of this fully implemented, so I have some
> questions for Felipe before I start tidying it up.
> 
> - Do you have a list of all the prefs I should be setting for this?
> `plugins.flashBlock.enabled` has been sufficient for testing the UI paths,
> but I'd like to get the full picture.

the prefs will be:
plugins.flashBlock.enabled -> true
plugins.favorfallback.mode -> "follow-ctp"
plugins.favorfallback.rules -> "embed,adobelink,installinstructions,true"
plugins.flashBlock.experiment.active -> true


> 
> - I've had to do some moderately complex/hacky things to get the information
> that the UI needs in order to show up at the right times and function
> properly, and I'm wondering if doing this inside the add-on is the right
> place. It would probably be cleaner if Firefox could just shoot out the
> specific events that I need in order to do what I want to do, but that's A)
> code that no one else needs, and B) more code that would have to be
> uplifted, right? To give an example of the hackiness I'm talking about:
> 
> Exhibit A (changing the "Learn more..." link in the CTP notification into a
> "Report a problem..." link):
> 
> ```
>     // Note: this is quite a hack just to get the "Report a problem..." link
> to show
>     // up. That link only makes sense in the context of our study, so it
> kind of makes
>     // sense to contain it here, but we could also just put it behind a pref.
>     chromeGlobal.PopupNotifications.panel.addEventListener('popupshowing',
> () => {
>       const ctpNotification =
> doc.getElementById("click-to-play-plugins-notification");
>       if (ctpNotification) {
>         const link = doc.getAnonymousElementByAttribute(ctpNotification,
>                                                       "anonid",
>                                                      
> "click-to-play-plugins-notification-link");
> 
>         link.textContent = localized("report_a_problem");
> 
>         link.classList.remove('text-link');
>         link.classList.add('ef-text-link');
> 
>         link.addEventListener('click', (e) => {
>           // Unfortunately we have to reach into PopupNotification's insides
> in order
>           // to dismiss the notification without making the brick go away.
>           chromeGlobal.PopupNotifications._dismiss();
> 
>           showView('tell-us-more');
>         }, { once: true });
>       }
>     });
> ```

In this case yeah.. Although this is hacky, it's fine for the experiment.. Since this extra UI code wouldn't be used in the release version anyway, it's unnecessary extra effort to get that reviewed and landed instead of doing this way.

> 
> Exhibit B (just detecting whether Flash is blocked - `Services.perms` and
> `SitePermissions` won't work since they don't take our deny/allow lists into
> account, so to do this more directly I think I would need to be in the
> content process. I could add a frame script to let me know from the content
> process, but that seems more complicated):
> 
> ```
> function getFlashPluginFromNotification(notification) {
>   for (const plugin of notification.options.pluginData.values()) {
>     if (plugin.pluginTag.name == "Shockwave Flash") {
>       return plugin;
>     }
>   }
>   return null;
> }
> 
> function flashIsBlockedForWindow(chromeGlobal) {
>   const notification = getPluginNotificationForWindow(chromeGlobal);
> 
>   const flash = getFlashPluginFromNotification(notification);
> 
>   if (flash) {
>     return flash.fallbackType != PLUGIN_ACTIVE;
>   } else {
>     return false;
>   }
> }
> ```

My goal is to have this information more easily available through bug 1335388. I'll get in touch with you to see the best way to make it available for the addon

> 
> - Where should this code go as far as source control goes? GitHub? I can't
> seem to dig up examples.

Let's talk with the Shield folks about what's the standard here

> 
> I think that's all the questions I have for now. If I think of any more I'll
> post them below.

Comment 5

[Alex Thayer [:alexical] (she/her)]

I ended up just putting it on GitHub for now until we hear back from the Shield team. Here's the link: https://github.com/squarewave/shield-study-essential-flash

For the time being I'm blocked on trying to figure out how to write tests for this thing. Pretty much everything substantial that the add-on is doing seems impossible to test with the standard add-on testing tools. Any suggestions in this area would be very much appreciated.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Talked on IRC: one idea is to write a browser-chrome test (that doesn't need to land), which will install the xpi and then perform checks on the feature.

Comment 7

[Alex Thayer [:alexical] (she/her)]

Hey Felipe, what's the status on bug 1335388? Am I right that it's a blocker for this bug?

Comment 8

[:Felipe Gomes (needinfo for replies!)]

(updated the status on the bug and irc)

Comment 9

[Alex Thayer [:alexical] (she/her)]

@Felipe, I believe this work is fully conformant with your data docs now (minus the mainPing field).

Do you have time to review?

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Yes, definitely! Can you post it here on Bugzilla?

Comment 11

[Alex Thayer [:alexical] (she/her)]

Not sure what the best way to do that is. Right now it's just a GitHub repo (https://github.com/squarewave/shield-study-essential-flash). Should I submit it as a PR to one-off-system-add-ons, or what would you prefer?

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Just generate a git diff against the previous revision that I had already read the code (without the ping stuff), and attach the diff here in Bugzilla.

If you want to commit and push it to the repo too it's fine as I'll probably take a look at the whole thing too if necessary to follow some of the logic

Comment 13

[Alex Thayer [:alexical] (she/her)]

Attachment: flashstudy.diff

Comment 14

[Alex Thayer [:alexical] (she/her)]

Hmm, Bugzilla does not like that format. Is there something I'm missing?

Comment 15

[Alex Thayer [:alexical] (she/her)]

Attachment: shield-study-plugin-safety.xpi

@jason, could we get this xpi signed so we can begin testing in Beta?

Comment 16

[Jason Thomas [:jason]]

I don't normally sign shield studies, why can't this be signed via AMO?

Comment 17

[Jason Thomas [:jason]]

Also, /cc :whd to add 'flash-shield-study' doctype to schema configuration.

Comment 18

[Alex Thayer [:alexical] (she/her)]

Attachment: plugin_safety-1.0.0-an+fx.xpi

Signed through |jpm sign|

Comment 19

[Wesley Dawson [:whd]]

> Also, /cc :whd to add 'flash-shield-study' doctype to schema configuration.

This has been added and deployed.

Comment 20

[Alex Thayer [:alexical] (she/her)]

Attachment: Signed a new version.

Comment 21

[Alex Thayer [:alexical] (she/her)]

Attachment: plugin_safety-1.0.3-an+fx.xpi

Updated prefs and set control group to 50%

Comment 22

[Alex Thayer [:alexical] (she/her)]

Attachment: plugin_safety-1.0.4-an+fx.xpi

Comment 23

[Alex Thayer [:alexical] (she/her)]

Attachment: plugin_safety-1.0.5-an+fx.xpi

Replaced window ID mechanism due to issues bytesized was seeing with inconsistent numbers in the data.

Comment 24

[Alex Thayer [:alexical] (she/her)]

Attachment: plugin_safety-1.0.8-an+fx.xpi

Fixes a few remaining issues with window IDs, and addresses a refresh issue noticed by Kirk.

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Clearing the needinfo as we've been constantly looking at the code and functionality on the last few weeks and it all looks good

Comment 26

[Alex Thayer [:alexical] (she/her)]

Attachment: plugin_safety-1.1.3-an+fx.xpi