Implement transition for DOM fullscreen on all desktop platforms

RESOLVED FIXED in Firefox 42

Status

()

Core
Widget
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Depends on: 3 bugs, Blocks: 1 bug)

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(10 attachments, 8 obsolete attachments)

5.00 KB, patch
roc
: review+
Details | Diff | Splinter Review
100.37 KB, image/png
Details
27.27 KB, image/png
Details
11.18 KB, patch
jimm
: review+
Details | Diff | Splinter Review
28.83 KB, patch
smaug
: review+
dao
: review+
Details | Diff | Splinter Review
7.25 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.25 KB, patch
jimm
: review+
Details | Diff | Splinter Review
14.03 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.95 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
7.25 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
OS: Unspecified → All
Hardware: Unspecified → All
(Assignee)

Updated

2 years ago
Blocks: 1160017
(Assignee)

Comment 1

2 years ago
s/bug 1126061/bug 1129061/ on comment 0
(Assignee)

Updated

2 years ago
Depends on: 1129061
No longer depends on: 1126061
(Assignee)

Updated

2 years ago
Depends on: 1161802
(Assignee)

Comment 2

2 years ago
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

2 years ago
(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.
(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.
(Assignee)

Comment 5

2 years ago
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?
Flags: needinfo?(mverdi)
Flags: needinfo?(bugs)

Comment 6

2 years ago
(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.
Flags: needinfo?(bugs)
(Assignee)

Updated

2 years ago
Depends on: 1168028

Comment 7

2 years ago
(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.
Flags: needinfo?(mverdi)

Comment 8

2 years ago
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)?
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 9

2 years ago
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.
Assignee: nobody → quanxunzhen
Attachment #8628197 - Flags: review?(roc)
Attachment #8628197 - Flags: review?(dao)
Attachment #8628197 - Flags: review?(bugs)
(Assignee)

Comment 10

2 years ago
Created attachment 8628198 [details] [diff] [review]
patch 2 - move some code to independent methods for reuse
Attachment #8628198 - Flags: review?(roc)
(Assignee)

Comment 11

2 years ago
Created attachment 8628200 [details] [diff] [review]
patch 3 - backout bug  634586
Attachment #8628200 - Flags: review?(jmathies)
(Assignee)

Comment 12

2 years ago
Created attachment 8628201 [details] [diff] [review]
patch 4 - implement transition for windows
Attachment #8628201 - Flags: review?(jmathies)
(Assignee)

Comment 13

2 years ago
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 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
Attachment #8628197 - Flags: review?(roc) → review+
Attachment #8628198 - Flags: review?(roc) → review+
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
Attachment #8628197 - Flags: review?(bugs) → review-
(Assignee)

Comment 16

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8628201 - Flags: review?(jmathies)
(Assignee)

Comment 17

2 years ago
Created attachment 8629821 [details] [diff] [review]
patch 1 - common part
Attachment #8628197 - Attachment is obsolete: true
Attachment #8628197 - Flags: review?(dao)
Attachment #8629821 - Flags: review?(roc)
Attachment #8629821 - Flags: review?(dao)
Attachment #8629821 - Flags: review?(bugs)
(Assignee)

Comment 18

2 years ago
Created attachment 8629822 [details] [diff] [review]
patch 4 - implement transition for windows
Attachment #8628201 - Attachment is obsolete: true
Attachment #8629822 - Flags: review?(jmathies)

Updated

2 years ago
Attachment #8628200 - Flags: review?(jmathies) → review+

Comment 19

2 years ago
Created attachment 8629943 [details]
expand out

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

Comment 20

2 years ago
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.
(Assignee)

Comment 21

2 years ago
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

2 years ago
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?
Attachment #8629822 - Flags: review?(jmathies) → review-
(Assignee)

Comment 23

2 years ago
(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

2 years ago
(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 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.
(Assignee)

Comment 26

2 years ago
(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
(Assignee)

Comment 27

2 years ago
(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.
(Assignee)

Comment 28

2 years ago
Created attachment 8630254 [details] [diff] [review]
patch 4 - implement transition for windows
Attachment #8629822 - Attachment is obsolete: true
Attachment #8630254 - Flags: review?(jmathies)
Attachment #8629821 - Flags: review?(roc) → review+

Comment 29

2 years ago
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.
Attachment #8630254 - Flags: review?(jmathies) → review+
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?
Attachment #8629821 - Flags: review?(bugs) → review-
(Assignee)

Updated

2 years ago
Depends on: 1181395
(Assignee)

Updated

2 years ago
Depends on: 1181413
(Assignee)

Comment 31

2 years ago
(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".
(Assignee)

Comment 32

2 years ago
Created attachment 8631328 [details] [diff] [review]
patch 1 - common part, r=roc
Attachment #8629821 - Attachment is obsolete: true
Attachment #8629821 - Flags: review?(dao)
Attachment #8631328 - Flags: review?(dao)
Attachment #8631328 - Flags: review?(bugs)

Updated

2 years ago
Attachment #8631328 - Flags: review?(bugs) → review+

Updated

2 years ago
Attachment #8631328 - Flags: review?(dao) → review+
(Assignee)

Comment 33

2 years ago
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.
Attachment #8631941 - Flags: review?(bugs)
(Assignee)

Comment 34

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d2e246241da
(Assignee)

Comment 35

2 years ago
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.
Attachment #8628200 - Attachment is obsolete: true
Attachment #8632081 - Flags: review?(jmathies)

Updated

2 years ago
Attachment #8631941 - Flags: review?(bugs) → review+

Updated

2 years ago
Attachment #8632081 - Flags: review?(jmathies) → review+

Comment 36

2 years ago
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

2 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ab0df390753
(Assignee)

Comment 38

2 years ago
Created attachment 8632614 [details] [diff] [review]
patch 0 - avoid mentioning DOM Fullscreen concept in widget part
Attachment #8632614 - Flags: review?(roc)
(Assignee)

Comment 39

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=70bf3084d4c8
Attachment #8632614 - Flags: review?(roc) → review+

Comment 40

2 years ago
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
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
(Assignee)

Updated

2 years ago
Blocks: 646374
(Assignee)

Updated

2 years ago
Blocks: 649067
(Assignee)

Comment 42

2 years ago
Created attachment 8633993 [details] [diff] [review]
patch 5 - implement transition for mac
Attachment #8633993 - Flags: review?(smichaud)
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.

Updated

2 years ago
Depends on: 1184201
status-firefox40: affected → ---
status-firefox42: --- → fixed
Target Milestone: --- → mozilla42
(Assignee)

Updated

2 years ago
Depends on: 1184443
Depends on: 1184529
Depends on: 1185132

Comment 44

2 years ago
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.
Depends on: 1185208
(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.
(Assignee)

Comment 46

2 years ago
(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

2 years ago
(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

2 years ago
Do we really want this? I find it extremely annoying with video full screen transitions.
(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.
Depends on: 1185775
(Assignee)

Comment 50

2 years ago
Created attachment 8636513 [details] [diff] [review]
patch 6 - implement transition for gtk
Attachment #8636513 - Flags: review?(roc)
Attachment #8636513 - Flags: review?(roc) → review+
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.
Attachment #8633993 - Flags: review?(smichaud) → review+

Comment 52

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/039f3d0c6e62
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3d392163411
(Assignee)

Updated

2 years ago
Keywords: leave-open
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
Flags: needinfo?(quanxunzhen)

Updated

2 years ago
Keywords: leave-open
Backed out the linux part, too https://hg.mozilla.org/integration/mozilla-inbound/rev/bf7d50538de6
(Assignee)

Comment 55

2 years ago
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.
Depends on: 1186003
(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.
(Assignee)

Comment 57

2 years ago
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
Attachment #8633993 - Attachment is obsolete: true
(Assignee)

Comment 58

2 years ago
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
Attachment #8636939 - Attachment is obsolete: true
Flags: needinfo?(quanxunzhen)
Attachment #8637162 - Flags: review?(smichaud)
Depends on: 1186384
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.
Attachment #8637162 - Flags: review?(smichaud) → review+
(Assignee)

Comment 60

2 years ago
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
Attachment #8636513 - Attachment is obsolete: true
Attachment #8637835 - Flags: review?(roc)
Please also do a Gtk+2 try (see my message on dev-platform).
(Assignee)

Comment 62

2 years ago
(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

2 years ago
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)
(Assignee)

Comment 64

2 years ago
(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

2 years ago
(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.
(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.

Updated

2 years ago
Depends on: 1187091
Attachment #8637835 - Flags: review?(roc) → review+

Comment 67

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6ef02746fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/a12d1b2e6e17
(Assignee)

Updated

2 years ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/cd6ef02746fe
https://hg.mozilla.org/mozilla-central/rev/a12d1b2e6e17
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Depends on: 1191112
(Assignee)

Updated

2 years ago
Depends on: 1190669

Updated

2 years ago
Depends on: 1192664

Updated

2 years ago
Depends on: 1192667
(Assignee)

Updated

2 years ago
Depends on: 1190316
(Assignee)

Updated

2 years ago
Depends on: 1198563
(Assignee)

Updated

2 years ago
Duplicate of this bug: 684628
(Assignee)

Updated

2 years ago
Blocks: 708174

Updated

2 years ago
Depends on: 1211311
(Assignee)

Updated

2 years ago
No longer depends on: 1211311
Depends on: 1222776

Updated

7 months ago
Depends on: 1326788

Updated

7 months ago
Depends on: 1327806

Updated

4 months ago
Depends on: 1348846
You need to log in before you can comment on or make changes to this bug.