Closed Bug 1476701 Opened 6 years ago Closed 6 years ago

Add a new way for the extension script to get the information which is about the amount of autoplay media top-domain

Categories

(Core :: Audio/Video: Playback, enhancement, P2)

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1476701 - notify observer when audible autoplay occurred.
59 bytes, text/x-review-board-request
cpearce
: review+
jaws
: review+
Details 
We want to collect how many website contains autoplay media in our shield-study, but there is no way to get this information on current API desigin.

Therefore, I would like to add new tab attribute 'contain-autoplay-media' in order to tell extension how many tab has autoplay media content.

What I want to do in my extension is like that,
```
tab.addEventListener("TabAttrModified", (event) => {
    if (event.detail.changed.includes("contain-autoplay-media")) {
        // do something
    }
});
```
Blocks: 1475099
Can you tell me more about what you're attempting to collect here? Is this an effort to count how many websites users visit that might display an autoplay doorhanger? If so, have we considered using a Telemetry scalar instead?
Flags: needinfo?(alwu)
(In reply to Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th)) from comment #3)
> Can you tell me more about what you're attempting to collect here? Is this
> an effort to count how many websites users visit that might display an
> autoplay doorhanger?

To expand on this question, is the count all we care about? Or do we want other data, like information about what kind of page it was or what kind of media it was? If it's just the count we care about, we might be able to do something simpler here.

Cancelling review request until I better understand what question we're trying to answer here.
Comment on attachment 8993086 [details]
Bug 1476701 - notify observer when audible autoplay occurred.

https://reviewboard.mozilla.org/r/257866/#review265388
Attachment #8993086 - Flags: review?(mconley)
Rank: 12
Priority: -- → P2
Hi, Mike,

One of data struct which we want to collect in our probe is like that,
```
// following data is running count until the probe is sent back and then reset to 0
"counters": {
    // amount of top level origins pages user visited 
    "totalPages": 200,

    // amount of top level origins pages with audible auto-play media
    "totalPagesAM": 20,

    // amount of autoplay videos that have been blocked
    "totalBlockedVideos": 20
},
```

We would collect those data in our shield-study extension. Since we want to count the amount of top level origins pages, so the simply counting how many time pages start autoplay is not work for us.

My plan is to maintain a hash map in extension which would store all top level origins (which would be salted-hash url) so that I can count for per top level origins pages.

Here are my WIP,
https://github.com/alastor0325/autoplay-shield-study/blob/develop/src/feature.js#L36
https://github.com/alastor0325/autoplay-shield-study/blob/develop/src/privileged/autoplay/api.js#L63

---

If you think this way is not good, I have another idea but I haven't tried it and don't know whether it can work or not.

That is, instead of adding a new attribute in tab, sending new observer notification when the autoplay occured.

platform C++ code      (send notification) -> experiments child code (receive notification)
                                                |
                                                V
extension background script (do something) <- experiments parent code (get msg from child code)

---

How do you think?

Thank you!!
Flags: needinfo?(alwu) → needinfo?(mconley)
I feel like counting the number of top-level origins is similar to the sort of research we're doing for Fission to count how many content processes we might conceivably need.

Maybe someone from the DOM team would have an idea on how best to count this stuff. Perhaps mrbkap?
Flags: needinfo?(mconley) → needinfo?(mrbkap)
Nika probably knows more about this than I do.
Flags: needinfo?(mrbkap) → needinfo?(nika)
Hi, Nika,

I'm planing to get some customized data from gecko (see comment7), and I want to use the way which is adding a new tab attribute or adding new event.

I'm wondering whether there would have a different ways which could alllow my extensions code to get those information? 

Thank you.
Summary: Add new attribute 'contain-autoplay-media' on tabbrowser in order to collect data in shield-study extension → Add a new way for the extension script to get the information which is about the amount of autoplay media top-domain
Since this data should be the same as what we report in the telemetry probes I'm adding in bug 1476456, should re-use the same collection point or fix the collection point I'm adding there if they're not correct.

Note: our data science guys asked us to collect this data in the shield study as well as in telemetry, so that the telemetry can give them an idea how big a samples to expect, and so that the data is available in the same place as the rest of the shield study data.
Comment on attachment 8993086 [details]
Bug 1476701 - notify observer when audible autoplay occurred.

https://reviewboard.mozilla.org/r/257866/#review265922

Clearing review; I think we should reuse the telemetry probe collection point added in bug 1476456, and also I said elsewhere that I don't think it makes sense for anything other than policy conformance to be reported by AutoplayPolicy; it should tell you whether to block, I don't think it should do anything like conditionally firing events itself.
Attachment #8993086 - Flags: review?(cpearce)
Flags: needinfo?(nika) → needinfo?(glind)
(In reply to Alastor Wu [:alwu] from comment #10)
> I'm planing to get some customized data from gecko (see comment7), and I
> want to use the way which is adding a new tab attribute or adding new event.
> 
> I'm wondering whether there would have a different ways which could alllow
> my extensions code to get those information? 

I'm not super familiar with the autoplay code, sorry. Not an area I'm super involved in.
(In reply to Chris Pearce (:cpearce) from comment #11)
> Since this data should be the same as what we report in the telemetry probes
> I'm adding in bug 1476456, should re-use the same collection point or fix
> the collection point I'm adding there if they're not correct.

Yes, I would collect the amount of blocked video from telemetry probe in bug 1476456.

What I want to solve in this bug is collecting the amount of top-level sites with autoplay, I would like to find a way which can allow extension background script to know which sites contains audible autoplay media.
Hi, Gregg,

My goal is to allow my custom experiments API to get the notification about (1)  autoplay media occurred and (2) the url where autoplay media occurred

I tried to use another way that is sending observer notification in browser.xml, instead of adding tab attribute. But if I use this way, I don't know how to get the current url when the subject object is a browser (browser.xml). Also, I don't know the way I'm trying is good or not.

Could you give me any suggestion?

Thank you!
In addition, here is our data probe structure,

```
// following data is running count until the probe is sent back and then reset to 0
"counters": {
    // amount of top level origins pages user visited 
    "totalPages": 200,

    // amount of top level origins pages with audible auto-play media
    "totalPagesAM": 20,

    // amount of autoplay videos that have been blocked
    "totalBlockedVideos": 20
},
```
Attachment #8993087 - Attachment is obsolete: true
Comment on attachment 8993086 [details]
Bug 1476701 - notify observer when audible autoplay occurred.

Hi, Gregg,
Could you give me some suggestion?
Thank you!
Attachment #8993086 - Flags: feedback?(glind)
Comment on attachment 8993086 [details]
Bug 1476701 - notify observer when audible autoplay occurred.

https://reviewboard.mozilla.org/r/257866/#review266530


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/tabbrowser.js:4411
(Diff revision 2)
> +      let tab = this.getTabForBrowser(browser);
> +      if (!tab) {
> +        return;
> +      }
> +
> +      let os = Cc["@mozilla.org/observer-service;1"]

Error: Use Services.obs rather than getService(). [eslint: mozilla/use-services]
The approach of adding an attribute to the tab browser seems sensible to me. The other idea that sprung to mind was copying the precedent set by nsIDOMWindowUtils.mediaSuspend.
Comment on attachment 8993086 [details]
Bug 1476701 - notify observer when audible autoplay occurred.

Hi, Dale,
Could you give me some suggestion about my patch? 
Thank you!
Attachment #8993086 - Flags: feedback?(glind) → feedback?(dharvey)
(In reply to Chris Pearce (:cpearce) from comment #20)
> The approach of adding an attribute to the tab browser seems sensible to me.
> The other idea that sprung to mind was copying the precedent set by
> nsIDOMWindowUtils.mediaSuspend.

Now I would like to use the observer way which background script could observe the notification and know whenever the autoplay occurred. But I still need to know the feedback from front-end team first.
Comment on attachment 8993086 [details]
Bug 1476701 - notify observer when audible autoplay occurred.

So as with Chris I am not certain why this isnt reusing / modifying the existing telemetry to support your probes needs, sending dom events through from c++ only to be used for telemetry doesnt seem ideal.

However if the code needs to go down the dom, this seems pretty reasonable looking code to me (although again, not a peer so would need a proper peer review)
Attachment #8993086 - Flags: feedback?(dharvey) → feedback+
(In reply to Dale Harvey (:daleharvey) from comment #23)
> So as with Chris I am not certain why this isnt reusing / modifying the
> existing telemetry to support your probes needs, sending dom events through
> from c++ only to be used for telemetry doesnt seem ideal.

We could get part of data from chris's telemetry, but we can't get the total amount of top domain of blocked autoplay websites. In addition, the telemetry "recording the document containing" (which is little like the topic we want to have in probe, but it still has a difference) is only recored when the docuement detroyed. It's not good timing to get the data from telemetry for extension script.
Comment on attachment 8993086 [details]
Bug 1476701 - notify observer when audible autoplay occurred.

https://reviewboard.mozilla.org/r/257866/#review267086

So this starts as an event from DOM code, then the content process listens for the event, and sends a message to the parent process which dispatches an event through the parent process. When the tabbrowser gets the event, it sends an observer notification to observers.

Do we need to do so many different hops here? Can the shield study listen for the same event that tabbrowser is listening for? Or could the shield study listen for the message?

Also, you'll need a DOM peer to review the HTMLMediaElement.h/HTMLMediaElement.cpp changes.

::: dom/html/HTMLMediaElement.cpp:4168
(Diff revision 3)
>  
>  void
> +HTMLMediaElement::NotifyDocAudibleAutoplayOccurred()
> +{
> +  // only notify document for audible autoplay
> +  if (Volume() == 0.0 || Muted()) {

I would add `!Autoplay() || OwnerDoc()->HasBeenUserGestureActivated()` to this early return condition and then you can remove them below.
Attachment #8993086 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #26)
> Comment on attachment 8993086 [details]
> Bug 1476701 - notify observer when audible autoplay occurred.
> 
> https://reviewboard.mozilla.org/r/257866/#review267086
> 
> So this starts as an event from DOM code, then the content process listens
> for the event, and sends a message to the parent process which dispatches an
> event through the parent process. When the tabbrowser gets the event, it
> sends an observer notification to observers.
> 
> Do we need to do so many different hops here? Can the shield study listen
> for the same event that tabbrowser is listening for? Or could the shield
> study listen for the message?

My extension script needs to know (1) the url of website where autoplay occured (2) the tab id of the tab where autoplay occured

(1) is used to know whether this tab has any autoplay media, and then I could use (2) to get the PopupNotifications in order to know what options user choose in our doorhanger. 

Here is my extension code 
(1) https://github.com/alastor0325/autoplay-shield-study/blob/develop/src/privileged/autoplay/api.js#L207
(2) https://github.com/alastor0325/autoplay-shield-study/blob/develop/src/privileged/autoplay/api.js#L264

I don't know whether there is other good way to achieve same goal.
Could you give me some suggestion?

Thank you.
Flags: needinfo?(jaws)
Comment on attachment 8993086 [details]
Bug 1476701 - notify observer when audible autoplay occurred.

https://reviewboard.mozilla.org/r/257866/#review267206

r+ on the C++ changes, with comments addressed. If what I've suggested here doesn't give you what you want, please re-request review. Thanks!

::: dom/html/HTMLMediaElement.cpp:4168
(Diff revision 3)
>  
>  void
> +HTMLMediaElement::NotifyDocAudibleAutoplayOccurred()
> +{
> +  // only notify document for audible autoplay
> +  if (Volume() == 0.0 || Muted()) {

+1 to jaw's intent. Early returns avoid indentation, which reduces mental load when reading code.

Though I think if we were to include those conditions in the same "if" statement here, the logic would need to be:

    if (Volume() == 0.0 || Muted() || (!Autoplay() && OwnerDoc()->HasBeenUserGestureActivated())) return;

That said, I think we can just put this event dispatch elsewhere to avoid duplciated logic, so see my next comment below.

::: dom/html/HTMLMediaElement.cpp:4174
(Diff revision 3)
> +    return;
> +  }
> +
> +  if (Autoplay() ||
> +      !OwnerDoc()->HasBeenUserGestureActivated()) {
> +    RefPtr<AsyncEventDispatcher> asyncDispatcher =

Why don't you put this event dispatch in nsIDocument::SetDocTreeHadAudibleMedia() if  topLevelDoc->mDocTreeHadAudibleMedia is false?

Then you'll dispatch the event the first time a doc tree autoplays audible media, not every time it any document anywhere in the tree does. You only want one event per tab, right?

That also results in less repetition of the logic here, and less chance that the telemetry and shield study logic will disagree.

Also, once you do that, I think you should rename HTMLMediaElement::ReportAutoplayTelemetry() to HTMLMediaElement::UpdateHadAudibleAutoplayState(), to better reflect that this function is no longer only just about the telemetry probes.
Attachment #8993086 - Flags: review?(cpearce) → review+
Comment on attachment 8993086 [details]
Bug 1476701 - notify observer when audible autoplay occurred.

https://reviewboard.mozilla.org/r/257866/#review267560

Sorry, I don't see a way to cut out one of those steps.
Attachment #8993086 - Flags: review- → review+
Flags: needinfo?(jaws)
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92cbea18e430
notify observer when audible autoplay occurred. r=cpearce,jaws
https://hg.mozilla.org/mozilla-central/rev/92cbea18e430
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: