Last Comment Bug 1160014 - Implement transition for DOM fullscreen on all desktop platforms
: Implement transition for DOM fullscreen on all desktop platforms
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla42
Assigned To: Xidorn Quan [:xidorn] (UTC+10)
:
:
Mentors:
: 684628 (view as bug list)
Depends on: 1192664 1222776 1327806 1105939 1129061 1161802 1168028 1181395 1181413 1184201 1184443 1184529 1185132 1185208 1185775 1186003 1186384 1187091 1190316 1190669 1191112 1192667 1198563 1326788
Blocks: 1121280 646374 649067 708174 1160017
  Show dependency treegraph
 
Reported: 2015-04-29 20:49 PDT by Xidorn Quan [:xidorn] (UTC+10)
Modified: 2017-01-01 02:54 PST (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch 1 - common part (26.45 KB, patch)
2015-07-01 03:15 PDT, Xidorn Quan [:xidorn] (UTC+10)
roc: review+
bugs: review-
Details | Diff | Splinter Review
patch 2 - move some code to independent methods for reuse (5.00 KB, patch)
2015-07-01 03:17 PDT, Xidorn Quan [:xidorn] (UTC+10)
roc: review+
Details | Diff | Splinter Review
patch 3 - backout bug 634586 (2.12 KB, patch)
2015-07-01 03:19 PDT, Xidorn Quan [:xidorn] (UTC+10)
jmathies: review+
Details | Diff | Splinter Review
patch 4 - implement transition for windows (7.16 KB, patch)
2015-07-01 03:20 PDT, Xidorn Quan [:xidorn] (UTC+10)
no flags Details | Diff | Splinter Review
patch 1 - common part (28.74 KB, patch)
2015-07-06 00:33 PDT, Xidorn Quan [:xidorn] (UTC+10)
bugs: review-
roc: review+
Details | Diff | Splinter Review
patch 4 - implement transition for windows (10.35 KB, patch)
2015-07-06 00:38 PDT, Xidorn Quan [:xidorn] (UTC+10)
jmathies: review-
Details | Diff | Splinter Review
expand out (100.37 KB, image/png)
2015-07-06 06:15 PDT, Jim Mathies [:jimm]
no flags Details
expand in (27.27 KB, image/png)
2015-07-06 06:16 PDT, Jim Mathies [:jimm]
no flags Details
patch 4 - implement transition for windows (11.18 KB, patch)
2015-07-06 18:08 PDT, Xidorn Quan [:xidorn] (UTC+10)
jmathies: review+
Details | Diff | Splinter Review
patch 1 - common part, r=roc (28.83 KB, patch)
2015-07-08 16:21 PDT, Xidorn Quan [:xidorn] (UTC+10)
bugs: review+
dao+bmo: review+
Details | Diff | Splinter Review
patch 1.1 - ensure fullscreen state before making widget fullscreen (7.25 KB, patch)
2015-07-09 19:13 PDT, Xidorn Quan [:xidorn] (UTC+10)
bugs: review+
Details | Diff | Splinter Review
patch 3 - backout bug 634586 (2.25 KB, patch)
2015-07-10 06:00 PDT, Xidorn Quan [:xidorn] (UTC+10)
jmathies: review+
Details | Diff | Splinter Review
patch 0 - avoid mentioning DOM Fullscreen concept in widget part (14.03 KB, patch)
2015-07-12 19:05 PDT, Xidorn Quan [:xidorn] (UTC+10)
roc: review+
Details | Diff | Splinter Review
patch 5 - implement transition for mac (6.47 KB, patch)
2015-07-15 02:53 PDT, Xidorn Quan [:xidorn] (UTC+10)
smichaud: review+
Details | Diff | Splinter Review
patch 6 - implement transition for gtk (5.80 KB, patch)
2015-07-21 03:14 PDT, Xidorn Quan [:xidorn] (UTC+10)
roc: review+
Details | Diff | Splinter Review
patch 5 - implement transition for mac (7.15 KB, patch)
2015-07-21 20:35 PDT, Xidorn Quan [:xidorn] (UTC+10)
no flags Details | Diff | Splinter Review
patch 5 - implement transition for mac (5.95 KB, patch)
2015-07-22 04:34 PDT, Xidorn Quan [:xidorn] (UTC+10)
smichaud: review+
Details | Diff | Splinter Review
patch 6 - implement transition for gtk (7.25 KB, patch)
2015-07-23 04:05 PDT, Xidorn Quan [:xidorn] (UTC+10)
roc: review+
Details | Diff | Splinter Review

Description User image Xidorn Quan [:xidorn] (UTC+10) 2015-04-29 20:49:02 PDT
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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-04-29 20:54:26 PDT
s/bug 1126061/bug 1129061/ on comment 0
Comment 2 User image Xidorn Quan [:xidorn] (UTC+10) 2015-05-18 16:25:34 PDT
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 User image Jet Villegas (:jet) 2015-05-18 19:56:48 PDT
(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 User image Anthony Jones (:kentuckyfriedtakahe, :k17e) 2015-05-18 20:10:00 PDT
(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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-05-21 21:43:51 PDT
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 User image Jet Villegas (:jet) 2015-05-22 13:57:26 PDT
(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 User image Verdi [:verdi] 2015-05-27 13:14:54 PDT
(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 User image Florian Bender 2015-06-01 02:25:05 PDT
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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-01 03:15:42 PDT
Created attachment 8628197 [details] [diff] [review]
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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-01 03:17:16 PDT
Created attachment 8628198 [details] [diff] [review]
patch 2 - move some code to independent methods for reuse
Comment 11 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-01 03:19:27 PDT
Created attachment 8628200 [details] [diff] [review]
patch 3 - backout bug  634586
Comment 12 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-01 03:20:23 PDT
Created attachment 8628201 [details] [diff] [review]
patch 4 - implement transition for windows
Comment 13 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-01 03:22:59 PDT
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2015-07-01 03:45:25 PDT
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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2015-07-02 10:56:25 PDT
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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-02 16:06:38 PDT
(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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-06 00:33:21 PDT
Created attachment 8629821 [details] [diff] [review]
patch 1 - common part
Comment 18 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-06 00:38:09 PDT
Created attachment 8629822 [details] [diff] [review]
patch 4 - implement transition for windows
Comment 19 User image Jim Mathies [:jimm] 2015-07-06 06:15:18 PDT
Created attachment 8629943 [details]
expand out

When I transition to fullscreen I often see this, is this intended?
Comment 20 User image Jim Mathies [:jimm] 2015-07-06 06:16:13 PDT
Created attachment 8629946 [details]
expand in

When exiting I see this window frame pretty often, which bug 634586 was trying to work around I think.
Comment 21 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-06 06:25:11 PDT
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 User image Jim Mathies [:jimm] 2015-07-06 06:29:51 PDT
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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-06 06:40:24 PDT
(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 User image Jim Mathies [:jimm] 2015-07-06 07:03:41 PDT
(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 User image Dão Gottwald [:dao] 2015-07-06 10:01:01 PDT
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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-06 16:03:07 PDT
(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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-06 16:13:20 PDT
(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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-06 18:08:02 PDT
Created attachment 8630254 [details] [diff] [review]
patch 4 - implement transition for windows
Comment 29 User image Jim Mathies [:jimm] 2015-07-07 06:28:09 PDT
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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2015-07-07 11:52:12 PDT
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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-07 21:40:07 PDT
(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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-08 16:21:33 PDT
Created attachment 8631328 [details] [diff] [review]
patch 1 - common part, r=roc
Comment 33 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-09 19:13:01 PDT
Created attachment 8631941 [details] [diff] [review]
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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-10 02:58:27 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d2e246241da
Comment 35 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-10 06:00:26 PDT
Created attachment 8632081 [details] [diff] [review]
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 38 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-12 19:05:43 PDT
Created attachment 8632614 [details] [diff] [review]
patch 0 - avoid mentioning DOM Fullscreen concept in widget part
Comment 39 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-12 22:34:59 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=70bf3084d4c8
Comment 42 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-15 02:53:39 PDT
Created attachment 8633993 [details] [diff] [review]
patch 5 - implement transition for mac
Comment 43 User image Steven Michaud [:smichaud] (Retired) 2015-07-15 09:34:34 PDT
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 User image Robert Johnston 2015-07-17 19:11:36 PDT
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 User image Virtual_ManPL [:Virtual] - (ni? me) 2015-07-18 06:20:46 PDT
(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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-18 06:28:15 PDT
(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 User image Robert Johnston 2015-07-18 08:32:44 PDT
(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 User image timbugzilla 2015-07-18 09:14:31 PDT
Do we really want this? I find it extremely annoying with video full screen transitions.
Comment 49 User image Virtual_ManPL [:Virtual] - (ni? me) 2015-07-20 07:30:18 PDT
(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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-21 03:14:29 PDT
Created attachment 8636513 [details] [diff] [review]
patch 6 - implement transition for gtk
Comment 51 User image Steven Michaud [:smichaud] (Retired) 2015-07-21 10:17:14 PDT
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 54 User image Wes Kocher (:KWierso) 2015-07-21 16:54:33 PDT
Backed out the linux part, too https://hg.mozilla.org/integration/mozilla-inbound/rev/bf7d50538de6
Comment 55 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-21 17:00:17 PDT
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 User image Mike Hommey [:glandium] 2015-07-21 18:32:08 PDT
(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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-21 20:35:36 PDT
Created attachment 8636939 [details] [diff] [review]
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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-22 04:34:08 PDT
Created attachment 8637162 [details] [diff] [review]
patch 5 - implement transition for mac

This patch seems to have better effect.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb45ff69a764
Comment 59 User image Steven Michaud [:smichaud] (Retired) 2015-07-22 09:34:08 PDT
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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-23 04:05:36 PDT
Created attachment 8637835 [details] [diff] [review]
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 User image Mike Hommey [:glandium] 2015-07-23 04:22:10 PDT
Please also do a Gtk+2 try (see my message on dev-platform).
Comment 62 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-23 04:25:29 PDT
(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 User image Alfred Kayser 2015-07-23 05:15:19 PDT
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 User image Xidorn Quan [:xidorn] (UTC+10) 2015-07-23 05:39:41 PDT
(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 User image Verdi [:verdi] 2015-07-23 08:25:23 PDT
(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 User image Mike Hommey [:glandium] 2015-07-23 14:54:18 PDT
(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 69 User image Xidorn Quan [:xidorn] (UTC+10) 2015-09-28 20:33:21 PDT
*** Bug 684628 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.