Closed Bug 1274919 Opened 8 years ago Closed 7 years ago

Resume suspended video decoders on tab mouse hover.

Categories

(Core :: Audio/Video: Playback, defect, P1)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: u480271, Assigned: alwu)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

Bug 1274919 - part1 : send the msg "Browser:UnselectedTabHover" when the cursor is hovering over or leaving the unselected tab
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1274919 - part2 : implement resume/suspend mechanism in MediaDecoder.
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1274919 - part3 : only send msg if someone is waiting for it.
59 bytes, text/x-review-board-request
jwwang
: review+
mikedeboer
: review+
Details
Bug 1274919 - part5 : add test.
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1274919 - part4 : add telemetry probe to measure how long the cursor is hovering before opening the tab.
59 bytes, text/x-review-board-request
benjamin
: review+
mikedeboer
: review+
Details 
To cover the time it takes for video decoder to spin up after being suspended in a background tab, start the video decoder when mouse hovers over the tab associated with media elements.
Assignee: nobody → dglastonbury
Status: NEW → ASSIGNED
Notify outer window when the mouse begin/ends hovering over the window's
tab.

Review commit: https://reviewboard.mozilla.org/r/54564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54564/
Attachment #8755315 - Flags: review?(mconley)
Attachment #8755315 - Flags: review?(bugs)
Attachment #8755316 - Flags: review?(cpearce)
Mike,

Not sure if you're a good person to review JS and XUL changes. If not, please suggest somebody more appropriate. Thanks.
Flags: needinfo?(mconley)
Comment on attachment 8755315 [details]
Bug 1274919 - Part 1: Notify window when mouse hovers over tab.

https://reviewboard.mozilla.org/r/54564/#review51160

I'm not familiar with what all media stuff this is supposed to spin up, but could we do something to cases when tab is switched using keyboard?

::: dom/base/nsGlobalWindow.cpp:4014
(Diff revision 1)
>    }
>    return topWindow->mServiceWorkersTestingEnabled;
>  }
>  
>  bool
> +nsPIDOMWindowOuter::GetTabHovered() const

So since the flag is ever set on topmost content window only, the flag is always false in iframes and so. For consistency it would be nice if GetTabHovered checked the top-window's state and returned that.
Because of this, r-

Or if you do want to return false in iframes, please explain why and ask review again.

::: dom/base/nsGlobalWindow.cpp:4035
(Diff revision 1)
> +
> +  if (mTabHovered == aHovered) {
> +    return;
> +  }
> +
> +  mTabHovered = aHovered;

I wonder if we should somehow ensure that only one of the background tabs may have the flag set, at least per process (multiple child processes would make such setup harder).

::: dom/base/nsPIDOMWindow.h:706
(Diff revision 1)
>  
>    // Let the service workers plumbing know that some feature are enabled while
>    // testing.
>    bool mServiceWorkersTestingEnabled;
> +
> +  // True if the mouse if the tab associated with the window is hovered.

Something wrong with the comment.

::: dom/interfaces/base/nsIDOMWindowUtils.idl:1877
(Diff revision 1)
>     * was last provided is what will be used.
>     */
>    void respectDisplayPortSuppression(in boolean aEnabled);
> +
> +  /**
> +   */

Please document this attribute.
Component: Audio/Video → Audio/Video: Playback
Comment on attachment 8755316 [details]
MozReview Request: Bug 1274919 - Part 2: Start decoding suspended video when mouse begins hovering tab. r?cpearce

https://reviewboard.mozilla.org/r/54566/#review51370
Attachment #8755316 - Flags: review?(cpearce) → review+
I think I'd like some more background on this bug. I can tell this is an attempt at a performance optimization. Is there a particular benchmark we're trying to hit? Is there data on how slow spinning up these decoders are in practice, and how much we might end up shaving off with this optimization?
Flags: needinfo?(mconley) → needinfo?(dglastonbury)
https://reviewboard.mozilla.org/r/54564/#review51160

Currently suspended video is started again when the window becomes visible. If the mouse is used to switch tabs, we can get notification a split-second earlier. I don't know if we could do the same with keyboard switching.

> So since the flag is ever set on topmost content window only, the flag is always false in iframes and so. For consistency it would be nice if GetTabHovered checked the top-window's state and returned that.
> Because of this, r-
> 
> Or if you do want to return false in iframes, please explain why and ask review again.

You are correct. iframes will need to check the topmost window so that suspended videos in iframes work correctly.

> I wonder if we should somehow ensure that only one of the background tabs may have the flag set, at least per process (multiple child processes would make such setup harder).

Should I add a debug-only global variable that tracks the window that is currently "tab hovered"? This would depend upon event delivery order to ensure that when the mouse moves from one tab to another that "unhover" arrived before the next "hover".
Comment on attachment 8755315 [details]
Bug 1274919 - Part 1: Notify window when mouse hovers over tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54564/diff/1-2/
Attachment #8755315 - Flags: review?(bugs)
Comment on attachment 8755316 [details]
MozReview Request: Bug 1274919 - Part 2: Start decoding suspended video when mouse begins hovering tab. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54566/diff/1-2/
Attachment #8755315 - Flags: review?(bugs) → review+
fwiw, the JS and XUL looks okay to me, but still waiting on information for comment 6 before setting a review flag.
Comment on attachment 8755315 [details]
Bug 1274919 - Part 1: Notify window when mouse hovers over tab.

Clearing review request until I have more information for comment 6.
Attachment #8755315 - Flags: review?(mconley)
Blocks: 1276556
Flags: needinfo?(dglastonbury)
Mass change P2 -> P3
Priority: P2 → P3
(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #6)
> I think I'd like some more background on this bug. I can tell this is an
> attempt at a performance optimization. Is there a particular benchmark we're
> trying to hit? Is there data on how slow spinning up these decoders are in
> practice, and how much we might end up shaving off with this optimization?

Hello Mike, 
So, we tried to suspend video decoders for invisible video elements (bug 1276556) for optimizing resource usage. While the invisible video elements become visible again, we need to resume their original video decoders which takes time and you're right that this bug is going to stealing some time for resuming.

Here, we have some data for how much time it takes to resume a video decoder.
In general, around half video decoders could be resumed in less than 200 msec, around 80% are less than 500 msec and around 93% are less than 1 sec. In fact, the resuming time depends on the video resolution, for high resolution videos, it takes more time to resume its video decoder. You see, for videos larger than 720P, there is a obvious drop.

+----------------+---------+----------+----------+----------+-----------+-----------+-----------+
| resuming time  | < 50 ms | < 101 ms | < 203 ms | < 516 ms | < 1040 ms | < 2100 ms | < 4950 ms |
+================|=========|==========|==========|==========|===========|===========|===========|
| All resolution |    8.66 |    27.56 |    54.41 |    81.55 |     93.13 |     97.86 |     99.44 |
+----------------|---------|----------|----------|----------|-----------|-----------|-----------|
|      < 240P    |   10.86 |    30.76 |    58.25 |     83.4 |     93.24 |     97.38 |     99.17 |
+----------------|---------|----------|----------|----------|-----------|-----------|-----------|
| 241P ~ 480P    |    8.25 |    28.19 |    55.41 |    82.74 |     93.82 |     98.16 |     99.55 |
+----------------|---------|----------|----------|----------|-----------|-----------|-----------|
| 481P ~ 720P    |    3.69 |    18.82 |    46.59 |    78.29 |     92.14 |     97.88 |     99.57 |
+----------------|---------|----------|----------|----------|-----------|-----------|-----------|
| 721P ~ 1080P   |    1.36 |     9.89 |    31.06 |    67.04 |     88.33 |     97.29 |     99.67 |
+----------------+---------+----------+----------+----------+-----------+-----------+-----------+

For more detailed data, please refer to this telemetry: https://mzl.la/2dWU4Ex

We don't really have a benchmark goal but I think this optimization does help the user experience, especially for high resolution videos. Not sure whether the provided data is enough for you to reconsider reviewing the 1st patch (comment 12)?
Flags: needinfo?(mconley)
I just wanted to chime in and say that I saw this needinfo, and I'm currently trying to dig myself out from a mountain of needinfo's, and I'll get to this tomorrow. Sorry for the delay!
Hm, so somewhere between 1 and 2 seconds to get the 95th percentile for all resolutions.

I guess this kind of micro-optimization can help, though I'm really not sure how much time there generally is between the user hovering a tab and a user clicking on it.

Anyhow, I think a half-second between a hover and a click isn't outlandish, and that'd give us something like the 80% percentile, which isn't too bad.

Out of curiosity, will spinning up a decoder block the main thread in the content process? I only ask because we're attempting to optimize tab switch times at the moment, and I'd hate to add more to the main thread during this critical time for the user.

I'm also kinda overloaded on reviews right now. I'm hoping mikedeboer doesn't mind if I redirect this his way.
Flags: needinfo?(mconley) → needinfo?(kaku)
Attachment #8755315 - Attachment description: MozReview Request: Bug 1274919 - Part 1: Notify window when mouse hovers over tab. r?mconley,smaug → Bug 1274919 - Part 1: Notify window when mouse hovers over tab.
Attachment #8755315 - Flags: review?(mdeboer)
(In reply to Mike Conley (:mconley) - (high latency on reviews and needinfos) from comment #16)
> Out of curiosity, will spinning up a decoder block the main thread in the
> content process? 
No for almost all cases. The decoder resuming is designed as an async operation.

There is one rare case that we need to block the main thread, that is if an already-suspended video element is used as the content source. In specific, that is an already-suspended video element is used as the source of drawImage(), createImageBitmap() and mozCaptureStream() APIs. The video element will then be marked as never suspend its video decoder again. (Bug 1295921 and Bug 1284389)

However, I think that there are few possibilities that an already-suspended video element is undertaking its first time been drawn onto a canvas while users are switching the tab. 

@Dan, please correct me if I was wrong.
Flags: needinfo?(kaku) → needinfo?(dglastonbury)
Comment on attachment 8755315 [details]
Bug 1274919 - Part 1: Notify window when mouse hovers over tab.

https://reviewboard.mozilla.org/r/54564/#review84512

r=me with comments addressed.
Are you writing a mochitest-browser test for this? I think that'd be very beneficial to make sure this feature a) works as expected and b) doesn't regress over time.

::: browser/base/content/tab-content.js:914
(Diff revision 2)
> +  if (!content) {
> +    return;
> +  }
> +  let windowUtils = content.QueryInterface(Ci.nsIInterfaceRequestor)
> +                           .getInterface(Ci.nsIDOMWindowUtils);
> +  if (!windowUtils) {

If `windowUtils` can be falsy, we're going to have a lot of problems in our code elsewhere. No need to check it, so you can remove the if-statement here.

::: browser/base/content/tabbrowser.xml:6370
(Diff revision 2)
>              }
>            }
>  
>            tabContainer._hoveredTab = this;
> +
> +          let browser = tabContainer.tabbrowser.getBrowserForTab(this);

Please use `this.linkedBrowser.messageManager.sendAsyncMessage("Browser:TabHover", { isHovered: true })`

::: browser/base/content/tabbrowser.xml:6390
(Diff revision 2)
>            }
>  
>            tabContainer._hoveredTab = null;
> +
> +          let browser = tabContainer.tabbrowser.getBrowserForTab(this);
> +          browser.messageManager.sendAsyncMessage("Browser:TabHover", { isHovered: false })

Same here!
Attachment #8755315 - Flags: review?(mdeboer) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #18)
> Comment on attachment 8755315 [details]
> Bug 1274919 - Part 1: Notify window when mouse hovers over tab.
> 
> https://reviewboard.mozilla.org/r/54564/#review84512
> 
> r=me with comments addressed.
> Are you writing a mochitest-browser test for this? I think that'd be very
> beneficial to make sure this feature a) works as expected and b) doesn't
> regress over time.

I wrote these tests by grepping tests that worked with tabs. If you have some advice on what kind of test this should be, with in-tree example to crib from if possible, I'd greatly appreciate the input.
 
> ::: browser/base/content/tab-content.js:914
> (Diff revision 2)
> > +  if (!content) {
> > +    return;
> > +  }
> > +  let windowUtils = content.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                           .getInterface(Ci.nsIDOMWindowUtils);
> > +  if (!windowUtils) {
> 
> If `windowUtils` can be falsy, we're going to have a lot of problems in our
> code elsewhere. No need to check it, so you can remove the if-statement here.

Thanks. I copied the code from a similiar function I found.
Flags: needinfo?(dglastonbury)
No longer blocks: 1276556
Flags: needinfo?(bwu)
Alastor, 
This is a urgent bug. We need your help to check it. You may need to talk with Dan.
Flags: needinfo?(bwu) → needinfo?(alwu)
It seems Dan have finished all the task, why don't we land those patches?
Flags: needinfo?(alwu) → needinfo?(dglastonbury)
I believe these patches require rebase and may need some modifications. Dan has been away from shutdown decoder for a while. It would be better to have alwu/kaku to take over this. 

alwu and kaku,
Please discuss it to see who is more interested in taking this bug.
Flags: needinfo?(dglastonbury)
Assignee: dglastonbury → alwu
Priority: P3 → P1
We would introduce new mechanism for resuming/suspending video decoding.
If the cursor is hovering on the unselected tab, we would immediately resume video decoding.
However, if the cursor doesn't click the tab and leave the tab afterward, we would automatically suspend video decoding again after the specific timeout.

I'm still working on testing my patches, will upload them soon.
Writing the test, but I can't simulate the cursor hovering over the tab using EventUtils.synthesizeMouse(). I'm still trying to make it work.
Depends on: 1029451
Attachment #8755315 - Attachment is obsolete: true
Attachment #8755316 - Attachment is obsolete: true
Next step is to add the telemetry probe to record the time how long the cursor is hovering before opening the tab.
Comment on attachment 8884745 [details]
Bug 1274919 - part4 : add telemetry probe to measure how long the cursor is hovering before opening the tab.

https://reviewboard.mozilla.org/r/155590/#review160746

data-review=me with the process issue fixed. I did not do a code review.

::: commit-message-ef850:6
(Diff revision 1)
> +Bug 1274919 - part5 : add telemetry probe to measure how long the cursor is hovering before opening the tab.
> +
> +Measure the time how long the cursor is hovering before opening the tab which
> +contains suspended playing media.
> +
> +If the cursor doesn't open the tab, and leaves tab after passing specific time

This doesn't block landing, but do you want to also measure how often/long people mouse over tabs but don't activate them?  That would likely inform your false-positive rate for this feature.

::: toolkit/components/telemetry/Histograms.json:13636
(Diff revision 1)
>      "n_buckets": 20,
>      "keyed": true,
>      "description": "Measures the number of milliseconds we spend synchronously notifying observers, keyed by topic. Note: only NotifyObservers calls which take over 500 microseconds are included in this probe."
> +  },
> +  "HOVER_UNTIL_MEDIA_TAB_OPENED": {
> +    "record_in_processes": ["main", "content"],

As a frontend metric, this should be the main process only.
Attachment #8884745 - Flags: review?(benjamin) → review+
Comment on attachment 8884185 [details]
Bug 1274919 - part2 : implement resume/suspend mechanism in MediaDecoder.

https://reviewboard.mozilla.org/r/155082/#review161184

You're coupling "register/unregister observer" with "suspend/resume decoder" and that's why you need to introduce TEMPORARY_RESUME state.

You see, whenever a tab is in background, its related decoders should always listen to the "background-video-decoding" event, and so only when a tab is going to foreground could its decoders disconnect to the "background-video-decoding" event. To prove this, you can see that in your mechanism, a background tab's decoder is only possible to get a TEMPORARY_RESUME state, but never get a NONE state which is the only situation could a observer be unregistered.

So, please try to decouple the Observer out of the Decoder, you could leverage HTMLMediaElement::NotifyOwnerDocumentActivityChanged() to register/unregister observer. Following the idea, you might need to keep the Observer in the element.

::: dom/media/MediaDecoder.h:117
(Diff revision 2)
> +  enum class SuspendDecodingState : uint8_t
> +  {
> +    NONE,
> +    TEMPORARY_RESUME,
> +    SUSPEND
> +  };

Looks like we don't need the TEMPORARY_RESUME mode..., see the overall comment.

::: dom/media/MediaDecoder.cpp:152
(Diff revision 2)
> +    NS_DECL_ISUPPORTS
> +
> +    explicit VideoDecodingRecoveringObserver(MediaDecoder* aDecoder)
> +      : mDecoder(aDecoder)
> +      , mIsRegisteredForEvent(false)
> +    {}

Better to assert mDecoder here.

::: dom/media/MediaDecoder.cpp:183
(Diff revision 2)
> +      MOZ_ASSERT(mIsRegisteredForEvent);
> +      nsCOMPtr<nsIObserverService> observerService = services::GetObserverService();
> +      if (observerService) {
> +        observerService->RemoveObserver(this, "background-video-decoding");
> +        mIsRegisteredForEvent = false;
> +        mDecoder->mResumeBackgroundVideoDecoding = false;

Shouldn't we also call `mDecoder->UpdateVideoDecodeMode();` here?

::: dom/media/MediaDecoder.cpp:1253
(Diff revision 2)
> +  SuspendDecodingState state = GetSuspendDecodingState();
> +  UpdateDecodingRecoveringObserver(state);

You see, the Observer is designed to send information to decide the suspend/resume state. But, here, you're using the suspend/resume state to update the observer's internal state. Personally, I think the current mechanism works, but I would like to decouple the bi-directional relationship into simple ones if you agree on the overall comment.

::: dom/media/MediaDecoder.cpp:1256
(Diff revision 2)
> +  VideoDecodeMode mode = state == SuspendDecodingState::SUSPEND ?
> +    VideoDecodeMode::Suspend : VideoDecodeMode::Normal;

Please follow the coding style: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

```
VideoDecodeMode mode = state == SuspendDecodingState::SUSPEND 
                       ? VideoDecodeMode::Suspend 
                       : VideoDecodeMode::Normal;
```
Attachment #8884185 - Flags: review?(kaku) → review-
Attachment #8884187 - Flags: review?(amarchesini) → review+
Comment on attachment 8884186 [details]
Bug 1274919 - part3 : only send msg if someone is waiting for it.

https://reviewboard.mozilla.org/r/155084/#review161548

Per offline discussion, please move the VideoDecodingRecoveringObserver to HTMLMedialElemnt because we don't want codes in MediaDacoder depends on its owner's type.
Attachment #8884186 - Flags: review?(kaku)
Comment on attachment 8884187 [details]
Bug 1274919 - part5 : add test.

https://reviewboard.mozilla.org/r/155086/#review161550

Per offline discussion, mozentervideosuspend/mozexitvideosuspend should be enough (hopefully) to write this test. Let's try it.
Attachment #8884187 - Flags: review?(kaku)
(In reply to Tzuhao Kuo [:kaku] from comment #40)
> Per offline discussion, please move the VideoDecodingRecoveringObserver to
> HTMLMedialElemnt because we don't want codes in MediaDacoder depends on its
> owner's type.

After some thought, I would still prefer to keep the observer on the MediaElement.
Actually, we have the API like MediaDecoderOwner::GetDocument(), so we don't need to depend on owner's type even the observer is on the MediaDecoder.

Other reasons are listed on following.

---

The first reason is about the MediaDecoder API. 

If we won't introduce the concept "suspend video decoding" to MediaElement, the observer would be used to notify "hovered tab" state. It means MediaDecoder needs to create new API which is used to notify the changed.

But think about that, if we change the resume condition, we need to change the API name again. eg. not just resume hovered tab, but resume the tabs in around. In addition, if we want to do the same thing on Fennec, we won't depend on the same condition so it would need another new API.

I would prefer to hide these resuming details in the MediaElement level, and let MediaDecoder to handle all related things.

---

Second is about the logic consideration when we unregistered the observer.

We would unregister it when (1) the tab goes to foreground (2) the video won't be suspended anymore (unbind from tree, mark as tainted).

In logic, I think it's not the good place to unregister the observer when we mark MediaElement as tainted. These two concepts has no relationship at all, when considering the observer is used to listen the tab hover state.

(In reply to Tzuhao Kuo [:kaku] from comment #38)
> ::: dom/media/MediaDecoder.h:117
> (Diff revision 2)
> > +  enum class SuspendDecodingState : uint8_t
> > +  {
> > +    NONE,
> > +    TEMPORARY_RESUME,
> > +    SUSPEND
> > +  };
> 
> Looks like we don't need the TEMPORARY_RESUME mode..., see the overall
> comment.

Yes, I'll remove it.

> ::: dom/media/MediaDecoder.cpp:1253
> (Diff revision 2)
> > +  SuspendDecodingState state = GetSuspendDecodingState();
> > +  UpdateDecodingRecoveringObserver(state);
> 
> You see, the Observer is designed to send information to decide the
> suspend/resume state. But, here, you're using the suspend/resume state to
> update the observer's internal state. Personally, I think the current
> mechanism works, but I would like to decouple the bi-directional
> relationship into simple ones if you agree on the overall comment.

Ok, I'll remove this bi-directional relationship.
Flags: needinfo?(kaku)
(In reply to Benjamin Smedberg [:bsmedberg] from comment #37)
> ::: commit-message-ef850:6
> (Diff revision 1)
> > +Bug 1274919 - part5 : add telemetry probe to measure how long the cursor is hovering before opening the tab.
> > +
> > +Measure the time how long the cursor is hovering before opening the tab which
> > +contains suspended playing media.
> > +
> > +If the cursor doesn't open the tab, and leaves tab after passing specific time
> 
> This doesn't block landing, but do you want to also measure how often/long
> people mouse over tabs but don't activate them?  That would likely inform
> your false-positive rate for this feature.

If the tab didn't be opened, the data won't be recorded. So I think this probe should be enough.
Comment on attachment 8884187 [details]
Bug 1274919 - part5 : add test.

https://reviewboard.mozilla.org/r/155086/#review162510
Attachment #8884187 - Flags: review?(amarchesini) → review+
Comment on attachment 8884184 [details]
Bug 1274919 - part1 : send the msg "Browser:UnselectedTabHover" when the cursor is hovering over or leaving the unselected tab

https://reviewboard.mozilla.org/r/155080/#review162516

This is going in the right direction, thanks Alastor! It does need a number of changes, please read them below.

I apologize for the delay, there were many things going on lately and my queue was temporarily swamped. All good again, now ;)

::: browser/base/content/tabbrowser.xml:7519
(Diff revision 3)
>        once the node is gone from the DOM, which causes us to think the tab is not
>        closed, which causes us to make wrong decisions. So we use an expando instead.
>        <field name="closing">false</field>
>        -->
>  
> +      <method name="resumeBackgroundVideoWhenTabIsHoveringByCursor">

Why did you give the tabbrowser and browser binding methods different names?

Please remove the two methods and change to:

```xbl
<method name="maybeToggleBackgroundVideo">
  <param name="aAction">
  <body><![CDATA[
    // Do nothing for lazy browsers.
    if (!this.linkedPanel) {
      return;
    }
    this.linkedBrowser.maybeToggleBackgroundVideo(aAction);
  </body>
</method>
```

And do the same for the methods in the browser binding.

::: modules/libpref/init/all.js:456
(Diff revision 3)
>  // video decoders. Defaults to 10 seconds.
>  pref("media.suspend-bkgnd-video.delay-ms", 10000);
> +// If the cursor is hovering over the tab which has suspended playing video,
> +// we would resume it in advance to avoid users see the incorrect video frame
> +// after opening the tab.
> +pref("media.suspend-bkgnd-video.resume-when-tab-is-hovering-by-cursor", true);;

nit: please rename the pref to 'resume-on-tabhover'

::: toolkit/content/widgets/browser.xml:853
(Diff revision 3)
>              }
>            ]]>
>          </body>
>        </method>
>  
> +      <method name="suspendBackgroundVideo">

For posterity, please change this to:

```xbl
<method name="maybeToggleBackgroundVideo">
  <param name="type">
  <body><![CDATA[
    if (!Services.prefs.getBoolPref("media.suspend-bkgnd-video.enabled") ||
        !Services.prefs.getBoolPref("media.suspend-bkgnd-video.resume-on-tabhover")) {
      return;
    }
    this.messageManager.sendAsyncMessage("Media:BackgroundVideoDecoding", { type });
  </body>
</method>
```

... or something similar.

::: toolkit/content/widgets/browser.xml:857
(Diff revision 3)
>  
> +      <method name="suspendBackgroundVideo">
> +        <body>
> +          <![CDATA[
> +            if (!Services.prefs.getBoolPref("media.suspend-bkgnd-video.enabled") ||
> +                !Services.prefs.getBoolPref("media.suspend-bkgnd-video.resume-when-tab-is-hovering-by-cursor")) {

Are we supporting changes to these prefs without restarting the browser?
Regardless, I'd like to see these prefs cached in fields on the binding.
Also, since this lives in toolkit/, please also add the relevant prefs to all.js. If you don't want to do that, or think it's unnecessary, you'll need the conditionals to tabbrowser.xml.
Attachment #8884184 - Flags: review?(mdeboer) → review-
Comment on attachment 8884186 [details]
Bug 1274919 - part3 : only send msg if someone is waiting for it.

https://reviewboard.mozilla.org/r/155084/#review162520

Looks good to me, but I'll need to look at this again when you made the changes to part 1 and rebased this patch on top of it.
Attachment #8884186 - Flags: review?(mdeboer)
Comment on attachment 8884745 [details]
Bug 1274919 - part4 : add telemetry probe to measure how long the cursor is hovering before opening the tab.

https://reviewboard.mozilla.org/r/155590/#review162522

I'd like to see this patch again in the next iteration.

::: browser/base/content/tabbrowser.xml:7411
(Diff revision 2)
>            this.parentNode.tabbrowser._tabAttrModified(this, ["visuallyselected"]);
>  
>            this._setPositionAttributes(val);
>  
> +          // Stop timer when the tab is opened.
> +          if (Services.prefs.getBoolPref("media.suspend-bkgnd-video.resume-when-tab-is-hovering-by-cursor") &&

This tells me that you really want to move this pref read out of the browser binding and into the tabbrowser one.
Attachment #8884745 - Flags: review?(mdeboer)
(In reply to Alastor Wu [:alwu][please needinfo? me] from comment #42)
> (In reply to Tzuhao Kuo [:kaku] from comment #40)
> > Per offline discussion, please move the VideoDecodingRecoveringObserver to
> > HTMLMedialElemnt because we don't want codes in MediaDacoder depends on its
> > owner's type.
> 
> After some thought, I would still prefer to keep the observer on the
> MediaElement.
> Actually, we have the API like MediaDecoderOwner::GetDocument(), so we don't
> need to depend on owner's type even the observer is on the MediaDecoder.
> 
> Other reasons are listed on following.
> 
> ---
> 
> The first reason is about the MediaDecoder API. 
> 
> If we won't introduce the concept "suspend video decoding" to MediaElement,
> the observer would be used to notify "hovered tab" state. It means
> MediaDecoder needs to create new API which is used to notify the changed.
> 
> But think about that, if we change the resume condition, we need to change
> the API name again. eg. not just resume hovered tab, but resume the tabs in
> around. In addition, if we want to do the same thing on Fennec, we won't
> depend on the same condition so it would need another new API.
> 
> I would prefer to hide these resuming details in the MediaElement level, and
> let MediaDecoder to handle all related things.
> 
> ---
> 
> Second is about the logic consideration when we unregistered the observer.
> 
> We would unregister it when (1) the tab goes to foreground (2) the video
> won't be suspended anymore (unbind from tree, mark as tainted).
> 
> In logic, I think it's not the good place to unregister the observer when we
> mark MediaElement as tainted. These two concepts has no relationship at all,
> when considering the observer is used to listen the tab hover state.
> 

My rough idea is that all the information needed to register/unregister the observer comes from the HTMLMediaElemet, so you should be able to keep the observer at the HTMLMediaElemet and have all the information be passed into a single policy method to make the decision. In this way, the policies of "register/unregister observer" and "suspend/resume decoder" are separated. 

Thanks JW for taking over the review, and so let's honor his opinion.
Flags: needinfo?(kaku) → needinfo?(jwwang)
Comment on attachment 8884184 [details]
Bug 1274919 - part1 : send the msg "Browser:UnselectedTabHover" when the cursor is hovering over or leaving the unselected tab

https://reviewboard.mozilla.org/r/155080/#review162744

::: toolkit/content/widgets/browser.xml:853
(Diff revision 3)
>              }
>            ]]>
>          </body>
>        </method>
>  
> +      <method name="suspendBackgroundVideo">

I don't like the change for I think it is bad to encode the video-decoding policy here. We should just propagate the mouse enter/leave events to the media element which can then change the video-decoding behavior according to the events.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #55)
> I don't like the change for I think it is bad to encode the video-decoding
> policy here. We should just propagate the mouse enter/leave events to the
> media element which can then change the video-decoding behavior according to
> the events.

The reason I don't want to propagate mouse enter/leave is on the following,

We don't need to propagate mouse enter/leave everytime, it's wasted. Everytime you propagate the event, it do the one ipc. If you don't always propagate these events, we shouldn't name them too general, its name should be more specific with our real purpose.
The cost should be negligible for mouse enter/leave events will fire at most 1 or 2 times per second. So I still don't want to pollute the tab/browser code with video-decoding specific policy.
Flags: needinfo?(jwwang)
(In reply to Mike de Boer [:mikedeboer] from comment #51)
> Why did you give the tabbrowser and browser binding methods different names?
>
> And do the same for the methods in the browser binding.

After offline discussed with JW, it's not proper to introduce the video decoding concept in front-end, we should only care about whether unselected tab is hovered or not.

So, I'll remove the tabbrowser's method.
 
> ::: modules/libpref/init/all.js:456
> nit: please rename the pref to 'resume-on-tabhover'

OK.

> ::: toolkit/content/widgets/browser.xml:857
> Are we supporting changes to these prefs without restarting the browser?
> Regardless, I'd like to see these prefs cached in fields on the binding.
> Also, since this lives in toolkit/, please also add the relevant prefs to
> all.js. If you don't want to do that, or think it's unnecessary, you'll need
> the conditionals to tabbrowser.xml.

Yes, we can change the pref without restarting the browser.

I'm planing to move the pref to gecko part, so we can skip those pref checking on front-end side.
Comment on attachment 8884185 [details]
Bug 1274919 - part2 : implement resume/suspend mechanism in MediaDecoder.

https://reviewboard.mozilla.org/r/155082/#review162800

::: dom/media/MediaDecoder.cpp:142
(Diff revision 4)
> +    explicit BackgroundVideoDecodingPermissionObserver(MediaDecoder* aDecoder)
> +      : mDecoder(aDecoder)
> +      , mIsRegisteredForEvent(false)
> +    {
> +      MOZ_ASSERT(mDecoder);
> +      Preferences::AddBoolVarCache(&sPrefResumeDecodingOnTabHover,

MediaPrefs will do that for you.

::: dom/media/MediaDecoder.cpp:164
(Diff revision 4)
> +
> +    void UpdateRegistration() {
> +      MOZ_ASSERT(mDecoder);
> +      // If the docuement is visible, always unregister event. If the docuement
> +      // is invisibe, depend on whether the decoding could be suspended.
> +      bool shouldNeverBeSuspended = mDecoder->mHasSuspendTaint ||

It is not good to mess up this observer with other policy code.

::: dom/media/MediaDecoder.cpp:216
(Diff revision 4)
> +        mDecoder->mIsBackgroundVideoDecodingAllowed = false;
> +        mDecoder->UpdateVideoDecodeMode();
> +      }
> +    }
> +
> +    bool IsValidEventSender(nsISupports* aSubject) const

Why do we need to varify the sender?

::: dom/media/MediaDecoder.cpp:1245
(Diff revision 4)
>  void
>  MediaDecoder::SetSuspendTaint(bool aTainted)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    mHasSuspendTaint = aTainted;
> +  mVideoDecodingOberver->UpdateRegistration();

It is not good to couple the observer with SuspendTaint. I would prefer to do registration on startup and deregistration on shutdown of MediaDecoder.

::: modules/libpref/init/all.js:453
(Diff revision 4)
>  // Whether to suspend decoding of videos in background tabs.
>  pref("media.suspend-bkgnd-video.enabled", true);
>  // Delay, in ms, from time window goes to background to suspending
>  // video decoders. Defaults to 10 seconds.
>  pref("media.suspend-bkgnd-video.delay-ms", 10000);
> +// If the cursor is hovering over the tab which has suspended playing video,

Fix the comment:
Resume video decoding when the cursor is hovering on a background tab to reduce the resume latency and improve the user experience.

::: modules/libpref/init/all.js:456
(Diff revision 4)
>  // video decoders. Defaults to 10 seconds.
>  pref("media.suspend-bkgnd-video.delay-ms", 10000);
> +// If the cursor is hovering over the tab which has suspended playing video,
> +// we would resume it in advance to avoid users see the incorrect video frame
> +// after opening the tab.
> +pref("media.suspend-bkgnd-video.resume-on-tabhover", true);;

The name is kinda weird. How about "media.resume.bkgnd-video.on.tabhover".
Attachment #8884185 - Flags: review?(jwwang) → review-
Comment on attachment 8884185 [details]
Bug 1274919 - part2 : implement resume/suspend mechanism in MediaDecoder.

https://reviewboard.mozilla.org/r/155082/#review163350
Attachment #8884185 - Flags: review?(jwwang) → review+
Comment on attachment 8884186 [details]
Bug 1274919 - part3 : only send msg if someone is waiting for it.

https://reviewboard.mozilla.org/r/155084/#review163352

::: browser/base/content/tabbrowser.xml:7553
(Diff revision 5)
>              }
>            }
>  
>            tabContainer._hoveredTab = this;
> -          if (!this.selected) {
> +          if (!this.selected &&
> +              this.linkedBrowser.shouldHandleUnselectedTabHover) {

1. mikedeboer should review this change.
2. shouldHandleUnselectedTabHover should checked in unselectedTabHover() instead of here.

::: dom/media/MediaDecoder.cpp:186
(Diff revision 5)
>    private:
>      ~BackgroundVideoDecodingPermissionObserver() {
>        MOZ_ASSERT(!mIsRegisteredForEvent);
>      }
>  
> +    void NotifyRegisteredEvent() const

I would like to call it "EnableEvent". Please also rename other strings accordinly.

::: toolkit/content/browser-content.js:1058
(Diff revision 5)
> +    }
> +  },
> +  handleEvent(aEvent) {
> +    let name = "UnselectedTabHoverMsg:";
> +    switch (aEvent.type) {
> +      case "notify-unselected-tab-hover-registered":

Why don't you choose the same event name so you won't need this switch?

::: toolkit/content/widgets/browser.xml:1114
(Diff revision 5)
>                this.activeMediaBlockStopped();
>                break;
>              case "AudioPlayback:MediaBlockStop":
>                this.mediaBlockStopped();
>                break;
> +            case "UnselectedTabHoverMsg:Registered":

1. I prefer "UnselectedTabHoverMsg:Enabled/Disabled".
2. We should disable the event only when the last listener is gone.
Attachment #8884186 - Flags: review?(jwwang) → review-
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #71)
> ::: toolkit/content/browser-content.js:1058
> (Diff revision 5)
> > +    }
> > +  },
> > +  handleEvent(aEvent) {
> > +    let name = "UnselectedTabHoverMsg:";
> > +    switch (aEvent.type) {
> > +      case "notify-unselected-tab-hover-registered":
> 
> Why don't you choose the same event name so you won't need this switch?

How can we use one event to distinguish registration and deregistration?

> ::: toolkit/content/widgets/browser.xml:1114
> (Diff revision 5)
> >                this.activeMediaBlockStopped();
> >                break;
> >              case "AudioPlayback:MediaBlockStop":
> >                this.mediaBlockStopped();
> >                break;
> > +            case "UnselectedTabHoverMsg:Registered":
> 
> 1. I prefer "UnselectedTabHoverMsg:Enabled/Disabled".
> 2. We should disable the event only when the last listener is gone.

Ah, yes, we should count how many listeners we have.
Comment on attachment 8884186 [details]
Bug 1274919 - part3 : only send msg if someone is waiting for it.

https://reviewboard.mozilla.org/r/155084/#review163352

> Why don't you choose the same event name so you won't need this switch?

I mean you can just call sendAsyncMessage(aEvent.type).
Comment on attachment 8884186 [details]
Bug 1274919 - part3 : only send msg if someone is waiting for it.

https://reviewboard.mozilla.org/r/155084/#review163414

::: dom/media/MediaDecoder.cpp:203
(Diff revision 6)
> +    }
> +
> +    void DisableEvent() const
> +    {
> +      nsCOMPtr<nsPIDOMWindowOuter> win = GetOwnerWindow();
> +      if (!win) {

If the window is gone before MediaDecoder shutdown, we will never be able to disable the event and _unselectedTabHoverMsgListenersNum will never be balanced.

Please check this issue with a DOM guy.

::: toolkit/content/browser-content.js:1046
(Diff revision 6)
>  };
>  AudioPlaybackListener.init();
>  
> +var unselectedTabHoverObserver = {
> +  init() {
> +    addMessageListener("Browser:UnselectedTabHover", this);

Can't we just do |addMessageListener("Browser:UnselectedTabHover", message => {| as before?

::: toolkit/content/browser-content.js:1047
(Diff revision 6)
>  AudioPlaybackListener.init();
>  
> +var unselectedTabHoverObserver = {
> +  init() {
> +    addMessageListener("Browser:UnselectedTabHover", this);
> +    addEventListener("UnselectedTabHoverMsg:Enabled", this);

Ditto. We should be able to do without using a global object (unselectedTabHoverObserver).
Attachment #8884186 - Flags: review?(jwwang) → review+
Comment on attachment 8884184 [details]
Bug 1274919 - part1 : send the msg "Browser:UnselectedTabHover" when the cursor is hovering over or leaving the unselected tab

https://reviewboard.mozilla.org/r/155080/#review163876

::: browser/base/content/tabbrowser.xml:7552
(Diff revision 6)
>                candidate.setAttribute("afterhovered", "true");
>              }
>            }
>  
>            tabContainer._hoveredTab = this;
> +          if (!this.selected) {

If you're doing this unconditionally, you'll flood the message manager because the huge amount mousemove events that are fired when hovering a tab.
There are two things you can do here:

1. Throttle the amount of messages sent to content process using a timer (least favorable).
2. Before the `tabContainer._hoveredTab` setter above, check `if (tabContainer._hoveredTab != this) { /* do stuff */ }`, because then it will not send a message again for a subsequent mousemove event on the same tab.

Also, please test your chosen method locally.

Another condition you'll need to add a guard for is `this.linkedPanel`, which indicates if the browser element is already inserted into the document.

Example:
```js
if (this.linkedPanel && tabContainer._hoveredTab != this) {
  this.linkedBrowser.unselectedTabHover(true);
}
tabContainer._hoveredTab = this;
```

::: toolkit/content/browser-content.js:966
(Diff revision 6)
>      Cu.reportError("WebChannel message failed. No message data.");
>    }
>  });
>  
> +addMessageListener("Browser:UnselectedTabHover", message => {
> +  Services.obs.notifyObservers(content.window, "unselected-tab-hover", message.data.isHovered);

If you make this just `hovered` instead of `isHovered`, you can...

::: toolkit/content/widgets/browser.xml:859
(Diff revision 6)
> +        <parameter name="hovered"/>
> +        <body>
> +          <![CDATA[
> +            // TODO : only dispatch event when someone is waiting for this message
> +            this.messageManager.sendAsyncMessage("Browser:UnselectedTabHover",
> +              { isHovered: hovered ? "true" : "false" });

...write this as `{ hovered }`.
Attachment #8884184 - Flags: review?(mdeboer) → review-
Attachment #8884186 - Flags: review?(mdeboer)
Attachment #8884745 - Flags: review?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #80)
> Comment on attachment 8884184 [details]
> Bug 1274919 - part1 : send the msg "Browser:UnselectedTabHover" when the
> cursor is hovering over or leaving the unselected tab
> 
> https://reviewboard.mozilla.org/r/155080/#review163876
> 
> ::: browser/base/content/tabbrowser.xml:7552
> (Diff revision 6)
> >                candidate.setAttribute("afterhovered", "true");
> >              }
> >            }
> >  
> >            tabContainer._hoveredTab = this;
> > +          if (!this.selected) {
> 
> If you're doing this unconditionally, you'll flood the message manager
> because the huge amount mousemove events that are fired when hovering a tab.
> There are two things you can do here:

This issue has been handled in patch3, the message won't be sent arbitrarily. It would only be sent when we mare sure someone is listening for it. 

> Another condition you'll need to add a guard for is `this.linkedPanel`,
> which indicates if the browser element is already inserted into the document.

OK.
(In reply to Mike de Boer [:mikedeboer] from comment #80)
> 2. Before the `tabContainer._hoveredTab` setter above, check `if
> (tabContainer._hoveredTab != this) { /* do stuff */ }`, because then it will
> not send a message again for a subsequent mousemove event on the same tab.

Why this condition is useful? I mean, the "mousemove" won't trigger the |_mouseenter()|.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #79)
> ::: dom/media/MediaDecoder.cpp:203
> If the window is gone before MediaDecoder shutdown, we will never be able to
> disable the event and _unselectedTabHoverMsgListenersNum will never be
> balanced.
> 
> Please check this issue with a DOM guy.

If the window is gone that means we closed the tab, and it also means we won't have a chance to enable the event again, and the _unselectedTabHoverMsgListenersNum won't be checked anymore.

So we don't need to care about the imbalance in this situation.

> ::: toolkit/content/browser-content.js:1047
> Ditto. We should be able to do without using a global object
> (unselectedTabHoverObserver).

In this file, other js objects also use this kind of style, I would like to keep the consistency in same file.
(In reply to Alastor Wu [:alwu][please needinfo? me][7/26-8/3 PTO] from comment #82)
> Why this condition is useful? I mean, the "mousemove" won't trigger the
> |_mouseenter()|.

Yuck, you were right. I misread 'mouseover' as 'mousemove'. It was late, my apology.
Comment on attachment 8884184 [details]
Bug 1274919 - part1 : send the msg "Browser:UnselectedTabHover" when the cursor is hovering over or leaving the unselected tab

https://reviewboard.mozilla.org/r/155080/#review164092

Thanks for the changes, Alastor. This LGTM.
Attachment #8884184 - Flags: review?(mdeboer) → review+
Comment on attachment 8884186 [details]
Bug 1274919 - part3 : only send msg if someone is waiting for it.

https://reviewboard.mozilla.org/r/155084/#review164098

This looks good to me, but I have too many nits and comments to r+ right away ;) I'm pretty sure next iteration will be good. If you have any question, please feel free to ask me on IRC as well!

::: dom/media/MediaDecoder.cpp:190
(Diff revision 7)
> +      if (!win) {
> +        return;
> +      }
> +      nsContentUtils::DispatchEventOnlyToChrome(
> +        GetOwnerDoc(), ToSupports(win),
> +        NS_LITERAL_STRING("UnselectedTabHoverMsg:Enabled"),

nit: Please call this `UnselectedTabHover:Enable`, because you want this to sound like an activating event.

::: dom/media/MediaDecoder.cpp:204
(Diff revision 7)
> +      if (!win) {
> +        return;
> +      }
> +      nsContentUtils::DispatchEventOnlyToChrome(
> +        GetOwnerDoc(), ToSupports(win),
> +        NS_LITERAL_STRING("UnselectedTabHoverMsg:Disabled"),

...and this one `UnselectedTabHover:Disable`.

::: toolkit/content/browser-content.js:1064
(Diff revision 7)
>      }
>    },
>  };
>  AudioPlaybackListener.init();
>  
> +var unselectedTabHoverObserver = {

nit: Please start this global variable with a capital 'U' --> 'UnselectedTabHoverObserver'

::: toolkit/content/browser-content.js:1070
(Diff revision 7)
> +  init() {
> +    addMessageListener("Browser:UnselectedTabHover", this);
> +    addEventListener("UnselectedTabHoverMsg:Enabled", this);
> +    addEventListener("UnselectedTabHoverMsg:Disabled", this);
> +  },
> +  receiveMessage(aMessage) {

nit: Please use modern argument naming, without the `a` prefix --> `receiveMessage(message) {`.

::: toolkit/content/browser-content.js:1071
(Diff revision 7)
> +    addMessageListener("Browser:UnselectedTabHover", this);
> +    addEventListener("UnselectedTabHoverMsg:Enabled", this);
> +    addEventListener("UnselectedTabHoverMsg:Disabled", this);
> +  },
> +  receiveMessage(aMessage) {
> +    if (aMessage.name == "Browser:UnselectedTabHover") {

nit: there's no need to filter for messages, because you added only one listener.

::: toolkit/content/browser-content.js:1075
(Diff revision 7)
> +  receiveMessage(aMessage) {
> +    if (aMessage.name == "Browser:UnselectedTabHover") {
> +      Services.obs.notifyObservers(content.window, "unselected-tab-hover", aMessage.data.hovered);
> +    }
> +  },
> +  handleEvent(aEvent) {

nit: s/aEvent/event/

::: toolkit/content/browser-content.js:1076
(Diff revision 7)
> +    if (aMessage.name == "Browser:UnselectedTabHover") {
> +      Services.obs.notifyObservers(content.window, "unselected-tab-hover", aMessage.data.hovered);
> +    }
> +  },
> +  handleEvent(aEvent) {
> +    sendAsyncMessage(aEvent.type);

Can you change this to one message type, called 'UnselectedTabHover:Toggle': `sendAsyncMessage("UnselectedTabHover:Toggle", { enable: event.type == ""UnselectedTabHover:Enable" });`

::: toolkit/content/widgets/browser.xml:853
(Diff revision 7)
>              }
>            ]]>
>          </body>
>        </method>
>  
> +      <method name="assert">

Please remove this debugging code.

::: toolkit/content/widgets/browser.xml:865
(Diff revision 7)
> +          ]]>
> +        </body>
> +      </method>
> +
> +      <!--
> +        Only send the msg "Browser:UnselectedTabHover" when someone requests

nit: s/msg/message/ and ', which can reduce...'

::: toolkit/content/widgets/browser.xml:868
(Diff revision 7)
> +
> +      <!--
> +        Only send the msg "Browser:UnselectedTabHover" when someone requests
> +        for the msg, it can reduce non-necessary communication.
> +      -->
> +      <field name="_registerUnselectedTabHoverMsg">false</field>

nit: I'd call this '_shouldSendUnselectedTabHover'

::: toolkit/content/widgets/browser.xml:870
(Diff revision 7)
> +        Only send the msg "Browser:UnselectedTabHover" when someone requests
> +        for the msg, it can reduce non-necessary communication.
> +      -->
> +      <field name="_registerUnselectedTabHoverMsg">false</field>
> +      <field name="_unselectedTabHoverMsgListenersNum">0</field>
> +      <property name="shouldHandleUnselectedTabHover"

If you are not using this property anywhere, please remove it.

::: toolkit/content/widgets/browser.xml:1018
(Diff revision 7)
>              this.messageManager.addMessageListener("AudioPlayback:Start", this);
>              this.messageManager.addMessageListener("AudioPlayback:Stop", this);
>              this.messageManager.addMessageListener("AudioPlayback:ActiveMediaBlockStart", this);
>              this.messageManager.addMessageListener("AudioPlayback:ActiveMediaBlockStop", this);
>              this.messageManager.addMessageListener("AudioPlayback:MediaBlockStop", this);
> +            this.messageManager.addMessageListener("UnselectedTabHoverMsg:Enabled", this);

With the change above, you can add one listener for 'UnselectedTabHover:Toggle'

::: toolkit/content/widgets/browser.xml:1126
(Diff revision 7)
>                this.activeMediaBlockStopped();
>                break;
>              case "AudioPlayback:MediaBlockStop":
>                this.mediaBlockStopped();
>                break;
> +            case "UnselectedTabHoverMsg:Enabled":

```js
case "UnselectedTabHover:Toggle":
  this._shouldSendUnselectedTabHover = data.enable ? 
    ++this._unselectedTabHoverMessageListenerCount > 0 :
    --this._unselectedTabHoverMessageListenerCount == 0
  break;
```

How does that look?
Attachment #8884186 - Flags: review?(mdeboer) → review-
Comment on attachment 8884745 [details]
Bug 1274919 - part4 : add telemetry probe to measure how long the cursor is hovering before opening the tab.

https://reviewboard.mozilla.org/r/155590/#review164102

r=me with comments addressed.

::: browser/base/content/tabbrowser.xml:5587
(Diff revision 6)
>            });
>          ]]>
>          </getter>
>        </property>
>        <field name="_soundPlayingAttrRemovalTimer">0</field>
> +      <field name="_hoverTabTimer">0</field>

Please initialize this as `null`.

::: browser/base/content/tabbrowser.xml:7614
(Diff revision 6)
> +      </method>
> +
> +      <method name="startUnselectedTabHoverTimer">
> +        <body><![CDATA[
> +          // Only record data when we need to.
> +          if (!this.linkedBrowser.shouldHandleUnselectedTabHover) {

Oh, this is where you're using the property. In that case, please add it to this patch instead - reduces possible confusion!

::: browser/base/content/tabbrowser.xml:7622
(Diff revision 6)
> +
> +          if (!TelemetryStopwatch.running("HOVER_UNTIL_UNSELECTED_TAB_OPENED", this)) {
> +            TelemetryStopwatch.start("HOVER_UNTIL_UNSELECTED_TAB_OPENED", this);
> +          }
> +
> +          if (this._hoverTabTimer != 0) {

nit: please change this to:
```js
if (this._hoverTabTimer) {
  clearTimeout(this._hoverTabTimer);
  this._hoverTabTimer = null;
}
```
Attachment #8884745 - Flags: review?(mdeboer) → review+
Comment on attachment 8884186 [details]
Bug 1274919 - part3 : only send msg if someone is waiting for it.

https://reviewboard.mozilla.org/r/155084/#review164632

LGTM. Thanks Alastor!
Attachment #8884186 - Flags: review?(mdeboer) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #98)
> Comment on attachment 8884186 [details]
> Bug 1274919 - part3 : only send msg if someone is waiting for it.
> 
> https://reviewboard.mozilla.org/r/155084/#review164632
> 
> LGTM. Thanks Alastor!

Very appreciate for your help :)
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0db83e6e2fac
part1 : send the msg "Browser:UnselectedTabHover" when the cursor is hovering over or leaving the unselected tab r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/2b788dca1f60
part2 : implement resume/suspend mechanism in MediaDecoder. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/ec8748c31ef3
part3 : only send msg if someone is waiting for it. r=jwwang,mikedeboer
https://hg.mozilla.org/integration/autoland/rev/04596e7f840b
part4 : add telemetry probe to measure how long the cursor is hovering before opening the tab. r=bsmedberg,mikedeboer
https://hg.mozilla.org/integration/autoland/rev/6ed469370daa
part5 : add test. r=baku
Depends on: 1383653
See Also: → 1385453
Depends on: 1378105
Depends on: 1416519
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: