1160014 - Implement transition for DOM fullscreen on all desktop platforms

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

It has been proposed to add transition for DOM fullscreen for all desktop platforms. The current discussion indicates that the transition would be fade-through-black, but the final decision depends on the result from bug 1126061.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

s/bug 1126061/bug 1129061/ on comment 0

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

k17e, you probably could discuss the transition in this bug as far as I haven't started implementing it.

verdi, k17e thinks fade-through-black is not the best effect as it would drop some frames if the video is playing during the transition. I think it's inevitable, though.

Comment 3

[Jet Villegas (inactive)]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #2)
> k17e, you probably could discuss the transition in this bug as far as I
> haven't started implementing it.
> 
> verdi, k17e thinks fade-through-black is not the best effect as it would
> drop some frames if the video is playing during the transition. I think it's
> inevitable, though.

Please see bug 1129061 comment 13. If we need to optimize the transition to keep it looking smooth, please file a separate bug for that.

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Jet Villegas (:jet) from comment #3)
> Please see bug 1129061 comment 13. If we need to optimize the transition to
> keep it looking smooth, please file a separate bug for that.

This isn't about optimisation. The intent is to have the video not at full brightness for a second. That sucks the fun out of it for one second.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

I wonder whether we should implement this on both entering fullscreen and leaving fullscreen, or just for entering.

The current demo video from :verdi has transition for both of them. But there are cases where we cannot apply transition, for example when we need to fully exit fullscreen because of tab switch, navigation, etc.

On the other hand, transition could always make some users unhappy (while no transition merely makes security guys unhappy :), we probably want to avoid unnecessary transitions. We do need transition for entering because of security consideration, but do we really need transition for exiting?

Any thoughts?

Comment 6

[Jet Villegas (inactive)]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #5)
> I wonder whether we should implement this on both entering fullscreen and
> leaving fullscreen, or just for entering.
> 
> The current demo video from :verdi has transition for both of them. But
> there are cases where we cannot apply transition, for example when we need
> to fully exit fullscreen because of tab switch, navigation, etc.
> 
> On the other hand, transition could always make some users unhappy (while no
> transition merely makes security guys unhappy :), we probably want to avoid
> unnecessary transitions. We do need transition for entering because of
> security consideration, but do we really need transition for exiting?
> 
> Any thoughts?

This would be easier to determine with an A/B comparison. I'd code it up with duration prefs so we can show the options on a live build. If it's easier, wire up the entry transition first and demo the results. I can see it being easier on the eyes with the transition on entry and exit, but I can be convinced with a live demo showing otherwise.

Comment 7

[Verdi [:verdi] Best to slack me]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #5)
> On the other hand, transition could always make some users unhappy (while no
> transition merely makes security guys unhappy :), 

I don't necessarily agree with this. Without a transition things look janky. This doesn't matter to everyone but it is a quality/fit and finish issue that does matter to some and (even unconsciously) adds to a feeling of incompleteness or low quality.

> we probably want to avoid
> unnecessary transitions. We do need transition for entering because of
> security consideration, but do we really need transition for exiting?

Like Jet, I could be convinced otherwise but my intention was for it to be both on entry and exit to convey what's happening and to make the transition smooth.

Comment 8

[Florian Bender]

On Safari/OS X, the transition is the video/element being scaled up to the full screen size (+ being moved to a new space). Is this a viable option and/or is it a goal to have one kind of transition for all platforms rather than using platform default ones (where available)?

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - common part

For whoever is not clear about what I'm implementing: this bug is for implementing an identical fullscreen transition for all desktop platforms. The transition is fade-through-black: when the page requests fullscreen or leaves fullscreen, the screen fade to black, and then fade back after change finishes.

The goal of the transition is to completely drop permission management of fullscreen without opening security hole. This change could also make our fullscreen change look nicer since our current change is not that atomic.

See bug 1129061 comment 2 for more details about the new design.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - move some code to independent methods for reuse

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - backout bug 634586

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4 - implement transition for windows

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

I'd like to have the code land on one platform before I start working on the other two. The concept should be similiar for different platforms.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8628197 [details] [diff] [review]
patch 1 - common part

Review of attachment 8628197 [details] [diff] [review]:
-----------------------------------------------------------------

I just reviewed the widget code

::: widget/nsIWidget.h
@@ +811,5 @@
> +struct FullscreenTransitionInfo {
> +  struct {
> +    uint16_t mFadeIn = 467;
> +    uint16_t mStop = 66;
> +    uint16_t mFadeOut = 467;

Document the units here

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8628197 [details] [diff] [review]
patch 1 - common part

>+NS_IMETHODIMP
>+MakeWidgetFullscreenCallback::Run()
>+{
>+  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
>+  nsCOMPtr<nsIObserver> observer = new Observer(this);
>+  obs->AddObserver(observer, "fullscreen-painted", false);
>+  mWidget->MakeFullScreen(mFullscreen, mScreen);
>+
>+  // Block the callback until the next paint finishes.
>+  // There are several edge cases where we may never get the paint
>+  // notification, including:
>+  // 1. the window/tab is closed before the next paint;
>+  // 2. the user switch to another tab before the callback starts.
>+  // Completely fixing those cases seems to be tricky, and since they
>+  // should rarely happen, it probably doesn't worth to fix. Hence we
>+  // simply add a timeout here to ensure we never hang forever.
>+  TimeStamp startTime = TimeStamp::NowLoRes();
>+  TimeDuration timeout = TimeDuration::FromMilliseconds(1000);
>+  while (!mRefreshed && TimeStamp::NowLoRes() - startTime < timeout) {
>+    NS_ProcessNextEvent(nullptr, true);
>+  }
Uh, spinning event loop. Do we really need that? We're trying to avoid nested event loop spinning whenever possible.
Why can't you just remove the observer if fullscreen-painted is got, and then have a separate timer to ensure the observer is certainly
removed at some point?
>+struct FullscreenTransitionInfo {
{ goes to its own line

>+  struct {
>+    uint16_t mFadeIn = 467;
>+    uint16_t mStop = 66;
>+    uint16_t mFadeOut = 467;
>+  } mDuration;
This needs some comment. What are the magical values?


>@@ -829,16 +845,17 @@ class nsIWidget : public nsISupports {
update IID of nsIWidget

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Olli Pettay [:smaug] from comment #15)
> Comment on attachment 8628197 [details] [diff] [review]
> patch 1 - common part
> 
> >+NS_IMETHODIMP
> >+MakeWidgetFullscreenCallback::Run()
> >+{
> >+  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> >+  nsCOMPtr<nsIObserver> observer = new Observer(this);
> >+  obs->AddObserver(observer, "fullscreen-painted", false);
> >+  mWidget->MakeFullScreen(mFullscreen, mScreen);
> >+
> >+  // Block the callback until the next paint finishes.
> >+  // There are several edge cases where we may never get the paint
> >+  // notification, including:
> >+  // 1. the window/tab is closed before the next paint;
> >+  // 2. the user switch to another tab before the callback starts.
> >+  // Completely fixing those cases seems to be tricky, and since they
> >+  // should rarely happen, it probably doesn't worth to fix. Hence we
> >+  // simply add a timeout here to ensure we never hang forever.
> >+  TimeStamp startTime = TimeStamp::NowLoRes();
> >+  TimeDuration timeout = TimeDuration::FromMilliseconds(1000);
> >+  while (!mRefreshed && TimeStamp::NowLoRes() - startTime < timeout) {
> >+    NS_ProcessNextEvent(nullptr, true);
> >+  }
> Uh, spinning event loop. Do we really need that? We're trying to avoid
> nested event loop spinning whenever possible.
> Why can't you just remove the observer if fullscreen-painted is got, and
> then have a separate timer to ensure the observer is certainly
> removed at some point?

Because this callback is mean to block the thread (in widget code) which calls it until the change finishes. I can understand that it looks a bit scary to have nested event loop, but as it should be called directly in another event loop, I thought it was fine. I can try to avoid that.

> >+  struct {
> >+    uint16_t mFadeIn = 467;
> >+    uint16_t mStop = 66;
> >+    uint16_t mFadeOut = 467;
> >+  } mDuration;
> 
> This needs some comment. What are the magical values?

The magical values... are just the default pref value of fullscreen-api.transition-duration.*. I probably should add comment to both places.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - common part

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4 - implement transition for windows

Comment 19

[Jim Mathies [:jimm]]

Attachment: expand out

When I transition to fullscreen I often see this, is this intended?

Comment 20

[Jim Mathies [:jimm]]

Attachment: expand in

When exiting I see this window frame pretty often, which bug 634586 was trying to work around I think.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Those issues are certainly not intended. They might be part of bug 1177155. But with the transition effect implemented in this bug, they should be relatively less annoying: you would not see the intermediate state because we would hide that behind the black.

But they are still worth fixing, since they may indicate extra painting work on the window.

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 8629822 [details] [diff] [review]
patch 4 - implement transition for windows

Review of attachment 8629822 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +2863,5 @@
>    return NS_OK;
>  }
>  
> +static const UINT kMsgFullscreenTransitionBefore = WM_USER;
> +static const UINT kMsgFullscreenTransitionAfter = WM_USER + 1;

Move these to nsWindowDefs.h.

@@ +2866,5 @@
> +static const UINT kMsgFullscreenTransitionBefore = WM_USER;
> +static const UINT kMsgFullscreenTransitionAfter = WM_USER + 1;
> +
> +static LRESULT CALLBACK
> +FullscreenTransitionWindowProc(HWND hWnd, UINT uMsg,

What color do we paint to this window? Looks like we don't, in which case we'll get some sort of system default background color? Is that intended?

@@ +2907,5 @@
> +  nsAutoString className;
> +  className.AssignLiteral(kClassNameTransition);
> +
> +  // Initialize window class
> +  static bool sInitialized = false;

why do you protect against multiple calls here?

@@ +2909,5 @@
> +
> +  // Initialize window class
> +  static bool sInitialized = false;
> +  if (!sInitialized) {
> +    WNDCLASSW wc;

Lets null this out when we define it that way we don't need most of the init code below.

@@ +2920,5 @@
> +    wc.hCursor = nullptr;
> +    wc.hbrBackground = ::CreateSolidBrush(RGB(0, 0, 0));
> +    wc.lpszMenuName  = nullptr;
> +    wc.lpszClassName = className.get();
> +    ::RegisterClassW(&wc);

doesn't this have to happen on the ui thread that will use it?

@@ +2988,5 @@
> +                            &initData.mBounds.width, &initData.mBounds.height);
> +  // Create a semaphore for synchronizing the window handle which will
> +  // be created by the transition thread and used by the main thread for
> +  // posting the transition messages.
> +  initData.mSemaphore = ::CreateSemaphore(nullptr, 0, 1, nullptr);

This could fail right? we should assert the value is valid.

@@ +2989,5 @@
> +  // Create a semaphore for synchronizing the window handle which will
> +  // be created by the transition thread and used by the main thread for
> +  // posting the transition messages.
> +  initData.mSemaphore = ::CreateSemaphore(nullptr, 0, 1, nullptr);
> +  HANDLE thread = ::CreateThread(

ditto

@@ +2991,5 @@
> +  // posting the transition messages.
> +  initData.mSemaphore = ::CreateSemaphore(nullptr, 0, 1, nullptr);
> +  HANDLE thread = ::CreateThread(
> +    nullptr, 0, FullscreenTransitionThreadProc, &initData, 0, nullptr);
> +  ::WaitForSingleObject(initData.mSemaphore, INFINITE);

if either of the previous calls fail, do we have here forever?

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jim Mathies [:jimm] from comment #22)
> 
> @@ +2866,5 @@
> > +static const UINT kMsgFullscreenTransitionBefore = WM_USER;
> > +static const UINT kMsgFullscreenTransitionAfter = WM_USER + 1;
> > +
> > +static LRESULT CALLBACK
> > +FullscreenTransitionWindowProc(HWND hWnd, UINT uMsg,
> 
> What color do we paint to this window? Looks like we don't, in which case
> we'll get some sort of system default background color? Is that intended?

We will paint black, which is specified by hbrBackground of the window class.

> @@ +2907,5 @@
> > +  nsAutoString className;
> > +  className.AssignLiteral(kClassNameTransition);
> > +
> > +  // Initialize window class
> > +  static bool sInitialized = false;
> 
> why do you protect against multiple calls here?

No need to register the window class multiple times, since we never unregister it.

> @@ +2909,5 @@
> > +
> > +  // Initialize window class
> > +  static bool sInitialized = false;
> > +  if (!sInitialized) {
> > +    WNDCLASSW wc;
> 
> Lets null this out when we define it that way we don't need most of the init
> code below.

Not sure how to null out it. I see all calls to ::RegisterClassW in our current codebase, as well as the sample code provided by Visual Studio, use this style.

> @@ +2920,5 @@
> > +    wc.hCursor = nullptr;
> > +    wc.hbrBackground = ::CreateSolidBrush(RGB(0, 0, 0));
> > +    wc.lpszMenuName  = nullptr;
> > +    wc.lpszClassName = className.get();
> > +    ::RegisterClassW(&wc);
> 
> doesn't this have to happen on the ui thread that will use it?

No... and actually this is the UI thread which will use this window class.

> @@ +2988,5 @@
> > +                            &initData.mBounds.width, &initData.mBounds.height);
> > +  // Create a semaphore for synchronizing the window handle which will
> > +  // be created by the transition thread and used by the main thread for
> > +  // posting the transition messages.
> > +  initData.mSemaphore = ::CreateSemaphore(nullptr, 0, 1, nullptr);
> 
> This could fail right? we should assert the value is valid.
> 
> @@ +2989,5 @@
> > +  // Create a semaphore for synchronizing the window handle which will
> > +  // be created by the transition thread and used by the main thread for
> > +  // posting the transition messages.
> > +  initData.mSemaphore = ::CreateSemaphore(nullptr, 0, 1, nullptr);
> > +  HANDLE thread = ::CreateThread(
> 
> ditto
> 
> @@ +2991,5 @@
> > +  // posting the transition messages.
> > +  initData.mSemaphore = ::CreateSemaphore(nullptr, 0, 1, nullptr);
> > +  HANDLE thread = ::CreateThread(
> > +    nullptr, 0, FullscreenTransitionThreadProc, &initData, 0, nullptr);
> > +  ::WaitForSingleObject(initData.mSemaphore, INFINITE);
> 
> if either of the previous calls fail, do we have here forever?

If CreateThread fails, probably yes. If CreateSemaphore fails, not sure what would happen if passing a null to WaitForSingleObject. I guess it will also fail.

We can check both of them anyway.

Comment 24

[Jim Mathies [:jimm]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #23)
> (In reply to Jim Mathies [:jimm] from comment #22)
> > 
> > @@ +2866,5 @@
> > > +static const UINT kMsgFullscreenTransitionBefore = WM_USER;
> > > +static const UINT kMsgFullscreenTransitionAfter = WM_USER + 1;
> > > +
> > > +static LRESULT CALLBACK
> > > +FullscreenTransitionWindowProc(HWND hWnd, UINT uMsg,
> > 
> > What color do we paint to this window? Looks like we don't, in which case
> > we'll get some sort of system default background color? Is that intended?
> 
> We will paint black, which is specified by hbrBackground of the window class.
> 
> > @@ +2907,5 @@
> > > +  nsAutoString className;
> > > +  className.AssignLiteral(kClassNameTransition);
> > > +
> > > +  // Initialize window class
> > > +  static bool sInitialized = false;
> > 
> > why do you protect against multiple calls here?
> 
> No need to register the window class multiple times, since we never
> unregister it.
> 
> > @@ +2909,5 @@
> > > +
> > > +  // Initialize window class
> > > +  static bool sInitialized = false;
> > > +  if (!sInitialized) {
> > > +    WNDCLASSW wc;
> > 
> > Lets null this out when we define it that way we don't need most of the init
> > code below.
> 
> Not sure how to null out it. I see all calls to ::RegisterClassW in our
> current codebase, as well as the sample code provided by Visual Studio, use
> this style.

You can use memset, ZeroMemory, or simply WNDCLASSW wc = {0};

> 
> > @@ +2920,5 @@
> > > +    wc.hCursor = nullptr;
> > > +    wc.hbrBackground = ::CreateSolidBrush(RGB(0, 0, 0));
> > > +    wc.lpszMenuName  = nullptr;
> > > +    wc.lpszClassName = className.get();
> > > +    ::RegisterClassW(&wc);
> > 
> > doesn't this have to happen on the ui thread that will use it?
> 
> No... and actually this is the UI thread which will use this window class.

Does more than one thread pass through this function during a single desktop session? Or do we create this thread once and hold it forever to reuse it? My concern is that you are calling RegisterClass on one thread and then trying to use the resulting class on another thread.

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 8629821 [details] [diff] [review]
patch 1 - common part

Can't nsGlobalWindow.cpp listen to DOMFullscreen:Painted directly? Doing so in browser-fullscreen.js only to send the fullscreen-painted notification feels a bit weird.

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Dão Gottwald [:dao] from comment #25)
> Comment on attachment 8629821 [details] [diff] [review]
> patch 1 - common part
> 
> Can't nsGlobalWindow.cpp listen to DOMFullscreen:Painted directly? Doing so
> in browser-fullscreen.js only to send the fullscreen-painted notification
> feels a bit weird.

It seems to me we do not currently have any message listener in C++ code [1], and comment in nsIMessageListener says it is for JS only [2].

[1] https://dxr.mozilla.org/mozilla-central/search?q=addmessagelistener+ext%3Acpp&case=false
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIMessageManager.idl#176

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jim Mathies [:jimm] from comment #24)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #23)
> > (In reply to Jim Mathies [:jimm] from comment #22)
> > > doesn't this have to happen on the ui thread that will use it?
> > 
> > No... and actually this is the UI thread which will use this window class.
> 
> Does more than one thread pass through this function during a single desktop
> session? Or do we create this thread once and hold it forever to reuse it?
> My concern is that you are calling RegisterClass on one thread and then
> trying to use the resulting class on another thread.

Ah, yes, there are more than one thread which will use this window class. But it doesn't seem to have any issue for a window class to be used in a different thread. I searched the Internet and didn't see any complaint about that, and according to my test, it works fine.

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4 - implement transition for windows

Comment 29

[Jim Mathies [:jimm]]

Comment on attachment 8630254 [details] [diff] [review]
patch 4 - implement transition for windows

Review of attachment 8630254 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to see some try builds of this work so we can see how things look in a release build.

::: widget/windows/nsWindow.cpp
@@ +2917,5 @@
> +static DWORD WINAPI
> +FullscreenTransitionThreadProc(LPVOID lpParam)
> +{
> +  nsAutoString className;
> +  className.AssignLiteral(kClassNameTransition);

Can we change kClassNameTransition to whchar_t and use it here and in the CreateWindowW call?

@@ +2936,5 @@
> +  HWND wnd = ::CreateWindowW(
> +    className.get(), L"", 0, 0, 0, 0, 0,
> +    nullptr, nullptr, nsToolkit::mDllInstance, nullptr);
> +  if (!wnd) {
> +    ::ReleaseSemaphore(data->mSemaphore, 1, nullptr);

might be nice if we got this into some sort of RAII helper so there's no chance of a release leak.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8629821 [details] [diff] [review]
patch 1 - common part

>@@ -161,16 +162,20 @@ var FullScreen = {
>       case "DOMFullscreen:NewOrigin": {
>         this.showWarning(aMessage.data.originNoSuffix);
>         break;
>       }
>       case "DOMFullscreen:Exit": {
>         this._windowUtils.remoteFrameFullscreenReverted();
>         break;
>       }
>+      case "DOMFullscreen:Painted": {
>+        Services.obs.notifyObservers(window, "fullscreen-painted", "");

So child process gets MozAfterPaint, sends DOMFullscreen:Painted, which then triggers fullscreen-painted...
Rather hairy, but don't have better suggestions.

>+struct FullscreenTransitionDuration
>+{
>+  // The unit of the durations is millisecond
>+  uint16_t mFadeIn = 0;
>+  uint16_t mFadeOut = 0;
>+  bool IsSuppressed() const
>+    { return mFadeIn == 0 && mFadeOut == 0; }
bool IsSuppressed() const
{
  return mFadeIn == 0 && mFadeOut == 0;
}

>+GetFullscreenTransitionDuration(bool aEnterFullscreen,
>+                                FullscreenTransitionDuration* aDuration)
>+{
>+  auto pref = aEnterFullscreen ?
I'd prefer not using auto here, since nothing here tells explicitly what the type is.
(code reader needs to think what is the type of literal string - and although that is rather trivial, it is just
extra burden for the reader.)

>+  auto prefValue = Preferences::GetCString(pref);
Please use some real type here, not auto. I have no idea if GetCString() returns
nsCString or nsCString& or what
(so far the only cases where I can live with 'auto' are *_cast<> and use of some STL iterators, which fortunately aren't used often in Gecko ;) )



>+  if (!prefValue.IsVoid()) {
is prefValue ever null? And since Void strings are also empty, I'd check here !prefValue.IsEmpty()


>+MakeWidgetFullscreen(nsIWidget* aWidget, gfx::VRHMDInfo* aHMD,
>+                     bool aFullscreen, bool aFullscreenMode)
What does aFullscreenMode mean here? Could you perhaps use different terminology.
(I know SetFullScreenInternal uses this too, but it is equally hard to read.)

>+{
>+  FullscreenTransitionDuration duration;
>+  bool performTransition = false;
>+  nsCOMPtr<nsISupports> transitionData;
What is transitionData? Nothing in this patch seems to set it to non-null.
Do we really need to use nsISupports there, or could we use some more concrete type?



>+    enum FullscreenTransitionStage
>+    {
>+      eBeforeFullscreenToggle,
>+      eAfterFullscreenToggle
>+    };
This could use some documentation. Is this only about non-fullscreen -> fullscreen, or also about
fullscreen -> non-fullscreen?

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Olli Pettay [:smaug] from comment #30)
> Comment on attachment 8629821 [details] [diff] [review]
> patch 1 - common part
> 
> >@@ -161,16 +162,20 @@ var FullScreen = {
> >       case "DOMFullscreen:NewOrigin": {
> >         this.showWarning(aMessage.data.originNoSuffix);
> >         break;
> >       }
> >       case "DOMFullscreen:Exit": {
> >         this._windowUtils.remoteFrameFullscreenReverted();
> >         break;
> >       }
> >+      case "DOMFullscreen:Painted": {
> >+        Services.obs.notifyObservers(window, "fullscreen-painted", "");
> 
> So child process gets MozAfterPaint, sends DOMFullscreen:Painted, which then
> triggers fullscreen-painted...
> Rather hairy, but don't have better suggestions.

Yeah... I didn't find any better way for observing that...

> >+  if (!prefValue.IsVoid()) {
> is prefValue ever null? And since Void strings are also empty, I'd check
> here !prefValue.IsEmpty()

Yes. The return value of Preferences::GetCString() is nsAdoptingCString, which is nullable when the pref does not exist. But anyway, checking IsEmpty() is fine since empty string simply makes sscanf read nothing.

> >+MakeWidgetFullscreen(nsIWidget* aWidget, gfx::VRHMDInfo* aHMD,
> >+                     bool aFullscreen, bool aFullscreenMode)
> What does aFullscreenMode mean here? Could you perhaps use different
> terminology.
> (I know SetFullScreenInternal uses this too, but it is equally hard to read.)
> 
> >+{
> >+  FullscreenTransitionDuration duration;
> >+  bool performTransition = false;
> >+  nsCOMPtr<nsISupports> transitionData;
> What is transitionData? Nothing in this patch seems to set it to non-null.
> Do we really need to use nsISupports there, or could we use some more
> concrete type?

`transitionData` is used to hold platform-specific data for the transition, hence nothing in this patch actually touches it. You can probably see patch 4 for how it is used.

Since it is platform-specific, I don't think we can use anything more concrete. Actually I was using "void*" for that, but later I felt using nsISupports is probably better since that ensures we never leak it.

> >+    enum FullscreenTransitionStage
> >+    {
> >+      eBeforeFullscreenToggle,
> >+      eAfterFullscreenToggle
> >+    };
> This could use some documentation. Is this only about non-fullscreen ->
> fullscreen, or also about
> fullscreen -> non-fullscreen?

Both direction. That's why I call it "Toggle".

Comment 32

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - common part, r=roc

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1.1 - ensure fullscreen state before making widget fullscreen

I'm going to merge this patch to patch 1.

This change fixes a test failure on fullscreen-api-race on opt build, which leaves the window in fullscreen mode after the test.

That could in theory only happen in tests, though, since content is not allowed to request fullscreen automatically without user input.

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d2e246241da

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - backout bug 634586

Compare to the last revision of this patch, the DispatchFocusToTopLevelWindow() call is preserved because of test failure like https://treeherder.mozilla.org/logviewer.html#?job_id=9207683&repo=try on Windows XP.

It fails because the sizemode attribute is unchanged when the "fullscreen" event is dispatched for fullscreen being exited.

On Windows 8.1, the sizemode attribute is changed via the path: MakeFullScreen() -> UpdateNonClientMargins() -> ResetLayout() -> ::SetWindowPos() -> [external code] -> OnWindowPosChanged() -> DispatchFocusToTopLevelWindow().

I'm not sure what's wrong with Windows XP, but I guess the reason might be that the [external code] does not call the window proc synchronously on Windows XP, which delays setting sizemode attribute.

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b7f4f09ed2
https://hg.mozilla.org/integration/mozilla-inbound/rev/fca26897d534
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4fa5c794e08
https://hg.mozilla.org/integration/mozilla-inbound/rev/87b00a9dac95

Comment 37

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ab0df390753

Comment 38

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 0 - avoid mentioning DOM Fullscreen concept in widget part

Comment 39

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70bf3084d4c8

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/578557645873
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e2d116989a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/9110eb085b13
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f941c54cf02
https://hg.mozilla.org/integration/mozilla-inbound/rev/b867bb9c50ee

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/578557645873
https://hg.mozilla.org/mozilla-central/rev/5e2d116989a5
https://hg.mozilla.org/mozilla-central/rev/9110eb085b13
https://hg.mozilla.org/mozilla-central/rev/b867bb9c50ee

Comment 42

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 5 - implement transition for mac

Comment 43

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8633993 [details] [diff] [review]
patch 5 - implement transition for mac

I'd like to put this off for a few days -- hopefully until early next week.

Comment 44

[Robert Johnston]

I would very much like a way to change the transition time for this (or even better, disable it completely). It's extremely jarring at the moment, and timed incorrectly on resumption (You get to see the regular window for a good eighth of a second *before* the screen fades to black, then fades back in). 

As the content (video, for example) keeps playing even while the screen is black, it obstructs using the browser and viewing the content. It's also immediately obvious that the transition is entirely cosmetic, which makes it a three-quarter second transition covering something that's a sixteenth of a second inconvenience at best.

Comment 45

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Robert Johnston from comment #44)
> I would very much like a way to change the transition time for this (or even
> better, disable it completely). It's extremely jarring at the moment, and
> timed incorrectly on resumption (You get to see the regular window for a
> good eighth of a second *before* the screen fades to black, then fades back
> in). 
You can control it with these 2 preferences in about:config
- full-screen-api.transition-duration.enter
- full-screen-api.transition-duration.leave
To disable it just set them to "0 0"

(In reply to Robert Johnston from comment #44)
> As the content (video, for example) keeps playing even while the screen is
> black, it obstructs using the browser and viewing the content. It's also
> immediately obvious that the transition is entirely cosmetic, which makes it
> a three-quarter second transition covering something that's a sixteenth of a
> second inconvenience at best.
Report a new bug and block this one.

Comment 46

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Virtual_ManPL [:Virtual] from comment #45)
> (In reply to Robert Johnston from comment #44)
> > I would very much like a way to change the transition time for this (or even
> > better, disable it completely). It's extremely jarring at the moment, and
> > timed incorrectly on resumption (You get to see the regular window for a
> > good eighth of a second *before* the screen fades to black, then fades back
> > in). 
> You can control it with these 2 preferences in about:config
> - full-screen-api.transition-duration.enter
> - full-screen-api.transition-duration.leave
> To disable it just set them to "0 0"

It is not recommended, though. It may expose oneself to the risk of the screen being controlled by some website without awareness as we are removing the approval step.

> (In reply to Robert Johnston from comment #44)
> > As the content (video, for example) keeps playing even while the screen is
> > black, it obstructs using the browser and viewing the content. It's also
> > immediately obvious that the transition is entirely cosmetic, which makes it
> > a three-quarter second transition covering something that's a sixteenth of a
> > second inconvenience at best.
> Report a new bug and block this one.

I don't see how this paragraph indicates any actual bug...

Comment 47

[Robert Johnston]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #46)
> (In reply to Virtual_ManPL [:Virtual] from comment #45)
> > (In reply to Robert Johnston from comment #44)
> > > I would very much like a way to change the transition time for this (or even
> > > better, disable it completely). It's extremely jarring at the moment, and
> > > timed incorrectly on resumption (You get to see the regular window for a
> > > good eighth of a second *before* the screen fades to black, then fades back
> > > in). 
> > You can control it with these 2 preferences in about:config
> > - full-screen-api.transition-duration.enter
> > - full-screen-api.transition-duration.leave
> > To disable it just set them to "0 0"

I have since found those values and set them to "50 50", which is just the right time in my opinion.

> It is not recommended, though. It may expose oneself to the risk of the
> screen being controlled by some website without awareness as we are removing
> the approval step.

And how does the fading to black change that appreciably? All it seems to do, to me, is make the transition to and from fullscreen take longer, which is even more egregious when it's being done unintentionally or maliciously.

> > (In reply to Robert Johnston from comment #44)
> > > As the content (video, for example) keeps playing even while the screen is
> > > black, it obstructs using the browser and viewing the content. It's also
> > > immediately obvious that the transition is entirely cosmetic, which makes it
> > > a three-quarter second transition covering something that's a sixteenth of a
> > > second inconvenience at best.
> > Report a new bug and block this one.
> 
> I don't see how this paragraph indicates any actual bug...

Actually, it's a reference to #1184443 which has already been reported (and I didn't see at the time)

Comment 48

[timbugzilla]

Do we really want this? I find it extremely annoying with video full screen transitions.

Comment 49

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #46)
> (In reply to Virtual_ManPL [:Virtual] from comment #45)
> > (In reply to Robert Johnston from comment #44)
> > > I would very much like a way to change the transition time for this (or even
> > > better, disable it completely). It's extremely jarring at the moment, and
> > > timed incorrectly on resumption (You get to see the regular window for a
> > > good eighth of a second *before* the screen fades to black, then fades back
> > > in). 
> > You can control it with these 2 preferences in about:config
> > - full-screen-api.transition-duration.enter
> > - full-screen-api.transition-duration.leave
> > To disable it just set them to "0 0"
> 
> It is not recommended, though. It may expose oneself to the risk of the
> screen being controlled by some website without awareness as we are removing
> the approval step.
How and why?
It control only time of the animation or I'm missing something?

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #46)
> > (In reply to Robert Johnston from comment #44)
> > > As the content (video, for example) keeps playing even while the screen is
> > > black, it obstructs using the browser and viewing the content. It's also
> > > immediately obvious that the transition is entirely cosmetic, which makes it
> > > a three-quarter second transition covering something that's a sixteenth of a
> > > second inconvenience at best.
> > Report a new bug and block this one.
> 
> I don't see how this paragraph indicates any actual bug...
So read it one more time.

Comment 50

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 6 - implement transition for gtk

Comment 51

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8633993 [details] [diff] [review]
patch 5 - implement transition for mac

This looks fine to me, though I haven't had a chance to test it.

Comment 52

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/039f3d0c6e62
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3d392163411

Comment 53

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back out the Mac part for OSX build bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/7c21805844be

https://treeherder.mozilla.org/logviewer.html#?job_id=11970089&repo=mozilla-inbound

Comment 54

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out the linux part, too https://hg.mozilla.org/integration/mozilla-inbound/rev/bf7d50538de6

Comment 55

[Xidorn Quan [:xidorn] UTC+10]

The bustage on linux build is probably related to the newly landed GTK3 switch. I didn't test GTK3 build so it is possible to fail. Also I use a function which is deprecated in GTK3 in that patch, which I'll replace with another approach if we are going to drop GTK2 support.

Comment 56

[Mike Hommey [:glandium]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #55)
> The bustage on linux build is probably related to the newly landed GTK3
> switch. I didn't test GTK3 build so it is possible to fail. Also I use a
> function which is deprecated in GTK3 in that patch, which I'll replace with
> another approach if we are going to drop GTK2 support.

We should keep GTK2 builds working.

Comment 57

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 5 - implement transition for mac

I should have tested the patches with try push first...

This patch also adds some change to ensure the transition time won't be much longer than the specified duration. It seems that the function does not work as what Apple's document states.

I really suspect that, creating a window and making it cover the whole screen, like what we do in other platforms, could be more smooth and efficient than animating the display config... I'll try.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc361115de57

Comment 58

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 5 - implement transition for mac

This patch seems to have better effect.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb45ff69a764

Comment 59

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8637162 [details] [diff] [review]
patch 5 - implement transition for mac

This looks fine to me.

This time I did test it, on OS X 10.7.5 and 10.10.4.  I played one of the videos at https://vimeo.com and toggled its fullscreen button.

It seemed to work as intended:  For every transition (to or away from fullscreen mode), there was a fade to black and then a "unfade" back again.  I saw some glitches in e10s mode -- pieces of movie image (ghosts) that displayed a fraction of a second longer than they should, interrupting the smoothness of the transition too or from black.  I didn't see these with e10s off.

Comment 60

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 6 - implement transition for gtk

This patch adds several items in mozgtk to make it build with GTK+3.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cb246217521

Comment 61

[Mike Hommey [:glandium]]

Please also do a Gtk+2 try (see my message on dev-platform).

Comment 62

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Mike Hommey [:glandium] from comment #61)
> Please also do a Gtk+2 try (see my message on dev-platform).

I believe Linux x64 asan is still doing GTK+2 build.

I have a previous try push without the mozgtk change, in which only that platform builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d577d98fdaee

Comment 63

[Alfred Kayser]

Smooth!
It seems though that the fade through black has a longer period of blackness that the video by Verdi.
One feels a bit left in the dark before the video reappears.
(this is on windows)

Comment 64

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Alfred Kayser from comment #63)
> Smooth!
> It seems though that the fade through black has a longer period of blackness
> that the video by Verdi.
> One feels a bit left in the dark before the video reappears.
> (this is on windows)

It is possible... The black stays there until the document completes the change. So if there's no black, you just see an intermediate state of the window. (I'm going to optimize that as well, probably in bug 1177155. It just has a relative lower priority currently)

Comment 65

[Verdi [:verdi] Best to slack me]

(In reply to Alfred Kayser from comment #63)
> Smooth!
> It seems though that the fade through black has a longer period of blackness
> that the video by Verdi.
> One feels a bit left in the dark before the video reappears.
> (this is on windows)

This is looking really nice! Overall the transition seems to take about 32 frames to complete (10 frames fade, 12 frames black, 10 frames fade). The demo I made was 30 frames to complete with 14 frames fade, 2 frames black and 14 frames fade. So at the moment the transition takes almost the same amount of time but it feels much slower because it's fully black for a long time. If it's possible to minimize the time that it's fully black, it would be better.

Comment 66

[Mike Hommey [:glandium]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #62)
> (In reply to Mike Hommey [:glandium] from comment #61)
> > Please also do a Gtk+2 try (see my message on dev-platform).
> 
> I believe Linux x64 asan is still doing GTK+2 build.

That's not going to be true for long.

Comment 67

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6ef02746fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/a12d1b2e6e17

Comment 68

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cd6ef02746fe
https://hg.mozilla.org/mozilla-central/rev/a12d1b2e6e17