Closed Bug 1347791 Opened 7 years ago Closed 7 years ago

The pinned tab shows play tab icon every time when restarts browser

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox55 + wontfix
firefox56 + verified

People

(Reporter: alwu, Assigned: alwu)

References

Details

Attachments

(5 files, 2 obsolete files)

Bug 1347791 - part1 : rename the attribute for the tab which has blocked the autoplay audio and would show the play-tab icon.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1347791 - part2 : the tab would always be resumed when it goes to foreground first time.
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1347791 - part3 : update block stauts to front-end side.
59 bytes, text/x-review-board-request
baku
: review+
mikedeboer
: review+
dao
: review+
Details
Bug 1347791 - part4 : keep tab's block state consistent after session restore.
59 bytes, text/x-review-board-request
mikedeboer
: review+
dao
: review+
Details
Bug 1347791 - part5 : only enable on desktop.
59 bytes, text/x-review-board-request
bwu
: review+
Details 
This bug is fork form bug1345947 comment5.

---

However, this situation is made by our design.

The play tab icon is used to notice user the tab was blocked but it was tried to produce some sound. It's not designed to remember which website shouldn't be blocked.

The main reason of this feature is to prevent unexpectedly playing multiple audio at the same time.
Ni mark for getting UX suggestion.
Flags: needinfo?(mliang)
Every new session should follow the same rule, which is blocking the media playback for tabs that are in the background. Remembering the websites that I have unblocked means we'll need to keep a list of exception websites, which is not in our current design.
Flags: needinfo?(mliang)
We all agree that the current behavior matches the design.

I'm arguing that the design is broken and we shouldn't ship it like this. I can't tell whether comment 2 disagrees with that, or is just restating the current design. I also don't understand which PM is in charge of the final decision here.
NI bwu/ryang, per comment3.
Flags: needinfo?(ryang)
Flags: needinfo?(bwu)
Ni PM Joe and Release Manager Gerry for syncup and further discussion.
Flags: needinfo?(ryang)
Flags: needinfo?(jcheng)
Flags: needinfo?(gchang)
Ni desktop PM Cindy for syncup and tracking , Thanks.
Flags: needinfo?(chsiang)
The decision was made with QA's sign off report. It seems QA didn't find/report this bug one week ago. At least, I don't see this in the report.  
IIUC, this could be annoying for users who like to restart their Firefox often and have not checked their newly-opened background tab with autoplay media inside.   

Alastor, 
I know this is not in our design scope, but would it be possible that we could fix it?
Flags: needinfo?(bwu) → needinfo?(alwu)
No, there's nothing related with QA, it's about our design concept.

First, the reason we implement the feature blocking autoplay is because we want to avoid multiple tab playing audio at the same time when user open new background tab. Every new background tab would be blocked until it has been visited at least once.

After that, new problem comes out, user might miss the the background's notification (ex. mail, irccloud, calendar) because the notification sound was also muted if user doesn't open that tab. Ideally, we should only block the media content (music, video), but not to block the notification sound.However, we can't distinguish which sound is notification sound or media content, we don't have such kinds of web API.

Therefore, we design the "play tab" icon, the icon is designed for notify user the background tab was blocked, so user won't be afraid of missing the important notification. Once the tab was blocked, the tab would show "play tab" icon. 

In this issue, when user restarts the firefox, all tabs would be blocked because user doesn't visit those tab, so showing the "play tab" icon is correct thing.

--- 

The question here is some users don't thinks this design is good, so we need to know who have the final decision about this feature. 

The solutions I can provide are 
(1) rethink the whole design 
The original design spec is in the bug1308399, but how to make sure the new design would be better than present one? (if we got same complain after shipping the new design....?)
    
(2) keep this design and ship it
For me, this design is good for my daily usage. It helps me to block the unexpected autoplay audio, and can notify me about missing notification.

(2) close this feature
Still block the background tab but remove the UX part. We can add new pref to close the UX part, that means no icon can notify user about the tab was blocked.
Flags: needinfo?(alwu) → needinfo?(bwu)
Alastor,
It is a very detailed feedback.
I think we don't need to redesign our feature since other part of this feature is great. we could just fix the current problem. Per comment 2, we should just remember what websites users have unblocked which we should add to our design scope. Mark, what do you think about adding it to our design?
Flags: needinfo?(bwu) → needinfo?(mliang)
Hi Blake, 

discussed with Alastor today,
At this moment, it might be infeasible to keep the list remembering which websites users have unblocked before.
May I suggest to ask for PM Cindy's support to evaluate our next step ?
Thank you very much !!
 


Hi Cindy, 
could you please provide feedback on this feature per comment8?
Because this feature is already enabled in 54 and should ride the train of release54, Please kindly let us know your view from PM's perspective.

Thank you very much !!
Flags: needinfo?(jcheng)
Flags: needinfo?(gchang)
(In reply to Rachelle Yang [:ryang][ryang@mozilla.com] from comment #10)
> Hi Blake, 
> 
> discussed with Alastor today,
> At this moment, it might be infeasible to keep the list remembering which
> websites users have unblocked before.
Yeah. it is not a technical issue. It is a UX issue. :-) 
> May I suggest to ask for PM Cindy's support to evaluate our next step ?
> Thank you very much !!
Sure. Just have a talk with Alastor via vidyo. I am with Alastor now. Personally, I like the current design. 
I would expect UX (Mark) and PM (Cindy) should talk to reach a consensus on this. :-)
FWIW, the current released version (Firefox 52) will play all the tabs with autoplay media playback after restart. Our current design looks better than that.
(In reply to Blake Wu [:bwu][:blakewu] from comment #11)
> (In reply to Rachelle Yang [:ryang][ryang@mozilla.com] from comment #10)
> > Hi Blake, 
> > 
> > discussed with Alastor today,
> > At this moment, it might be infeasible to keep the list remembering which
> > websites users have unblocked before.
> Yeah. it is not a technical issue. It is a UX issue. :-) 
> > May I suggest to ask for PM Cindy's support to evaluate our next step ?
> > Thank you very much !!
> Sure. Just have a talk with Alastor via vidyo. I am with Alastor now.
> Personally, I like the current design. 
> I would expect UX (Mark) and PM (Cindy) should talk to reach a consensus on
> this. :-)

Sorry for the late reply, I agree to keep the current design unless we have more time to rethink the design and check the feasibilities.

I could definitely come up with a whole new design for the situation in bug1345947 comment5 (https://bugzilla.mozilla.org/show_bug.cgi?id=1345947#c5), but I'm not sure if it fits our intention for this feature. And again there might be more concerns for the new design. Probably needs PM to define the scope here.
Flags: needinfo?(mliang)
I'd like to note that having "pinned" a tab generally means that a user has shown interest in the contents of that tab. 

For instance, I have three pinned tabs: a chat window, a mail window, and a calendar window. All of these can alert (or otherwise make noise) and generally, I'm ok with that. It's why those windows are pinned. If I have to visit the sites before one of the more useful aspects of pinning is realized, then I am not sure what the value of pinning really is?

In this case, I don't believe the browser would have to keep a list of specially permitted windows, since it already has one.
Can we clarify what this bug is requesting since I think there are two interpretations of what people are saying.

1. The feature should remember which websites have been unblocked so when they are visited in the future after a restart they can play audio automatically.
2. The feature should remember which tabs have been unblocked so when they are restored by session restore they can still play audio automatically.

Comment 2 and comment 10 appear to be talking about option 1 but I only care about option 2.

Option 2 should also be quite simple to implement compared to option 1.
Flags: needinfo?(benjamin)
(In reply to Dave Townsend [:mossop] from comment #15)
> Can we clarify what this bug is requesting since I think there are two
> interpretations of what people are saying.
> 
> 1. The feature should remember which websites have been unblocked so when
> they are visited in the future after a restart they can play audio
> automatically.
> 2. The feature should remember which tabs have been unblocked so when they
> are restored by session restore they can still play audio automatically.
> 
> Comment 2 and comment 10 appear to be talking about option 1 but I only care
> about option 2.
> 
> Option 2 should also be quite simple to implement compared to option 1.

Good point here. We could focus on option 2, though I'm not sure about feasibility. I like JR's idea about unblocking pinned tabs by default.
(In reply to Dave Townsend [:mossop] from comment #15)
> 2. The feature should remember which tabs have been unblocked so when they
> are restored by session restore they can still play audio automatically.

I also don't think it's good behavior.

I always have multiple youtube tabs in my list, they're all video. Because they're long video, so I won't finish them at once.

In this situation, I really don't want they automatically starts playing when I restarts browser, it would be very annoying.
But do you actually _pin_ youtube tabs? If so, can you describe your reasoning so we can understand this workflow better?
This bug can just be about the restoration of pinned tabs. I'm not sure there is a separate bug, although I will run some tests and report a new one if appropriate.
Flags: needinfo?(benjamin)
Sorry for the delay in my response.  I agree with the latest comments that pinned tabs seem like a clear signal that a user would be fine if those particular tabs played audio after a restore.  
Keeping the current design overall makes sense, but if pinned tabs were given an exception, it seems as though that would cover the two distinct sets of use cases.
(In reply to Panos Astithas [:past] (please needinfo?) from comment #18)
> But do you actually _pin_ youtube tabs? If so, can you describe your
> reasoning so we can understand this workflow better?

Sometime I would pin the youtube tab when I would like to watch that video later because the pinned tab is a good signal as reminder.
Another case I would pin the music-related tab is that I want to make the tab more easily to be found when I have lots tabs, it can help me conveniently mute/unmute that tab.
Might it be a reasonable work around to move music tabs to the start of a tab list? I note that under nightly, the speaker icon appears in the drop down list of hidden tabs. 

We don't have data on actual usage patterns that I'm aware of, and I'm uncomfortable with "Well, I use it this way..." as a justification. Is there a resource or a Test Pilot option we could take advantage of to find out how most customers want to use it?
(In reply to JR Conlin [:jrconlin,:jconlin] from comment #23)
> Might it be a reasonable work around to move music tabs to the start of a
> tab list? I note that under nightly, the speaker icon appears in the drop
> down list of hidden tabs. 

But the icon in the drop down list doesn't support for mutting/unmuting, it's just a icon.
Blocks: delay-autoplay
Summary: The tab shows play tab icon every time when restarts browser → The pinned tab shows play tab icon every time when restarts browser
I think we can think of pin tab as rather important to the users. Therefore, if I pinned Gmail tab and I've already visited that tab, the notification sound should stay unblocked, even after I restarted the browser + restored the tabs and the Gmail tab is still in the background.

But if a user opens a new tab in the background and pins that tab without visiting that site, the media playback should remain blocked after restoring the tab since it hasn't been unblocked/visited yet.
After offline discussed with Mark (our UX), we decided to take the pinned tab as a special case and it won't be blocked after session restore.
Assignee: nobody → alwu
Attachment #8855204 - Flags: review?(jaws)
Attachment #8855205 - Flags: review?(amarchesini)
Comment on attachment 8855205 [details]
Bug 1347791 - part2 : the tab would always be resumed when it goes to foreground first time.

https://reviewboard.mozilla.org/r/127072/#review130338
Attachment #8855205 - Flags: review?(amarchesini) → review+
Comment on attachment 8855204 [details]
Bug 1347791 - part1 : rename the attribute for the tab which has blocked the autoplay audio and would show the play-tab icon.

https://reviewboard.mozilla.org/r/127070/#review131156

::: browser/components/sessionstore/SessionStore.jsm:3554
(Diff revision 1)
> +      // We don't block the autoplay media for the pinned tab.
> +      browser.resumeMedia();

But media would be blocked in a pinned tab before the user restarts their browser?

If the tab has been unblocked, why can't we store that data in the tabData and then use tabData.mediaUnblocked to know if we should restore the media?
Attachment #8855204 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #31)
> But media would be blocked in a pinned tab before the user restarts their
> browser?

All tabs are blocked by default, and the blocked status of the tab is stored in outer window. 

Tab would be resumed by two cases,
(1) first time tab goes to foreground
(2) user click the 'play tab' icon

But for the second case, the 'play tab' icon would only be showed when there is any audible autoplay media. If there is no autoplay media in the tab, the tab won't know whether the tab itself has been blocked or resumed.

> If the tab has been unblocked, why can't we store that data in the tabData
> and then use tabData.mediaUnblocked to know if we should restore the media?

The usage of function resumeMedia() is more like to resume the tab, even there is no any active media.
We won't have any unblocked tab after restarting the browser, because all tabs are blocked by default.

---

Hi, Jared,
How do you think?
Thanks!
Flags: needinfo?(jaws)
(In reply to Alastor Wu [:alwu] from comment #32)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #31)
> > But media would be blocked in a pinned tab before the user restarts their
> > browser?
> 
> All tabs are blocked by default, and the blocked status of the tab is stored
> in outer window. 
> 
> Tab would be resumed by two cases,
> (1) first time tab goes to foreground
> (2) user click the 'play tab' icon
> 
> But for the second case, the 'play tab' icon would only be showed when there
> is any audible autoplay media. If there is no autoplay media in the tab, the
> tab won't know whether the tab itself has been blocked or resumed.
> 
> > If the tab has been unblocked, why can't we store that data in the tabData
> > and then use tabData.mediaUnblocked to know if we should restore the media?
> 
> The usage of function resumeMedia() is more like to resume the tab, even
> there is no any active media.

Please see the implementation of tabData.muteReason:
http://searchfox.org/mozilla-central/search?q=tabData.muteReason&path=

When a user unblocks a tab we should store that information with the TabState. Then when a session is restored we will be able to call resumeMedia for *any* tab that was previously unblocked, not just pinned tabs.

> We won't have any unblocked tab after restarting the browser, because all
> tabs are blocked by default.

Correct, but we can use the TabState functionality to restore state of the tabs to what they were prior to restarting.
Flags: needinfo?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #33)
> When a user unblocks a tab we should store that information with the
> TabState. Then when a session is restored we will be able to call
> resumeMedia for *any* tab that was previously unblocked, not just pinned
> tabs.

Hi, Jared,
We only want to resume the pinned tabs, because we take it as a special case. All the other tabs would still follow the same rule that is resumed after first time being foreground. (all tabs are blocked by default)

> Correct, but we can use the TabState functionality to restore state of the
> tabs to what they were prior to restarting.

Since the resuming tab doesn't need to know previous states before restarting, we just simply resume the pinned tabs. In this situation, do we still need to use the TabState?

Thanks!
Flags: needinfo?(jaws)
I don't understand why we would only want to resume pinned tabs though? With TabState.jsm we should be able to recover the tab state for all tabs. We do that with many other features. Why should this feature work differently?
Flags: needinfo?(jaws) → needinfo?(alwu)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #35)
> I don't understand why we would only want to resume pinned tabs though? With
> TabState.jsm we should be able to recover the tab state for all tabs. We do
> that with many other features. Why should this feature work differently?

This is our UX design. This feature is used to prevent automatically playing media in the unvisited tab, because it's annoying especially when user opens multiple tabs and each tab has autoplay media.

For this case, after session restore, only one tab would be in the foreground, other tabs are still unvisited after restarting, so they should still follow our design, not to play autoplay media until the tab is visited.

In this bug, we take the pinned tab as a special case, because the pinned tab might be more important for users and we think the types of pinned tab are more likely the notification website (like gmail, IRC), not media content website (like youtube).

So, after restarting, we want to resume all pinned tab and the tab which is in the foreground.
Flags: needinfo?(alwu) → needinfo?(jaws)
Yes, I understand what your desired UX design is. But in order for a tab to have this state the user would have had to unblock it in the previous session so it would not be unwarranted for us to carry forward that unblocking in to the next session.
Flags: needinfo?(jaws) → needinfo?(alwu)
Now I got the problem that can't know the actual time when XUL binding is generated, so it caused some test case fail.
Hi, Jared,

I would like to block the tab by default so that I want to call the binding function to block the tab in tabbrowser::addTab().

Ex. (you can see the more details in patch4)
> t.setMediaBlock(true /* block */);
> or
> t.linkedBrowser.blockMedia();

However, it can work well when I manually open a tab, but fail in some test cases.

I found that the binding have not been generated yet in the addTab(), so the function I called would be regard as undefined. I also tried to call setTimeout in order to ensure the binding has been generated, but it didn't work.

Could you give me some help or suggestion?
Thanks!
Flags: needinfo?(jaws)
Sorry I didn't respond to you sooner. I hope you weren't waiting for my response. The code to restore the tab should be placed in SessionStore.jsm's restoreTab function, http://searchfox.org/mozilla-central/rev/313e5199bf58200f158c6fcbe193e41b88ed58a6/browser/components/sessionstore/SessionStore.jsm#3611

You can see how that function is already doing related work like calling tab.toggleMuteAudio(tabData.muteReason).
Flags: needinfo?(jaws)
Hi, Jared,

Now the problem is not about how to block the tab when restoring the session, the problem is I want to block the tab in the beginning.

Since we need to preserve the block state in tab, so I think to move all block/resume behaviors to front-end side will be more easier to understand. We can know whether the tab is blocked or not by checking |browser.mediaBlocked|.

The problem I got is I want to block the new generated tab in tabbrowser's addTab(), so I call the method from xul binding element, eg. browser.blockMedia() or tab.setMediaBlock().

However, in a test case [1], I got the error message like that "XXX is not a function", and I guess that is because the binding hasn't been generated yet.

And I would like to know whether we have method to ensure the binding has been generated, and then I can call the binding's method. 

Thanks!

[1] http://searchfox.org/mozilla-central/source/addon-sdk/source/test/test-tab.js
Flags: needinfo?(jaws)
Redirecting to Mike as he can answer this better than I can.
Flags: needinfo?(jaws) → needinfo?(mdeboer)
I'm a bit confused by your changes in tabbrowser.xml; first you rename 'blocked' to 'soundblocked' and then later you use 'blocked' everywhere. I think 'blocked' is too generic and 'soundblocked' not appropriate, because you're blocking all media (don't really know what that means, but okay).
I think you'll want want as much as possible to be done by attribute getters and setters, not specific methods. Then you can use the generic SessionStore mechanism to record the 'mediablocked' attribute and have it restored when the session is restored after a shutdown or crash.

Since we restore browsers lazily (not the tabs themselves, just the browsers inside them), you need to make sure to check if `_browserBindingProperties` in tabbrowser.xml needs any adjustments. Feel free to reach out to any of the frontend devs for more info and/ or help if and when you need it.
Flags: needinfo?(mdeboer)
To make sure the tab attribute is persisted, you can call `SessionStore.persistTabAttribute("mediablocked");`. It's used in several places already, so a code search should clarify things much.
(In reply to Mike de Boer [:mikedeboer] from comment #59)
> I'm a bit confused by your changes in tabbrowser.xml; first you rename
> 'blocked' to 'soundblocked' and then later you use 'blocked' everywhere. I
> think 'blocked' is too generic and 'soundblocked' not appropriate, because
> you're blocking all media (don't really know what that means, but okay).

Sorry for the confusion, I'll explain it.

Why I rename the attribute is to make them more clearly and can fit the real situation.

For all tabs, we don't allow them to play any autoplay media before the tab is visited, so the default state of all tabs are "blocked". The tab would be resumed after it is visited at least once, and remove the "blocked" attribute.

The "soundblocked" attribute means that there was autoplay media tried to start and was blocked by this mechanism, the state of tab would add the attribute "soundblocked" and the tab would show the specific icon for notify user this situation.

So, all tab would have "blocked" attribute before the tab is visited, and only the tab has blocked media should have "soundblocked" attribute.

> I think you'll want want as much as possible to be done by attribute getters
> and setters, not specific methods.

But the attribute is only used in the xul element, we should also notify gecko the tab was blocked. Therefore, I want to call the binding method (eg. browser.blockMedia() or tab.setMediaBlock()) to start the blocking in gecko.

> Then you can use the generic SessionStore
> mechanism to record the 'mediablocked' attribute and have it restored when
> the session is restored after a shutdown or crash.

The session restore part is working well, now the problem I got is not related with that.

The problem is I got the error message like that "XXX is not a function" in a test case [1], and I guess that is because the binding hasn't been generated yet.

I don't know whether the test is something wrong or I shouldn't call the binding in tabbrowser's addTab(), because the binding might not be generated yet.

[1] http://searchfox.org/mozilla-central/source/addon-sdk/source/test/test-tab.js

I would like to know is there any way to ensure the binding has been generated, and then I can call the binding's method?

Or calling binding method in tabbrowser's addTab() is an acceptable way, so there is something wrong in the test [1] and we should modify it.

Thanks!
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #60)
> To make sure the tab attribute is persisted, you can call
> `SessionStore.persistTabAttribute("mediablocked");`. It's used in several
> places already, so a code search should clarify things much.

Yes, I've added the test in patch5.
(In reply to Alastor Wu [:alwu][please needinfo? me] from comment #61)
> Sorry for the confusion, I'll explain it.
> 
> Why I rename the attribute is to make them more clearly and can fit the real
> situation.
> 
> For all tabs, we don't allow them to play any autoplay media before the tab
> is visited, so the default state of all tabs are "blocked". The tab would be
> resumed after it is visited at least once, and remove the "blocked"
> attribute.
> 
> The "soundblocked" attribute means that there was autoplay media tried to
> start and was blocked by this mechanism, the state of tab would add the
> attribute "soundblocked" and the tab would show the specific icon for notify
> user this situation.
> 
> So, all tab would have "blocked" attribute before the tab is visited, and
> only the tab has blocked media should have "soundblocked" attribute.

OK, in that case I'd still use 'mediablocked', because 'blocked' makes me think of other things when I read it in the context of a tab: loading blocked, input blocked, etc. Since you're renaming it to 'soundblocked' already, let's prevent having to do that another time as well!

> 
> > I think you'll want want as much as possible to be done by attribute getters
> > and setters, not specific methods.
> 
> But the attribute is only used in the xul element, we should also notify
> gecko the tab was blocked. Therefore, I want to call the binding method (eg.
> browser.blockMedia() or tab.setMediaBlock()) to start the blocking in gecko.

Please remember that in XBL (like tabbrowser.xml and browser.xml, which you modify) you can add properties with custom getters and setters, using `onget="" onset=""` or more elaborate with `<setter></setter>` to have more lines. If you want properties to be persisted as attributes, you still need to do `this.setAttribute("blah", true)` or `this.removeAttribute("blah")` to have it on the XUL node (which is an instance of that XBL binding).
It works very similar to JavaScript getters and setters.

> The problem is I got the error message like that "XXX is not a function" in
> a test case [1], and I guess that is because the binding hasn't been
> generated yet.

True. As you can see, not a single line of code uses method call to set a property, flag or trigger behavior. All this should be done as:
1. use DOM attribute; `t.setAttribute("mediablocked", Services.prefs.getBoolPref("media.block-autoplay-until-in-foreground"))` or
2. in the tabbrowser-tab binding, add a convenience property that returns the value of the DOM node attribute
3. when a new tab is inserted, you don't want to block just that tab, but you want to block on each location change, because that changes the docShell in the active browser.

When you look at the 'mTabProgressListener' method in tabbrowser.xml and search for the line 'if (this.mTab.hasAttribute("muted")) {', you'll see exactly where I'd like you to put your things:
```js
if (this.mTab.hasAttribute("mediablocked")) {
  this.mTab.linkedBrowser.blockMedia();
  // Or linkedBrowser.mediablocked = this.mTab.getAttribute("mediablocked");, whichever you prefer. 
}
```
http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#802

Because there you can be sure that the browser has been inserted into the DOM and you can be sure that the media will be blocked for each subsequent document load in the same tab/ browser.
Yes, it's NOT guaranteed that when you add a tab, a browser element is also inserted with it! This is done lazily to safe resources, especially during an expensive session restore operation.

> I don't know whether the test is something wrong or I shouldn't call the
> binding in tabbrowser's addTab(), because the binding might not be generated
> yet.
> 
> [1]
> http://searchfox.org/mozilla-central/source/addon-sdk/source/test/test-tab.js
> 
> I would like to know is there any way to ensure the binding has been
> generated, and then I can call the binding's method?
> 
> Or calling binding method in tabbrowser's addTab() is an acceptable way, so
> there is something wrong in the test [1] and we should modify it.

Like I said above it's not acceptable to call a method in addTab, because that won't do anything since the browser element may not even be inserted into the DOM yet.

I'm actually surprised that there aren't more test failures. But not fixing this will surely lead to an increased number of intermittent failures.

If you'd like more direct assistance, instead of bugmail, please feel free to reach out to us/ me on IRC in the #fx-team channel!
We're there to help ;-)
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #63)
> If you'd like more direct assistance, instead of bugmail, please feel free
> to reach out to us/ me on IRC in the #fx-team channel!
> We're there to help ;-)

Hi, Mike,
Your suggestion is super helpful! I'll ask for a review later :)
Thanks!
Attachment #8857828 - Attachment is obsolete: true
Flags: needinfo?(chsiang)
Comment on attachment 8855204 [details]
Bug 1347791 - part1 : rename the attribute for the tab which has blocked the autoplay audio and would show the play-tab icon.

https://reviewboard.mozilla.org/r/127070/#review140670

This looks very clear to me! Thanks for the change. I'd also be OK with 'autoplaymedia-blocked', which may be more descriptive and you also use that term in your commit message. But camelCase is not something we use normally in attribute names, so I'd like you to at least upload a patch that renames the attribute to 'activemedia-blocked'.
Attachment #8855204 - Flags: review?(mdeboer) → review+
Comment on attachment 8857824 [details]
Bug 1347791 - part3 : update block stauts to front-end side.

https://reviewboard.mozilla.org/r/129824/#review140674

r=me on the browser parts. Thanks!

::: commit-message-2bbd0:20
(Diff revision 6)
> +---
> +
> +In addition, introduce new attribute "mediaBlocked".
> +
> +The "mediaBlocked" attribute indicates that whether the tab is allowed to play autoplay media.
> +The "activeMediaBlocked" attribute indicates whether the tab has blocked the autoplay media.

Ah, thanks for explaining this distinction here! 'mediaBlocked' needs to become 'media-blocked'. This also means that it's better to _not_ rename it to 'autoplaymedia-blocked', but 'activemedia-blocked' like you had. Thanks!
Comment on attachment 8857824 [details]
Bug 1347791 - part3 : update block stauts to front-end side.

https://reviewboard.mozilla.org/r/129824/#review140678
Attachment #8857824 - Flags: review?(mdeboer) → review+
Comment on attachment 8857825 [details]
Bug 1347791 - part4 : keep tab's block state consistent after session restore.

https://reviewboard.mozilla.org/r/129826/#review140684

This is starting to look really good to me, but I'd like to review this patch once you've pushed the name changes in the previous commits.
Attachment #8857825 - Flags: review?(mdeboer)
Comment on attachment 8857825 [details]
Bug 1347791 - part4 : keep tab's block state consistent after session restore.

https://reviewboard.mozilla.org/r/129826/#review142498

I'd like to see the last change I request here before I r+ it. I'll take a look right when you upload the new version. Thanks!

::: browser/base/content/tabbrowser.xml:2127
(Diff revision 8)
>                    getter = () => {
>                      return SessionStore.getLazyTabValue(aTab, name);
>                    };
>                    break;
> +                case "blockMedia":
> +                case "resumeMedia":

Since this is a switch statement, you can group these together with the `reload` and `reloadWithFlags` cases.

::: browser/base/content/tabbrowser.xml:2139
(Diff revision 8)
> +                      }, { once: true });
> +                      gBrowser._insertBrowser(aTab);
> +                    };
> +                  };
> +                  break;
> +                case "mediaBlocked":

You can put this case together with `userTypedValue` and `userTypedClear` above. There's no need to copy code here.
Attachment #8857825 - Flags: review?(mdeboer) → review-
review ping?
Thanks!
Flags: needinfo?(amarchesini)
Comment on attachment 8857825 [details]
Bug 1347791 - part4 : keep tab's block state consistent after session restore.

https://reviewboard.mozilla.org/r/129826/#review143030

Nice! Thanks, Alastor!
Attachment #8857825 - Flags: review?(mdeboer) → review+
Comment on attachment 8857824 [details]
Bug 1347791 - part3 : update block stauts to front-end side.

https://reviewboard.mozilla.org/r/129824/#review143098

The DOM part looks OK. You need a r+ from a front-end peer.
Attachment #8857824 - Flags: review?(amarchesini) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #97)
> Nice! Thanks, Alastor!

Thanks for review :)

(In reply to Andrea Marchesini [:baku] from comment #98)
> The DOM part looks OK. You need a r+ from a front-end peer.

Thanks! I've got the front-end review by :mikedeboer.
Flags: needinfo?(amarchesini)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 39b9d6be4244 -d 10841f0c5aff: rebasing 396295:39b9d6be4244 "Bug 1347791 - part1 : rename the attribute for the tab which has blocked the autoplay audio and would show the play-tab icon. r=mikedeboer"
merging browser/base/content/browser.js
merging browser/base/content/tabbrowser.xml
merging toolkit/content/tests/browser/head.js
warning: conflicts while merging toolkit/content/tests/browser/head.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a9ac97cc2efa
part1 : rename the attribute for the tab which has blocked the autoplay audio and would show the play-tab icon. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/be04f96bf78b
part2 : the tab would always be resumed when it goes to foreground first time. r=baku
https://hg.mozilla.org/integration/autoland/rev/37d295d7eec9
part3 : move block/resume logic from back-end to front-end. r=baku,mikedeboer
https://hg.mozilla.org/integration/autoland/rev/888dc371081a
part4 : keep tab's block state consistent after session restore. r=mikedeboer
This patch caused perma failures for Browser chrome, Marionette, and Firefox ui tests. Anyone who can back this out?
(In reply to Henrik Skupin (:whimboo) from comment #107)
> This patch caused perma failures for Browser chrome, Marionette, and Firefox
> ui tests. Anyone who can back this out?

done!

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=99635434&repo=autoland
Flags: needinfo?(alwu)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db2fabb21b82
Backed out changeset 888dc371081a 
https://hg.mozilla.org/integration/autoland/rev/76ed52c3ca1a
Backed out changeset 37d295d7eec9 
https://hg.mozilla.org/integration/autoland/rev/219bfc72300d
Backed out changeset be04f96bf78b 
https://hg.mozilla.org/integration/autoland/rev/95990be385ca
Backed out changeset a9ac97cc2efa for test failures in browser_ext_sessions_restore.js | restored tab has the expected url - Got about:blank, expected about:config
Hi, Mike,

I noticed that the session restore codes changes a lots recently, and my patch could not work now, but it worked well on 5/10 m-c. 

I got the fail in the test "browser_ext_sessions_restore.js", the error is tab's url would not keep consistently after restoring the tab. After some investigation, I'm wondering whether calling the lazy browser's helper method would cause some unexpectedly reset or change the tab's data.

---

Why I would say that is if I remove calling those lazy methods, then I could pass the test.

In my patch, I would call tabbrowser's method, |setMediaBlock()|, in |restoreTab()| in sessionRestore.js, and that method would call other browser's methods, |browser.blockMedia()/resumeMedia()|.

Since we have the lazy browser mechanism, so whenever I call |browser.blockMedia()|, it would active the code like [1], to wait until the real browser has been created.

In [1], it not only calls browser's method, but also calls |gBrowser._insertBrowser(aTab)|. I found that after calling |gBrowser._insertBrowser(aTab)|, the test would fail, but if I remove that line, the test would pass.

In order to verify the problem, I try to create an empty method in browser, |browser.testOnly()|, and call this method via lazy browser [1]. The result is the test would still fail, so I assume that the |gBrowser._insertBrowser(aTab)| would affect the session's tab data.

Do you have any idea why that would happen?
Thanks!

[1] https://goo.gl/VzdApV
Flags: needinfo?(alwu) → needinfo?(mdeboer)
Maybe the changes from bug 1054740 are causing the issues?
Ah, I missed something during the review indeed! My apologies.

What your patch does is _always_ insert a browser element upon restore, therefore bypassing the lazy browser mechanism altogether.
What you need to change for sure is call `setMediaBlock()` in `restoreTab()` _only_ when it's different than the default, just like with the `audioMuted` attribute. I'll reset the review again and add comments there for clarity.
Flags: needinfo?(mdeboer)
Comment on attachment 8857825 [details]
Bug 1347791 - part4 : keep tab's block state consistent after session restore.

https://reviewboard.mozilla.org/r/129826/#review143442

These are the changes you need to make (please push to try to make sure):

::: browser/components/sessionstore/SessionStore.jsm:3591
(Diff revision 10)
>  
>      if (!!tabData.muted != browser.audioMuted) {
>        tab.toggleMuteAudio(tabData.muteReason);
>      }
>  
> +    tab.setMediaBlock(!!tabData.mediaBlocked);

Please make this equivalent to the 'audioMuted' case above:
```js
let mediaBlocked = !!tabData.mediaBlocked;
if (mediaBlocked != browser.mediaBlocked)  {
  tab.setMediaBlock(mediaBlocked);
}
```

... to make sure lazy browser insertion still works in the default case.

::: browser/components/sessionstore/TabState.jsm:105
(Diff revision 10)
>      if (browser.audioMuted) {
>        tabData.muted = true;
>        tabData.muteReason = tab.muteReason;
>      }
>  
> +    tabData.mediaBlocked = browser.mediaBlocked;

Please only add properties to the JSON blob when they differ from the default attribute value:
```js
if (browser.mediaBlocked) {
  tabData.mediaBlocked = browser.mediaBlocked;
}
```
Attachment #8857825 - Flags: review+ → review-
(In reply to Mike de Boer [:mikedeboer] from comment #112)
> What your patch does is _always_ insert a browser element upon restore,
> therefore bypassing the lazy browser mechanism altogether.

I'm not sure whether it really bypass the lazy browser mechanism, because I could see the callstack of |browser.resumeMedia()| which would be called from |_createLazyBrowser()|.

> What you need to change for sure is call `setMediaBlock()` in `restoreTab()`
> _only_ when it's different than the default, just like with the `audioMuted`
> attribute. I'll reset the review again and add comments there for clarity.

I still got the same failure with running "browser_ext_sessions_restore.js" even I've applied the change in comment113.

Do you think it's possible that the problem is returning the function via lazy browser mechanism? Or we shouldn't call |gBrowser._insertBrowser(aTab)| when returning the browser's method? Because the test would be passed when I remove that line.

Sorry, I don't very understand the code here, so maybe my assumption is little ridiculous.

Could you give me any suggestion?
Thanks!
Flags: needinfo?(mdeboer)
Attached patch testFail.patch (obsolete) — — Splinter Review 
Hi, Mike,
It's high possibility that the problem is caused by lazy browser's mechanism.
I wrote a patch which call a simple empty method via lazy browser during session restore, and then it would easily make the test fail.
Could you help me to try this patch and see what's happened?
Thanks!

---

STR.
1. Apply this patch
2. Run "./mach mochitest 
browser/components/extensions/test/browser/browser_ext_sessions_restore.js"

Expect.
3. Test pass

Actual.
3. Test fail
(In reply to Mike de Boer [:mikedeboer] from comment #112)
> What your patch does is _always_ insert a browser element upon restore,
> therefore bypassing the lazy browser mechanism altogether.

Is it necessary to call |gBrowser._insertBrowser(aTab)| after I called |browser.blockMedia()/resumeMedia()|? [1]

[1] https://goo.gl/aB9nKe
Comment on attachment 8857824 [details]
Bug 1347791 - part3 : update block stauts to front-end side.

https://reviewboard.mozilla.org/r/129824/#review143882

::: browser/base/content/tabbrowser.xml:2516
(Diff revision 10)
>                });
>              }
>  
> +            // All tabs would be blocked by default, and would be resumed when
> +            // tab is visited or by clicking the play tab icon.
> +            t.setAttribute("media-blocked", Services.prefs.getBoolPref("media.block-autoplay-until-in-foreground"));

Move this up to before we start loading the page.

::: browser/base/content/tabbrowser.xml:7426
(Diff revision 10)
> +          let browser = this.linkedBrowser;
> +
> +          let modifiedAttrs = [];
> +          if (aBlock) {
> +            this.setAttribute("media-blocked", true);
> +            browser.blockMedia();

So it looks like tab.setMediaBlock will be the primary API and calling browser.blockMedia won't automatically update the tab attributes? This seems backwards.
(In reply to Dão Gottwald [::dao] from comment #117) 
> Move this up to before we start loading the page.

OK, do you mean before [1]?

[1] https://goo.gl/xXCILw

> So it looks like tab.setMediaBlock will be the primary API and calling
> browser.blockMedia won't automatically update the tab attributes? This seems
> backwards.

Since I don't see any browser method doing such kinds things, so I make tab and browser to modify their own attributes indivisually.

But the problem doesn't be fixed by these two changes.
Could you help me to check the comment116?
Thanks!
Flags: needinfo?(dao+bmo)
(In reply to Alastor Wu [:alwu][please needinfo? me] from comment #118)
> (In reply to Dão Gottwald [::dao] from comment #117) 
> > Move this up to before we start loading the page.
> 
> OK, do you mean before [1]?
> 
> [1] https://goo.gl/xXCILw

Yes.

> > So it looks like tab.setMediaBlock will be the primary API and calling
> > browser.blockMedia won't automatically update the tab attributes? This seems
> > backwards.
> 
> Since I don't see any browser method doing such kinds things, so I make tab
> and browser to modify their own attributes indivisually.

I think the reason you're not seeing this is that usually these kind of things are handled by Gecko, and then tabbrowser gets an event or notification to update the tab... This bug removes the Gecko part of the equation, so it seems like the browser should be the canonical place to hold this information.

> But the problem doesn't be fixed by these two changes.
> Could you help me to check the comment116?
> Thanks!

No, you should not call _insertBrowser after calling browser.blockMedia.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #119)
> I think the reason you're not seeing this is that usually these kind of
> things are handled by Gecko, and then tabbrowser gets an event or
> notification to update the tab... This bug removes the Gecko part of the
> equation, so it seems like the browser should be the canonical place to hold
> this information.

OK.

> No, you should not call _insertBrowser after calling browser.blockMedia.

OK, I'll modify my patch and ask for a review later.
Flags: needinfo?(mdeboer)
Comment on attachment 8857825 [details]
Bug 1347791 - part4 : keep tab's block state consistent after session restore.

https://reviewboard.mozilla.org/r/129826/#review144534

LGTM, but Dão's review is leading here - I was already expecting he'd be able to help finish this up better than I could.
Attachment #8857825 - Flags: review?(mdeboer) → review+
Comment on attachment 8857824 [details]
Bug 1347791 - part3 : update block stauts to front-end side.

https://reviewboard.mozilla.org/r/129824/#review146850

::: browser/base/content/tabbrowser.xml:651
(Diff revision 11)
>                  this.mBlank = false;
>                }
>  
> +              // Should block media before loading content.
> +              if (this.mTab.hasAttribute("media-blocked") &&
> +                  !this.mTab.linkedBrowser.mediaBlocked) {

Why are you checking both the tab and the browser here?

::: browser/base/content/tabbrowser.xml:652
(Diff revision 11)
>                }
>  
> +              // Should block media before loading content.
> +              if (this.mTab.hasAttribute("media-blocked") &&
> +                  !this.mTab.linkedBrowser.mediaBlocked) {
> +                this.mTab.setMediaBlock(true /* block */);

setMediaBlock is a pretty bad API if it makes you add /* block */ and /* unblock */ comments like this. Why don't you just call browser.blockMedia / browser.resumeMedia instead?
Attachment #8857824 - Flags: review?(dao+bmo) → review-
(In reply to Dão Gottwald [::dao] from comment #127)
> Why are you checking both the tab and the browser here?

The original idea is to prevent block media twice, but I forgot I've done the check in |blockMedia()|.

I would remove that.
 
> ::: browser/base/content/tabbrowser.xml:652
> setMediaBlock is a pretty bad API if it makes you add /* block */ and /*
> unblock */ comments like this. Why don't you just call browser.blockMedia /
> browser.resumeMedia instead?

The original idea is to set the tab's attribute and control the browser at the same time.

But we've moved the attribute changed into |browser.blockMedia/resumeMedia|, so we don't need the |setMediaBlock()| anymore.

I would remove that.
Attachment #8868677 - Attachment is obsolete: true
Comment on attachment 8857824 [details]
Bug 1347791 - part3 : update block stauts to front-end side.

https://reviewboard.mozilla.org/r/129824/#review148278

::: browser/base/content/tabbrowser.xml:651
(Diff revision 12)
>                  this.mBlank = false;
>                }
>  
> +              // Should block media before loading content.
> +              if (this.mTab.hasAttribute("media-blocked")) {
> +                this.mTab.linkedBrowser.blockMedia();

Hmmm, do we really need to call this on every page load?

::: browser/base/content/tabbrowser.xml:3454
(Diff revision 12)
>              aTab.dispatchEvent(event);
>            }
> +
> +          if (aTab.hasAttribute("media-blocked")) {
> +            aTab.linkedBrowser.resumeMedia();
> +          }

showTab is for showing previously hidden tabs. This code doesn't belong here at all.

::: toolkit/content/widgets/browser.xml:815
(Diff revision 12)
> -            this._audioBlocked = true;
> +            if (this._mediaBlocked) {
> +              return;
> +            }
> +
> +            let tab = this.getTabBrowser().getTabForBrowser(this);
> +            if (!tab.hasAttribute("media-blocked")) {

tab may be null here... not every browser has a tab. In fact, getTabBrowser() may already be null.

I'm still somewhat confused about what the primary source of truth is -- is it browser.mediaBlocked or the tab's media-blocked attribute?
Attachment #8857824 - Flags: review?(dao+bmo) → review-
Comment on attachment 8857824 [details]
Bug 1347791 - part3 : update block stauts to front-end side.

https://reviewboard.mozilla.org/r/129824/#review148278

> Hmmm, do we really need to call this on every page load?

The ideal situation is to block the tab in addTab(), but we couldn't ensure the xul binding is generated yet at that time.
That causes the blocking mechanism being async, we need to add the "media-blocked" attribute first, and then we do the real implementation by calling blockMedia().

In comment63, Mike suggests me adding this code in onLocationChange(), but we need to block the tab before loading the page's content. The onLocationChange() doesn't ensure that point, so I move the code into onStateChange().

> showTab is for showing previously hidden tabs. This code doesn't belong here at all.

Every tab would be blocked when it was created, in addTab(), and we need to unblock it when it goes to the foreground.
I think the showTab() is the place which fit my need, so I don't know why you say the code shouldn't belong there.
Do you know any other place where is more suitable for my usage?

> tab may be null here... not every browser has a tab. In fact, getTabBrowser() may already be null.
> 
> I'm still somewhat confused about what the primary source of truth is -- is it browser.mediaBlocked or the tab's media-blocked attribute?

I could add the check to ensure we add the attribute only when tab exists.

The browser.mediaBlocked is mainly attribute, the reason adding tab's media-blocked attribute is xul binding problem. Since we can't call binding method (blockMedia) in addTab(), we need to make the implementation as async, adding the extra tab's attribute to make thing happens.
Flags: needinfo?(dao+bmo)
(In reply to Alastor Wu [:alwu][please needinfo? me] from comment #137)
> > showTab is for showing previously hidden tabs. This code doesn't belong here at all.
> 
> Every tab would be blocked when it was created, in addTab(), and we need to
> unblock it when it goes to the foreground.
> I think the showTab() is the place which fit my need, so I don't know why
> you say the code shouldn't belong there.
> Do you know any other place where is more suitable for my usage?

showTab doesn't mean the tab gets selected. It's in fact entirely unrelated to that. If I understand you correctly, want you want to do belongs in updateCurrentBrowser.

> > tab may be null here... not every browser has a tab. In fact, getTabBrowser() may already be null.
> > 
> > I'm still somewhat confused about what the primary source of truth is -- is it browser.mediaBlocked or the tab's media-blocked attribute?
> 
> I could add the check to ensure we add the attribute only when tab exists.

Can you set an attribute on the browser instead? Tab attributes should mainly be limited to what's needed for the tab strip UI.

> The browser.mediaBlocked is mainly attribute, the reason adding tab's
> media-blocked attribute is xul binding problem.

Bindings should be attached synchronously, except when the browser isn't even in the document, i.e. when it's "lazy". Is that the case you're talking about?

On a more general note, are we sure that moving this stuff from Gecko to front-end is the right thing to do? This all seems somewhat messy.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #138)
> showTab doesn't mean the tab gets selected. It's in fact entirely unrelated
> to that. If I understand you correctly, want you want to do belongs in
> updateCurrentBrowser.

OK, I'm trying to move the code into updateCurrentBrowser() and see what's the result.

> Can you set an attribute on the browser instead? Tab attributes should
> mainly be limited to what's needed for the tab strip UI.

OK, I would try it.

> Bindings should be attached synchronously, except when the browser isn't
> even in the document, i.e. when it's "lazy". Is that the case you're talking
> about?

Because the binding would be attached asynchronously, in addTab() I can't ensure it is generated or not. Therefore, I can't call the binding method at that time, eg. blockMedia().
I described this issue in the last paragraph of comment61.

> On a more general note, are we sure that moving this stuff from Gecko to
> front-end is the right thing to do? This all seems somewhat messy.

IIRC, we can't directly get the value of content window's attribute crossing the different process, so I think the block status should be stored in the front-end side.

In the original design, the tab's block status would be stored in outer-window, and the tab doesn't need to know that. Since now we need to save this status into session data, we have to store this value in front-end side.
Tracking this instead of the duplicate (bug 1345947).  This sounds a bit complicated, when you do land a fix let's try to get QE to verify.
status-firefox55: --- → affected
status-firefox56: --- → ?
tracking-firefox55: --- → +
tracking-firefox56: --- → +
Flags: qe-verify+
(In reply to Alastor Wu [:alwu][please needinfo? me] from comment #139)
> > On a more general note, are we sure that moving this stuff from Gecko to
> > front-end is the right thing to do? This all seems somewhat messy.
> 
> IIRC, we can't directly get the value of content window's attribute crossing
> the different process, so I think the block status should be stored in the
> front-end side.
> 
> In the original design, the tab's block status would be stored in
> outer-window, and the tab doesn't need to know that. Since now we need to
> save this status into session data, we have to store this value in front-end
> side.

Front-end needs to know about it, but it doesn't need to /manage/ all of it. How this usually works is that the content process sends a message when the value changes and then front-end code updates its copy.
(In reply to Dão Gottwald [::dao] from comment #146)
> Front-end needs to know about it, but it doesn't need to /manage/ all of it.
> How this usually works is that the content process sends a message when the
> value changes and then front-end code updates its copy.

Do you have any example to show how they communicate with each other?
I only know the way like "audio-playback" [1], notify observer first, then send another async message, and finally dispatch event to tabbrowser, but I think it's not a good way, too many steps. I would like to know whether there has other better way.

Thanks!

[1] https://goo.gl/zp56WO
Flags: needinfo?(dao+bmo)
Don't worry about too many steps. That's the right way to do it, although it's a bit weird that Gecko sends a notification and browser.xml makes up its own DOM events. This would be more straightforward if both were DOM events using the same event names.
Flags: needinfo?(dao+bmo)
Comment on attachment 8857824 [details]
Bug 1347791 - part3 : update block stauts to front-end side.

Clear review, and I'll ask it again if the try result is good.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1044aaee9d42ef220748f0445ee2c80241bd716f
Attachment #8857824 - Flags: review?(dao+bmo)
Comment on attachment 8876673 [details]
Bug 1347791 - part5 : only enable on desktop.

https://reviewboard.mozilla.org/r/147954/#review153952
Attachment #8876673 - Flags: review?(bwu) → review+
Comment on attachment 8857824 [details]
Bug 1347791 - part3 : update block stauts to front-end side.

https://reviewboard.mozilla.org/r/129824/#review154454

Still a few minor issues but overall this looks much better.

::: browser/base/content/tabbrowser.xml:5028
(Diff revision 15)
>                stringID = tab.linkedBrowser.audioMuted ?
>                  "tabs.unmuteAudio.tooltip" :
>                  "tabs.muteAudio.tooltip";
>                label = stringWithShortcut(stringID, "key_toggleMute");
>              } else {
> -              if (tab.linkedBrowser.audioBlocked) {
> +              if (tab.hasAttribute("activemedia-blocked")) {

Shouldn't this just use tab.linkedBrowser.mediaBlocked?

::: browser/base/content/tabbrowser.xml:7450
(Diff revision 15)
>            let tabContainer = this.parentNode;
>            let browser = this.linkedBrowser;
>            let modifiedAttrs = [];
>            let hist = Services.telemetry.getHistogramById("TAB_AUDIO_INDICATOR_USED");
>  
> -          if (browser.audioBlocked) {
> +          if (this.hasAttribute("activemedia-blocked")) {

ditto

::: toolkit/content/widgets/browser.xml:748
(Diff revision 15)
>              this.dispatchEvent(event);
>            ]]>
>          </body>
>        </method>
>  
> +      <method name="audioPlaybackResumeBlock">

This method name makes no sense. Should probably be audioPlaybackBlockResumed or mediaPlaybackBlockResumed or something, and some documentation for how this is different from audioPlaybackBlockStarted would be nice too.

::: toolkit/content/widgets/browser.xml:769
(Diff revision 15)
> +          <![CDATA[
> +            // If pref is on, the browser would be blocked by default in gecko.
> +            if (this.mPrefs.getBoolPref("media.block-autoplay-until-in-foreground", true)) {
> +              return this._mediaBlocked;
> +            }
> +            return false;

This looks wrong... What if something calls browser.blockMedia() -- browser.mediaBlocked should be true then regardless of media.block-autoplay-until-in-foreground, right?
Attachment #8857824 - Flags: review?(dao+bmo) → review-
Comment on attachment 8857825 [details]
Bug 1347791 - part4 : keep tab's block state consistent after session restore.

https://reviewboard.mozilla.org/r/129826/#review154460

::: browser/components/sessionstore/TabAttributes.jsm:20
(Diff revision 15)
>  // 'iconLoadingPrincipal' is same as 'image' that it should be handled by
>  //                        using the gBrowser.getIcon()/setIcon() methods.
> +// 'mediaBlocked' should not be accessed directly but handled by using the
> +//                tab.linkedBrowser.mediaBlocked/blockMedia()/resumeMedia() methods.
>  const ATTRIBUTES_TO_SKIP = new Set(["image", "muted", "pending", "iconLoadingPrincipal",
> -                                    "skipbackgroundnotify"]);
> +                                    "skipbackgroundnotify", "mediaBlocked"]);

Did you mean activemedia-blocked? mediaBlocked is a property, not an attribute.

::: browser/components/sessionstore/test/browser_attributes.js:32
(Diff revision 15)
>  
>    // Make sure we do not persist 'image' or 'muted' attributes.
>    ss.persistTabAttribute("image");
>    ss.persistTabAttribute("muted");
>    ss.persistTabAttribute("iconLoadingPrincipal");
> +  ss.persistTabAttribute("mediaBlocked");

ditto
Attachment #8857825 - Flags: review?(dao+bmo) → review-
Comment on attachment 8857824 [details]
Bug 1347791 - part3 : update block stauts to front-end side.

https://reviewboard.mozilla.org/r/129824/#review154454

> Shouldn't this just use tab.linkedBrowser.mediaBlocked?

No, audioBlocked is old name and it's mapping to "activemedia-blocked".

If the tab doesn't have any active media been blocked, we don't need to show the play tab icon.

For example,
open an new background tab and it doesn't contain any playing media, this browser's mediaBlocked is TRUE, but tab doesn't have "activemedia-blocked". We shouldn't show the play tab icon and tooltip should be "tabs.muteAudio.background.tooltip".

> ditto

Same as the above comment.

> This method name makes no sense. Should probably be audioPlaybackBlockResumed or mediaPlaybackBlockResumed or something, and some documentation for how this is different from audioPlaybackBlockStarted would be nice too.

OK.

> This looks wrong... What if something calls browser.blockMedia() -- browser.mediaBlocked should be true then regardless of media.block-autoplay-until-in-foreground, right?

If the pref is off, that mean we don't support to block the autoplay media.
Therefore, even someone calls browser.blockMedia(), it shouldn't take a effect.
Comment on attachment 8857825 [details]
Bug 1347791 - part4 : keep tab's block state consistent after session restore.

https://reviewboard.mozilla.org/r/129826/#review154460

> Did you mean activemedia-blocked? mediaBlocked is a property, not an attribute.

Ah, sorry, I should use activemedia-blocked here.
Comment on attachment 8857824 [details]
Bug 1347791 - part3 : update block stauts to front-end side.

https://reviewboard.mozilla.org/r/129824/#review155030

::: toolkit/content/widgets/browser.xml:728
(Diff revision 16)
> -      <method name="audioPlaybackBlockStarted">
> +      <!--
> +        When the pref "media.block-autoplay-until-in-foreground" is on, all
> +        windows would be blocked by default in gecko. The "block" means the
> +        autoplay media can't be started in that tab unless the tab has been
> +        visited or resumed by tab's play tab icon. Since the window is blocked
> +        by default, so we don't have function like WindowBlockStarted().

nit: "Since the window is blocked by default, there's no method to signal entering that state."

::: toolkit/content/widgets/browser.xml:761
(Diff revision 16)
>              this.dispatchEvent(event);
>            ]]>
>          </body>
>        </method>
>  
> +      <method name="windowBlockStopped">

Still not happy with that name. :/ "Window block stopped" could refer to popup blocking or freezing the whole page or whatever.
Attachment #8857824 - Flags: review?(dao+bmo) → review-
Comment on attachment 8857825 [details]
Bug 1347791 - part4 : keep tab's block state consistent after session restore.

https://reviewboard.mozilla.org/r/129826/#review155048
Attachment #8857825 - Flags: review?(dao+bmo) → review+
Andrei, this might be of interest to QE for nightly testing for auto play or for what happens on restart.

Alastor, can you describe the behavior we are changing here? Is this just for pinned tabs?  And, is this meant only for 56 or for uplift to 55? It may be better to keep in 56.
status-firefox56: ? → affected
Flags: needinfo?(andrei.vaida)
Flags: needinfo?(alwu)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #174)
> Alastor, can you describe the behavior we are changing here? Is this just
> for pinned tabs?  And, is this meant only for 56 or for uplift to 55? It may
> be better to keep in 56.

See the full description in UX spec, bug 1308399 comment10.
I would like to keep it in 56.
Flags: needinfo?(alwu)
Target Milestone: --- → Firefox 56
Version: 47 Branch → unspecified
Moving to Emil as he's the owner of the Media Block Autoplay feature.
Flags: needinfo?(andrei.vaida) → needinfo?(emil.ghitta)
QA Contact: emil.ghitta
review ping?
Thanks!
Flags: needinfo?(dao+bmo)
Comment on attachment 8857824 [details]
Bug 1347791 - part3 : update block stauts to front-end side.

https://reviewboard.mozilla.org/r/129824/#review157728

::: toolkit/content/widgets/browser.xml:1101
(Diff revision 17)
> +              break;
> +            case "AudioPlayback:ActiveMediaBlockStop":
> +              this.activeMediaBlockStopped();
>                break;
> -            case "AudioPlayback:BlockStop":
> -              this.audioPlaybackBlockStopped();
> +            case "AudioPlayback:WindowBlockStop":
> +              this.windowMediaBlockStopped();

Sorry, this is still confusing. The naming difference between "activeMediaBlock" and "windowMediaBlock" is very vague; it seems impossible to infer meaning from that without reading docs. Can you please:

1. remove "window" from the name

2. add some clearly distinctive attribute(s) such as "initial" and/or "background"
Attachment #8857824 - Flags: review?(dao+bmo) → review-
Flags: needinfo?(dao+bmo)
Comment on attachment 8857824 [details]
Bug 1347791 - part3 : update block stauts to front-end side.

https://reviewboard.mozilla.org/r/129824/#review157956

::: toolkit/content/widgets/browser.xml:734
(Diff revision 18)
> +        (1) If the window is blocked, and the autoplay media starts
> +            - will call activeMediaBlockStarted()
> +        (2) If the window is resume, and has blocked autoplay media
> +            - will call activeMediaBlockStopped()
> +        (3) If the window is resume, but no autoplay media in the window
> +            - will call initialMediaBlockStopped()

If I read AudioChannelService::AudioChannelWindow::NotifyMediaBlockStop correctly, it will actually send initialMediaBlockStop regardless of whether there's autoplay media, and also regardless of whether we're unblocking due to the initially blocked tab being selected. Is this correct?
Thanks Liz and Florin for the heads up! 

I am tracking this change and waiting for the fix to land in order to begin testing.
Flags: needinfo?(emil.ghitta)
(In reply to Dão Gottwald [::dao] from comment #189)
> If I read AudioChannelService::AudioChannelWindow::NotifyMediaBlockStop
> correctly, it will actually send initialMediaBlockStop regardless of whether
> there's autoplay media, and also regardless of whether we're unblocking due
> to the initially blocked tab being selected. Is this correct?

The "initialMediaBlockStop" would be sent when the tab is unblocked, no matter it has autoplay media or not. Unblocking the tab would be done by selecting the tab or clicking the play tab icon.

Thanks for pointing it out, these comment are not precise, I would like to change it as following, 

(1) If the window is resumed, no matter it has autoplay media or not
    - will call initialMediaBlockStopped()
(2) If the window has blocked any autoplay media
    - will call activeMediaBlockStarted()
(3) If the window has resumed any autoplay media
    - will call activeMediaBlockStopped()

How do you think?
Thanks!
Flags: needinfo?(dao+bmo)
(In reply to Alastor Wu [:alwu][please needinfo? me] from comment #191)
> (In reply to Dão Gottwald [::dao] from comment #189)
> > If I read AudioChannelService::AudioChannelWindow::NotifyMediaBlockStop
> > correctly, it will actually send initialMediaBlockStop regardless of whether
> > there's autoplay media, and also regardless of whether we're unblocking due
> > to the initially blocked tab being selected. Is this correct?
> 
> The "initialMediaBlockStop" would be sent when the tab is unblocked, no
> matter it has autoplay media or not. Unblocking the tab would be done by
> selecting the tab or clicking the play tab icon.
> 
> Thanks for pointing it out, these comment are not precise, I would like to
> change it as following, 
> 
> (1) If the window is resumed, no matter it has autoplay media or not
>     - will call initialMediaBlockStopped()
> (2) If the window has blocked any autoplay media
>     - will call activeMediaBlockStarted()
> (3) If the window has resumed any autoplay media
>     - will call activeMediaBlockStopped()
> 
> How do you think?
> Thanks!

Sounds good, but let's rename initialMediaBlockStopped to just mediaBlockStopped since "initial" isn't accurate after all. This should finally be ready to land then. *fingers crossed*
Flags: needinfo?(dao+bmo)
Comment on attachment 8857824 [details]
Bug 1347791 - part3 : update block stauts to front-end side.

https://reviewboard.mozilla.org/r/129824/#review158388
Attachment #8857824 - Flags: review?(dao+bmo) → review+
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/02d43ba1ce16
part1 : rename the attribute for the tab which has blocked the autoplay audio and would show the play-tab icon. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/9f4b8045297c
part2 : the tab would always be resumed when it goes to foreground first time. r=baku
https://hg.mozilla.org/integration/autoland/rev/a5fbb7e2d1d0
part3 : update block stauts to front-end side. r=baku,dao,mikedeboer
https://hg.mozilla.org/integration/autoland/rev/49b533231388
part4 : keep tab's block state consistent after session restore. r=dao,mikedeboer
https://hg.mozilla.org/integration/autoland/rev/36fbfac0ea96
part5 : only enable on desktop. r=bwu
I reproduced this issue using Firefox 56.0a1(20170628100353).

This issue is no longer reproducible using Firefox 56.0a1 (Build Id:20170703030203) on Windows 10 64bit, Ubuntu 16.04 64bit and macOS 10.11.6.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+
Depends on: 1449022
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: